Repository navigation
Conversation
There was a problem hiding this comment.
🤖 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.
Coly010
left a comment
There was a problem hiding this comment.
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:
- retarget to
nextand retitle asfix(config)!: ... - 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!
enable_confirmations on config push (CLI-2624)enable_confirmations on config push (CLI-2624)
…le_confirmations-un-negated-as
TL;DR
makes
config pushapply[auth.sms] enable_confirmationsthe waystartand email confirmations dowhats hurting the user?
config pushsentenable_confirmations = trueassms_autoconfirm = true, so phone signups andupdateUser({ phone })skipped the phone codeconfig diffandpullread it back the same wrong way, sodiffstayed cleannow fixed by?
push, diff and pull now send and read
sms_autoconfirmas the opposite ofenable_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"]ref:
BREAKING CHANGE:
[auth.sms] enable_confirmations = false, theinitdefault, now turns phone confirmation off on the nextconfig push, set it totrueto keep the phone code and runconfig diffbefore pushing