Repository navigation
Migrate legacy signed-out device identifiers - #360
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/convex/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/*/test/**/*.ts⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
📝 WalkthroughSummary
Affected areas
Migration and operations
Validation
WalkthroughThis change migrates legacy Trusted Device identifiers, records per-account completion, and gates reconnection and revocation until migration completes. New accounts start complete, while tombstoned accounts remain fail-closed. ChangesTrusted-device migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Operator
participant migrateLegacyTrustedDeviceIdentifiers
participant ProductAccount
participant TrustedDeviceRegistration
Operator->>migrateLegacyTrustedDeviceIdentifiers: Import retained legacy identifiers
migrateLegacyTrustedDeviceIdentifiers->>ProductAccount: Store identifier history and completion marker
TrustedDeviceRegistration->>ProductAccount: Check identifier and migration state
ProductAccount-->>TrustedDeviceRegistration: Allow migrated identifier or reject unseen identifier
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
|
|
@codex review |
![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: 4ad737908c
ℹ️ 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".
# Conflicts: # docs/adr/0020-revoke-devices-with-sync-key-rotation.md
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/convex/test/productAccount.test.ts`:
- Around line 1760-1782: Add two validation tests near the existing legacy
identifier migration tests: one invoking migrateLegacyTrustedDeviceIdentifiers
with 101 identifiers and asserting the batch-size error, and one invoking it
with an unknown tokenIdentifier and asserting the Product Account required
error. Follow the existing convexTest/schema setup and assertion conventions.
- Around line 186-193: Refactor expectLegacyIdentifierMigration to accept one
scenario object containing existingRevocationTombstone, migrationComplete, and
completeAfterFirstBatch, making the migration contract explicit. In the helper
body, read scenario.existingRevocationTombstone and scenario.migrationComplete,
and invoke scenario.completeAfterFirstBatch only when migrationComplete is
false; update both callers accordingly.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a04a7f1-7afc-4b88-86f1-6e99c94e31bc
📒 Files selected for processing (6)
.changeset/migrate-legacy-trusted-device-identifiers.mdCONTEXT.mddocs/adr/0020-revoke-devices-with-sync-key-rotation.mdpackages/convex/convex/productAccount.tspackages/convex/convex/schema.tspackages/convex/test/productAccount.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
.changeset/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Create a Changeset for runtime behavior changes or exported type/API changes; tests-only, internal refactors, documentation-only changes, and JSDoc-only maintenance may omit one at maintainer discretion.
Files:
.changeset/migrate-legacy-trusted-device-identifiers.md
packages/convex/convex/**/*.ts
📄 CodeRabbit inference engine (packages/convex/CLAUDE.md)
For work on Convex code, always read
convex/_generated/ai/guidelines.mdfirst; its rules override general Convex knowledge when using Convex APIs and patterns.For Convex code, always read
convex/_generated/ai/guidelines.mdfirst; its rules override general Convex knowledge when using Convex APIs and patterns.
Files:
packages/convex/convex/schema.tspackages/convex/convex/productAccount.ts
⚙️ CodeRabbit configuration file
packages/convex/convex/**/*.ts: Treat authentication, authorization, input validation, and privacy
boundaries as mandatory. Flag public functions that do not validate
arguments and returns, data access that is not scoped to the authenticated
Product Account or trusted device, plaintext sensitive data, secrets in
logs, unbounded reads, and queries that bypass appropriate indexes.
Changes to runtime behavior should include focused Convex tests covering
success, unauthorized access, malformed input, and relevant retries.
Files:
packages/convex/convex/schema.tspackages/convex/convex/productAccount.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use Oxlint with@rajzik/oxlint-configfor TypeScript linting, Oxfmt with@rajzik/oxfmt-configfor formatting, and extend@rajzik/tsconfigfor TypeScript configuration.
Use existing abstractions and conventions before introducing new ones; prefer the minimum code that solves the requested problem and avoid speculative features, single-use abstractions, and unrequested configurability.
When editing TypeScript, make surgical changes: do not refactor unrelated code or formatting, match existing style, and remove only imports, variables, or functions made unused by the change.
Use the mise-managed repository toolchain for TypeScript validation; trust.mise.tomland runmise install, or invoke commands throughmise exec --when mise is not activated.
Files:
packages/convex/convex/schema.tspackages/convex/test/productAccount.test.tspackages/convex/convex/productAccount.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
For ad hoc runnable TypeScript, use a temporary file under
scratchpad/, run it withnode scratchpad/<file>.ts, and delete it afterward; use Node 24 directly instead oftsxunless Node fails.
Files:
packages/convex/convex/schema.tspackages/convex/test/productAccount.test.tspackages/convex/convex/productAccount.ts
packages/convex/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Treat
packages/convexas the backend package and follow the repository TypeScript tooling and validation requirements for its Convex code.
Files:
packages/convex/convex/schema.tspackages/convex/test/productAccount.test.tspackages/convex/convex/productAccount.ts
packages/*/test/**/*.ts
⚙️ CodeRabbit configuration file
packages/*/test/**/*.ts: Verify externally observable behavior, authorization and privacy
invariants, malformed inputs, and deterministic isolation. Flag tests that
duplicate the implementation or omit meaningful negative cases.
Files:
packages/convex/test/productAccount.test.ts
CONTEXT.md
📄 CodeRabbit inference engine (AGENTS.md)
Update
CONTEXT.mdwhen product language, domain terms, or resolved ambiguity changes.
Files:
CONTEXT.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Treat docs as product truth, not marketing. Prefer precise domain language
from CONTEXT.md; flag undefined terms, synonym drift, and conflicts with
existing ADRs under docs/adr/. For ADR changes, require a clear decision,
consequences, and privacy or security boundary impact; flag silent
reversals of prior ADRs without an explicit supersession note. For
docs/agents/ and bootstrap docs, verify agent workflow, validation, and
triage guidance stay aligned with AGENTS.md. Flag broken cross-links,
speculative future claims presented as shipped behavior, and docs that
omit the Apple vs TypeScript ownership of a boundary they describe.
Files:
CONTEXT.mddocs/adr/0020-revoke-devices-with-sync-key-rotation.md
docs/adr/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Record durable architecture decisions and privacy-boundary decisions in
docs/adr/.
Files:
docs/adr/0020-revoke-devices-with-sync-key-rotation.md
🪛 markdownlint-cli2 (0.23.2)
.changeset/migrate-legacy-trusted-device-identifiers.md
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (11)
packages/convex/convex/schema.ts (1)
9-9: LGTM!packages/convex/convex/productAccount.ts (6)
16-16: LGTM!Also applies to: 35-35, 54-58
297-297: LGTM!
318-356: LGTM!
358-425: LGTM!
514-532: LGTM!
1189-1193: LGTM!docs/adr/0020-revoke-devices-with-sync-key-rotation.md (1)
5-6: LGTM!CONTEXT.md (1)
678-679: LGTM!.changeset/migrate-legacy-trusted-device-identifiers.md (1)
1-5: LGTM!packages/convex/test/productAccount.test.ts (1)
1745-1757: 📐 Maintainability & Code QualityNo change required
trustedDeviceCredentialis optional. Both devices are connected withoutsupportsDeviceCredentials, so the omitted credential does not block authentication. The call reaches the migration check.> Likely an incorrect or invalid review comment.
1 participant
Summary
Why
Legacy sign-out deleted the only backend row containing an installation identifier. Once an account had its first revocation tombstone, the backend could not distinguish that previously known installation from a newly minted identifier. Guessing or relaxing the lock would weaken revocation; enforcing it without migration would permanently reject legitimate reconnects.
Impact
Operators must import retained pre-enforcement Trusted Device exports before completing migration for an existing Product Account. Existing accounts cannot begin another revocation until completion, and completed migrations cannot admit additional identifiers. New Product Accounts are complete at creation.
Validation
mise exec -- pnpm lintmise exec -- pnpm formatmise exec -- pnpm turbo run check-typesmise exec -- pnpm test(185 tests passed)mise exec -- pnpm fallow(zero issues)git diff --checkCloses #271
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.