nnb9: black-box internal/db tests (bookshelf-nnb9.7) #1407

Merged

zombor merged 3 commits from bd-bookshelf-nnb9.7 into main 2026-08-09 12:19:07 +00:00

Conversation 2 Commits 3 Files changed 5 +115 -334

zombor commented 2026-08-09 02:35:17 +00:00

Owner

Copy link

Converts internal/db's white-box test file (export_test.go) to black-box, removing the last test-only backdoor from the db package.

What changed

• Deleted export_test.go, which re-exported unexported db.go symbols (maskDSN, min, retryParams, sqlOpenFunc, newMigrateSource/newDBDriver/newMigrator, migrator) purely so a white-box shim would compile.
• RetryConnect now takes an explicit RetryPolicy parameter (replacing the package-private mutable retryParams global + SetRetryParams shim) — tests pass a fast policy directly, no injection backdoor needed.
• Deleted the hand-rolled min() helper — Go 1.21+ has a builtin min; RetryConnect now calls the builtin directly.
• Open()'s malformed-DSN error path is now exercised with a real invalid DSN (go-sql-driver/mysql parses the DSN eagerly inside sql.Open's OpenConnector, so it fails for real, no SetSQLOpenFunc injection needed).
• RunMigrations' "migrate driver" error branch already had a real test (closed DB → mysql.WithInstance's Ping fails); added a new real "migrate up" test that marks schema_migrations dirty (a genuine crashed-migration scenario) to exercise that branch too.
• Removed the newMigrateSource/newDBDriver/newMigrator/migrator injection seams entirely — RunMigrations now calls iofs.New / mysql.WithInstance / migrate.NewWithInstance directly, since nothing swaps them anymore. migrate.NewWithInstance never actually returns an error in the pinned golang-migrate version, so that error branch was unreachable test-only dead weight; removing the seam doesn't lose any coverage guarantee (internal/db is excluded from the make coverage gate — it's verified by make integration instead).
• maskDSN's masking behavior is now asserted end-to-end via Open()'s real logged output instead of calling the unexported func directly.
• Removed internal/db/export_test.go from scripts/test_policy_check/allowlist.txt (23 → 22 entries).

Test plan

• go build ./..., go vet ./... — clean.
• go run ./scripts/test_policy_check . — OK, no net-new white-box offenders.
• golangci-lint run ./internal/db/... — 0 issues.
• go test -tags integration ./internal/db/... — all specs pass locally (real MySQL via testcontainers), including the two new real-path tests (malformed DSN, dirty-migration).

Docs: N/A (test-only change, no user-facing surface).

Closes bead bookshelf-nnb9.7 on merge.

Converts internal/db's white-box test file (export_test.go) to black-box, removing the last test-only backdoor from the db package. ## What changed - Deleted `export_test.go`, which re-exported unexported db.go symbols (`maskDSN`, `min`, `retryParams`, `sqlOpenFunc`, `newMigrateSource`/`newDBDriver`/`newMigrator`, `migrator`) purely so a white-box shim would compile. - `RetryConnect` now takes an explicit `RetryPolicy` parameter (replacing the package-private mutable `retryParams` global + `SetRetryParams` shim) — tests pass a fast policy directly, no injection backdoor needed. - Deleted the hand-rolled `min()` helper — Go 1.21+ has a builtin `min`; `RetryConnect` now calls the builtin directly. - `Open()`'s malformed-DSN error path is now exercised with a real invalid DSN (go-sql-driver/mysql parses the DSN eagerly inside `sql.Open`'s `OpenConnector`, so it fails for real, no `SetSQLOpenFunc` injection needed). - `RunMigrations`' "migrate driver" error branch already had a real test (closed DB → `mysql.WithInstance`'s `Ping` fails); added a new real "migrate up" test that marks `schema_migrations` dirty (a genuine crashed-migration scenario) to exercise that branch too. - Removed the `newMigrateSource`/`newDBDriver`/`newMigrator`/`migrator` injection seams entirely — `RunMigrations` now calls `iofs.New` / `mysql.WithInstance` / `migrate.NewWithInstance` directly, since nothing swaps them anymore. `migrate.NewWithInstance` never actually returns an error in the pinned golang-migrate version, so that error branch was unreachable test-only dead weight; removing the seam doesn't lose any coverage guarantee (`internal/db` is excluded from the `make coverage` gate — it's verified by `make integration` instead). - `maskDSN`'s masking behavior is now asserted end-to-end via `Open()`'s real logged output instead of calling the unexported func directly. - Removed `internal/db/export_test.go` from `scripts/test_policy_check/allowlist.txt` (23 → 22 entries). ## Test plan - `go build ./...`, `go vet ./...` — clean. - `go run ./scripts/test_policy_check .` — OK, no net-new white-box offenders. - `golangci-lint run ./internal/db/...` — 0 issues. - `go test -tags integration ./internal/db/...` — all specs pass locally (real MySQL via testcontainers), including the two new real-path tests (malformed DSN, dirty-migration). Docs: N/A (test-only change, no user-facing surface). Closes bead bookshelf-nnb9.7 on merge.

zombor added 1 commit 2026-08-09 02:35:18 +00:00

nnb9: convert internal/db tests to black-box ... 332ce0265e

export_test.go re-exported unexported db.go symbols (maskDSN, min,
retryParams, sqlOpenFunc, newMigrateSource/newDBDriver/newMigrator,
migrator) purely so white-box shims could compile — a test-policy-check
offender. Removed the backdoor and rewired every test path to a real
public caller instead:

- RetryConnect now takes an explicit RetryPolicy param (replacing the
  package-private mutable retryParams + SetRetryParams shim) — tests
  pass a fast policy directly, no injection needed.
- Deleted the hand-rolled min() helper (Go 1.21+ has a builtin min);
  RetryConnect now calls the builtin.
- Open()'s malformed-DSN error path is exercised with a real invalid
  DSN (go-sql-driver/mysql parses the DSN eagerly in OpenConnector, so
  sql.Open itself fails) instead of injecting via SetSQLOpenFunc.
- RunMigrations' "migrate driver" error branch already had a real
  test (closed DB -> mysql.WithInstance's Ping fails); added a new
  real "migrate up" test that marks schema_migrations dirty (a real
  crashed-migration scenario) to exercise that branch too.
- Removed the newMigrateSource/newDBDriver/newMigrator/migrator
  injection seams entirely (RunMigrations now calls iofs.New /
  mysql.WithInstance / migrate.NewWithInstance directly) since nothing
  needs to swap them anymore. NewWithInstance never actually returns
  an error in this golang-migrate version, so that error branch was
  unreachable test-only dead weight — removing the seam doesn't lose
  coverage guarantees (internal/db is excluded from the coverage gate;
  it's covered by make integration instead).
- maskDSN's masking behavior is now asserted end-to-end via Open()'s
  real logged output instead of calling the unexported func directly.

internal/db/export_test.go removed from
scripts/test_policy_check/allowlist.txt (23 -> 22 entries; only
db.go's own package db_test files remain).

Docs: N/A (test-only change, no user-facing surface).

zombor added 1 commit 2026-08-09 02:42:11 +00:00

fix(test): compare exact user:pass@ prefix, not bare password substring ... a8a8bb3124

CI's MySQL service uses matching user/pass (root:root), so a naive
substring check on the password alone spuriously failed — the
unmasked username "root" trivially contains the string "root".
Compare the full "user:pass@" credential prefix instead.

zombor commented 2026-08-09 02:49:12 +00:00

Author

Owner

Copy link

Security review — PR #1407 (bd-bookshelf-nnb9.7)

Reviewed the full diff (internal/db/db.go, internal/db/db_test.go, deletion of internal/db/export_test.go + internal/db/helpers_test.go, scripts/test_policy_check/allowlist.txt) against the PR description and the suspicion raised by sibling PR #1406 (production security-guard deletion disguised as "dead code").

Verdict up front: this one is clean — genuine test-only conversion + a trivial, behavior-preserving production simplification. No error-handling was removed, no credential exposure introduced, no retry-classification change.

Migration error-path check

RunMigrations still performs the exact same three error-wrapped steps it did before:

src, err := iofs.New(migrationsFS, "migrations")       // was: newMigrateSource()
if err != nil { return fmt.Errorf("migrate source: %w", err) }
drv, err := mysql.WithInstance(db, &mysql.Config{})     // was: newDBDriver(db)
if err != nil { return fmt.Errorf("migrate driver: %w", err) }
m, err := migrate.NewWithInstance("iofs", src, "mysql", drv) // was: newMigrator(src, drv)
if err != nil { return fmt.Errorf("migrate init: %w", err) }
if err := m.Up(); err != nil && !errors.Is(err, migrate.ErrNoChange) {
    return fmt.Errorf("migrate up: %w", err)
}

What was deleted (newMigrateSource, newDBDriver, newMigrator, the migrator interface) were only test-injection seams — package-level var indirections whose sole purpose was letting helpers_test.go swap in fakes to hit each error branch without a real DB. No if err != nil check, no return, no log call was removed. The two migration-failure integration tests that previously relied on fake injection (SetNewMigrateSource/SetNewDBDriver/SetNewMigrator returning canned errors) are replaced with real failure scenarios in db_test.go: a closed DB (real mysql.WithInstance ping failure → "migrate driver") and a dirty=1 schema_migrations row (real golang-migrate dirty-guard → "migrate up"). This is a coverage improvement, not a loss — no silent-failure risk introduced.

DSN / credential-substring change

maskDSN itself is byte-for-byte unchanged — still finds the last @, masks everything between the last : before it and the @ with ***. The only new code is a test-only helper dsnUserPassAt in db_test.go used to assert Open()'s real logged output never contains the full user:pass@ credential pair (replacing a prior white-box call to maskDSN directly). The helper's own doc comment explains why it compares the full "user:pass@" string rather than a bare password substring: DSNs where user and password are identical (e.g. root:root@) would otherwise let the password "coincidentally" appear (as the username) and pass a naive substring check. This is a stricter, not weaker, test assertion, and it's test code — it doesn't touch what gets logged in production. Confirmed no plaintext DSN/credential logging is introduced anywhere in the diff.

RetryPolicy signature change

RetryConnect(ctx, logger, connect) → RetryConnect(ctx, logger, policy RetryPolicy, connect). This replaces a mutable package-global retryParams (previously mutated in-place by tests via SetRetryParams, a real shared-state hazard) with an explicit, immutable-per-call struct argument. Open() still calls it with defaultRetryPolicy{MaxAttempts:5, BaseSleep:500ms, MaxSleep:30s} — identical values to the old defaults, so production retry behavior is unchanged. RetryConnect's loop still blindly retries connect() up to MaxAttempts regardless of error type (no classification of permanent vs transient failures) — that was already the pre-existing behavior; this PR does not change what errors get retried, only how the policy is threaded in. Not a regression.

Exported-surface check

RetryPolicy (a plain 3-field numeric struct) is newly exported from internal/db. This is an intentional, narrow widening (needed so both Open() and tests can construct a policy value without a mutable-global backdoor) — no sensitive data, no new attack surface. Minor note, not a finding worth blocking on.

Dead code claim verification

The PR description asserts migrate.NewWithInstance "never actually returns an error in the pinned golang-migrate version," which is why that fake-injection seam was dropped without a replacement real-path test. I did not independently verify this against the vendored golang-migrate version, but this only affects test coverage of an already-present if err != nil { return ... } guard in production code — the guard itself remains in place and will correctly propagate an error if the underlying library's behavior ever changes. Not a security concern regardless.

Comparison to PR #1406

Unlike #1406 (which reportedly deleted a live production security guard under a "dead code" label), this PR only removes test-injection indirection (var function pointers / interfaces that existed purely to let white-box tests fake dependencies) and replaces fake-injected failure paths with real ones. Every production if err != nil branch, every fmt.Errorf wrap, and maskDSN's masking logic survive unchanged.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security review — PR #1407 (bd-bookshelf-nnb9.7) Reviewed the full diff (`internal/db/db.go`, `internal/db/db_test.go`, deletion of `internal/db/export_test.go` + `internal/db/helpers_test.go`, `scripts/test_policy_check/allowlist.txt`) against the PR description and the suspicion raised by sibling PR #1406 (production security-guard deletion disguised as "dead code"). **Verdict up front: this one is clean — genuine test-only conversion + a trivial, behavior-preserving production simplification. No error-handling was removed, no credential exposure introduced, no retry-classification change.** ### Migration error-path check `RunMigrations` still performs the exact same three error-wrapped steps it did before: ```go src, err := iofs.New(migrationsFS, "migrations") // was: newMigrateSource() if err != nil { return fmt.Errorf("migrate source: %w", err) } drv, err := mysql.WithInstance(db, &mysql.Config{}) // was: newDBDriver(db) if err != nil { return fmt.Errorf("migrate driver: %w", err) } m, err := migrate.NewWithInstance("iofs", src, "mysql", drv) // was: newMigrator(src, drv) if err != nil { return fmt.Errorf("migrate init: %w", err) } if err := m.Up(); err != nil && !errors.Is(err, migrate.ErrNoChange) { return fmt.Errorf("migrate up: %w", err) } ``` What was deleted (`newMigrateSource`, `newDBDriver`, `newMigrator`, the `migrator` interface) were **only test-injection seams** — package-level `var` indirections whose sole purpose was letting `helpers_test.go` swap in fakes to hit each error branch without a real DB. No `if err != nil` check, no return, no log call was removed. The two migration-failure integration tests that previously relied on fake injection (`SetNewMigrateSource`/`SetNewDBDriver`/`SetNewMigrator` returning canned errors) are replaced with **real failure scenarios** in `db_test.go`: a closed DB (real `mysql.WithInstance` ping failure → "migrate driver") and a `dirty=1` `schema_migrations` row (real golang-migrate dirty-guard → "migrate up"). This is a coverage *improvement*, not a loss — no silent-failure risk introduced. ### DSN / credential-substring change `maskDSN` itself is **byte-for-byte unchanged** — still finds the last `@`, masks everything between the last `:` before it and the `@` with `***`. The only new code is a **test-only** helper `dsnUserPassAt` in `db_test.go` used to assert `Open()`'s real logged output never contains the full `user:pass@` credential pair (replacing a prior white-box call to `maskDSN` directly). The helper's own doc comment explains why it compares the full `"user:pass@"` string rather than a bare password substring: DSNs where user and password are identical (e.g. `root:root@`) would otherwise let the password "coincidentally" appear (as the username) and pass a naive substring check. This is a **stricter**, not weaker, test assertion, and it's test code — it doesn't touch what gets logged in production. Confirmed no plaintext DSN/credential logging is introduced anywhere in the diff. ### RetryPolicy signature change `RetryConnect(ctx, logger, connect)` → `RetryConnect(ctx, logger, policy RetryPolicy, connect)`. This replaces a mutable package-global `retryParams` (previously mutated in-place by tests via `SetRetryParams`, a real shared-state hazard) with an explicit, immutable-per-call struct argument. `Open()` still calls it with `defaultRetryPolicy{MaxAttempts:5, BaseSleep:500ms, MaxSleep:30s}` — **identical values** to the old defaults, so production retry behavior is unchanged. `RetryConnect`'s loop still blindly retries `connect()` up to `MaxAttempts` regardless of error type (no classification of permanent vs transient failures) — that was already the pre-existing behavior; this PR does not change what errors get retried, only how the policy is threaded in. Not a regression. ### Exported-surface check `RetryPolicy` (a plain 3-field numeric struct) is newly exported from `internal/db`. This is an intentional, narrow widening (needed so both `Open()` and tests can construct a policy value without a mutable-global backdoor) — no sensitive data, no new attack surface. Minor note, not a finding worth blocking on. ### Dead code claim verification The PR description asserts `migrate.NewWithInstance` "never actually returns an error in the pinned golang-migrate version," which is why that fake-injection seam was dropped without a replacement real-path test. I did not independently verify this against the vendored golang-migrate version, but this only affects *test coverage* of an already-present `if err != nil { return ... }` guard in production code — the guard itself remains in place and will correctly propagate an error if the underlying library's behavior ever changes. Not a security concern regardless. ### Comparison to PR #1406 Unlike #1406 (which reportedly deleted a live production security guard under a "dead code" label), this PR only removes **test-injection indirection** (`var` function pointers / interfaces that existed purely to let white-box tests fake dependencies) and replaces fake-injected failure paths with real ones. Every production `if err != nil` branch, every `fmt.Errorf` wrap, and `maskDSN`'s masking logic survive unchanged. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-09 02:51:39 +00:00

Author

Owner

Copy link

Code Review — PR #1407 (bd-bookshelf-nnb9.7)

Adversarial diff review of internal/db/db.go + test changes, with the sibling #1406 false-dead-code incident specifically in mind.

Verification performed

1. Deleted newDBDriver/newMigrator/newMigrateSource/migrator DI seams — these were test-injection-only indirection layers, not guards or security checks. Every error branch they used to let tests hit (migrate source, migrate driver, migrate init, migrate up) is still present verbatim in db.go post-diff; nothing was deleted from the actual error-handling logic, only the seam that let a fake implementation be substituted.
2. migrate.NewWithInstance "never errors" claim — verified directly against the vendored source (github.com/golang-migrate/migrate/v4@v4.19.1/migrate.go:171-181): the function body is m := newCommon(); ...; return m, nil — it is structurally incapable of returning a non-nil error. The claim is factually correct, not an excuse.
3. iofs.New for the embedded migrations FS — this is a build-time invariant (fixed embedded FS, not user input); realistically unreachable at runtime same as before. Not gated by coverage either (see #4), so no incentive to hide it.
4. Coverage gate scope — confirmed via scripts/check-coverage.sh that internal/db is explicitly excluded from the 100%-statement gate ("Integration- and DB-backed packages (internal/db, internal/dbtest) are NOT coverage-gated"). This removes the gate-gaming motive entirely — the seam deletion is driven by the black-box test-policy conversion (killing export_test.go's white-box symbol exports), not by chasing a coverage number.
5. RetryPolicy export — genuinely consumed by production code: Open() calls RetryConnect(ctx, logger, defaultRetryPolicy, ...) with defaultRetryPolicy as a real production value; it is not a test-only affordance. Passes the nnb9 no-export-to-game-blackbox check.
6. min() deletion — Go 1.21+ ships a builtin generic min; the hand-rolled one-liner was pure duplication, not a guard. Confirmed min(sleep*2, policy.MaxSleep) still compiles against the builtin.
7. New tests are real black-box, exercise genuine failure conditions: malformed DSN (sql.Open eager-parse failure), unreachable host + retry exhaustion, a broken/closed DB connection for mysql.WithInstance, and a dirty=1 schema_migrations row to hit golang-migrate's real dirty-check refusal path for Up(). These are strictly better tests than the deleted fake-injection versions — they hit the actual library code paths instead of a stand-in.
8. DSN-mask test password-substring fix: the new dsnUserPassAt helper compares the full user:pass@ credential prefix rather than a bare password substring, correctly avoiding a false pass when user == password (e.g. root:root). This is a new test (no prior masked-DSN test existed), not a "flake fix" that papers over something — legitimate hardening of the assertion.
9. Package hygiene: only internal/db/db_test.go remains and declares package db_test; export_test.go (white-box, package db) and helpers_test.go (which depended on export_test.go's exported aliases) are both deleted in full. allowlist.txt drops exactly the one now-nonexistent internal/db/export_test.go entry — a legitimate removal, not a broadened exception.

Findings

None. No production error path, guard, or security-relevant check was deleted — every branch removed was pure test-injection scaffolding, and the two theoretically-still-dead branches (migrate init, migrate source) were already effectively dead before this PR and remain untouched in behavior. This is a clean, honest nnb9 conversion, unlike the sibling #1406 case.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Code Review — PR #1407 (bd-bookshelf-nnb9.7) Adversarial diff review of `internal/db/db.go` + test changes, with the sibling #1406 false-dead-code incident specifically in mind. ### Verification performed 1. **Deleted `newDBDriver`/`newMigrator`/`newMigrateSource`/`migrator` DI seams** — these were test-injection-only indirection layers, not guards or security checks. Every error branch they used to let tests hit (`migrate source`, `migrate driver`, `migrate init`, `migrate up`) is **still present verbatim** in `db.go` post-diff; nothing was deleted from the actual error-handling logic, only the seam that let a fake implementation be substituted. 2. **`migrate.NewWithInstance` "never errors" claim** — verified directly against the vendored source (`github.com/golang-migrate/migrate/v4@v4.19.1/migrate.go:171-181`): the function body is `m := newCommon(); ...; return m, nil` — it is *structurally incapable* of returning a non-nil error. The claim is factually correct, not an excuse. 3. **`iofs.New` for the embedded `migrations` FS** — this is a build-time invariant (fixed embedded FS, not user input); realistically unreachable at runtime same as before. Not gated by coverage either (see #4), so no incentive to hide it. 4. **Coverage gate scope** — confirmed via `scripts/check-coverage.sh` that `internal/db` is explicitly **excluded** from the 100%-statement gate ("Integration- and DB-backed packages (internal/db, internal/dbtest) are NOT coverage-gated"). This removes the gate-gaming motive entirely — the seam deletion is driven by the black-box test-policy conversion (killing `export_test.go`'s white-box symbol exports), not by chasing a coverage number. 5. **`RetryPolicy` export** — genuinely consumed by production code: `Open()` calls `RetryConnect(ctx, logger, defaultRetryPolicy, ...)` with `defaultRetryPolicy` as a real production value; it is not a test-only affordance. Passes the nnb9 no-export-to-game-blackbox check. 6. **`min()` deletion** — Go 1.21+ ships a builtin generic `min`; the hand-rolled one-liner was pure duplication, not a guard. Confirmed `min(sleep*2, policy.MaxSleep)` still compiles against the builtin. 7. **New tests are real black-box, exercise genuine failure conditions**: malformed DSN (`sql.Open` eager-parse failure), unreachable host + retry exhaustion, a broken/closed DB connection for `mysql.WithInstance`, and a `dirty=1` `schema_migrations` row to hit golang-migrate's real dirty-check refusal path for `Up()`. These are strictly better tests than the deleted fake-injection versions — they hit the *actual* library code paths instead of a stand-in. 8. **DSN-mask test password-substring fix**: the new `dsnUserPassAt` helper compares the full `user:pass@` credential prefix rather than a bare password substring, correctly avoiding a false pass when `user == password` (e.g. `root:root`). This is a new test (no prior masked-DSN test existed), not a "flake fix" that papers over something — legitimate hardening of the assertion. 9. **Package hygiene**: only `internal/db/db_test.go` remains and declares `package db_test`; `export_test.go` (white-box, `package db`) and `helpers_test.go` (which depended on `export_test.go`'s exported aliases) are both deleted in full. `allowlist.txt` drops exactly the one now-nonexistent `internal/db/export_test.go` entry — a legitimate removal, not a broadened exception. ### Findings None. No production error path, guard, or security-relevant check was deleted — every branch removed was pure test-injection scaffolding, and the two theoretically-still-dead branches (`migrate init`, `migrate source`) were already effectively dead before this PR and remain untouched in behavior. This is a clean, honest nnb9 conversion, unlike the sibling #1406 case. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor added 1 commit 2026-08-09 12:11:48 +00:00

Merge branch 'main' into bd-bookshelf-nnb9.7 85ed683720

zombor merged commit ae45cd023f into main 2026-08-09 12:19:07 +00:00

zombor referenced this pull request from a commit 2026-08-09 12:19:11 +00:00

Merge pull request 'nnb9: black-box internal/db tests (bookshelf-nnb9.7)' (#1407) from bd-bookshelf-nnb9.7 into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1407

No description provided.