feat: port project_totp_event to Go listener (#97)#118
Merged
Conversation
Second projector ported under the pattern set by #96. Replaces the PL/pgSQL project_totp_event function with projectors.TotpListener, covering all five TOTP event types: - TOTPSetupInitiated: upsert totp_projection (resets verified + enabled to FALSE on re-init) - TOTPVerified: flip totp_projection (verified + enabled = TRUE) AND cross-stream users_projection.totp_enabled = TRUE - TOTPDisabled: drop the totp_projection row AND cross-stream users_projection.totp_enabled = FALSE - TOTPBackupCodeUsed: mark a single 1-based slot in backup_codes_used (event payload uses 0-based index, listener converts) - TOTPBackupCodesRegenerated: replace backup_codes_hash and reset backup_codes_used to a fresh all-FALSE array of matching length Async-vs-sync analysis matches #96: TotpEnabled is read on the auth/login path (not on the same request that emits a TOTP event), so the post-commit projection write closes the gap well before the next read in practice. Migration 018 replaces project_totp_event with a no-op stub. The shared project_event() dispatcher trigger still PERFORMs it for every totp-stream event; the no-op keeps that quiet (no projection_errors entries) until the dispatcher itself is rewritten to skip stream types with Go projectors. Down restores the original function verbatim. New sqlc queries: UpsertTotpProjection, VerifyTotpProjection, DeleteTotpProjection, MarkTotpBackupCodeUsed, RegenerateTotpBackupCodes, SetUserTotpEnabled. Idempotent shapes throughout — every UPDATE matches by user_id + projection_version, the upsert uses ON CONFLICT. Tests: - TestTotpListener_LifecycleEndToEnd walks Setup → Verify → BackupCodeUsed → Disable through testcontainers Postgres, asserting both totp_projection AND users_projection.totp_enabled flips at each step. - TestTotpListener_BackupCodesRegenerated covers the regenerate branch separately, including verifying that backup_codes_used resets to fresh-FALSE-of-new-length. - TestTotpListener_IgnoresWrongStreamType pins the stream-type guard so a future classifier loosening can't silently start inserting totp_projection rows for non-totp events. Refs #97, tracker #107
|
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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe pull request ports the PL/pgSQL ChangesTOTP Projector Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 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)
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. Review rate limit: 0/1 reviews remaining, refill in 24 minutes and 2 seconds.Comment |
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review 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: 2
🧹 Nitpick comments (1)
internal/projectors/totp_test.go (1)
29-175: ⚡ Quick winPlease add a re-init test for the
ON CONFLICTpath.The new setup query has important second-write behavior — replace the secret/codes and reset
verified/enabledtoFALSE— but the suite only exercises first-time setup. A secondTOTPSetupInitiatedafterTOTPVerifiedis the branch most likely to regress in this port.🤖 Prompt for 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. In `@internal/projectors/totp_test.go` around lines 29 - 175, Add a new test to exercise the ON CONFLICT "re-init" path: after creating a user and running TestTotpListener_LifecycleEndToEnd’s happy-path events (TOTPSetupInitiated -> TOTPVerified), append a second TOTPSetupInitiated event with a different secret_encrypted and different backup_codes_hash, then poll the totp projection (using pollForTotpRow) and users projection (using pollForUserTotpEnabled) to assert that the totp row now contains the new secret and new backup_codes_hash, that Verified and Enabled are reset to false, and that BackupCodesUsed has been recreated with the new length all set to false; add this as a separate test (e.g., TestTotpListener_ReinitOnConflict) or extend the lifecycle test to include this second-setup scenario.
🤖 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 `@internal/projectors/totp_listener.go`:
- Around line 70-87: The two related writes (VerifyTotpProjection +
SetUserTotpEnabled) are performed as separate autocommits; wrap each pair in a
single DB transaction so both succeed or both roll back. Modify the TOTPVerified
branch to begin a transaction, call VerifyTotpProjection and SetUserTotpEnabled
inside that transaction and commit only if both succeed (rollback on error), and
do the same for the corresponding disable branch (the pair handling the opposite
projection and SetUserTotpEnabled(false)). Use the code paths/DB helpers already
available in this package (transaction begin/commit/rollback or the project's tx
helper) and keep the same logging on errors, but log and rollback the
transaction when either operation fails.
In `@internal/store/queries/totp.sql`:
- Around line 22-82: The statements must enforce projection_version monotonicity
to avoid stale replays: change UpsertTotpProjection's ON CONFLICT DO UPDATE to
only update when totp_projection.projection_version <
EXCLUDED.projection_version (add a WHERE totp_projection.projection_version <
EXCLUDED.projection_version clause), and modify the other exec queries
(VerifyTotpProjection, DeleteTotpProjection, MarkTotpBackupCodeUsed,
RegenerateTotpBackupCodes, SetUserTotpEnabled) to include a projection_version
comparison in their WHERE clauses (e.g. WHERE user_id = $1 AND
projection_version < $N) so you pass the event's projection_version as the
additional parameter and only apply the write when the stored projection_version
is older than the incoming one.
---
Nitpick comments:
In `@internal/projectors/totp_test.go`:
- Around line 29-175: Add a new test to exercise the ON CONFLICT "re-init" path:
after creating a user and running TestTotpListener_LifecycleEndToEnd’s
happy-path events (TOTPSetupInitiated -> TOTPVerified), append a second
TOTPSetupInitiated event with a different secret_encrypted and different
backup_codes_hash, then poll the totp projection (using pollForTotpRow) and
users projection (using pollForUserTotpEnabled) to assert that the totp row now
contains the new secret and new backup_codes_hash, that Verified and Enabled are
reset to false, and that BackupCodesUsed has been recreated with the new length
all set to false; add this as a separate test (e.g.,
TestTotpListener_ReinitOnConflict) or extend the lifecycle test to include this
second-setup scenario.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44eab09d-4866-444c-b0dd-4f86a1e7abb0
⛔ Files ignored due to path filters (1)
internal/store/generated/totp.sql.gois excluded by!**/generated/**
📒 Files selected for processing (6)
cmd/control/main.gointernal/projectors/totp.gointernal/projectors/totp_listener.gointernal/projectors/totp_test.gointernal/store/migrations/018_totp_projector_to_go.sqlinternal/store/queries/totp.sql
Comment on lines
+70
to
+87
| case "TOTPVerified": | ||
| if err := q.VerifyTotpProjection(ctx, db.VerifyTotpProjectionParams{ | ||
| UserID: userID, | ||
| UpdatedAt: updatedAt, | ||
| ProjectionVersion: deref(e.SequenceNum), | ||
| }); err != nil { | ||
| logger.Warn("totp projector: failed to flip totp_projection to verified", | ||
| "event_id", e.ID, "user_id", userID, "error", err) | ||
| } | ||
| if err := q.SetUserTotpEnabled(ctx, db.SetUserTotpEnabledParams{ | ||
| ID: userID, | ||
| TotpEnabled: true, | ||
| UpdatedAt: &updatedAt, | ||
| ProjectionVersion: deref(e.SequenceNum), | ||
| }); err != nil { | ||
| logger.Warn("totp projector: failed to flip users_projection.totp_enabled=TRUE", | ||
| "event_id", e.ID, "user_id", userID, "error", err) | ||
| } |
There was a problem hiding this comment.
Make the verify/disable dual writes atomic.
Both branches split the totp_projection change and the users_projection.totp_enabled flip into separate autocommit statements. If one succeeds and the other fails, the auth path can read the wrong TOTP state until a later event or rebuild repairs it. The old PL/pgSQL projector committed those effects together, so this port should preserve that by wrapping each pair in a transaction.
Suggested shape
- q := st.Queries()
+ q := st.Queries()
switch e.EventType {
case "TOTPVerified":
- if err := q.VerifyTotpProjection(...); err != nil { ... }
- if err := q.SetUserTotpEnabled(...); err != nil { ... }
+ if err := st.WithTx(ctx, func(q *generated.Queries) error {
+ if err := q.VerifyTotpProjection(ctx, ...); err != nil {
+ return err
+ }
+ return q.SetUserTotpEnabled(ctx, ...)
+ }); err != nil {
+ logger.Warn("totp projector: failed to apply TOTPVerified", ...)
+ }
case "TOTPDisabled":
- if err := q.DeleteTotpProjection(...); err != nil { ... }
- if err := q.SetUserTotpEnabled(...); err != nil { ... }
+ if err := st.WithTx(ctx, func(q *generated.Queries) error {
+ if err := q.DeleteTotpProjection(ctx, ...); err != nil {
+ return err
+ }
+ return q.SetUserTotpEnabled(ctx, ...)
+ }); err != nil {
+ logger.Warn("totp projector: failed to apply TOTPDisabled", ...)
+ }Also applies to: 89-102
🤖 Prompt for 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.
In `@internal/projectors/totp_listener.go` around lines 70 - 87, The two related
writes (VerifyTotpProjection + SetUserTotpEnabled) are performed as separate
autocommits; wrap each pair in a single DB transaction so both succeed or both
roll back. Modify the TOTPVerified branch to begin a transaction, call
VerifyTotpProjection and SetUserTotpEnabled inside that transaction and commit
only if both succeed (rollback on error), and do the same for the corresponding
disable branch (the pair handling the opposite projection and
SetUserTotpEnabled(false)). Use the code paths/DB helpers already available in
this package (transaction begin/commit/rollback or the project's tx helper) and
keep the same logging on errors, but log and rollback the transaction when
either operation fails.
Comment on lines
+22
to
+82
| -- name: UpsertTotpProjection :exec | ||
| INSERT INTO totp_projection ( | ||
| user_id, secret_encrypted, verified, enabled, | ||
| backup_codes_hash, backup_codes_used, | ||
| created_at, updated_at, projection_version | ||
| ) VALUES ( | ||
| $1, $2, FALSE, FALSE, | ||
| $3, | ||
| array_fill(FALSE::boolean, ARRAY[array_length($3::text[], 1)]), | ||
| $4, $4, $5 | ||
| ) | ||
| ON CONFLICT (user_id) DO UPDATE SET | ||
| secret_encrypted = EXCLUDED.secret_encrypted, | ||
| verified = FALSE, | ||
| enabled = FALSE, | ||
| backup_codes_hash = EXCLUDED.backup_codes_hash, | ||
| backup_codes_used = EXCLUDED.backup_codes_used, | ||
| updated_at = EXCLUDED.updated_at, | ||
| projection_version = EXCLUDED.projection_version; | ||
|
|
||
| -- name: VerifyTotpProjection :exec | ||
| UPDATE totp_projection | ||
| SET verified = TRUE, | ||
| enabled = TRUE, | ||
| updated_at = $2, | ||
| projection_version = $3 | ||
| WHERE user_id = $1; | ||
|
|
||
| -- name: DeleteTotpProjection :exec | ||
| DELETE FROM totp_projection WHERE user_id = $1; | ||
|
|
||
| -- TOTPBackupCodeUsed updates a 1-indexed slot in backup_codes_used. | ||
| -- The PL/pgSQL projector used `[(idx)::int + 1]` — we add the +1 in | ||
| -- Go before passing the parameter so the SQL stays clean. | ||
| -- | ||
| -- name: MarkTotpBackupCodeUsed :exec | ||
| UPDATE totp_projection | ||
| SET backup_codes_used[$2::int] = TRUE, | ||
| updated_at = $3, | ||
| projection_version = $4 | ||
| WHERE user_id = $1; | ||
|
|
||
| -- name: RegenerateTotpBackupCodes :exec | ||
| UPDATE totp_projection | ||
| SET backup_codes_hash = $2, | ||
| backup_codes_used = array_fill(FALSE::boolean, ARRAY[array_length($2::text[], 1)]), | ||
| updated_at = $3, | ||
| projection_version = $4 | ||
| WHERE user_id = $1; | ||
|
|
||
| -- Cross-stream effect: TOTP events also flip | ||
| -- users_projection.totp_enabled. Was an inline UPDATE inside the | ||
| -- deleted PL/pgSQL projector. Sqlc'd here so the Go listener can | ||
| -- call it explicitly. | ||
| -- | ||
| -- name: SetUserTotpEnabled :exec | ||
| UPDATE users_projection | ||
| SET totp_enabled = $2, | ||
| updated_at = $3, | ||
| projection_version = $4 | ||
| WHERE id = $1; |
There was a problem hiding this comment.
Guard these writes with projection_version monotonicity.
These statements currently update/delete by user_id only, so a stale replay can roll the projection backwards. The clearest failure mode is an older TOTPDisabled deleting a row recreated by a newer TOTPSetupInitiated, but the same problem exists for the other branches too. The PR objective called out write-once/idempotent projector semantics, so these should be conditional on the stored projection_version being older than the event being applied.
Suggested shape
-- name: UpsertTotpProjection :exec
...
ON CONFLICT (user_id) DO UPDATE SET
secret_encrypted = EXCLUDED.secret_encrypted,
verified = FALSE,
enabled = FALSE,
backup_codes_hash = EXCLUDED.backup_codes_hash,
backup_codes_used = EXCLUDED.backup_codes_used,
updated_at = EXCLUDED.updated_at,
- projection_version = EXCLUDED.projection_version;
+ projection_version = EXCLUDED.projection_version
+WHERE totp_projection.projection_version < EXCLUDED.projection_version;
-- name: VerifyTotpProjection :exec
UPDATE totp_projection
SET verified = TRUE,
enabled = TRUE,
updated_at = $2,
projection_version = $3
-WHERE user_id = $1;
+WHERE user_id = $1
+ AND projection_version < $3;DeleteTotpProjection and SetUserTotpEnabled need the same treatment, which means passing the event sequence into the delete as well.
🤖 Prompt for 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.
In `@internal/store/queries/totp.sql` around lines 22 - 82, The statements must
enforce projection_version monotonicity to avoid stale replays: change
UpsertTotpProjection's ON CONFLICT DO UPDATE to only update when
totp_projection.projection_version < EXCLUDED.projection_version (add a WHERE
totp_projection.projection_version < EXCLUDED.projection_version clause), and
modify the other exec queries (VerifyTotpProjection, DeleteTotpProjection,
MarkTotpBackupCodeUsed, RegenerateTotpBackupCodes, SetUserTotpEnabled) to
include a projection_version comparison in their WHERE clauses (e.g. WHERE
user_id = $1 AND projection_version < $N) so you pass the event's
projection_version as the additional parameter and only apply the write when the
stored projection_version is older than the incoming one.
…ojection writes
Root cause: testutil.SetupPostgres did not register the Go-side
projector listeners that production boot wires in cmd/control/main.go.
Migration 018 turned project_totp_event into a no-op stub, so
without the Go listener firing in tests, no projector ran at all.
Tests that emitted a TOTP event and then read back from
totp_projection / users_projection saw stale state — manifesting as
TestVerifyTOTP_Success failing with 'TOTP not set up, call SetupTOTP
first', plus 3 sibling auth tests reading users_projection.totp_enabled.
Why this is the right fix vs the architectural alternatives we
considered (transactional projector dispatcher inside AppendEvent;
inline writes from each handler):
- Store.fireListeners runs synchronously after the event commit
and BEFORE AppendEvent returns. Once the listener is wired,
handlers see the projection on the next line of code, same as
they did under the deleted PL/pgSQL triggers.
- The production behaviour was correct; only tests were broken.
internal/projectors/wire.go centralises the listener-registration
list so a new port (#98 through #106) only adds itself in one
place — testutil and production boot pick it up automatically.
Tests previously failing on PR #118 now pass:
- TestVerifyTOTP_Success
- TestVerifyTOTP_InvalidCode
- TestLogin_TOTPRequired
- TestListAuthMethods_WithEmailUserHasTOTP
Refs #97
This was referenced May 5, 2026
This was referenced May 16, 2026
PaulDotterer
added a commit
that referenced
this pull request
Jun 14, 2026
Records the WS15 trust-model decisions (sdk #110 / agent #118): per-message panic recovery + bounded fan-out, malformed-oneof nil-guards, inbound size bound, terminal Cols/Rows bounds, fail-closed maintenance window, clock clamp, crash marker, and the consolidated https gateway-URL guard. Implementation lives in the sdk/agent repos; recorded here for trust-model continuity.
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
1 participant
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
Second projector port under the pattern proven by #96. Replaces
project_totp_event(PL/pgSQL, 5 event-type branches + cross-stream effect onusers_projection.totp_enabled) withprojectors.TotpListener(Go, post-commit listener).Closes #97.
Coverage
totp_projection(resets verified+enabled to FALSE on re-init)totp_projection.verified+enabled = TRUEANDusers_projection.totp_enabled = TRUEtotp_projectionrow ANDusers_projection.totp_enabled = FALSEbackup_codes_used(event payload uses 0-based index; listener converts)backup_codes_hashand resetbackup_codes_usedto a fresh all-FALSE array of matching lengthAsync-vs-sync analysis
Same shape as #96.
TotpEnabledis read on the auth/login path (not on the same request that emits a TOTP event), so the post-commit projection write closes the gap well before the next read. The login flow's logout/login cycle dwarfs the listener latency.Migration shape
project_totp_eventis dispatched by the sharedproject_event()trigger (not its own dedicated trigger like security_alert had). Migration 018 replaces it with a no-op stub — the dispatcher keeps PERFORMing it (noprojection_errorsnoise), Go listener does the actual work. Once the dispatcher itself is rewritten to skip Go-ported streams, the no-op stub can be dropped.Test plan
TestTotpListener_LifecycleEndToEnd— Setup → Verify → BackupCodeUsed → Disable, asserting both projections at each stepTestTotpListener_BackupCodesRegenerated— separate coverage of the regenerate branch including length-matching resetTestTotpListener_IgnoresWrongStreamType— defensive: TOTPSetupInitiated understream_type=useris a no-opgo build ./...clean🤖 Generated with Claude Code
Summary by CodeRabbit