Skip to content

fix(config)!: honor sms enable_confirmations on config push (CLI-2624) - #7221

Merged
7ttp merged 2 commits into
nextfrom
7ttp/cli-2624-config-push-sends-authsms-enable_confirmations-un-negated-as
Oct 19, 2026
Merged

7ttp merged 2 commits into
nextfrom
7ttp/cli-2624-config-push-sends-authsms-enable_confirmations-un-negated-as

Conversation

@7ttp

@7ttp 7ttp commented Oct 18, 2026 •

Copy link
Copy Markdown
Member

TL;DR

makes config push apply [auth.sms] enable_confirmations the way start and email confirmations do

whats hurting the user?

config push sent enable_confirmations = true as sms_autoconfirm = true, so phone signups and
updateUser({ phone }) skipped the phone code

config diff and pull read it back the same wrong way, so diff stayed clean

now fixed by?

push, diff and pull now send and read sms_autoconfirm as the opposite of enable_confirmations,
like email already does

visual rep
%%{init: {"flowchart": {"wrappingWidth": 400, "rankSpacing": 60, "nodeSpacing": 30}}}%%
flowchart LR
    cfg["enable_confirmations = true"]
    cfg -- before --> oldBit["sms_autoconfirm = true"] --> oldOut["phone code skipped"]
    cfg -- now --> newBit["sms_autoconfirm = false"] --> newOut["phone code required"]
Loading

ref:

BREAKING CHANGE: [auth.sms] enable_confirmations = false, the init default, now turns phone confirmation off on the next config push, set it to true to keep the phone code and run config diff before pushing

@7ttp 7ttp self-assigned this Oct 18, 2026
@7ttp
7ttp requested a review from a team as a code owner October 18, 2026 23:16

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI Review

The SMS inversion is correct and consistent across push, pull, and local start. Claude's minor migration advisory is confirmed only for explicitly declared false values; omitted keys are never pushed. No additional findings were identified. Tests were not run because dependencies are unavailable.

Findings

Severity Location Category Sources Claim
🟡 MINOR packages/config/src/project-config/registry-auth.ts:785 behavior-change claude For existing projects explicitly declaring auth.sms.enable_confirmations = false while hosted sms_autoconfirm is false, the next approved config push now sets sms_autoconfirm = true, disabling phone-number confirmation. Although this correctly implements the setting, users relying on the previous mapping need a migration note explaining the security-relevant behavior change.

Stats

Claude findings: 1 · Codex findings: 0 · Confirmed: 1 · Refuted: 0 · Uncertain: 0


Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: auto · Workflow run

This review runs once per PR. A maintainer can request another with a /ai-review comment.

Comment thread packages/config/src/project-config/registry-auth.ts

@Coly010 Coly010 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up! I ran it against a staging project and the true case now behaves the way you'd expect

The one thing I think needs to change is where it lands. supabase init writes [auth.sms] enable_confirmations = false, so for a project still on that default whose hosted sms_autoconfirm is false, config diff goes from clean on develop to:

auth.sms.enable_confirmations [update]
  local:  false
  remote: true

and the next config push --yes (pretty common in CI) turns phone confirmations off. That's the correct reading of the config, but it's the same config.toml producing a different hosted result, so I think it falls under the breaking-change rule in AGENTS.md:

  1. retarget to next and retitle as fix(config)!: ...
  2. add a BREAKING CHANGE: footer at the very end of the PR body with the migration note, your existing NOTE text works fine for it

The NOTE in the description is good, but on develop it never makes it into the release notes, so the folk most affected wouldn't see it.

If you think the security side means it should ship on develop sooner, I'm open to that conversation, I'd just rather we didn't flip it quietly for everyone on the init default. Otherwise it looks good to me!

@7ttp 7ttp changed the title fix(config): honor sms enable_confirmations on config push (CLI-2624) fix(config)!: honor sms enable_confirmations on config push (CLI-2624) Oct 19, 2026
@7ttp
7ttp changed the base branch from develop to next October 19, 2026 09:18
@7ttp
7ttp requested a review from Coly010 October 19, 2026 09:21
@7ttp
7ttp added this pull request to the merge queue Oct 19, 2026
Merged via the queue into next with commit fdb5914 Oct 19, 2026
45 checks passed
@7ttp
7ttp deleted the 7ttp/cli-2624-config-push-sends-authsms-enable_confirmations-un-negated-as branch October 19, 2026 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

config push sends [auth.sms] enable_confirmations un-negated as sms_autoconfirm, so hosted projects get the opposite of local

2 participants