[IAmJSD]

Summary

Two leftovers from #313. Other parts of that issue stay open.

1. One users column list for every SELECT, INSERT and scanUser

Root cause. GetUserByID, GetUserByEmail, ListUsers and GetUserByResetToken each had their own hand-aligned copy of the 20-column list that scanUser reads by position. CreateUser and CreateAdminIfNone each had their own 18-column INSERT list plus a hand-ordered argument list. Several neighboring columns share a Go type (mfa_secret/mfa_pending_secret, the three []string columns, the seven timestamps). Swapping two of them in one copy produces no error and keeps every existing test green. I checked this on main: swapping mfa_secret and mfa_pending_secret in GetUserByID alone leaves go test ./internal/auth/ passing.

Fix (internal/auth/store_postgres.go, internal/auth/store_postgres_admin.go):

• userInsertColumns is the single INSERT column list, and userInsertArgs(user, groupIDs) returns its values in the same order. Both CreateUser and CreateAdminIfNone use them. CreateAdminIfNone still forces the Administrators group in and appends $19 for the NOT EXISTS guard. The nil-to-{} substitution for recovery codes moved into the helper unchanged.
• userSelectColumns = userInsertColumns + ", deactivated_at, password_version" is the single list that all four SELECTs embed and that scanUser reads.
• The SQL text, statement count and transaction or advisory-lock behavior are all unchanged. This is a pure refactor with no new concurrent path, so I didn't add a race test.

Tests:

• internal/auth/store_postgres_user_columns_test.go (unit, pgxmock). Every column gets a value that no other same-typed column holds, keyed by column name, and is placed in row or argument position by looking each name in the shared list up in that map. The tests cover:
  • all four getters scanning every field into the right User field;
  • CreateUser and CreateAdminIfNone binding every argument from the right field;
  • each query embedding the shared list verbatim;
  • the select list extending the insert list.
• internal/auth/store_postgres_user_columns_db_test.go (integration, real migrated Postgres). Rows are written and read back with SQL that names its own columns in an unrelated order, so these tests don't rely on the constants they check:
  • a raw INSERT, then all four getters, must return the identical User;
  • CreateUser and CreateAdminIfNone, then a raw SELECT by name, must return the identical row.

2. Remove the never-called SendWelcomeEmail

Before removing it I confirmed there is no non-test caller: git grep SendWelcomeEmail shows only definitions, mocks, stubs and tests. Invited users get SendUserInviteEmail. The method still carried the role parameter from the removed role model. I removed:

• the method from email.SenderInterface (internal/email/interfaces.go) and from Sender (templates.go), SMTPSender (smtp_sender.go) and NopSender (nop_sender.go);
• the method from mocks.MockEmailSender and from the EmailSenderAPI interface (internal/mocks/email.go);
• the test stubs in internal/{api,auth,purchase,scheduler,server};
• welcomeUserTemplate, welcomeUserHTMLTemplate, WelcomeUserData, RenderWelcomeEmail and RenderWelcomeEmailHTML, which only that method used, plus the tests that pinned them.

I also changed four comments that called the invite email the "welcome email" (internal/auth/service.go, service_user.go, service_user_test.go, internal/email/sender.go/templates.go).

Refs #313

Verification

All evidence below is unit, mock or fixture evidence. Item 1 also has local integration evidence on a real migrated Postgres (testcontainers).

export PATH=$HOME/.local/go/bin:$HOME/go/bin:$HOME/.local/bin:$PATH
export AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true
go build ./... && go vet ./... && go vet -tags integration ./...                          # clean
for p in auth email mocks api purchase scheduler server; do go test -count=1 ./internal/$p/; done   # all ok
go test -tags integration -count=1 -timeout 20m -run 'TestIntegration_UserColumns' ./internal/auth/  # 3/3 PASS
golangci-lint run --allow-parallel-runners ./internal/{auth,email,mocks,api,purchase,scheduler,server}/...  # 0 issues
golangci-lint run --allow-parallel-runners --build-tags integration ./internal/auth/...                  # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/auth/store_postgres.go internal/auth/store_postgres_admin.go internal/email/*.go internal/mocks/email.go  # none

Fail-before on main (7b86e38). I copied the integration test and its fixture into a main worktree. On clean main, TestIntegration_UserColumns_* passes. Then I swapped mfa_secret and mfa_pending_secret in GetUserByID's hand-written list:

• the existing go test ./internal/auth/ stays green (this is the gap the issue describes);
• TestIntegration_UserColumns_ScanUserRoundTripsEveryColumn fails: MFASecret: "mfa-secret-pending" vs "mfa-secret-enrolled".

Mutation probes on this branch (each applied alone, then reverted):

Mutation	Unit tests that fail	Integration tests that fail
swap mfa_secret/mfa_pending_secret in the shared constant	ScanUserReadsEveryColumn, both Insert tests (and the existing CreateAdminIfNone regex tests)	all 3
swap &mfaSecret/&mfaPendingSecret in scanUser	ScanUserReadsEveryColumn, existing ListUsers_Success	ScanUserRoundTrips
swap MFASecret/MFAPendingSecret in userInsertArgs	both Insert tests	CreateUser and CreateAdminIfNone
swap &lastLoginAt/&deactivatedAt in scanUser	ScanUserReadsEveryColumn, existing ListUsers_Success	ScanUserRoundTrips

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 400d256b = origin/main 7b86e386 plus these heads, merged in this order:

Branch	Tested head
fix/issue-243-rest	007c163
fix/issue-1206	312072f
fix/issue-111-email	6d422e2
fix/issue-111-fail-closed	3817e29
fix/issue-712-age-cap	a91120b
fix/issue-712-fargate	aeaf0d4
fix/issue-613-legacy-ids	350e837
fix/issue-120-guards	6f38ebc
fix/issue-120-shared-predicate	cb25735
fix/issue-151-account-update-keep	d20cf01
fix/issue-713-upcoming-filter	e73fdfb
fix/issue-263-bastion-opts	7861f32
fix/issue-43-fixture-ids	8cee203
fix/issue-177-makefile-walk	365a9d6
fix/issue-27-seller-not-registered	d845f66
fix/issue-313-user-columns	6d759dd
fix/issue-353-mock-panics	aac2edf
fix/issue-8-config-stamps	b548ae5
fix/issue-30-monthly-totals	f637921

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran on f7c48020, which differs from this tree only by test-only commits on fix/issue-30, fix/issue-713 and fix/issue-263 that replace placeholder account-number literals with named constants:

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package that has them, one package at a time (-tags integration -count=1): internal/analytics, internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase, internal/scheduler and internal/server all pass. Migrations 000149, 000150 and 000151 apply in order on this tree.
• Frontend: the changed jest files (approval-details, settings-accounts, api) pass (221 tests).
• Secrets: the first run flagged 9 new fixture lines (placeholder account numbers next to an account key); fixed with named constants. Re-run on this tree: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL.
• Terraform .tftest.hcl files were not run locally (CI runs them). Nothing was applied to any cloud.

Out of scope (deliberately left for later)

• Dropping users.salt. This has to wait until chore(auth): drop the unused User.Salt field and stop reading or writing users.salt #1514 is deployed everywhere; see the issue thread.
• Choosing one of the redactEmail copies. That is an owner decision.
• Everything else still open on chore(auth): role-model leftovers, inline loadUser copies, inert constant-time compares #313; this branch touches only the two items above.

Labels

Maintainer: please mirror the issue's triage labels onto this PR: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/eventually, effort/m.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 7b86e38
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: a6d9cf10-7943-4138-8353-df0abdb33b61

Commits

Reviewing files that changed from the base of the PR and between 7b86e38 and 7b84ab9.

Files ignored due to path filters: 1

• go.sum is excluded by !**/*.sum

Files selected for processing: 28

• go.mod
• internal/api/coverage_gaps_test.go
• internal/auth/service.go
• internal/auth/service_reset_invalidation_db_test.go
• internal/auth/service_user.go
• internal/auth/service_user_test.go
• internal/auth/store_postgres.go
• internal/auth/store_postgres_admin.go
• internal/auth/store_postgres_user_columns_db_test.go
• internal/auth/store_postgres_user_columns_test.go
• internal/email/coverage_test.go
• internal/email/interfaces.go
• internal/email/nop_sender.go
• internal/email/nop_sender_test.go
• internal/email/sender.go
• internal/email/sender_test.go
• internal/email/smtp_sender.go
• internal/email/smtp_sender_test.go
• internal/email/smtp_server_test.go
• internal/email/template_renderers.go
• internal/email/template_renderers_test.go
• internal/email/templates.go
• internal/email/templates_test.go
• internal/mocks/email.go
• internal/purchase/mocks_test.go
• internal/scheduler/scheduler_test.go
• internal/server/app_test.go
• internal/server/handler_ri_exchange_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 7b84ab9f1f7bda081bd4b16728a2194ca02898ef

Blocking:

• none

Non-blocking:

• userInsertColumns and userInsertArgs stay in sync only by convention plus the tests. Both tests key every value by column name, and the integration tests use SQL that names its own columns, so a swap in either one is caught. That's enough.
• Remaining chore(auth): role-model leftovers, inline loadUser copies, inert constant-time compares #313 items stay open, so Refs #313 is right.

Attacked:

• SQL equivalence: compared each new statement with the old one. All four SELECTs produce the same 20 columns in the same order, both INSERTs the same 18 columns, and CreateAdminIfNone still binds $19 = DefaultAdminGroupID with the forced, deduped Administrators group at $4. The nil-to-{} recovery-codes substitution moved into userInsertArgs unchanged. The advisory lock and the tx are untouched. Every scanUser caller now uses userSelectColumns, and no other user SELECT feeds scanUser.
• Fail-before (mutation): swapped &mfaSecret and &mfaPendingSecret in scanUser. The new unit test TestUserSelectColumns_ScanUserReadsEveryColumnIntoItsField failed, and so did the new integration test TestIntegration_UserColumns_ScanUserRoundTripsEveryColumn on real Postgres. Restored afterwards.
• SendWelcomeEmail removal: git grep shows no SendWelcomeEmail, RenderWelcomeEmail or WelcomeUserData left outside docs/audits. The removed tests are only the welcome-email ones. email is under internal/, so no other module can be calling it.
• Commands: go build ./...; go vet ./...; go vet -tags integration on auth, email, mocks, api, purchase, scheduler and server; tests/e2e go vet -tags e2e. go test -race -short -count=1 on auth, email, mocks, purchase, scheduler, server and api (all ok). go test -race -tags integration -count=1 on the whole of internal/auth and internal/email (ok).
• Head moved: the brief named 6d759dd2. The new head 7b84ab9f only adds the golang.org/x/net v0.62.0 bump. That diff is byte-identical to fix(deps): bump golang.org/x/net to v0.62.0 (GO-2026-6860, GO-2026-6866) #1564's, and all the checks above ran on 7b84ab9f.

Body-stale: the body doesn't mention the x/net bump commit (non-blocking).

_{Reviewer: rG4. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 7b84ab9f1f7bda081bd4b16728a2194ca02898ef

Blocking:

• none

Non-blocking:

• none

Attacked:

• Callers: scanUser has only four callers, and all of them now embed userSelectColumns. The only non-migration INSERT INTO users statements are CreateUser and CreateAdminIfNone, and both use userInsertColumns and userInsertArgs.
• CreateAdminIfNone still forces the Administrators group, and binds it at $19 after the 18 shared args. The nil→{} recovery-code substitution is carried over unchanged.
• SendWelcomeEmail, RenderWelcomeEmail* and WelcomeUserData have no references left in this repo. cli, mcp and go don't reference them either (the package is internal/).
• Mutation probes on this head (fail-before):
  • Swapping &mfaSecret and &mfaPendingSecret in scanUser makes TestUserSelectColumns_ScanUserReadsEveryColumnIntoItsField and TestPGXMock_ListUsers_Success fail.
  • Swapping MFASecret and MFAPendingSecret in userInsertArgs makes both *_BindsEveryColumnFromItsField insert tests fail.
  • Both mutations were reverted.
• Ran:
  • go build ./...
  • go vet ./..., with and without -tags integration
  • go test -race -short -count=1 on auth, email, mocks, purchase, scheduler, server and api: ok
  • go test -race -tags integration -count=1 on auth (whole package, including TestIntegration_UserColumns_*) and email: ok
  • golangci-lint, with and without integration tags: 0 issues
  • gocyclo on the changed files: clean

Body-stale: none

_{Reviewer: rH2. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 7b84ab9f1f7bda081bd4b16728a2194ca02898ef merged onto main (bbf0a839), together with #1552, #1553, #1551, #1246, #1548, #1549, #1554, #1560 in that order (combined tree 7f9817d4, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on 7b84ab9f (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 7b84ab9f1f7bda081bd4b16728a2194ca02898ef.