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 accounts now automatically have correct group assignments restored during system upgrades, ensuring no admin accounts lack proper group associations.

• Chores

  • Improved logging output for administrative group assignment operations.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR adds an idempotent Postgres migration (000053) to backfill the Administrators group UUID into users.group_ids for admin users with empty or null group_ids. A companion logging change redirects backfill messages from stdout to the standard logger, with integration tests validating both the SQL migration behavior and logging output stream.

Changes

Admin Group ID Backfill and Logging

Layer / File(s)	Summary
SQL migration 000053: admin group_ids backfill internal/database/postgres/migrations/000053_backfill_admin_group_ids.up.sql, 000053_backfill_admin_group_ids.down.sql, backfill_admin_group_ids_test.go	Migration 000053 idempotently appends the Administrators group UUID to users.group_ids rows where role='admin' and group_ids is NULL or empty, with deduplication and an EXISTS guard. Down migration is a no-op with explanation. Integration test validates backfill works even when Go-level admin assignment is not triggered.
Logging redirection: fmt.Printf → log.Printf with test coverage internal/database/postgres/migrations/migrate.go, migrate_security_integration_test.go	Updates assignAdminGroupAndWarn to route backfill messages from stdout to standard logger. Adds stdout and logger capture test helpers and an integration test that verifies "Backfilled" appears in logger output but not stdout.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

• #546: Directly addresses the gap where admin group_ids backfill lived only in Go by adding an idempotent SQL migration and integration tests to exercise the migration behavior.
• #545: Addresses the same logging regression in migrate.go by ensuring log.Printf is used instead of fmt.Printf and advocates for integration tests that exercise real migration functions.

Possibly related PRs

• LeanerCloud/CUDly#520: Both PRs update logging in migrate.go's assignAdminGroupAndWarn to route messages from stdout to logger/stderr.
• LeanerCloud/CUDly#393: Both PRs implement idempotent backfill of admin group_ids via assignAdminGroupAndWarn and ensureAdminUser logic with corresponding integration tests.

Suggested labels

urgency/this-sprint, effort/s

Poem

🐰 A migration hops in line,
Administrators' groups align,
Empty arrays fill with care,
Logging streams flow everywhere—
No stdout hopping, stderr's fair!

🚥 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 PR title accurately summarizes the main changes: adding tests for a stdout-leak guard (#440) and a SQL backfill migration for admin group assignment (#351), with clear issue references (#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