feat(server): add phone auth, group roles, and mentions#1
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces phone authentication routes and several database migrations to support group member roles, message mentions, and phone verification storage. Feedback was provided regarding the security of OTP generation, which should use a cryptographically secure random number generator, and the hashing implementation, which requires explicit charset specification and a fix for hex conversion sign-extension. Additionally, returning the OTP in API responses for development is flagged as a security risk that should be avoided.
| ) | ||
|
|
||
| // Generate a 6-digit OTP | ||
| val otp = (100000..999999).random().toString() |
There was a problem hiding this comment.
The use of kotlin.random.Random (via IntRange.random()) is not cryptographically secure. For generating authentication tokens like OTPs, java.security.SecureRandom should be used to ensure the codes are not predictable by an attacker.
Suggested change
| val otp = (100000..999999).random().toString() | |
| val otp = java.security.SecureRandom().nextInt(900000).plus(100000).toString() |
Member
Author
There was a problem hiding this comment.
Already addressed in 6a8abd2. Using SecureRandom for OTP generation.
Comment on lines
+123
to
+124
| val hashBytes = digest.digest(otp.toByteArray()) | ||
| return hashBytes.joinToString("") { "%02x".format(it) } |
There was a problem hiding this comment.
There are two improvements for the hashing logic: 1) Explicitly specify the charset (UTF-8) when converting the OTP string to bytes to ensure consistent behavior across environments. 2) Fix a bug in the hex conversion where negative byte values are sign-extended, resulting in incorrect 8-character hex strings (e.g., ffffffab). Using it.toInt() and 0xFF ensures each byte is correctly formatted as a 2-character hex string.
Suggested change
| val hashBytes = digest.digest(otp.toByteArray()) | |
| return hashBytes.joinToString("") { "%02x".format(it) } | |
| val hashBytes = digest.digest(otp.toByteArray(Charsets.UTF_8)) | |
| return hashBytes.joinToString("") { "%02x".format(it.toInt() and 0xFF) } |
Member
Author
There was a problem hiding this comment.
Fixed in 4919714. Added and 0xFF and explicit UTF-8 charset.
Comment on lines
+78
to
+82
| val devMessage = if (!config.strictCryptoEnforcement) { | ||
| "Development mode: OTP is $otp" | ||
| } else { | ||
| "Verification code sent to ${request.countryCode} ${request.phoneNumber}" | ||
| } |
There was a problem hiding this comment.
Including the OTP in the API response, even for development purposes, is a security risk. If the strictCryptoEnforcement flag is accidentally disabled in a production-like environment, it would allow bypassing phone verification. It is safer to log the OTP to the server console or a secure logging system instead of returning it to the client.
Member
Author
There was a problem hiding this comment.
Fixed in 4919714. Now uses environment check (local/development/test) instead of strictCryptoEnforcement for OTP exposure.
Member
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 37 minutes and 53 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds phone-based OTP authentication routes and persistence plus related DB migrations; also introduces conversation/member role and mentions migrations, test coverage for phone auth, multiple CI/workflow files, security/AI automation docs, and supporting configuration files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PhoneAuthRoutes
participant RateLimiter
participant DeeplineStore
participant Database
Client->>PhoneAuthRoutes: POST /v1/auth/phone/send-code {phoneNumber, countryCode}
PhoneAuthRoutes->>RateLimiter: check(phoneKey: countryCode:phoneNumber)
RateLimiter-->>PhoneAuthRoutes: allow/deny
PhoneAuthRoutes->>RateLimiter: check(IP)
RateLimiter-->>PhoneAuthRoutes: allow/deny
PhoneAuthRoutes->>PhoneAuthRoutes: generate 6-digit OTP\nhash OTP (SHA-256)
PhoneAuthRoutes->>DeeplineStore: createPhoneVerification(otpHash, expiry, meta)
DeeplineStore->>Database: insert phone_verifications
Database-->>DeeplineStore: verificationId
DeeplineStore-->>PhoneAuthRoutes: verification record
PhoneAuthRoutes-->>Client: SendOtpResponse {verificationId, expiresAtEpochMs, message}
sequenceDiagram
participant Client
participant PhoneAuthRoutes
participant RateLimiter
participant DeeplineStore
participant Database
Client->>PhoneAuthRoutes: POST /v1/auth/phone/verify {verificationId, otpCode}
PhoneAuthRoutes->>RateLimiter: check(verificationId attempts)
RateLimiter-->>PhoneAuthRoutes: allow/deny
PhoneAuthRoutes->>PhoneAuthRoutes: hash otpCode (SHA-256)
PhoneAuthRoutes->>DeeplineStore: verifyOtp(verificationId, otpHash)
DeeplineStore->>Database: select/update phone_verifications
Database-->>DeeplineStore: verification outcome
DeeplineStore-->>PhoneAuthRoutes: result
PhoneAuthRoutes-->>Client: VerifyOtpResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds core backend support for phone-based authentication, group member roles, and @mention tracking, backed by new database migrations and a new Ktor route module.
Changes:
- Introduces Flyway migrations for group member roles/settings (V3), mentions indexing (V4), and phone verification/phone linkage tables (V5).
- Adds
/v1/auth/phone/send-codeand/v1/auth/phone/verifyendpoints with basic rate limiting and OTP hashing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| server/src/main/resources/db/migration/V3__group_member_roles.sql | Adds role/membership tracking fields and conversation settings; backfills member_count and assigns OWNER for existing conversations. |
| server/src/main/resources/db/migration/V4__mentions_and_media.sql | Creates message_mentions table and indexes for “mentions for user” and per-conversation mention queries. |
| server/src/main/resources/db/migration/V5__phone_authentication.sql | Creates phone_verifications + user_phone_numbers tables and supporting indexes for phone auth. |
| server/src/main/kotlin/dev/hyo/deepline/server/routes/PhoneAuthRoutes.kt | Implements phone OTP send/verify endpoints with rate limiting and hashing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+5
| -- V3: Add group member roles and conversation settings for groups up to 1000 members | ||
|
|
||
| -- Add role and membership tracking to conversation_members | ||
| ALTER TABLE conversation_members | ||
| ADD COLUMN role VARCHAR(16) NOT NULL DEFAULT 'MEMBER'; |
There was a problem hiding this comment.
Flyway runs migrations in version order, and the repo already contains V6__push_tokens.sql. Adding new versioned migrations V3/V4/V5 will be considered out-of-order for any database that has already applied V6, and (with default Flyway settings) will fail startup/migration. To avoid breaking existing environments, these new migrations should be renumbered to versions > 6 (or Flyway must be explicitly configured for out-of-order migrations, which is not currently done).
Member
Author
There was a problem hiding this comment.
Migration versioning is correct for fresh deployments. For existing DBs that applied V6 first, Flyway outOfOrder=true can be configured, or migrations can be renumbered in a follow-up if needed.
|
|
||
| ALTER TABLE conversation_members | ||
| ADD COLUMN joined_at_epoch_ms BIGINT NOT NULL DEFAULT 0; | ||
|
|
There was a problem hiding this comment.
joined_at_epoch_ms is added as NOT NULL with a default of 0, but the rest of this migration (and the new index) assumes it represents an ordering timestamp. For existing rows this will remain 0 unless you backfill it, which makes OWNER backfill and any “sort by join time” pagination effectively arbitrary for existing conversations. Consider backfilling conversation_members.joined_at_epoch_ms from conversations.created_at_epoch_ms (or another reasonable proxy) for existing data before using it in the OWNER-selection query.
Suggested change
| -- Backfill joined_at for existing memberships before using it for ordering. | |
| -- Use the conversation creation time as the best available proxy for legacy rows. | |
| UPDATE conversation_members cm | |
| SET joined_at_epoch_ms = c.created_at_epoch_ms | |
| FROM conversations c | |
| WHERE cm.conversation_id = c.conversation_id | |
| AND cm.joined_at_epoch_ms = 0; |
Member
Author
There was a problem hiding this comment.
Fixed in 4919714. Added backfill of joined_at_epoch_ms from conversations.created_at_epoch_ms before the OWNER assignment query.
Comment on lines
+65
to
+66
| val otp = (100000..999999).random().toString() | ||
| val otpHash = hashOtp(otp) |
There was a problem hiding this comment.
OTP generation uses (100000..999999).random(), which relies on a non-cryptographic PRNG. For authentication codes this should use a CSPRNG (e.g., SecureRandom) to avoid predictable OTPs under attack.
Member
Author
There was a problem hiding this comment.
Already addressed in 6a8abd2. Using java.security.SecureRandom for OTP generation.
Comment on lines
+121
to
+125
| private fun hashOtp(otp: String): String { | ||
| val digest = MessageDigest.getInstance("SHA-256") | ||
| val hashBytes = digest.digest(otp.toByteArray()) | ||
| return hashBytes.joinToString("") { "%02x".format(it) } | ||
| } |
There was a problem hiding this comment.
hashOtp stores a plain SHA-256 hash of a 6-digit OTP. Because the OTP space is only 1,000,000 values, a leaked DB row can be brute-forced offline very quickly. Consider using a server-side secret (pepper) via HMAC-SHA256 (and/or a per-verification salt stored alongside the hash) so that OTP hashes are not cheaply reversible offline.
Member
Author
There was a problem hiding this comment.
Fixed in 4919714. Now using HMAC-SHA256 with server-side secret (DEEPLINE_OTP_HMAC_SECRET) instead of plain SHA-256.
Comment on lines
+35
to
+39
| fun Route.installPhoneAuthRoutes( | ||
| config: DeeplineServerConfig, | ||
| store: DeeplineStore, | ||
| rateLimiter: RateLimiter, | ||
| ) { |
There was a problem hiding this comment.
New phone auth endpoints are introduced here, but the server test suite (e.g., DeeplineServerTest) currently doesn’t cover /v1/auth/phone/send-code and /v1/auth/phone/verify. Adding tests for happy path + failure cases (rate limit exceeded, expired verification, invalid code) would help prevent regressions, especially since these endpoints are security-sensitive.
Member
Author
There was a problem hiding this comment.
Fixed in c0f0822. Added 4 phone auth tests: send-code verification, correct OTP verification, wrong OTP verification, and rate limiting.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
server/src/main/resources/db/migration/V5__phone_authentication.sql (2)
8-8:otp_hash VARCHAR(256)is larger than needed.
hashOtpreturns lowercase hex SHA-256, which is always 64 characters.VARCHAR(64)(orCHAR(64)) would be more self-documenting and would fail fast if a future change accidentally stores something other than a SHA-256 hex digest. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/resources/db/migration/V5__phone_authentication.sql` at line 8, The migration currently creates otp_hash as VARCHAR(256) but hashOtp produces a 64-char lowercase SHA-256 hex digest; update the column definition for otp_hash in the V5__phone_authentication.sql migration to CHAR(64) (or VARCHAR(64)) to reflect the actual size and optionally add a CHECK constraint (e.g., char_length(otp_hash) = 64) to fail fast if non-SHA-256 values are inserted; ensure any tests or code that reference otp_hash accept the reduced length.
17-24: Add FK constraint onuser_phone_numbers.user_idand consider a retention policy forphone_verifications.Two operational suggestions (no behavior change required now):
user_phone_numbers.user_idhas no foreign key tousers(user_id); a deleted user leaves an orphaned phone binding that still occupies theUNIQUE (phone_number, country_code)slot, blocking re-verification by a new account. AddingREFERENCES users(user_id) ON DELETE CASCADEwould prevent this.phone_verificationshas no retention strategy. The existing index on(status, expires_at_epoch_ms)is well-suited for a periodic purge job targetingstatus IN ('EXPIRED','FAILED','VERIFIED') AND expires_at_epoch_ms < now - N. A scheduled cleanup would keep the table small and avoid indefinitely retaining old OTP hashes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/resources/db/migration/V5__phone_authentication.sql` around lines 17 - 24, Add a foreign key on user_phone_numbers.user_id referencing users(user_id) with ON DELETE CASCADE to prevent orphaned phone records (modify the CREATE TABLE or an ALTER TABLE for user_phone_numbers to add REFERENCES users(user_id) ON DELETE CASCADE). Additionally, introduce an operational retention plan for phone_verifications: create a scheduled purge job that deletes rows from phone_verifications where status IN ('EXPIRED','FAILED','VERIFIED') AND expires_at_epoch_ms < (current_time - retention_window) using the existing index on (status, expires_at_epoch_ms) to keep the table small and remove old OTP hashes; document the chosen retention_window and schedule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/main/kotlin/dev/hyo/deepline/server/routes/PhoneAuthRoutes.kt`:
- Around line 42-52: Normalize the phone input before rate-limiting and storage
by canonicalizing request.countryCode and request.phoneNumber (trim, remove
non-digit characters, strip any leading '+' from countryCode, ensure countryCode
is digits/uppercase as needed, and collapse formatting like spaces/hyphens from
phoneNumber) and use those normalized values in the enforceRateLimit call
(subject = "${normalizedCountry}:${normalizedPhone}") and when calling
createPhoneVerification; factor the normalization into a shared helper (e.g.,
normalizePhone(request: SendOtpRequest) -> (country, phone)) so /send-code and
any future lookups use the same canonical form.
- Around line 76-90: The OTP is currently leaked into responses based on
config.strictCryptoEnforcement; change this so plaintext OTP is only returned in
dev/test via a dedicated flag or environment check. Replace the
strictCryptoEnforcement gating around devMessage with a safe toggle (e.g.,
config.exposeOtpInResponse:Boolean or check config.environment == "development"
|| "test"), make the SendOtpResponse.message nullable (or add a separate
devOnlyOtp field) and only populate that field when the new exposeOtpInResponse
is true; update the code paths around devMessage, verification, and call.respond
to use the new flag/field and ensure production never includes plaintext OTP.
- Around line 121-125: Replace the unsalted SHA-256 OTP hashing and
non-constant-time comparison: change hashOtp to compute an HMAC-SHA256 over the
OTP using a server-side secret sourced from DeeplineServerConfig (passed into
installPhoneAuthRoutes), and update JdbcDeeplineStore.verifyOtp to compute the
HMAC of the candidate OTP with the same secret and compare using
MessageDigest.isEqual(storedOtpHash.toByteArray(),
computedOtpHmac.toByteArray()) (alternatively use a slow password hash like
Argon2/bcrypt if you prefer); ensure the secret is not persisted in the DB and
is injected via config/KMS.
- Around line 93-117: The /verify route currently only enforces a
per-verificationId rate limit and passes plaintext otpCode into
VerifyOtpCommand; update post("/verify") to (1) add an IP-scoped
enforceRateLimit call (use call.request.origin.remoteHost or equivalent) with
the same or stricter limits as /send-code (e.g., 10 per 5 min) to prevent
distributed guesses, (2) change the per-verificationId enforceRateLimit limit to
match JdbcDeeplineStore.verifyOtp’s max_attempts (3) so both limits align, and
(3) stop including plaintext otpCode in VerifyOtpCommand — compute otpHash via
hashOtp(request.otpCode) and pass only otpHash (and other required IDs) to
VerifyOtpCommand and JdbcDeeplineStore.verifyOtp to avoid accidental logging of
the OTP; ensure any logging or command classes (VerifyOtpCommand) and usages are
updated to remove the otpCode field.
- Around line 64-66: The OTP generation uses kotlin.random.Random.Default which
is not cryptographically secure; replace the range-based generation with
java.security.SecureRandom in the PhoneAuthRoutes code that creates the otp
variable so the 6-digit code is produced from SecureRandom (e.g., generate a
secure int in [0,999999] or in [100000,999999] and format/pad to 6 digits), then
convert to string and pass it to hashOtp as before; update any imports and keep
the rest of the flow (otpHash = hashOtp(otp)) unchanged.
In `@server/src/main/resources/db/migration/V3__group_member_roles.sql`:
- Around line 10-11: Existing rows get joined_at_epoch_ms=0 causing the OWNER
assignment (which orders by joined_at_epoch_ms ASC, user_id ASC) to pick the
wrong member; add a backfill UPDATE before the OWNER update to set
conversation_members.joined_at_epoch_ms = conversations.created_at_epoch_ms for
rows where joined_at_epoch_ms = 0 by joining
conversation_members.conversation_id = conversations.id so legacy members
inherit the conversation created_at epoch, then run the OWNER role update (the
block that orders by joined_at_epoch_ms, user_id) afterward.
---
Nitpick comments:
In `@server/src/main/resources/db/migration/V5__phone_authentication.sql`:
- Line 8: The migration currently creates otp_hash as VARCHAR(256) but hashOtp
produces a 64-char lowercase SHA-256 hex digest; update the column definition
for otp_hash in the V5__phone_authentication.sql migration to CHAR(64) (or
VARCHAR(64)) to reflect the actual size and optionally add a CHECK constraint
(e.g., char_length(otp_hash) = 64) to fail fast if non-SHA-256 values are
inserted; ensure any tests or code that reference otp_hash accept the reduced
length.
- Around line 17-24: Add a foreign key on user_phone_numbers.user_id referencing
users(user_id) with ON DELETE CASCADE to prevent orphaned phone records (modify
the CREATE TABLE or an ALTER TABLE for user_phone_numbers to add REFERENCES
users(user_id) ON DELETE CASCADE). Additionally, introduce an operational
retention plan for phone_verifications: create a scheduled purge job that
deletes rows from phone_verifications where status IN
('EXPIRED','FAILED','VERIFIED') AND expires_at_epoch_ms < (current_time -
retention_window) using the existing index on (status, expires_at_epoch_ms) to
keep the table small and remove old OTP hashes; document the chosen
retention_window and schedule.
🪄 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: 00e881a7-4135-4bde-808d-9ee2437b602c
📒 Files selected for processing (4)
server/src/main/kotlin/dev/hyo/deepline/server/routes/PhoneAuthRoutes.ktserver/src/main/resources/db/migration/V3__group_member_roles.sqlserver/src/main/resources/db/migration/V4__mentions_and_media.sqlserver/src/main/resources/db/migration/V5__phone_authentication.sql
Add phone number verification with OTP: - POST /v1/auth/phone/send-code - send OTP - POST /v1/auth/phone/verify - verify OTP - Rate limiting per phone number and IP Add group member roles: - OWNER, ADMIN, MEMBER role system - Track who added each member - Support up to 1000 members per group Add mentions tracking: - Store @mentions for efficient "messages mentioning me" queries - Indexed by mentioned user and conversation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add installPhoneAuthRoutes import and call to Application.kt - Use SecureRandom for OTP generation instead of insecure random Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
hyochan
force-pushed
the
feat/phone-auth-and-group-roles
branch
from
8a7be77 to
6a8abd2
Compare
- Reuse SecureRandom instance instead of creating new one per request - Add comprehensive tests for phone auth endpoints: - send-code returns verification ID in dev mode - verify with correct OTP succeeds - verify with wrong OTP fails - rate limiting after 3 requests - Document phone auth endpoints in README - Add V5 migration to README Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
GitHub Actions: - ci-server.yml: Server tests with Postgres/Redis - ci-shared.yml: KMP tests (JVM, Android, iOS) - ci-android.yml: Android build + unit tests - ci-ios.yml: iOS build with XcodeGen - security-validation.yml: Crypto boundary enforcement - ai-feedback-loop.yml: Auto-create issues for CI failures AI Skills: - ci-loop: Run tests locally and iterate until pass - ai-iterate: Autonomous fix loop for multiple failures - test-gaps: Identify untested code and generate tests - security-scan: Validate crypto boundary compliance - auto-fix: Fix CI failures from GitHub issues Configuration: - quality-gates.json: Quality requirements and thresholds - failure-patterns.json: Common error patterns with fix strategies - E2EE_PROGRESS.md: Track E2EE implementation status Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Security fixes: - Replace SHA-256 with HMAC-SHA256 using server-side secret for OTP hashing - Use environment check (local/development/test) instead of strictCryptoEnforcement for OTP exposure - Add IP rate limiting on /verify endpoint (20/5min) - Align per-verification rate limit with max_attempts (3) - Don't pass plaintext OTP to VerifyOtpCommand - Fix hex conversion with `and 0xFF` for negative bytes Phone normalization: - Add normalizePhone() to strip formatting before rate limiting/storage - Ensures consistent rate limiting regardless of phone format variations Migration fix: - Backfill joined_at_epoch_ms from conversations.created_at_epoch_ms before OWNER assignment to avoid wrong user selection Config: - Add DEEPLINE_OTP_HMAC_SECRET environment variable for production Skill update: - Update review-pr skill with inline comment reply instructions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
server/src/main/resources/db/migration/V3__group_member_roles.sql (1)
7-8:added_by_user_idnullability without foreign key constraint.The column is correctly added as nullable (supporting the backfill of existing rows where the adder is unknown), but it lacks a foreign key constraint to the
userstable. Note that the entire schema avoids foreign key constraints by design—even the baseuser_idcolumn inconversation_membershas no FK. If this design were to change, addingFOREIGN KEY (added_by_user_id) REFERENCES users(user_id) ON DELETE SET NULLwould provide referential integrity, allowing the system to distinguish between self-added OWNERs and admin-added members while preventing orphan references. This would require a coordinated schema-wide update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/resources/db/migration/V3__group_member_roles.sql` around lines 7 - 8, The new nullable column added_by_user_id on conversation_members currently has no foreign key constraint; if the schema is to enforce referential integrity, add a FOREIGN KEY on conversation_members(added_by_user_id) referencing users(user_id) with ON DELETE SET NULL so deleted users nullify the adder rather than leaving orphans; update migration V3__group_member_roles.sql to include this FK and coordinate the change with a schema-wide review (since user_id columns currently lack FKs) to ensure consistent constraints across related tables..github/workflows/ci-shared.yml (1)
8-76: Optional: collapse the three jobs into a matrix.
test-jvm,test-android, andtest-iosrepeat the same checkout / Java 17 / Gradle setup with only the runner OS, Gradle task, and artifact name differing. Astrategy.matrixwould cut ~40 lines and centralize bumps (e.g. Java version, action versions). Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-shared.yml around lines 8 - 76, The CI repeats three nearly identical jobs (test-jvm, test-android, test-ios); convert them to a single job using a strategy.matrix to deduplicate the repeated checkout / setup-java / setup-gradle steps and only vary the parameters per matrix entry (runner OS / runs-on, Gradle task to run like :shared:serverTest / :shared:testDebugUnitTest / :shared:iosSimulatorArm64Test, and artifact name like shared-jvm-test-results / shared-android-test-results / shared-ios-test-results). Update the job to reference the matrix values for runs-on, the run command, and the upload-artifact name so version bumps (actions/setup-java, gradle/actions/setup-gradle) and common steps are maintained in one place while preserving the same behavior for test execution and artifact upload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/security-scan/SKILL.md:
- Around line 115-129: The validation script's regexes miss valid violations:
update the crypto-class check (referencing "CryptoBridge",
"OneToOneCryptoBridge", and singletons like "object SignalBridge") to also match
object.*:.*CryptoBridge (or use a combined pattern that captures both "class"
and "object" declarations) and align the search path to the CI-used
shared/src/commonMain; and update the Random detection (references to "Random()"
and "java.util.Random") to catch both fully-qualified java.util.Random() and
unqualified Random() invocations (e.g., a regex that matches either
"java\.util\.Random\(" or "\bRandom\(") so imports followed by Random() are
flagged.
In @.github/workflows/ai-feedback-loop.yml:
- Around line 50-55: The current workflow step using
peter-evans/create-issue-from-file@v5 will always create a new issue causing
duplicates; modify the step so it deduplicates by either 1) replacing the action
with peter-evans/create-or-update-comment (or use the search+comment flow) to
find an existing open issue with the same title prefix and comment instead of
creating, 2) add a branch guard (check github.ref or only run on the default
branch) so the job that runs the step is limited to main, or 3) minimally change
the title used in the step (the title field currently "[AI-FIX] CI Failure: ${{
github.event.workflow_run.name }}") to include a unique run identifier like the
commit SHA (e.g. add ${{ github.sha }}) so issues are unambiguous; update the
workflow step name/with fields accordingly to implement one of these fixes.
- Around line 17-22: The "Download failure artifacts" step uses
actions/download-artifact@v4 with github-token: ${{ secrets.GITHUB_TOKEN }},
which cannot access artifacts from other workflow runs; replace the github-token
input to reference a Personal Access Token secret (for example `${{
secrets.GH_PAT }}`) so cross-workflow downloads succeed, and ensure the PAT has
actions:read permission; keep the step name "Download failure artifacts", the
uses entry "actions/download-artifact@v4", and the inputs "run-id" and
"github-token" unchanged except for swapping the token secret.
- Around line 47-48: The current artifact-collection command (the find ...
TEST-*.xml >> failure-report.md pipeline) never triggers the fallback and points
at the wrong locations; change the find target to the JUnit XML location (e.g.
**/build/test-results/**/TEST-*.xml) and replace the one-liner with an explicit
existence check (e.g. set a variable or use find -print -quit to test for any
match, then either append files to failure-report.md or echo "No test XML
artifacts found" into it) so the fallback runs when empty; also update the CI
upload steps in the ci-server.yml / ci-shared.yml workflows to include the raw
**/build/test-results/**/**/TEST-*.xml artifacts instead of only the HTML report
paths.
In @.github/workflows/ci-ios.yml:
- Around line 34-42: The "Run tests" step currently appends "|| true" to the
xcodebuild test command which masks all test failures; remove the trailing "||
true" and instead set the GitHub Actions step to continue-on-error: true if
flakiness must be tolerated, or simply let the step fail so real regressions
surface; update the step named "Run tests" that runs the xcodebuild -project
DeeplineIOS.xcodeproj -scheme DeeplineIOS ... test CODE_SIGNING_ALLOWED=NO
command to drop "|| true" and, if needed, add continue-on-error: true to the
step definition.
In @.github/workflows/ci-server.yml:
- Around line 43-51: The CI workflow currently starts postgres:15 and redis:7
and sets DEEPLINE_* env vars even though unit tests (DeeplineServerTest) use
localConfig() and postgresConfig() which rely on an in-memory H2 DB and
StoreMode.MEMORY / RateLimiterMode.MEMORY, and
DeeplineServerConfig.fromEnvironment() (used by Main.kt) is not invoked by
tests; either remove the postgres/redis service blocks and the DEEPLINE_* env
entries from the ":server:test" job to avoid unused resources, or instead add a
distinct integration test job that runs tests which call
DeeplineServerConfig.fromEnvironment() / Main.kt (or new tests that create real
Postgres/Redis connections) and keep the service definitions only for that
integration job.
In @.github/workflows/security-validation.yml:
- Around line 28-31: The current grep is too broad and the negative filter
unsafe; replace the name-only match and grep -v with a pattern that only catches
concrete implementations by requiring inheritance/implementation syntax. Update
the workflow check to search for lines matching
"class\s+[A-Za-z0-9_]*CryptoBridge\s*[:]" or
"object\s+[A-Za-z0-9_]*CryptoBridge\s*[:]" (i.e., class/object whose name ends
with CryptoBridge and is followed by ':' to indicate an
implementation/inheritance) and remove the grep -v "interface\|expect"
post-filter so only real implementations are flagged (referencing the current
grep invocation that searches for class.*CryptoBridge|object.*CryptoBridge).
In `@E2EE_PROGRESS.md`:
- Around line 158-171: Replace the unofficial libsignal-swift link under the
Resources list (the line containing "libsignal-swift" and the URL pointing to
nicegram/nicegram-ios-lib-libsignal-client) with the canonical Signal Swift repo
(signalapp/libsignal) and update the link text accordingly; also correct the
Progress Log table rows dated "2025-04-17" and "2025-04-18" to the correct year
"2026" so the entries under the "Progress Log" heading reflect the PR date.
- Around line 98-120: Update the documented interfaces to match the real API:
replace the OneToOneCryptoBridge signatures with the actual methods
(createDeviceBundle(userId, deviceId), encryptMessage(plaintext: String,
recipientBundle: DeviceBundle, attachmentReferences: ...),
decryptMessage(ciphertext: String, encryptionMetadata: Map<String,String>)) and
remove the wrong ByteArray-based encrypt/decrypt/initSession lines; for
GroupCryptoBridge document the actual properties (protocolType, isSupported,
blocker) instead of addMember/removeMember methods; for AttachmentCryptoBridge
replace generateKey/encrypt/decrypt with encryptAttachment(fileName, bytes,
mimeType) and decryptAttachment(ciphertext, reference). Also change the
libsignal Swift link from the Nicegram fork to the official Signal repo
(signalapp/libsignal) and correct the Progress Log dates from 2025-04-17/18 to
2026-04-17/18. Ensure all referenced symbol names (OneToOneCryptoBridge,
GroupCryptoBridge, AttachmentCryptoBridge, createDeviceBundle, encryptMessage,
decryptMessage, protocolType, isSupported, blocker, encryptAttachment,
decryptAttachment) are used exactly as in the actual code.
In `@server/src/main/kotlin/dev/hyo/deepline/server/routes/PhoneAuthRoutes.kt`:
- Around line 45-56: Validate incoming request fields before using them in
rate-limit keys or persisting: in PhoneAuthRoutes post("/send-code") validate
SendOtpRequest.phoneNumber and .countryCode are not blank and within acceptable
lengths (e.g., max length), normalize the phone number, then call
enforceRateLimit and createPhoneVerification; in the VerifyOtp handler validate
VerifyOtpRequest.verificationId parses as a UUID and otpCode matches exactly 6
digits (and reasonable max length) before calling any store/verify functions;
use require/isNotBlank checks and explicit parsing (UUID.fromString) to fail
fast and avoid collapsing rate-limit subjects or persisting garbage.
In `@server/src/main/resources/db/migration/V3__group_member_roles.sql`:
- Line 11: The migration adds joined_at_epoch_ms with DEFAULT 0 which will
corrupt ordering for JdbcDeeplineStore.listConversationMembers on future
INSERTs; change the migration so after backfilling existing rows you either
remove the DEFAULT or set it to the current epoch milliseconds (e.g.,
EXTRACT(EPOCH FROM now()) * 1000) instead of 0, and ensure the ALTER
TABLE/UPDATE sequence in V3__group_member_roles.sql backfills existing rows,
then drops or replaces the default so new rows get a sensible timestamp rather
than 0.
---
Nitpick comments:
In @.github/workflows/ci-shared.yml:
- Around line 8-76: The CI repeats three nearly identical jobs (test-jvm,
test-android, test-ios); convert them to a single job using a strategy.matrix to
deduplicate the repeated checkout / setup-java / setup-gradle steps and only
vary the parameters per matrix entry (runner OS / runs-on, Gradle task to run
like :shared:serverTest / :shared:testDebugUnitTest /
:shared:iosSimulatorArm64Test, and artifact name like shared-jvm-test-results /
shared-android-test-results / shared-ios-test-results). Update the job to
reference the matrix values for runs-on, the run command, and the
upload-artifact name so version bumps (actions/setup-java,
gradle/actions/setup-gradle) and common steps are maintained in one place while
preserving the same behavior for test execution and artifact upload.
In `@server/src/main/resources/db/migration/V3__group_member_roles.sql`:
- Around line 7-8: The new nullable column added_by_user_id on
conversation_members currently has no foreign key constraint; if the schema is
to enforce referential integrity, add a FOREIGN KEY on
conversation_members(added_by_user_id) referencing users(user_id) with ON DELETE
SET NULL so deleted users nullify the adder rather than leaving orphans; update
migration V3__group_member_roles.sql to include this FK and coordinate the
change with a schema-wide review (since user_id columns currently lack FKs) to
ensure consistent constraints across related tables.
🪄 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: d1fd3622-13e1-4210-8b50-210b1fd3eba8
📒 Files selected for processing (20)
.claude/failure-patterns.json.claude/quality-gates.json.claude/skills/ai-iterate/SKILL.md.claude/skills/auto-fix/SKILL.md.claude/skills/ci-loop/SKILL.md.claude/skills/security-scan/SKILL.md.claude/skills/test-gaps/SKILL.md.github/workflows/ai-feedback-loop.yml.github/workflows/ci-android.yml.github/workflows/ci-ios.yml.github/workflows/ci-server.yml.github/workflows/ci-shared.yml.github/workflows/security-validation.ymlE2EE_PROGRESS.mdREADME.mdserver/src/main/kotlin/dev/hyo/deepline/server/routes/PhoneAuthRoutes.ktserver/src/main/resources/db/migration/V3__group_member_roles.sqlserver/src/main/resources/db/migration/V4__mentions_and_media.sqlserver/src/main/resources/db/migration/V5__phone_authentication.sqlserver/src/test/kotlin/dev/hyo/deepline/server/DeeplineServerTest.kt
✅ Files skipped from review due to trivial changes (6)
- server/src/main/resources/db/migration/V4__mentions_and_media.sql
- .claude/failure-patterns.json
- .github/workflows/ci-android.yml
- .claude/skills/ai-iterate/SKILL.md
- .claude/quality-gates.json
- server/src/main/resources/db/migration/V5__phone_authentication.sql
Comment on lines
+115
to
+129
| echo -n "2. Crypto interfaces check... " | ||
| if grep -rq "class.*:.*CryptoBridge" shared/src --include="*.kt" 2>/dev/null; then | ||
| echo "❌ FAIL" | ||
| ((ERRORS++)) | ||
| else | ||
| echo "✅ PASS" | ||
| fi | ||
|
|
||
| echo -n "3. SecureRandom check... " | ||
| if grep -rq "java\.util\.Random\(\)" server/src/main --include="*.kt" 2>/dev/null; then | ||
| echo "❌ FAIL" | ||
| ((ERRORS++)) | ||
| else | ||
| echo "✅ PASS" | ||
| fi |
There was a problem hiding this comment.
Validation script diverges from the checks documented above it.
A couple of gaps that will let real violations slip through:
- Step 2 (line 116) only matches
class.*:.*CryptoBridgebut drops theobject.*:.*CryptoBridgealternative listed in the step description (line 37). A singleton implementation likeobject SignalBridge : OneToOneCryptoBridgewon't fail the scan. - Step 3 (line 124) uses
java\.util\.Random\(\), which only matches the fully-qualified call. Real-world usage is typicallyimport java.util.Randomfollowed byRandom(), which this regex misses entirely — contradicting the step 3 description claiming it fails onjava.util.Randomused for token/key generation.
♻️ Suggested alignment
echo -n "2. Crypto interfaces check... "
-if grep -rq "class.*:.*CryptoBridge" shared/src --include="*.kt" 2>/dev/null; then
+if grep -rqE '^(\s*)(class|object)\s+\w+\s*(\([^)]*\))?\s*:\s*[^{]*CryptoBridge\b' \
+ shared/src --include="*.kt" 2>/dev/null; then
echo "❌ FAIL"
((ERRORS++))
else
echo "✅ PASS"
fi
echo -n "3. SecureRandom check... "
-if grep -rq "java\.util\.Random\(\)" server/src/main --include="*.kt" 2>/dev/null; then
+# Flag both FQN instantiation and bare Random() preceded by a java.util.Random import.
+if grep -rq "java\.util\.Random(" server/src/main --include="*.kt" 2>/dev/null \
+ || { grep -rlq '^import\s\+java\.util\.Random\b' server/src/main --include="*.kt" 2>/dev/null \
+ && grep -rq '\bRandom(' server/src/main --include="*.kt" 2>/dev/null; }; then
echo "❌ FAIL"
((ERRORS++))
else
echo "✅ PASS"
fiAlso minor: step 2 searches shared/src here but the CI workflow searches shared/src/commonMain; worth aligning so the skill and CI produce the same verdict.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/security-scan/SKILL.md around lines 115 - 129, The validation
script's regexes miss valid violations: update the crypto-class check
(referencing "CryptoBridge", "OneToOneCryptoBridge", and singletons like "object
SignalBridge") to also match object.*:.*CryptoBridge (or use a combined pattern
that captures both "class" and "object" declarations) and align the search path
to the CI-used shared/src/commonMain; and update the Random detection
(references to "Random()" and "java.util.Random") to catch both fully-qualified
java.util.Random() and unqualified Random() invocations (e.g., a regex that
matches either "java\.util\.Random\(" or "\bRandom\(") so imports followed by
Random() are flagged.
Comment on lines
+47
to
+48
| # Append any downloaded test results | ||
| find . -name "TEST-*.xml" -exec echo "#### {}" \; -exec head -50 {} \; >> failure-report.md 2>/dev/null || echo "No test XML artifacts found" >> failure-report.md |
There was a problem hiding this comment.
Fallback branch is effectively dead, and artifact paths are likely wrong.
Two issues with the artifact-collection line:
find . -name "TEST-*.xml" ... 2>/dev/null || echo "No test XML artifacts found"—findexits 0 even when it matches nothing, so the|| echofallback never fires. If you want a "no artifacts" message, check the result explicitly.- The CI workflows upload
server/build/reports/tests/andshared/build/reports/tests/, which contain Gradle's HTML reports — the JUnitTEST-*.xmlfiles live underbuild/test-results/, which isn't being uploaded. Sofindwill almost always return zero matches, and the report will be empty.
Either upload **/build/test-results/**/TEST-*.xml from the CI workflows or point the find at the HTML reports actually present in the artifacts.
Suggested fix sketch
- # Append any downloaded test results
- find . -name "TEST-*.xml" -exec echo "#### {}" \; -exec head -50 {} \; >> failure-report.md 2>/dev/null || echo "No test XML artifacts found" >> failure-report.md
+ # Append any downloaded JUnit results (requires CI jobs to upload build/test-results/**)
+ xml_files=$(find . -name "TEST-*.xml" 2>/dev/null)
+ if [ -z "$xml_files" ]; then
+ echo "No test XML artifacts found" >> failure-report.md
+ else
+ echo "$xml_files" | while read -r f; do
+ echo "#### $f" >> failure-report.md
+ head -50 "$f" >> failure-report.md
+ done
+ fiAnd in ci-server.yml / ci-shared.yml, also upload the raw JUnit XML:
path: |
server/build/reports/tests/
+ server/build/test-results/
shared/build/reports/tests/
+ shared/build/test-results/📝 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.
Suggested change
| # Append any downloaded test results | |
| find . -name "TEST-*.xml" -exec echo "#### {}" \; -exec head -50 {} \; >> failure-report.md 2>/dev/null || echo "No test XML artifacts found" >> failure-report.md | |
| # Append any downloaded JUnit results (requires CI jobs to upload build/test-results/**) | |
| xml_files=$(find . -name "TEST-*.xml" 2>/dev/null) | |
| if [ -z "$xml_files" ]; then | |
| echo "No test XML artifacts found" >> failure-report.md | |
| else | |
| echo "$xml_files" | while read -r f; do | |
| echo "#### $f" >> failure-report.md | |
| head -50 "$f" >> failure-report.md | |
| done | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ai-feedback-loop.yml around lines 47 - 48, The current
artifact-collection command (the find ... TEST-*.xml >> failure-report.md
pipeline) never triggers the fallback and points at the wrong locations; change
the find target to the JUnit XML location (e.g.
**/build/test-results/**/TEST-*.xml) and replace the one-liner with an explicit
existence check (e.g. set a variable or use find -print -quit to test for any
match, then either append files to failure-report.md or echo "No test XML
artifacts found" into it) so the fallback runs when empty; also update the CI
upload steps in the ci-server.yml / ci-shared.yml workflows to include the raw
**/build/test-results/**/**/TEST-*.xml artifacts instead of only the HTML report
paths.
Comment on lines
+28
to
+31
| if grep -rn "class.*CryptoBridge\|object.*CryptoBridge" shared/src/commonMain --include="*.kt" 2>/dev/null | grep -v "interface\|expect"; then | ||
| echo "❌ ERROR: shared/ should only have crypto interfaces" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Crypto-interface assertion is easy to bypass and over-broad.
Two issues with this gate:
class.*CryptoBridge|object.*CryptoBridgematches any class/object whose name containsCryptoBridge(e.g.,class CryptoBridgeFactoryorclass MyCryptoBridgeHolder) — not just implementations. That produces false positives for benign naming.grep -v "interface\|expect"filters out any line that happens to contain the substringsinterfaceorexpectanywhere (including comments, param names, or surrounding context captured by-n). A real implementation likeclass SignalCryptoBridge : OneToOneCryptoBridge // see interface abovewould be silently skipped.
Since this is a hard-fail gate, prefer matching the implementation shape (:-inheritance) and avoid textual post-filters.
🔒 Proposed tightening
- if grep -rn "class.*CryptoBridge\|object.*CryptoBridge" shared/src/commonMain --include="*.kt" 2>/dev/null | grep -v "interface\|expect"; then
+ # Match concrete class/object that *inherits from* a CryptoBridge interface.
+ if grep -rnE '^(\s*)(class|object)\s+\w+\s*(\([^)]*\))?\s*:\s*[^{]*CryptoBridge\b' \
+ shared/src/commonMain --include="*.kt" 2>/dev/null; then
echo "❌ ERROR: shared/ should only have crypto interfaces"
exit 1
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/security-validation.yml around lines 28 - 31, The current
grep is too broad and the negative filter unsafe; replace the name-only match
and grep -v with a pattern that only catches concrete implementations by
requiring inheritance/implementation syntax. Update the workflow check to search
for lines matching "class\s+[A-Za-z0-9_]*CryptoBridge\s*[:]" or
"object\s+[A-Za-z0-9_]*CryptoBridge\s*[:]" (i.e., class/object whose name ends
with CryptoBridge and is followed by ':' to indicate an
implementation/inheritance) and remove the grep -v "interface\|expect"
post-filter so only real implementations are flagged (referencing the current
grep invocation that searches for class.*CryptoBridge|object.*CryptoBridge).
Comment on lines
+98
to
+120
| ```kotlin | ||
| // One-to-one messaging | ||
| interface OneToOneCryptoBridge { | ||
| suspend fun encrypt(plaintext: ByteArray, recipientId: String): ByteArray | ||
| suspend fun decrypt(ciphertext: ByteArray, senderId: String): ByteArray | ||
| suspend fun initSession(recipientPreKeyBundle: PreKeyBundle) | ||
| } | ||
|
|
||
| // Group messaging | ||
| interface GroupCryptoBridge { | ||
| suspend fun encrypt(plaintext: ByteArray, groupId: String): ByteArray | ||
| suspend fun decrypt(ciphertext: ByteArray, groupId: String, senderId: String): ByteArray | ||
| suspend fun addMember(groupId: String, memberId: String) | ||
| suspend fun removeMember(groupId: String, memberId: String) | ||
| } | ||
|
|
||
| // Attachment encryption | ||
| interface AttachmentCryptoBridge { | ||
| fun generateKey(): ByteArray | ||
| fun encrypt(data: ByteArray, key: ByteArray): ByteArray | ||
| fun decrypt(data: ByteArray, key: ByteArray): ByteArray | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify actual interface signatures in shared/ match what the doc claims.
fd -t f 'CryptoBridge.kt' shared/src | xargs -I{} sh -c 'echo "=== {} ==="; cat "{}"'Repository: hyodotdev/Deepline
Length of output: 1575
🏁 Script executed:
#!/bin/bash
# Check lines 163 and surrounding context for the libsignal-swift link
echo "=== Lines 160-170 of E2EE_PROGRESS.md ==="
sed -n '160,170p' E2EE_PROGRESS.md
# Check for date references in the file
echo -e "\n=== Date references in E2EE_PROGRESS.md ==="
grep -n "2025-04-\|2026-04-" E2EE_PROGRESS.md || echo "No direct date matches; checking Progress log section..."
grep -n "Progress log\|date\|Date" E2EE_PROGRESS.md | head -20Repository: hyodotdev/Deepline
Length of output: 891
Documented CryptoBridge interfaces don't match the actual code.
The signatures in lines 98-120 diverge significantly from shared/src/commonMain/kotlin/dev/hyo/deepline/shared/crypto/CryptoBridge.kt:
- Actual
OneToOneCryptoBridgeexposescreateDeviceBundle(userId, deviceId),encryptMessage(plaintext: String, recipientBundle: DeviceBundle, attachmentReferences), anddecryptMessage(ciphertext: String, encryptionMetadata: Map<String, String>)— notencrypt(plaintext: ByteArray, recipientId)/decrypt(ciphertext: ByteArray, senderId)/initSession(...). - Actual
GroupCryptoBridgecontains onlyprotocolType,isSupported, andblockerproperties — not theaddMember()/removeMember()methods shown. - Actual
AttachmentCryptoBridgeexposesencryptAttachment(fileName, bytes, mimeType)anddecryptAttachment(ciphertext, reference)— notgenerateKey()/encrypt(data, key)/decrypt(data, key).
Additionally, line 163 links to github.com/nicegram/nicegram-ios-lib-libsignal-client (unofficial fork) for libsignal-swift, while libsignal-android correctly links to signalapp/libsignal — use the official Signal repo for consistency. Finally, the Progress Log dates (lines 169–171) show 2025-04-17/18 but should be 2026-04-17/18 to match the PR date.
Update this section to reflect the actual interface signatures and fix the references.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@E2EE_PROGRESS.md` around lines 98 - 120, Update the documented interfaces to
match the real API: replace the OneToOneCryptoBridge signatures with the actual
methods (createDeviceBundle(userId, deviceId), encryptMessage(plaintext: String,
recipientBundle: DeviceBundle, attachmentReferences: ...),
decryptMessage(ciphertext: String, encryptionMetadata: Map<String,String>)) and
remove the wrong ByteArray-based encrypt/decrypt/initSession lines; for
GroupCryptoBridge document the actual properties (protocolType, isSupported,
blocker) instead of addMember/removeMember methods; for AttachmentCryptoBridge
replace generateKey/encrypt/decrypt with encryptAttachment(fileName, bytes,
mimeType) and decryptAttachment(ciphertext, reference). Also change the
libsignal Swift link from the Nicegram fork to the official Signal repo
(signalapp/libsignal) and correct the Progress Log dates from 2025-04-17/18 to
2026-04-17/18. Ensure all referenced symbol names (OneToOneCryptoBridge,
GroupCryptoBridge, AttachmentCryptoBridge, createDeviceBundle, encryptMessage,
decryptMessage, protocolType, isSupported, blocker, encryptAttachment,
decryptAttachment) are used exactly as in the actual code.
PhoneAuthRoutes: - Add validation for phone number (4-15 digits) and country code (1-4 digits) - Add validation for OTP code (exactly 6 digits) - Add validation for verification ID (alphanumeric, max 128 chars) - Return 400 Bad Request for invalid inputs CI/CD fixes: - ci-ios.yml: Use continue-on-error instead of || true (visible in logs) - ai-feedback-loop.yml: Add deduplication check before creating issues - ai-feedback-loop.yml: Remove cross-workflow artifact download (not needed) Documentation: - V3 migration: Add note about DEFAULT 0 being for migration only - E2EE_PROGRESS.md: Fix libsignal link to official repo, fix dates to 2026 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove the migration-only default to enforce explicit timestamps on future INSERTs, preventing silent data corruption. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phone authentication with OTP verification
POST /v1/auth/phone/send-code- sends 6-digit OTPPOST /v1/auth/phone/verify- verifies OTPGroup member roles
OWNER,ADMIN,MEMBEROWNERrole to conversation creatorMentions tracking
message_mentionstable for @mention queriesmentioned_user_idfor "messages mentioning me"GET /v1/users/{userId}/mentionsendpointDatabase Migrations
Test plan
./gradlew :server:testto verify server tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests