feat(cli): auto-bump marketing version when already released#2596
feat(cli): auto-bump marketing version when already released#2596riderx wants to merge 12 commits into
Conversation
Wire marketing version auto-bump controls through CLI credentials and build request payloads so the builder can bump CFBundleShortVersionString when the live App Store version is already released. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughAdds a ChangesCLI: skipMarketingVersionBump flag
CLI: prescan request gate test harness
DB: Org ownership transfer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
🧪 Builder onboarding TUI preview — ✅ passed▶ Open the interactive HTML report (zoomable journey tree + cast playback) Commit: 97f6be7 · Job summary with the result table |
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6381a799-fe29-4a2d-8d7d-dd361bcc5cda) |
Co-authored-by: Cursor <cursoragent@cursor.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_bead8c37-ce0b-4234-b649-fe94b8edf049) |
* test: cover org ownership transfer failure * fix: transfer org owner on self-removal
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2a0f9f0a-370f-449e-8720-b8511236e5e1) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/migrations/20260630133811_transfer_org_owner_on_self_removal.sql`:
- Around line 1-104: Add Postgres-level tests for the new SECURITY DEFINER
function public.delete_org_member_role to cover owner self-removal with
ownership transfer, rejection when a non-owner tries to remove the org owner,
and protection against removing the last super_admin. Use the function name, the
org ownership logic around created_by, and the super_admin role-binding checks
to locate the behavior, and add the corresponding database test coverage
alongside the existing Vitest/e2e coverage.
- Around line 39-48: Document the execution model for this RPC by adding
migration comments near the successor lookup and the broader transfer/delete
flow in the transfer_org_owner_on_self_removal function. Specify the expected
cardinality, caller roles, and call frequency, and call out the exact supporting
indexes used for the role_bindings and roles lookups so the planner bounds are
clear. Reference the successor-selection query in the function and the
super-admin count / role-binding deletion steps to describe how these operations
scale under PostgREST exposure.
In `@tests/org-ownership-transfer.test.ts`:
- Line 1: The test file is importing the Supabase type with a relative path
instead of the required `~/` alias. Update the `Database` import to use the `~/`
alias that maps to `src/`, matching the project’s TypeScript import convention
and keeping the import style consistent with other `*.ts` files.
- Around line 89-143: These two org ownership transfer cases are isolated and
should run in parallel with the test suite’s concurrency convention. Update the
two test cases in org-ownership-transfer.test.ts to use it.concurrent instead of
it, keeping the existing assertions and setup intact; the relevant symbols are
the two test blocks that exercise update_org_member_role,
delete_org_member_role, and getSupabaseClient.
- Around line 71-86: The cleanup in afterAll currently ignores Supabase mutation
failures, so the deletes for role_bindings, org_users, orgs, and stripe_info can
silently fail. Update the cleanup in org-ownership-transfer.test.ts to use
throwOnError() or explicitly check each returned error on the
getSupabaseClient() calls, so any failure in the afterAll teardown is surfaced
immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a90e3574-1519-45b7-bbac-a6e9569c099b
📒 Files selected for processing (9)
cli/src/build/credentials-command.tscli/src/build/credentials.tscli/src/build/request.tscli/src/index.tscli/src/schemas/build.tscli/test/test-payload-split.mjsprivate/cli-mcp-testssupabase/migrations/20260630133811_transfer_org_owner_on_self_removal.sqltests/org-ownership-transfer.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
Add Postgres-level coverage, document RPC execution model, and tighten org ownership transfer test cleanup and concurrency. Co-authored-by: Cursor <cursoragent@cursor.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_787cb715-eeaa-40f1-abd4-8d865f39fb19) |
Co-authored-by: Cursor <cursoragent@cursor.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6d799890-c2c1-465b-ad31-c2cffb204e51) |
Delete test orgs directly so role_bindings cascade past the last super-admin guard, and mock Supabase fetch in the prescan flag test to avoid CI timeouts on real network calls. Co-authored-by: Cursor <cursoragent@cursor.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_7ce6251c-b1df-4da7-8edf-77390127c2d1) |
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)
tests/org-ownership-transfer.test.ts (1)
90-144: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winCover the
CANNOT_REMOVE_LAST_SUPER_ADMINclient path too.The pgTAP suite covers that branch, but this Vitest file only verifies successful transfer and
CANNOT_CHANGE_OWNER_ROLE. Add the remaining rejection case here so the RPC/error surface is exercised through the client path as well. As per coding guidelines, database changes should be covered with Postgres-level tests and complemented with end-to-end tests for affected user flows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/org-ownership-transfer.test.ts` around lines 90 - 144, Add a client-path test in org-ownership-transfer.test.ts to cover the CANNOT_REMOVE_LAST_SUPER_ADMIN rejection through the RPC flow. Extend the existing org membership tests around update_org_member_role and delete_org_member_role by creating a scenario where the owner is the only super admin, then verify that attempting to remove that last super admin returns the expected error message and leaves org.created_by unchanged. Keep the new assertion consistent with the existing owner-transfer and CANNOT_CHANGE_OWNER_ROLE checks.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/test/prescan/request-gate.test.ts`:
- Around line 32-34: The fetch stub in request-gate.test.ts has a syntax issue
caused by the trailing comma after the async arrow expression, which prevents
the test file from being parsed. Update the globalThis.fetch assignment so the
async function expression is written without that trailing comma, keeping the
stub behavior the same and making sure the test module loads correctly.
In `@supabase/tests/61_test_org_owner_self_removal.sql`:
- Around line 47-136: Add a regression test in the org owner self-removal
migration that exercises `public.delete_org_member_role` on an org with
`orgs.enforcing_2fa = true` while the authenticated caller does not have 2FA
enabled. Use the existing test flow around `tests.authenticate_as`,
`tests.clear_authentication`, and `throws_like` to verify the RPC is rejected
for a super admin without 2FA. Make sure the new case is placed alongside the
current ownership-removal assertions so `delete_org_member_role` is covered by
the repo’s 2FA invariant.
---
Outside diff comments:
In `@tests/org-ownership-transfer.test.ts`:
- Around line 90-144: Add a client-path test in org-ownership-transfer.test.ts
to cover the CANNOT_REMOVE_LAST_SUPER_ADMIN rejection through the RPC flow.
Extend the existing org membership tests around update_org_member_role and
delete_org_member_role by creating a scenario where the owner is the only super
admin, then verify that attempting to remove that last super admin returns the
expected error message and leaves org.created_by unchanged. Keep the new
assertion consistent with the existing owner-transfer and
CANNOT_CHANGE_OWNER_ROLE checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d8922651-e32a-44d9-a1b8-1bd5c4a3c0e5
📒 Files selected for processing (4)
cli/test/prescan/request-gate.test.tssupabase/migrations/20260630133811_transfer_org_owner_on_self_removal.sqlsupabase/tests/61_test_org_owner_self_removal.sqltests/org-ownership-transfer.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
| globalThis.fetch = (async () => | ||
| new Response(JSON.stringify({}), { status: 200, headers: { 'Content-Type': 'application/json' } }), | ||
| ) as typeof fetch |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the trailing comma in the fetch stub. Line 34 makes the async arrow expression unparsable, so this test file won’t load.
Suggested fix
- globalThis.fetch = (async () =>
- new Response(JSON.stringify({}), { status: 200, headers: { 'Content-Type': 'application/json' } }),
- ) as typeof fetch
+ globalThis.fetch = (async () =>
+ new Response(JSON.stringify({}), { status: 200, headers: { 'Content-Type': 'application/json' } })
+ ) as typeof fetch📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| globalThis.fetch = (async () => | |
| new Response(JSON.stringify({}), { status: 200, headers: { 'Content-Type': 'application/json' } }), | |
| ) as typeof fetch | |
| globalThis.fetch = (async () => | |
| new Response(JSON.stringify({}), { status: 200, headers: { 'Content-Type': 'application/json' } }) | |
| ) as typeof fetch |
🧰 Tools
🪛 Biome (2.5.1)
[error] 34-34: Expected an expression but instead found ')'.
(parse)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/test/prescan/request-gate.test.ts` around lines 32 - 34, The fetch stub
in request-gate.test.ts has a syntax issue caused by the trailing comma after
the async arrow expression, which prevents the test file from being parsed.
Update the globalThis.fetch assignment so the async function expression is
written without that trailing comma, keeping the stub behavior the same and
making sure the test module loads correctly.
| SELECT tests.authenticate_as('org_owner_self_owner'); | ||
|
|
||
| SELECT is( | ||
| public.update_org_member_role( | ||
| '70000000-0000-4000-8000-000000000061'::uuid, | ||
| tests.get_supabase_uid('org_owner_self_successor'), | ||
| public.rbac_role_org_super_admin() | ||
| ), | ||
| 'OK', | ||
| 'owner can promote a successor to org_super_admin' | ||
| ); | ||
|
|
||
| SELECT is( | ||
| public.delete_org_member_role( | ||
| '70000000-0000-4000-8000-000000000061'::uuid, | ||
| tests.get_supabase_uid('org_owner_self_owner') | ||
| ), | ||
| 'OK', | ||
| 'owner can remove themselves after another org_super_admin exists' | ||
| ); | ||
|
|
||
| SELECT tests.clear_authentication(); | ||
| SELECT tests.authenticate_as_service_role(); | ||
|
|
||
| SELECT is( | ||
| ( | ||
| SELECT created_by | ||
| FROM public.orgs | ||
| WHERE id = '70000000-0000-4000-8000-000000000061' | ||
| ), | ||
| tests.get_supabase_uid('org_owner_self_successor'), | ||
| 'ownership transfers to the promoted successor' | ||
| ); | ||
|
|
||
| SELECT tests.clear_authentication(); | ||
| SELECT tests.authenticate_as('org_owner_self_owner'); | ||
|
|
||
| SELECT is( | ||
| public.update_org_member_role( | ||
| '70000000-0000-4000-8000-000000000062'::uuid, | ||
| tests.get_supabase_uid('org_owner_self_peer'), | ||
| public.rbac_role_org_super_admin() | ||
| ), | ||
| 'OK', | ||
| 'owner can promote a peer super admin on the protected org' | ||
| ); | ||
|
|
||
| SELECT tests.clear_authentication(); | ||
| SELECT tests.authenticate_as('org_owner_self_peer'); | ||
|
|
||
| SELECT throws_like( | ||
| $$ | ||
| SELECT public.delete_org_member_role( | ||
| '70000000-0000-4000-8000-000000000062'::uuid, | ||
| tests.get_supabase_uid('org_owner_self_owner') | ||
| ) | ||
| $$, | ||
| '%CANNOT_CHANGE_OWNER_ROLE%', | ||
| 'non-owner super admin cannot remove the org owner' | ||
| ); | ||
|
|
||
| SELECT tests.clear_authentication(); | ||
| SELECT tests.authenticate_as_service_role(); | ||
|
|
||
| SELECT is( | ||
| ( | ||
| SELECT created_by | ||
| FROM public.orgs | ||
| WHERE id = '70000000-0000-4000-8000-000000000062' | ||
| ), | ||
| tests.get_supabase_uid('org_owner_self_owner'), | ||
| 'org owner remains unchanged after blocked peer removal' | ||
| ); | ||
|
|
||
| SELECT tests.clear_authentication(); | ||
| SELECT tests.authenticate_as('org_owner_self_peer'); | ||
|
|
||
| SELECT tests.clear_authentication(); | ||
| SELECT tests.authenticate_as('org_owner_self_owner'); | ||
|
|
||
| SELECT throws_like( | ||
| $$ | ||
| SELECT public.delete_org_member_role( | ||
| '70000000-0000-4000-8000-000000000063'::uuid, | ||
| tests.get_supabase_uid('org_owner_self_owner') | ||
| ) | ||
| $$, | ||
| '%CANNOT_REMOVE_LAST_SUPER_ADMIN%', | ||
| 'owner cannot remove themselves when no successor super admin exists' | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add an enforcing-2FA regression case for this RPC.
These assertions cover the ownership branches, but they never prove that delete_org_member_role is blocked when orgs.enforcing_2fa = true and the caller lacks 2FA. That leaves this new org-management RPC without coverage for the repo’s 2FA invariant. Based on learnings, org functions on enforcing_2fa=true orgs must reject callers without 2FA, including super admins, and that rule should be covered in the migration tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/tests/61_test_org_owner_self_removal.sql` around lines 47 - 136, Add
a regression test in the org owner self-removal migration that exercises
`public.delete_org_member_role` on an org with `orgs.enforcing_2fa = true` while
the authenticated caller does not have 2FA enabled. Use the existing test flow
around `tests.authenticate_as`, `tests.clear_authentication`, and `throws_like`
to verify the RPC is rejected for a super admin without 2FA. Make sure the new
case is placed alongside the current ownership-removal assertions so
`delete_org_member_role` is covered by the repo’s 2FA invariant.
Source: Learnings
Point private/cli-mcp-tests at the merged feat/marketing-version-bump-golden commit that integrates Appflow e2e changes from main with marketing version bump golden updates. Co-authored-by: Cursor <cursoragent@cursor.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ebc454f1-21c0-43c9-b71d-2edf87dd1c55) |
|
1 participant
Summary (AI generated)
--skip-marketing-version-bump/--no-skip-marketing-version-bumptobuild requestand credential save/update commandsskipMarketingVersionBumpthrough build options payload andSKIP_MARKETING_VERSION_BUMPenv/credentials--skip-marketing-version-bump)Motivation (AI generated)
iOS App Store uploads fail when
CFBundleShortVersionStringmatches an already approved live version (error 90062), even if the build number was incremented. The builder already auto-bumps build numbers; this adds the matching CLI flag plumbing for marketing version auto-bump.Business Impact (AI generated)
Reduces failed native iOS builds and support tickets for customers re-shipping hotfixes on an already-released marketing version.
Test Plan (AI generated)
bun run cli:checkbun test/test-payload-split.mjsGenerated with AI
Note
Medium Risk
The
SECURITY DEFINERRPC change affects org ownership and member removal in production RBAC paths; CLI changes are additive and low-risk but depend on a companion builder PR for actual version bumping.Overview
Cloud build CLI: Introduces
--skip-marketing-version-bump/--no-skip-marketing-version-bumponbuild requestand credential save/update flows, mirroring the existing build-number bump flags. The setting is stored asSKIP_MARKETING_VERSION_BUMPin saved credentials and env, excluded from the secrets blob viaNON_CREDENTIAL_KEYS, and sent to the builder asskipMarketingVersionBumpinbuildOptions(default off = bump allowed when the worker supports it).Organizations (RBAC): Replaces
delete_org_member_roleso an org owner may remove only themselves when anotherorg_super_adminexists—orgs.created_byis updated to that successor under row lock before bindings are deleted. Other super admins cannot remove the owner (CANNOT_CHANGE_OWNER_ROLE); last-super-admin guards are unchanged.Tests: Extends payload-split coverage for the new option; adds SQL and Vitest coverage for ownership transfer; minor prescan gate test fetch mock cleanup.
Reviewed by Cursor Bugbot for commit 4a3fad0. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
--skip-marketing-version-bump/--no-skip-marketing-version-bumpto build-related commands.SKIP_MARKETING_VERSION_BUMPwhen loading saved build credentials.Bug Fixes
Tests