feat: forward split buildOptions/buildCredentials to builder#1717
Conversation
📝 WalkthroughWalkthroughIntroduced a payload builder function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
@copilot does this PR make sense? |
|
@WcaleNieWolny I've opened a new pull request, #1719, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot Does this PR make sense - do not make new pr - tell me if this one makes sense |
|
@WcaleNieWolny I've opened a new pull request, #1720, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Updates the public build request proxy (/build/request) to forward the new split builder payload fields (buildOptions, buildCredentials) instead of the legacy flat credentials, aligning the proxy payload structure with the builder’s expected API.
Changes:
- Extends
RequestBuildBodyto includebuildOptionsandbuildCredentials. - Updates request parsing defaults to use
buildOptions/buildCredentialsand stops readingcredentials. - Forwards
buildOptions/buildCredentialsin the JSON payload sent to the builder/jobsendpoint.
… map to camelCase for builder
Extract buildBuilderPayload() from the inline fetch body so the snake_case → camelCase mapping and exact key set can be tested. 6 vitest cases verify: camelCase output, no legacy credentials field, correct metadata keys, and pass-through of contents.
Add unit tests for builder payload shape
Old CLI clients sending the flat `credentials` field would have it silently dropped, causing confusing builder failures. Now the proxy explicitly rejects non-empty `credentials` with a migration message pointing to `build_credentials`.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a07602aafa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (credentials && Object.keys(credentials).length > 0) { | ||
| cloudlogErr({ |
There was a problem hiding this comment.
credentials payload compatibility
This new guard turns previously valid /build/request payloads into hard failures: any client still sending the legacy credentials object now gets invalid_parameter and never reaches the builder. Since this endpoint is already in use, older CLI versions will stop being able to trigger builds after this deploy. Keep backward compatibility by accepting legacy credentials (at least as a fallback to build_credentials) instead of rejecting the request outright.
Useful? React with 👍 / 👎.
![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)
supabase/functions/_backend/public/build/request.ts (1)
40-46: Narrowplatformtype to the endpoint contract.The
platformparameter at line 43 is typed asstring, which weakens compile-time safety. SinceRequestBuildBody['platform']is strictly'ios' | 'android', type the parameter asRequestBuildBody['platform']to prevent accidental invalid values at future call sites.Suggested diff
export function buildBuilderPayload(input: { orgId: string uploadPath: string - platform: string + platform: RequestBuildBody['platform'] buildOptions: Record<string, unknown> buildCredentials: Record<string, string> }) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/public/build/request.ts` around lines 40 - 46, The buildBuilderPayload function currently types the platform parameter as string which allows invalid values; change its type to the endpoint contract type by replacing platform: string with platform: RequestBuildBody['platform'] (importing or referencing RequestBuildBody as needed) so platform is narrowed to 'ios' | 'android' and matches the RequestBuildBody contract used by the endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/builder-payload.unit.test.ts`:
- Around line 7-105: Replace all synchronous Jest test declarations using
it(...) with concurrent test declarations using it.concurrent(...) in this test
file; update each of the six tests (e.g., the tests with titles "maps
build_options (snake_case input) to buildOptions (camelCase output)", "maps
build_credentials (snake_case input) to buildCredentials (camelCase output)",
"does not include a legacy flat credentials field", "includes userId,
artifactKey, and fastlane with correct values", "contains exactly the expected
top-level keys", and "passes through buildOptions and buildCredentials contents
unchanged") so they run in parallel via it.concurrent, leaving the test bodies
and assertions unchanged.
---
Nitpick comments:
In `@supabase/functions/_backend/public/build/request.ts`:
- Around line 40-46: The buildBuilderPayload function currently types the
platform parameter as string which allows invalid values; change its type to the
endpoint contract type by replacing platform: string with platform:
RequestBuildBody['platform'] (importing or referencing RequestBuildBody as
needed) so platform is narrowed to 'ios' | 'android' and matches the
RequestBuildBody contract used by the endpoint.
| it('maps build_options (snake_case input) to buildOptions (camelCase output)', () => { | ||
| const payload = buildBuilderPayload({ | ||
| orgId: 'org-123', | ||
| uploadPath: 'orgs/org-123/apps/com.test/native-builds/session.zip', | ||
| platform: 'ios', | ||
| buildOptions: { platform: 'ios', buildMode: 'release', cliVersion: '7.83.0' }, | ||
| buildCredentials: {}, | ||
| }) | ||
|
|
||
| expect(payload).toHaveProperty('buildOptions') | ||
| expect(payload.buildOptions).toEqual({ platform: 'ios', buildMode: 'release', cliVersion: '7.83.0' }) | ||
| // Must NOT contain the snake_case input key | ||
| expect(payload).not.toHaveProperty('build_options') | ||
| }) | ||
|
|
||
| it('maps build_credentials (snake_case input) to buildCredentials (camelCase output)', () => { | ||
| const payload = buildBuilderPayload({ | ||
| orgId: 'org-123', | ||
| uploadPath: 'orgs/org-123/apps/com.test/native-builds/session.zip', | ||
| platform: 'android', | ||
| buildOptions: {}, | ||
| buildCredentials: { KEYSTORE_KEY_ALIAS: 'alias', KEYSTORE_KEY_PASSWORD: 'val' }, | ||
| }) | ||
|
|
||
| expect(payload).toHaveProperty('buildCredentials') | ||
| expect(payload.buildCredentials).toEqual({ KEYSTORE_KEY_ALIAS: 'alias', KEYSTORE_KEY_PASSWORD: 'val' }) | ||
| // Must NOT contain the snake_case input key | ||
| expect(payload).not.toHaveProperty('build_credentials') | ||
| }) | ||
|
|
||
| it('does not include a legacy flat credentials field', () => { | ||
| const payload = buildBuilderPayload({ | ||
| orgId: 'org-123', | ||
| uploadPath: 'path.zip', | ||
| platform: 'ios', | ||
| buildOptions: {}, | ||
| buildCredentials: { SOME_SECRET: 'val' }, | ||
| }) | ||
|
|
||
| expect(payload).not.toHaveProperty('credentials') | ||
| }) | ||
|
|
||
| it('includes userId, artifactKey, and fastlane with correct values', () => { | ||
| const payload = buildBuilderPayload({ | ||
| orgId: 'org-456', | ||
| uploadPath: 'orgs/org-456/apps/com.example/native-builds/uuid.zip', | ||
| platform: 'android', | ||
| buildOptions: {}, | ||
| buildCredentials: {}, | ||
| }) | ||
|
|
||
| expect(payload.userId).toBe('org-456') | ||
| expect(payload.artifactKey).toBe('orgs/org-456/apps/com.example/native-builds/uuid.zip') | ||
| expect(payload.fastlane).toEqual({ lane: 'android' }) | ||
| }) | ||
|
|
||
| it('contains exactly the expected top-level keys', () => { | ||
| const payload = buildBuilderPayload({ | ||
| orgId: 'org-789', | ||
| uploadPath: 'path/to/artifact.zip', | ||
| platform: 'ios', | ||
| buildOptions: { foo: 'bar' }, | ||
| buildCredentials: { baz: 'qux' }, | ||
| }) | ||
|
|
||
| const keys = Object.keys(payload).sort() | ||
| expect(keys).toEqual([ | ||
| 'artifactKey', | ||
| 'buildCredentials', | ||
| 'buildOptions', | ||
| 'fastlane', | ||
| 'userId', | ||
| ]) | ||
| }) | ||
|
|
||
| it('passes through buildOptions and buildCredentials contents unchanged', () => { | ||
| const complexOptions = { | ||
| platform: 'ios', | ||
| buildMode: 'debug', | ||
| cliVersion: '7.84.0', | ||
| nested: { deep: { value: 42 } }, | ||
| array: [1, 2, 3], | ||
| } | ||
| const complexCredentials = { | ||
| BUILD_CERTIFICATE_BASE64: 'base64data', | ||
| P12_PASSWORD: 'test-val', | ||
| } | ||
|
|
||
| const payload = buildBuilderPayload({ | ||
| orgId: 'org-test', | ||
| uploadPath: 'test/path.zip', | ||
| platform: 'ios', | ||
| buildOptions: complexOptions, | ||
| buildCredentials: complexCredentials, | ||
| }) | ||
|
|
||
| expect(payload.buildOptions).toEqual(complexOptions) | ||
| expect(payload.buildCredentials).toEqual(complexCredentials) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the file exists and view its test cases
if [ -f "tests/builder-payload.unit.test.ts" ]; then
echo "=== File found, checking test structure ==="
head -120 tests/builder-payload.unit.test.ts
else
echo "=== File does not exist at expected location ==="
find tests -name "*builder-payload*" 2>/dev/null
fiRepository: Cap-go/capgo
Length of output: 3658
🏁 Script executed:
#!/bin/bash
# Check for it() usage in all test files to understand repo pattern
echo "=== Checking it() vs it.concurrent() usage across test files ==="
echo "Files using it():"
rg -l "^\s*it\(" tests --type=ts | head -20
echo ""
echo "Files using it.concurrent():"
rg -l "it\.concurrent\(" tests --type=ts | head -20Repository: Cap-go/capgo
Length of output: 1079
🏁 Script executed:
#!/bin/bash
# Check if tests/test-utils.ts exists (mentioned in guidelines)
if [ -f "tests/test-utils.ts" ]; then
echo "=== tests/test-utils.ts exists ==="
head -50 tests/test-utils.ts
else
echo "=== tests/test-utils.ts not found ==="
fiRepository: Cap-go/capgo
Length of output: 2364
Use it.concurrent() for all test cases in this file.
This file matches the tests/**/*.{test,spec}.{ts,js} pattern where all tests must run in parallel using it.concurrent() instead of it() for faster CI/CD. All 6 test cases here violate this rule.
Suggested diff
- it('maps build_options (snake_case input) to buildOptions (camelCase output)', () => {
+ it.concurrent('maps build_options (snake_case input) to buildOptions (camelCase output)', () => {
@@
- it('maps build_credentials (snake_case input) to buildCredentials (camelCase output)', () => {
+ it.concurrent('maps build_credentials (snake_case input) to buildCredentials (camelCase output)', () => {
@@
- it('does not include a legacy flat credentials field', () => {
+ it.concurrent('does not include a legacy flat credentials field', () => {
@@
- it('includes userId, artifactKey, and fastlane with correct values', () => {
+ it.concurrent('includes userId, artifactKey, and fastlane with correct values', () => {
@@
- it('contains exactly the expected top-level keys', () => {
+ it.concurrent('contains exactly the expected top-level keys', () => {
@@
- it('passes through buildOptions and buildCredentials contents unchanged', () => {
+ it.concurrent('passes through buildOptions and buildCredentials contents unchanged', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/builder-payload.unit.test.ts` around lines 7 - 105, Replace all
synchronous Jest test declarations using it(...) with concurrent test
declarations using it.concurrent(...) in this test file; update each of the six
tests (e.g., the tests with titles "maps build_options (snake_case input) to
buildOptions (camelCase output)", "maps build_credentials (snake_case input) to
buildCredentials (camelCase output)", "does not include a legacy flat
credentials field", "includes userId, artifactKey, and fastlane with correct
values", "contains exactly the expected top-level keys", and "passes through
buildOptions and buildCredentials contents unchanged") so they run in parallel
via it.concurrent, leaving the test bodies and assertions unchanged.
Summary
buildOptionsandbuildCredentialsinstead of flatcredentialsTest plan
Summary by CodeRabbit
New Features
build_optionsandbuild_credentialsfields for builder API requests.credentialsfield deprecated in favor ofbuild_credentials.Bug Fixes
Tests