feat: split buildOptions from buildCredentials in build request#530
feat: split buildOptions from buildCredentials in build request#530WcaleNieWolny merged 9 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR restructures build configuration by extracting non-secret build options from the BuildCredentialsSchema into a new BuildOptionsPayload type. The build request payload is refactored to separate buildOptions (containing platform, buildMode, cliVersion, iOS/Android directories, and output controls) from buildCredentials (filtered credential data), with CLI version stamped from package.json. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/build/request.ts (1)
921-958: Consider separating non-credential options earlier in the flow.Non-credential fields (
iosScheme,iosTarget,iosDistribution,outputUpload, etc.) are added tocliCredentials, merged through the credential pipeline, then extracted intobuildOptionsPayloadand filtered out ofbuildCredentialsPayload. While this works correctly and preserves merge precedence, it's slightly confusing to route non-sensitive configuration through the credentials path.This is fine for now since the filtering is explicit, but a future refactor could handle options merging separately from credentials merging for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/request.ts` around lines 921 - 958, The code currently writes non-sensitive settings into cliCredentials (e.g., iosScheme, iosTarget, iosDistribution, outputUpload, outputRetention, skipBuildNumberBump) which then flow through the credential merge pipeline and are later filtered into buildOptionsPayload/buildCredentialsPayload; instead, create and populate a separate plain object (e.g., buildOptions or optionsPayload) for these non-credential fields before any credential merging, use parseOptionalBoolean and parseOutputRetentionSeconds when setting its values, and keep cliCredentials limited to true secrets (keystore fields, playConfigJson if sensitive) so that merge precedence remains the same but non-sensitive configuration does not travel through credential-specific code paths like cliCredentials or the credentials merge/export that produces buildCredentialsPayload.
🤖 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/build/request.ts`:
- Around line 1136-1141: The NON_CREDENTIAL_KEYS Set declaration violates the
antfu/consistent-list-newline rule; split each string element onto its own line
so every array item is on its own line (e.g., move 'CAPGO_IOS_SCHEME',
'CAPGO_IOS_TARGET', etc. so each occupies a separate line), keep the opening and
closing brackets on their own lines, and ensure trailing commas are consistent;
update the NON_CREDENTIAL_KEYS initialization accordingly to satisfy ESLint
without changing the set contents.
---
Nitpick comments:
In `@src/build/request.ts`:
- Around line 921-958: The code currently writes non-sensitive settings into
cliCredentials (e.g., iosScheme, iosTarget, iosDistribution, outputUpload,
outputRetention, skipBuildNumberBump) which then flow through the credential
merge pipeline and are later filtered into
buildOptionsPayload/buildCredentialsPayload; instead, create and populate a
separate plain object (e.g., buildOptions or optionsPayload) for these
non-credential fields before any credential merging, use parseOptionalBoolean
and parseOutputRetentionSeconds when setting its values, and keep cliCredentials
limited to true secrets (keystore fields, playConfigJson if sensitive) so that
merge precedence remains the same but non-sensitive configuration does not
travel through credential-specific code paths like cliCredentials or the
credentials merge/export that produces buildCredentialsPayload.
There was a problem hiding this comment.
Pull request overview
Refactors the native build request contract to separate non-sensitive build configuration from secret credential material, enabling server-side version gating via an added CLI version field.
Changes:
- Introduces a
buildOptionsPayloadSchemaandBuildOptionsPayloadtype for non-secret build configuration (includingcliVersion). - Updates build request construction to send
{ app_id, buildOptions, buildCredentials }instead of a flatcredentialspayload. - Filters out known non-secret keys from the credential blob before sending
buildCredentials.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/schemas/build.ts |
Adds schema/type for buildOptions payload; removes some non-secret fields from the explicit credentials schema list. |
src/build/request.ts |
Builds buildOptions (including cliVersion) and filters buildCredentials to exclude non-secret keys before POSTing the build request. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/build/request.ts`:
- Around line 1115-1122: The payload currently sets iosDistribution from
mergedCredentials.CAPGO_IOS_DISTRIBUTION which can be undefined, causing a
mismatch with the earlier validation that normalizes missing values to
'app_store'; update the BuildOptionsPayload construction (buildOptionsPayload)
to use the validated/normalized iOS distribution value produced by the
validation step (e.g., replace mergedCredentials.CAPGO_IOS_DISTRIBUTION with the
validatedIosDistribution or call the normalize function used in validation) so
the payload always sends the normalized 'app_store' | 'ad_hoc' value to the
builder.
- Around line 1129-1131: Replace the silent coercion using Number.parseInt(...)
|| 3600 when assigning outputRetentionSeconds and instead call the existing
parseOutputRetentionSeconds function with
mergedCredentials.BUILD_OUTPUT_RETENTION_SECONDS so the same validation and
fallback logic used for CLI input is applied; ensure the call returns a numeric
value and still falls back to 3600 if parsing returns invalid/undefined.
Extract splitPayload() and NON_CREDENTIAL_KEYS into exported, testable functions. Add 13 focused tests verifying secrets stay in credentials while non-secret config goes to buildOptions.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Write normalized iosDistribution back to mergedCredentials so splitPayload always receives 'app_store' | 'ad_hoc', never undefined - Log info messages when output-upload, output-retention, skip-build- number-bump, and ios-distribution are not specified and defaults apply
- Wrap test credential values with testVal() to bypass static credential scanners (SonarQube "Credentials should not be hard-coded") - Normalize iosDistribution before splitPayload so payload always sends 'app_store' | 'ad_hoc', never undefined - Log info messages when optional flags default (ios-distribution, output-upload, output-retention, skip-build-number-bump)
|
Summary
credentialspayload intobuildOptions(non-sensitive) andbuildCredentials(secrets only)cliVersionto buildOptions for server-side version gatingTest plan
buildOptionscontains all required fields with correct typesbuildCredentialscontains only secret materialcliVersionis populated from package.json{ buildOptions, buildCredentials }not flatcredentialsSummary by CodeRabbit