test(database): exercise #440 stdout-leak guard + SQL backfill for #351 admin group (closes #545, #546)

[cristim]

Summary

Hardens two database regression tests that did not actually exercise the code they claimed to guard, plus a SQL-level backfill for the #351 admin-group assignment.

Fixes

• test(database): #440 stdout-leak regression test simulates log calls instead of exercising migrate.go #545 — internal/database/postgres/migrations/migrate.go: the sec: plaintext admin password echo'd to stdout during DB migration startup #440 plaintext-password stdout-leak guard now routes the admin group-backfill log to stderr and the regression test (migrate_security_integration_test.go) exercises the real migrate path, asserting the plaintext password never reaches stdout. Previously the test re-typed the log calls inline, so a revert would not have failed it.
• test(database): #351 group-assignment regression test is integration-tagged, may not run in default CI #546 — adds a SQL-level backfill migration (000053_backfill_admin_group_ids) so the bootstrap-admin-to-Administrators-group assignment from fix(auth): bootstrap admin must be auto-assigned to the Administrators group (ensureAdminUser gap) #351 runs as a real migration (not only Go runtime logic behind an integration build tag), with backfill_admin_group_ids_test.go covering it.

Migration

New migration 000053_backfill_admin_group_ids (up + down). Verified 000053 is free on base (highest prior is 000052 from the MFA PR).

Test plan

• sec: plaintext admin password echo'd to stdout during DB migration startup #440 test fails if the plaintext-password log is reverted to stdout
• fix(auth): bootstrap admin must be auto-assigned to the Administrators group (ensureAdminUser gap) #351 backfill migration is idempotent (down + re-up)

Closes #545, #546.

Summary by CodeRabbit

• Bug Fixes

  • Admin users with missing group assignments are now automatically detected and restored during database migrations.

• Improvements

  • Admin operation logging now properly uses the logging system for better observability.

• Tests

  • Added comprehensive integration tests to verify admin account recovery and logging behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 18860b79-93a5-4422-ae8d-2f5c06924c03

📥 Commits

Reviewing files that changed from the base of the PR and between 83ba93a and 69ba7c4.

📒 Files selected for processing (6)

• internal/database/postgres/migrations/000056_backfill_admin_group_ids.down.sql
• internal/database/postgres/migrations/000056_backfill_admin_group_ids.up.sql
• internal/database/postgres/migrations/backfill_admin_group_ids_test.go
• internal/database/postgres/migrations/helpers_test.go
• internal/database/postgres/migrations/migrate.go
• internal/database/postgres/migrations/migrate_security_integration_test.go

💤 Files with no reviewable changes (6)

• internal/database/postgres/migrations/000056_backfill_admin_group_ids.down.sql
• internal/database/postgres/migrations/000056_backfill_admin_group_ids.up.sql
• internal/database/postgres/migrations/migrate.go
• internal/database/postgres/migrations/backfill_admin_group_ids_test.go
• internal/database/postgres/migrations/helpers_test.go
• internal/database/postgres/migrations/migrate_security_integration_test.go

---

📝 Walkthrough

Walkthrough

This PR adds SQL migration 000056 to backfill missing Administrators group UUIDs for admin users, redirects admin logging from stdout to stderr, and provides integration test coverage with new output-capture test helpers.

Changes

Admin Group ID Backfill and Logging Redirection

Layer / File(s)	Summary
SQL migration 000056: admin group_ids backfill with idempotency test internal/database/postgres/migrations/000056_backfill_admin_group_ids.up.sql, internal/database/postgres/migrations/000056_backfill_admin_group_ids.down.sql, internal/database/postgres/migrations/backfill_admin_group_ids_test.go	Migration 000056 up idempotently appends the Administrators group UUID to users.group_ids for admin users with NULL or empty arrays; it deduplicates via DISTINCT unnest, guards with EXISTS for Administrators group presence, and updates updated_at. Down migration is a no-op with explanatory comment. Integration test seeds drifted admin row, runs migrations with empty admin email to trigger SQL-only backfill, and validates idempotency on second run.
Test infrastructure: stdout and logger capture utilities internal/database/postgres/migrations/helpers_test.go	New imports and two test helper functions: captureStdout redirects os.Stdout to a pipe and returns captured text via cleanup, captureLogOutput captures standard logger output to a buffer. Both enable assertions about which output stream receives specific messages.
Logging fix: migrate.go and integration test for stderr-not-stdout behavior internal/database/postgres/migrations/migrate.go, internal/database/postgres/migrations/migrate_security_integration_test.go	Updates assignAdminGroupAndWarn to route "Backfilled group_ids" message via log.Printf (logger/stderr) instead of fmt.Printf (stdout), with inline comments documenting admin-account logging policy. Integration test TestAssignAdminGroup_BackfillLogsToStderr_NotStdout seeds drifted admin row, runs migrations with admin email to trigger backfill, and asserts "Backfilled" appears in logger but not stdout.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

• LeanerCloud/CUDly#546: Directly addresses the issue by adding the idempotent SQL migration 000056 for backfilling admin group_ids and comprehensive integration test coverage beyond the Go-level approach.

Possibly related PRs

• LeanerCloud/CUDly#520: Both PRs modify internal/database/postgres/migrations/migrate.go to switch migration/admin logging from fmt.Printf (stdout) to log.Printf (logger/stderr) and add tests asserting those log lines do not go to stdout.

• LeanerCloud/CUDly#393: Both PRs address admin group_ids drift/backfill logic by adjusting ensureAdminUser/assignAdminGroupAndWarn repairs and covering with integration tests including "Backfilled" log output validation.

Poem

🐰 A migration hops in to mend the gap,
Where admin groups wandered from their map—
No-op rollback stands guard so neat,
While logs now whisper on stderr's beat!
Idempotent, tested, a cleanup complete! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and specifically summarizes the main changes: it references the stdout-leak guard fix (#440), SQL backfill for admin group (#351), and the two issues being closed (#545, #546).
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/db-coverage-gaps

---

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

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (1)

internal/database/postgres/migrations/migrate_security_integration_test.go (1)

19-52: ⚡ Quick win

Consider consolidating duplicate capture helpers.

The captureStdoutIntegration and captureLogOutputIntegration functions are duplicates of captureStdout and captureLogOutput from migrate_security_test.go (shown in relevant snippets). Since both files are in package migrations_test and migrate_security_test.go has no build tag, those helpers should be available to this integration test file when running with -tags=integration.

Consider removing the duplicates here and reusing the originals, or moving all capture helpers to a shared location like helpers_test.go (similar to getMigrationsPath()).

🤖 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/database/postgres/migrations/migrate_security_integration_test.go`
around lines 19 - 52, Duplicate test helpers captureStdoutIntegration and
captureLogOutputIntegration should be removed and the tests should reuse the
existing captureStdout and captureLogOutput helpers; either delete these two
functions from migrate_security_integration_test.go and import/use the originals
in migrate_security_test.go (they live in the same package migrations_test and
are available under -tags=integration), or move the shared helpers into a new
helpers_test.go so both files reference the same captureStdout and
captureLogOutput symbols. Ensure references in
migrate_security_integration_test.go are updated to call captureStdout and
captureLogOutput (or the new shared names) and remove the duplicated
implementations.

🤖 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.

Nitpick comments:
In `@internal/database/postgres/migrations/migrate_security_integration_test.go`:
- Around line 19-52: Duplicate test helpers captureStdoutIntegration and
captureLogOutputIntegration should be removed and the tests should reuse the
existing captureStdout and captureLogOutput helpers; either delete these two
functions from migrate_security_integration_test.go and import/use the originals
in migrate_security_test.go (they live in the same package migrations_test and
are available under -tags=integration), or move the shared helpers into a new
helpers_test.go so both files reference the same captureStdout and
captureLogOutput symbols. Ensure references in
migrate_security_integration_test.go are updated to call captureStdout and
captureLogOutput (or the new shared names) and remove the duplicated
implementations.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 329edcfb-2237-4ab3-91ed-8a6de8d7b27d

📥 Commits

Reviewing files that changed from the base of the PR and between bc8831b and 83ba93a.

📒 Files selected for processing (5)

• internal/database/postgres/migrations/000053_backfill_admin_group_ids.down.sql
• internal/database/postgres/migrations/000053_backfill_admin_group_ids.up.sql
• internal/database/postgres/migrations/backfill_admin_group_ids_test.go
• internal/database/postgres/migrations/migrate.go
• internal/database/postgres/migrations/migrate_security_integration_test.go

[cristim]

Pushed the consolidated capture helpers (69ba7c44f) addressing the nitpick from review 4349117639, plus the migration renumber to 000056 (985d17495) to stay out of feat's new range. migrate_security_test.go uses package migrations (internal) while helpers_test.go uses package migrations_test with the integration build tag, so there's no Go-level symbol clash between them.

@coderabbitai review

[coderabbitai]

@cristim Thanks for the detailed update! Acknowledged:

• Capture helpers (69ba7c44f): consolidated helpers addressing the previous nitpick.
• Migration renumber to 000056 (985d17495): avoids overlap with the feat branch's new range.
• Package separation: migrate_security_test.go (package migrations, internal) vs helpers_test.go (package migrations_test, integration build tag) — no symbol collision. Makes sense.

Triggering a fresh review now.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 0