[codex] Fix /updates private defaultChannel oracle#1993
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. 📝 WalkthroughWalkthroughPostgres selection for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 7 minutes and 29 seconds. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (2)
tests/updates.test.ts (2)
540-587: ⚡ Quick winAdd a positive regression for private + self-settable + platform-compatible
defaultChannel.The new coverage is strong on deny-cases, but it doesn’t directly lock the allow-case introduced in backend logic. Add one companion test where
public=false,allow_device_self_set=true, platform-compatible, and expect a valid update response (notno_channel).Also applies to: 589-635
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/updates.test.ts` around lines 540 - 587, The test suite only covers the deny-case for a private defaultChannel that is not self-settable; add a positive regression test alongside 'hides private defaultChannel before major upgrade checks' that creates a private channel with allow_device_self_set=true (and platform flags matching the request), sets baseData.defaultChannel to that channel, then calls postUpdate and asserts a successful update response (status 200) with a valid json.version (not 'no_channel') and non-undefined json.old or json.major as appropriate; reuse helpers createAppVersions, getBaseData, postUpdate and the channel construction in the existing test but set allow_device_self_set=true and ensure platform compatibility (ios/android/electron flags) so the backend returns a valid channel instead of 'no_channel'.
540-587: Useit.concurrent()for the changed tests.These modified/added tests are still using
it(...). In this repo, test files should useit.concurrent(...)for parallel execution.♻️ Minimal change pattern
- it('hides private defaultChannel before major upgrade checks', async () => { + it.concurrent('hides private defaultChannel before major upgrade checks', async () => { - it('hides platform-incompatible private defaultChannel before platform checks', async () => { + it.concurrent('hides platform-incompatible private defaultChannel before platform checks', async () => { - it('hides private channel that does not allow self-assignment', async () => { + it.concurrent('hides private channel that does not allow self-assignment', async () => {As per coding guidelines "
tests/**/*.test.ts: ... use it.concurrent() instead of it() to maximize parallelism within test files".Also applies to: 589-635, 966-997
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/updates.test.ts` around lines 540 - 587, Replace the synchronous Mocha/Jest tests with concurrent tests by changing the three occurrences of it( ... ) to it.concurrent( ... ); specifically update the test whose title starts with "hides private defaultChannel before major upgrade checks" and the two other modified/added tests noted in the review (the tests around the other changed blocks) so they call it.concurrent(...) instead of it(...), keeping the test names and bodies unchanged.
🤖 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/updates.test.ts`:
- Around line 623-627: The test relies on getBaseData(APP_NAME_UPDATE)'s
implicit platform; make the platform explicit to ensure the
platform-incompatible path is always exercised by setting the baseData.platform
(or baseData.os/platform field used in the test) to a concrete value like
"android" after obtaining baseData and before using it (alongside the existing
baseData.defaultChannel and version fields); update the test that references
baseData so it asserts incompatibility for that explicit platform.
---
Nitpick comments:
In `@tests/updates.test.ts`:
- Around line 540-587: The test suite only covers the deny-case for a private
defaultChannel that is not self-settable; add a positive regression test
alongside 'hides private defaultChannel before major upgrade checks' that
creates a private channel with allow_device_self_set=true (and platform flags
matching the request), sets baseData.defaultChannel to that channel, then calls
postUpdate and asserts a successful update response (status 200) with a valid
json.version (not 'no_channel') and non-undefined json.old or json.major as
appropriate; reuse helpers createAppVersions, getBaseData, postUpdate and the
channel construction in the existing test but set allow_device_self_set=true and
ensure platform compatibility (ios/android/electron flags) so the backend
returns a valid channel instead of 'no_channel'.
- Around line 540-587: Replace the synchronous Mocha/Jest tests with concurrent
tests by changing the three occurrences of it( ... ) to it.concurrent( ... );
specifically update the test whose title starts with "hides private
defaultChannel before major upgrade checks" and the two other modified/added
tests noted in the review (the tests around the other changed blocks) so they
call it.concurrent(...) instead of it(...), keeping the test names and bodies
unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b2f0e42-7d8d-429f-8fd2-c23ad8dea106
📒 Files selected for processing (2)
supabase/functions/_backend/utils/pg.tstests/updates.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/updates.test.ts (1)
1017-1109: ⚡ Quick winAvoid mutating shared
productionstate in tests; make these cases parallel-safeThese tests toggle shared
productionchannel flags and device overrides, which makes them fragile under parallel execution and conflicts with the test guideline. Prefer dedicated per-test channels/fixtures, then switch toit.concurrent(...)once isolated.As per coding guidelines: "Design all tests for parallel execution across files; use it.concurrent() instead of it() ... create dedicated seed data when tests modify resources or when resource state matters for assertions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/updates.test.ts` around lines 1017 - 1109, These tests mutate the shared "production" channel; instead create a dedicated per-test channel (use a unique name via randomUUID()) and operate on that channel instead of 'production' by calling updateChannel(<uniqueName>, {...}) and using that name when building baseData.defaultChannel and when inserting into channel_devices via supabase; ensure you clean up the channel and any channel_devices in the finally block and then convert the test to it.concurrent(...) once isolated. Use the existing helpers: randomUUID(), updateChannel, getBaseData, postUpdate, APP_NAME_UPDATE and remove any references to the shared 'production' channel to make the tests parallel-safe.
🤖 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/updates.test.ts`:
- Around line 682-686: The private-channel platform-incompatibility test in
tests/updates.test.ts currently asserts json.error, json.version, and json.old
but omits the `major` field; update the test that reads the response into `json:
UpdateRes` to also assert that `major` is not leaked (e.g., add the same
expect(json.major).toBeUndefined() used in the other privacy regression tests)
so metadata disclosure is fully enforced for the platform-incompatible case.
---
Nitpick comments:
In `@tests/updates.test.ts`:
- Around line 1017-1109: These tests mutate the shared "production" channel;
instead create a dedicated per-test channel (use a unique name via randomUUID())
and operate on that channel instead of 'production' by calling
updateChannel(<uniqueName>, {...}) and using that name when building
baseData.defaultChannel and when inserting into channel_devices via supabase;
ensure you clean up the channel and any channel_devices in the finally block and
then convert the test to it.concurrent(...) once isolated. Use the existing
helpers: randomUUID(), updateChannel, getBaseData, postUpdate, APP_NAME_UPDATE
and remove any references to the shared 'production' channel to make the tests
parallel-safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/updates.test.ts (2)
540-687: ⚡ Quick winUse
it.concurrentfor the newly added parallel-safe casesThese three tests create isolated channel names and do not mutate shared seeded channel state, so they should be
it.concurrent(...)per test policy.Suggested change
- it('hides private defaultChannel before major upgrade checks', async () => { + it.concurrent('hides private defaultChannel before major upgrade checks', async () => { - it('allows private self-settable platform-compatible defaultChannel', async () => { + it.concurrent('allows private self-settable platform-compatible defaultChannel', async () => { - it('hides platform-incompatible private defaultChannel before platform checks', async () => { + it.concurrent('hides platform-incompatible private defaultChannel before platform checks', async () => {As per coding guidelines "Design all tests for parallel execution across files; use it.concurrent() instead of it() to maximize parallelism within test files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/updates.test.ts` around lines 540 - 687, Replace the three standard Jest tests that use it(...) with parallel-safe it.concurrent(...): specifically change the tests with titles "hides private defaultChannel before major upgrade checks", "allows private self-settable platform-compatible defaultChannel", and "hides platform-incompatible private defaultChannel before platform checks" to use it.concurrent(...) so they can run in parallel; no other logic needs changing—just update the test declarations for those three it blocks to it.concurrent.
1018-1049: ⚡ Quick winAvoid mutating shared
productionchannel state in this new testThis test flips shared
productionflags, which makes parallel execution harder and increases inter-test coupling. Prefer seeding a dedicated private channel for this scenario (like the other new oracle-regression tests), then target it viadefaultChannel.As per coding guidelines "Create dedicated seed data when tests modify resources or when resource state matters for assertions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/updates.test.ts` around lines 1018 - 1049, The test currently mutates the shared 'production' channel via updateChannel, which breaks isolation; instead create and use a dedicated seeded private channel (e.g., a new channel name like 'private_no_self_set') for this scenario: seed or create the channel with public: false and allow_device_self_set: false, call postUpdate with baseData.defaultChannel set to that new channel name, and remove any updateChannel calls that change 'production'; ensure any created test channel is cleaned up after the test. Use the existing helpers mentioned (updateChannel, postUpdate, getBaseData, APP_NAME_UPDATE, defaultChannel) to locate where to switch from mutating 'production' to a dedicated seeded channel.
🤖 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/updates.test.ts`:
- Around line 1040-1042: The test currently asserts only that json.error is
'no_channel' and json.version is undefined; add assertions to ensure all
metadata fields are hidden by also asserting json.old and json.major are
undefined (mirroring the other "new hidden-channel" tests) so the test enforces
non-disclosure of all metadata fields when a channel is hidden.
---
Nitpick comments:
In `@tests/updates.test.ts`:
- Around line 540-687: Replace the three standard Jest tests that use it(...)
with parallel-safe it.concurrent(...): specifically change the tests with titles
"hides private defaultChannel before major upgrade checks", "allows private
self-settable platform-compatible defaultChannel", and "hides
platform-incompatible private defaultChannel before platform checks" to use
it.concurrent(...) so they can run in parallel; no other logic needs
changing—just update the test declarations for those three it blocks to
it.concurrent.
- Around line 1018-1049: The test currently mutates the shared 'production'
channel via updateChannel, which breaks isolation; instead create and use a
dedicated seeded private channel (e.g., a new channel name like
'private_no_self_set') for this scenario: seed or create the channel with
public: false and allow_device_self_set: false, call postUpdate with
baseData.defaultChannel set to that new channel name, and remove any
updateChannel calls that change 'production'; ensure any created test channel is
cleaned up after the test. Use the existing helpers mentioned (updateChannel,
postUpdate, getBaseData, APP_NAME_UPDATE, defaultChannel) to locate where to
switch from mutating 'production' to a dedicated seeded channel.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
1 participant
Summary (AI generated)
/updatesnamed channel resolution to platform-compatible public or self-settable channels.defaultChannelprobes returningno_channelwithout version metadata.Motivation (AI generated)
GHSA-pgmr-gw53-7f77 reported that unauthenticated callers could resolve private channel names through
defaultChannelbefore privacy and platform checks ran.Business Impact (AI generated)
This protects private rollout metadata from unauthenticated enumeration while preserving valid public and self-settable channel update flows.
Test Plan (AI generated)
bun run lint:backendbunx eslint tests/updates.test.tsbun run test:updatesbun run cli:build && vue-tsc --noEmitGenerated with AI
Summary by CodeRabbit
Bug Fixes
Tests