Database Utilities for our boxes

[iancooper]

We had some useful feedback that are two biggest usability issues were the complexity of configuration: there is an ADR for that, and the management of a box (inbox/outbox).

This addresses the management of a box. It derives from lessons from WebAPI sample.

It also leans into Aspire, because that is the expectation for developers.

[claude]

PR Review: Box Database Migration — ADR & Specification

This is a well-thought-out design for an important gap in Brighter's usability story. The problem statement is clear, the role-based architecture follows Brighter's Responsibility-Driven Design principles, and the alternatives-considered section is thorough. The concurrency control and bootstrap path for pre-migration installations are the two trickiest aspects — both are addressed.

Here are my findings, ranging from a design bug to minor consistency issues:

---

Design Bug: internal Registrations Breaks Cross-Package Extension

File: docs/adr/0053-box-database-migration.md (section 3)

The ADR defines BoxProvisioningOptions with an internal list:

public class BoxProvisioningOptions
{
    internal List<Action<IServiceCollection>> Registrations { get; } = [];
}

But then the MSSQL backend package (Paramore.Brighter.BoxProvisioning.MsSql) is shown calling options.Registrations.Add(...) from its extension methods. Because Registrations is internal, it is only visible within the Paramore.Brighter.BoxProvisioning assembly. Extension methods in a separate package (Paramore.Brighter.BoxProvisioning.MsSql) will get a compile error.

Suggested fix: Expose a public registration method while keeping the list internal:

public class BoxProvisioningOptions
{
    private readonly List<Action<IServiceCollection>> _registrations = [];
    internal IReadOnlyList<Action<IServiceCollection>> Registrations => _registrations;

    public void Add(Action<IServiceCollection> registration)
        => _registrations.Add(registration);
}

Then backend extension methods call options.Add(services => { ... }) instead of directly accessing options.Registrations.

---

Missing Error Handling in BoxProvisioningHostedService

File: docs/adr/0053-box-database-migration.md (section 2)

If ProvisionAsync throws (e.g., database unreachable, migration failed), the exception propagates unhandled through StartAsync and crashes the generic host at startup. While fail-fast on schema errors is often desirable, it should be a documented decision. The current code also logs "Provisioned {BoxType}" before the outcome is known — if the provisioner throws, the success log is never emitted but no failure log appears either.

Consider: Either wrap each ProvisionAsync in try/catch with structured error logging and re-throw, or explicitly document in the ADR that fail-fast on provisioning errors is intentional.

---

Invalid MSSQL Syntax in Migration History DDL

File: docs/adr/0053-box-database-migration.md (section 5)

The ADR shows:

CREATE TABLE IF NOT EXISTS [__BrighterMigrationHistory] (

MSSQL does not support IF NOT EXISTS in CREATE TABLE statements. The correct MSSQL idiom uses sys.tables:

IF NOT EXISTS (SELECT 1 FROM sys.tables WHERE name = '__BrighterMigrationHistory' AND schema_id = SCHEMA_ID('dbo'))
BEGIN
    CREATE TABLE [__BrighterMigrationHistory] ( ... )
END

This is a documentation-level issue now but will become a runtime bug when implemented. Worth correcting in the ADR so the implementation has accurate reference material.

---

Provisioner Ordering Not Enforced

File: docs/adr/0053-box-database-migration.md (section 1 / section 2)

The ADR states the BoxProvisioningHostedService "decides ordering (outbox before inbox if both present)" but the shown implementation iterates IEnumerable<IAmABoxProvisioner> in DI registration order with no ordering logic. If the ordering guarantee matters, enforce it explicitly:

var ordered = _provisioners.OrderBy(p => p.BoxType == BoxType.Outbox ? 0 : 1);

If ordering is deliberately left to DI registration order, the ADR should say so rather than implying the service itself handles ordering.

---

ADR Status Is "Accepted" on a Draft PR

The ADR header reads Status: Accepted but the PR is in DRAFT state. The conventional status for an ADR under review is "Proposed" — update to "Accepted" when the PR is merged.

---

Naming Inconsistency Between Requirements and ADR

File: specs/0023-box_database_migration/requirements.md (FR-3)

The requirements doc names the package Paramore.Brighter.Extensions.Hosting.MsSql, but the ADR uses Paramore.Brighter.BoxProvisioning.MsSql. Both documents should agree before implementation begins. The ADR naming (BoxProvisioning.*) is more consistent with the feature name and Brighter's existing conventions.

---

Forward-Only Migrations Should Be Explicitly Documented

File: docs/adr/0053-box-database-migration.md (section 2)

IAmABoxMigration has UpScript but no DownScript. Since the property name implies there could be a DownScript, the ADR should explicitly state this is a forward-only design and why (e.g., DDL rollbacks are rarely safe or needed for additive column changes).

---

MySQL Advisory Lock Timeout Unspecified

File: docs/adr/0053-box-database-migration.md (section 5, Concurrency Control)

The MySQL locking entry shows GET_LOCK('BrighterMigration_{tableName}', timeout) with timeout undefined. In slow environments (cold-starting containers, overloaded databases), a too-short timeout causes spurious failures; too long blocks startup indefinitely. Recommend specifying a default (e.g., 30 seconds) and making it configurable in BoxProvisioningOptions.

---

What is Done Well

• Problem statement is precise: The GetExistsQuery inconsistency table across backends is excellent motivation for a unified abstraction.
• Bootstrap path (section 7): Handling pre-migration installations via DetectCurrentVersionAsync() is the right approach and is well-explained.
• Concurrency control (section 5): Documenting per-backend locking mechanisms is exactly the right level of detail for an ADR.
• Alternatives considered: FluentMigrator, EF Core, single-package, and extending existing builders are all reasonably rejected with clear rationale.
• Backward compatibility: Preserving the existing static builders while the new library calls them internally is a clean approach.
• UpScript naming: The note about naming UpScript rather than UpSql for future non-SQL backend extensibility is a thoughtful detail.

---

Summary: The core architecture is sound. The internal Registrations accessor is the only issue that would prevent the design from compiling as written — worth resolving before tasks are created from this ADR. The MSSQL IF NOT EXISTS syntax and the other points are lower priority but worth addressing in the ADR before implementation begins so the implementation has accurate reference material.

[claude]

Follow-up Review: ADR 0053 after Design Review Fixes

All six issues from the previous review have been addressed correctly. This follow-up covers remaining issues in the updated document.

---

1. Design Gap: MigrationLockTimeout Not Wired to the Migration Runner

File: docs/adr/0053-box-database-migration.md (sections 3 and 5)

MigrationLockTimeout is added to BoxProvisioningOptions but the provisioner shown in section 4 does not receive it. The runner is created as new MsSqlBoxMigrationRunner(_configuration) with no timeout parameter, and the extension method registers new MsSqlOutboxProvisioner(configuration) without passing options.MigrationLockTimeout.

The ADR should specify how this value flows to the runner. One approach: backend extension methods capture the timeout and pass it to the provisioner constructor. Without this, MigrationLockTimeout is configurable but silently ignored at runtime.

---

2. MSSQL Advisory Lock Resource Not Per-Table

File: docs/adr/0053-box-database-migration.md (section 5, Concurrency Control)

MSSQL uses sp_getapplock with Resource='BrighterMigration' (a global string), while MySQL correctly scopes the lock to 'BrighterMigration_{tableName}'. This serializes migrations across all tables on the same database instance rather than just the target table. Since provisioners run sequentially this will not deadlock, but it adds unnecessary contention between unrelated applications on the same DB server. The MSSQL lock should include the table name for consistency with MySQL.

---

3. PostgreSQL Advisory Lock Hash Function Not Specified

File: docs/adr/0053-box-database-migration.md (section 5, Concurrency Control)

The table says pg_advisory_lock(hash) where hash is derived from the table name but does not specify the hash function. Without this, two implementers could choose different algorithms and produce non-colliding locks. Recommend specifying explicitly, e.g. hashtext('BrighterMigration_' || tableName) cast to bigint.

---

4. Component Table Still Says up/down DDL

File: docs/adr/0053-box-database-migration.md (section 1)

The IAmABoxMigration row in the component overview table says 'Describes a single migration step: version number, description, up/down DDL'. The phrase up/down DDL implies a DownScript exists, which contradicts the forward-only design documented in section 2. Should say 'forward migration DDL' or 'up DDL only'.

---

5. README Status Is Stale

File: specs/0023-box_database_migration/README.md

The README shows Status: Requirements and marks both checklist items as 'awaiting approval'. The third commit added both .requirements-approved and .design-approved marker files but did not update the README. It should reflect Status: Design (Approved) with both items checked as approved.

---

What the Fixes Got Right

• BoxProvisioningOptions pattern (private _registrations, internal IReadOnlyList Registrations, public Add()) correctly solves the cross-assembly accessibility problem.
• StartAsync ordering (outbox before inbox via OrderBy) is correct; the explanatory comment is valuable.
• Fail-fast catch/log/ConfigurationException wrapping is consistent with Brighter conventions.
• MSSQL history table DDL is now correct (sys.tables pattern).
• MigrationLockTimeout with 30-second default is the right approach for MySQL.
• Package naming is now consistent between requirements and ADR.

---

Summary: Items 2-5 are minor. Item 1 (MigrationLockTimeout wiring) is worth resolving in the ADR before tasks are written — an implementer following the ADR as written would produce code where the timeout is configurable but has no effect.

[claude]

Test comment from review

[claude]

PR Review: Database Utilities for Box Management (ADR + Spec)

This is a well-structured spec PR. The ADR is thorough and the decision to build a dedicated BoxProvisioning library rather than extending existing static builders is sound.

---

Strengths

• Clear problem statement - The four gaps (no migrations, no unified abstraction, sample-only orchestration, no Aspire integration) are well-articulated. The table of inconsistent GetExistsQuery signatures is a compelling motivator.
• Role-Based Architecture naming - Labelling components with RDD roles (Coordinator, Service Provider, Information Holder) is consistent with Brighter design vocabulary.
• Deliberate forward-only migrations - The rationale for no DownScript is sound for a library-level tool.
• Bootstrap for pre-migration installs - The synthetic history row approach is pragmatic and avoids breaking existing deployments.
• Alternatives considered - Rejecting FluentMigrator and EF Core Migrations is well-reasoned; avoids forcing heavy dependencies on consumers.
• UpScript over UpSql - Smart naming for future non-SQL backend extensibility.

---

Issues

1. Bug in BoxProvisioningHostedService log statement

The example has one {BoxType} placeholder but passes the value twice. The second argument is unused. Either remove the duplicate or add a second named placeholder.

2. DetectCurrentVersionAsync() bootstrap assumption

The default returns 1, assuming all pre-migration installs are at version 1. If a version-2 table was created manually, the bootstrapper could insert a synthetic v1 history row and then try to apply v2 migrations to a table that already has v2 columns. Recommend documenting the v1 assumption in the interface contract, and considering column-inspection via INFORMATION_SCHEMA.COLUMNS on MSSQL/PostgreSQL to infer a more accurate starting version.

3. PostgreSQL advisory lock hash collision

pg_advisory_lock(hashtext('BrighterMigration_' || tableName)::bigint) - two different table names could theoretically produce the same hash, causing spurious contention. Using the two-argument form pg_advisory_lock(bigint, bigint) with a Brighter namespace constant as arg1 and the hash as arg2 eliminates this. Worth noting as a known trade-off at minimum.

4. MigrationLockTimeout unit conversion

MigrationLockTimeout is a TimeSpan, but MySQL GET_LOCK expects whole seconds and MSSQL sp_getapplock expects milliseconds as an int. The ADR (Section 5) should document the per-backend unit conversion explicitly to prevent implementation bugs.

5. Aspire Hosting extension - insufficient detail

Section 8 mentions WithBrighterOutbox(sqlServer, tableName: "Outbox") but does not say what it does. Does it provision database infrastructure, annotate the resource, or set environment variables? This needs more detail for implementers.

6. Binary vs text payload and migration versioning

binaryMessagePayload produces a structurally different table schema. If a user switches this flag after initial provisioning, version 1 is already marked applied in the history table but the schema does not match the new mode. Recommend documenting this as unsupported post-creation, or adding a schema validation step.

7. UseBoxProvisioning - double-registration

Calling UseBoxProvisioning more than once would register multiple BoxProvisioningHostedService instances, running all provisioners multiple times at startup. A simple registration guard is worth adding.

---

Minor Nits

• PostgreSQL outbox binary variant missing IF NOT EXISTS - confirm this is addressed in migration v1 DDL or tracked separately.
• No tasks.md yet - expected for a DRAFT PR, but this is the next step before implementation.

---

Summary

The architecture is coherent, trade-offs are well-reasoned, and the modular package structure is the right call. Key items before creating tasks.md:

1. Fix the duplicate argument in the LogError call.
2. Clarify DetectCurrentVersionAsync contract and v1 assumption.
3. Expand the Aspire Hosting extension description in Section 8.
4. Document TimeSpan-to-backend-unit conversions in Section 5.
5. Document binary/text payload mode as unsupported to change post-creation, or add a guard.

Looking forward to tasks.md and the implementation!

[claude]

PR Review: Database Utilities for Boxes (ADR 53 + Spec 0023)

This is a design-only PR (ADR + requirements spec, no implementation yet), so the review focuses on the architectural decisions and their implications.

Overall Assessment

The design is well-thought-out. The problem statement is precise, the role-based architecture is consistent with Brighter's RDD approach, and the alternatives analysis is thorough. A few areas worth discussing before implementation begins.

---

Strengths

• Fail-fast behavior on startup is the right call - an app that cannot provision its box tables should not start.
• Forward-only migrations is the right safety choice; DDL rollbacks risk data loss.
• Bootstrap path for pre-migration installations is well-designed and necessary for brownfield adoption.
• Advisory lock concurrency control is properly per-backend - MySQL GET_LOCK, MSSQL sp_getapplock, PostgreSQL pg_advisory_lock. The table of backend/unit conversions is a nice touch.
• Reusing existing builder DDL for version-1 migrations avoids DDL drift and is the right call.
• Modular package structure is consistent with Brighter's existing per-backend pattern.

---

Issues and Questions

1. PostgreSQL advisory lock has no timeout

The design notes:

PostgreSQL: pg_advisory_lock ... blocks indefinitely

In a Kubernetes rolling deployment, a pod crashing mid-migration releases the session-level lock on connection close, so true indefinite deadlock is not the main concern. The concern is that an indefinitely blocking startup gives operators no signal about what is happening. Consider using pg_try_advisory_lock with a retry loop and the configurable MigrationLockTimeout, matching the MSSQL/MySQL behavior. At minimum, emit a "waiting for migration lock" log message so operators understand why startup is slow.

2. DetectCurrentVersionAsync default of 1 is risky for newer fresh installs

The default implementation returns 1, assuming the table was created by the original static builder. But if a user creates a fresh database with a current Brighter version that already includes DataRef/SpecVersion columns in the DDL, the bootstrap path would record version 1 as applied and then try to apply version 2's ALTER TABLE ADD COLUMN - which fails with "column already exists" on MySQL (no IF NOT EXISTS COLUMN support).

The mitigation ("backend-specific provisioners should override to introspect actual column existence") needs to be called out as required for every backend that ships migrations beyond version 1, not optional. The default returning 1 should be a last resort, not the expected path for installs created with newer DDL.

3. __BrighterMigrationHistory PK does not include schema name

The composite PK is (BoxTableName, MigrationVersion). In a multi-tenant deployment where dbo.Outbox and tenant2.Outbox coexist in the same database, migration history rows will collide. Consider (SchemaName, BoxTableName, MigrationVersion) as the PK, with SchemaName defaulting to the database default schema where omitted.

4. MsSqlOutboxProvisioner constructs MsSqlBoxMigrationRunner internally

The ADR says IAmABoxProvisioner and IAmABoxMigrationRunner are interfaces testable via mocks, but the example implementation hard-codes runner construction inside ProvisionAsync:

public async Task ProvisionAsync(CancellationToken cancellationToken = default)
{
    var runner = new MsSqlBoxMigrationRunner(_configuration, _migrationLockTimeout);

The runner should be injected (constructor injection or via a factory) to preserve the testability guarantee stated in the ADR.

5. binaryMessagePayload mode mismatch is silent

The provisioner does not validate that the existing table schema matches the configured payload mode.

A misconfigured payload mode risks silent data corruption (binary data stored as text or vice versa) rather than a startup failure. Consider adding a schema introspection step that checks the actual column type against the configured mode and logs a warning or throws on mismatch. This class of misconfiguration can be detected at startup.

6. Connection lifecycle is unspecified in the design

Provisioners receive IAmARelationalDatabaseConfiguration (connection string) but the design does not show how connections are opened and closed inside the migration runner. Is this raw SqlConnection, IAmARelationalDbConnectionProvider, or something else? The implementation tasks should be explicit about the connection strategy, especially for the advisory lock which must be held on the same connection for the duration of migrations.

7. specs/.current-spec pointer change

The PR changes .current-spec from 0021-Error-Examples to 0023-box_database_migration. Is spec 0021 complete? If still active, this could affect spec tooling for that spec.

---

Minor Notes

• NFR-2 says "Migration version checks should be lightweight (single query to check current version)", but the runner makes several queries per startup (create history table, query applied versions, conditionally run migrations). "A small number of lightweight queries" would be a more accurate requirement.
• The Aspire hosting package is named Paramore.Brighter.BoxProvisioning.Aspire.Hosting. As Aspire integration grows beyond BoxProvisioning (e.g. transport provisioning), a broader Paramore.Brighter.Aspire.Hosting may be a better long-term home for all Aspire AppHost extensions - worth considering before the package name is published.
• The BoxType ordering in BoxProvisioningHostedService uses OrderBy(p => p.BoxType == BoxType.Outbox ? 0 : 1). A comment explaining the rationale ("outbox first: critical path for message production") would help future maintainers.

---

Summary

The architecture is solid and ready for task breakdown and implementation. The most important issues to address:

1. PostgreSQL lock timeout - add a timeout/retry or at minimum a "waiting for lock" log message
2. DetectCurrentVersionAsync correctness - make backend override mandatory for backends with v2+ migrations; ensure bootstrap is safe for fresh installs on newer Brighter builds
3. Migration history PK - include schema name to support multi-tenant scenarios
4. Runner injection - inject IAmABoxMigrationRunner via constructor rather than constructing inline, to preserve the stated testability guarantee

[claude]

PR Review: ADR-0053 Box Database Migration

This is a well-thought-out design PR with a clear ADR and requirements document. The architecture is solid and addresses a genuine usability gap. Below is detailed feedback.

---

Overall Assessment

The design is sound. The Responsibility-Driven approach (coordinator + service providers), forward-only migrations, fail-fast hosted service, and advisory locking for concurrency are all good choices. The ADR does an excellent job documenting rejected alternatives.

---

Design Issues Worth Addressing Before Implementation

1. Design Gap: Who Calls DetectCurrentVersionAsync?

Section 7 (Bootstrap) says the migration runner calls provisioner.DetectCurrentVersionAsync(), but IAmABoxMigrationRunner.MigrateAsync receives only (tableName, schemaName, migrations, cancellationToken) -- it has no reference to the provisioner.

Either:

• The provisioner calls DetectCurrentVersionAsync() itself inside ProvisionAsync() before delegating to the runner, passing a currentVersion to MigrateAsync; or
• MigrateAsync receives a Func<CancellationToken, Task<int>> detectVersion delegate; or
• A new int currentVersion parameter is added to MigrateAsync.

The current ADR leaves this unresolved. Recommend option 1: the provisioner is the right owner since it holds backend-specific schema introspection knowledge, and the runner's responsibility is purely "apply these migrations starting from this version".

2. MigrationLockTimeout Capture Timing

In the extension methods, lockTimeout is captured when AddMsSqlOutbox() is called:

var lockTimeout = options.MigrationLockTimeout; // captured NOW
options.Add(services => { var runner = new MsSqlBoxMigrationRunner(config, lockTimeout); });

If a caller sets MigrationLockTimeout after calling AddMsSqlOutbox(), their value is silently ignored. This is a developer footgun. Consider capturing it lazily (read from options inside the lambda) or documenting clearly that MigrationLockTimeout must be set before calling Add*Outbox().

3. Spanner DDL Cannot Run in Transactions

The ADR states "Spanner transactions provide serializable isolation by default" and implies DDL runs within transactions. However, Cloud Spanner DDL statements cannot be run inside read-write transactions -- they must be submitted via ExecuteDdlAsync (a separate DDL batch operation). The concurrency model for Spanner migrations therefore needs a different design (Spanner-level DDL is inherently serialized; the history table update can use a normal transaction, but the DDL itself cannot). This should be called out explicitly in the ADR rather than leaving it to the implementer to discover.

4. Missing Symmetric Inbox Overloads

The ADR shows AddMsSqlOutbox(connectionName, ...) but only AddMsSqlInbox(configuration). If AddMsSqlOutbox has a connection-name overload for Aspire, AddMsSqlInbox should too. The same applies to all other backends. The ADR should explicitly address this or note it as a follow-up.

---

Minor Issues

5. PostgreSQL Advisory Lock Hash Collision Scope

The ADR correctly notes that hashtext is 32-bit and collisions are possible, and concludes this causes "spurious contention but no correctness issue." However, pg_advisory_lock is database-scoped -- a collision with an application-level advisory lock could cause unrelated operations to block. The two-argument form pg_advisory_lock(namespaceConstant, hashtext(tableName)) would eliminate cross-application interference entirely and is only marginally more complex.

6. MSSQL Nested Transaction Semantics

The ADR describes MSSQL as: a "lock transaction" spans the entire run (for sp_getapplock), while each migration also runs "in its own transaction". MSSQL does not support true nested transactions -- BEGIN TRANSACTION inside an outer transaction increments @@TRANCOUNT but the inner COMMIT only decrements the count. A rollback at any level rolls back everything. The ADR should clarify whether per-migration transactions are implemented via savepoints or whether the lock and migration transactions are actually the same transaction.

7. IAmABoxMigration as Interface

IAmABoxMigration is a read-only data contract with three properties. An interface here implies extensibility, but the ADR does not identify a scenario where a custom implementation is needed. A record or sealed record for BoxMigration used directly in the list would be simpler and harder to misuse.

8. Bootstrap Concurrency Not Explicitly Covered by Lock

Section 7 describes the bootstrap path but does not explicitly state it is protected by the advisory lock. Two instances concurrently bootstrapping the same table could each insert synthetic history rows. Explicitly noting "the bootstrap path runs within the advisory lock" would prevent implementers from inadvertently placing the lock acquisition after the exists-check.

9. StopAsync No-op and Cancellation

If the CancellationToken passed to StartAsync is cancelled mid-migration (e.g. the host is shutting down before startup completes), individual migration steps will continue unless they propagate the token to the DbCommand. The ADR should note that DbCommand.ExecuteNonQueryAsync(cancellationToken) must be used throughout.

---

Nits

• MySQL ALTER TABLE ... IF NOT EXISTS for columns: only available from MySQL 8.0+. If Brighter targets older MySQL versions, this safety net is not available. Worth documenting the minimum supported MySQL version for migrations.
• specs/.current-spec: This file is changed from 0021-Error-Examples to 0023-box_database_migration. If spec-0021 implementation is still in progress, this tracking file change could confuse the tooling. Intentional?
• The PostgreSQL locking cell in the concurrency table (section 5) is notably longer than the other cells -- consider extracting it to a separate paragraph for readability.

---

Summary

The design is solid and ready for implementation with the following clarifications needed:

Priority	Issue
Must address	Bootstrap / DetectCurrentVersionAsync ownership (section 7 design gap)
Must address	Spanner DDL transaction model
Should address	MigrationLockTimeout capture timing footgun
Should address	Symmetric Add*/Inbox(connectionName) overloads
Nice to have	MSSQL nested transaction clarification
Nice to have	PostgreSQL advisory lock scope discussion

Great work on the thorough requirements and ADR -- the "Alternatives Considered" section is especially useful for future maintainers.

[claude]

ADR Review: Box Database Migration (ADR 0053 / Spec 0023)

This is a well-researched design document that addresses a real pain point in Brighter. The architecture is clearly reasoned, the alternatives are well-considered, and the Responsibility-Driven Design role table in section 1 is exemplary. The following is a detailed review to harden the design before implementation begins.

Strengths

• Bootstrap path is explicitly modeled as a first-class BoxTableState record, eliminating the ambiguity of implicit null or sentinel version numbers.
• Forward-only migrations rationale is well-articulated. The call-out that the Up prefix is retained for convention alignment (not to imply a Down) avoids future confusion.
• Concurrency section is thorough: backend-specific locking mechanisms, TimeSpan-to-backend-unit conversion table, and the Spanner DDL/history atomicity failure window are all documented with mitigations.
• The UpScript naming rationale (avoiding UpSql for future non-SQL backend extensibility) is a nice forward-looking detail.

---

Issues to Address Before Implementation

1. Bootstrap Race Condition (Medium)

The provisioner detects BoxTableState before the advisory lock is acquired (the lock lives in the migration runner, not the provisioner). Two concurrent instances could both detect BoxTableState(TableExists: true, HistoryExists: false, CurrentVersion: 1) before either holds the lock. The runner for the second instance would then receive stale state and attempt to insert synthetic history rows that the first instance already inserted, causing a primary-key violation on (SchemaName, BoxTableName, MigrationVersion).

The ADR states "The entire bootstrap path runs within the advisory lock", but does not show the runner re-verifying HistoryExists after lock acquisition.

Recommendation: Either (a) move DetectTableStateAsync to occur after the advisory lock is acquired inside the runner, or (b) use INSERT IF NOT EXISTS / MERGE semantics for synthetic history rows so duplicate inserts are idempotent.

---

2. MSSQL Single-Transaction Design (Medium)

Section 5 describes using a single transaction for both the advisory lock and all migration DDL/history inserts. While MSSQL supports transactional DDL (unlike MySQL), there are practical risks: with many migrations, holding one open transaction throughout all steps may cause lock escalation or timeout issues in high-concurrency schemas. There is also ambiguity about whether partial success is desired.

Recommendation: Clarify whether the design intends one transaction per migration (each UpScript + history insert as one unit) vs. one transaction for all migrations. If the latter, document the rationale explicitly.

---

3. SchemaName Interface Gap: Prerequisite Ordering (Minor)

The ADR correctly identifies in section 10 that IAmARelationalDatabaseConfiguration must gain a SchemaName property. However, this is a breaking change to a public interface — all implementors, including third-party ones, must add the member.

Recommendation: In the tasks document, mark this as the very first task and note it requires either a SemVer major bump or a default interface member (string? SchemaName => null;) to remain backward-compatible.

---

4. Existing Bug in SchemaCreation.cs (Informational)

There is a parameter-order mismatch in samples/WebAPI/WebAPI_Common/DbMaker/SchemaCreation.cs. It calls SqlInboxBuilder.GetExistsQuery(tableSchema, INBOX_TABLE_NAME) but the actual method signature is GetExistsQuery(string inboxTableName, string schemaName). The sample currently swaps schema and table name when checking inbox existence.

This is a pre-existing bug, not introduced by this PR. Worth fixing as part of the FR-4 sample update task.

---

5. BoxProvisioningOptions API Underspecified (Minor)

The ADR shows UseBoxProvisioning(options => { options.AddMsSqlOutbox(config); }) but does not define:

• The full signature of AddMsSqlOutbox(config) vs. AddMsSqlOutbox() (the Aspire variant)
• Whether BoxProvisioningOptions holds IAmABoxProvisioner instances directly or factories
• How MigrationLockTimeout is configured (referenced in the concurrency table but not shown on any options class)

Recommendation: Add a BoxProvisioningOptions class definition to the ADR to make the registration API concrete before implementation starts.

---

6. BoxMigration Record Undefined (Minor)

BoxMigration is listed in the package structure as a "Simple record implementation" and is used in migration factory examples, but its definition is never shown. Worth adding to the ADR since it is the canonical concrete type implementors will use.

---

7. Payload Mode Validation Adds Startup Coupling (Minor)

Section 6 describes introspecting the actual column type at startup to validate binaryMessagePayload matches the deployed schema. This adds complexity (an extra DB round-trip, per-backend column-type mapping with ambiguous MySQL entries: LONGTEXT / TEXT) and creates a failure mode if INFORMATION_SCHEMA is slow or blocked.

Recommendation: Consider whether this validation is strictly necessary given that mismatches would surface at runtime via serialisation failures. If retained, the MySQL column type match logic needs to handle both LONGTEXT and TEXT explicitly.

---

Nits

• The BoxType enum uses { Inbox, Outbox } without explicit integer values. Consider { Outbox = 0, Inbox = 1 } to make the ordering in OrderBy(p => p.BoxType == BoxType.Outbox ? 0 : 1) less fragile if the enum ever gains new members.
• ADR title is "Database Migration" but the PR title is "Database Utilities" — worth aligning.
• Section 8 states Aspire integration is out of scope, but the PR body says "It also leans into Aspire." The ADR and PR description should align on this scope boundary.

---

Summary

The design is architecturally sound and well-suited for the Brighter ecosystem. The main item to resolve before implementation is the bootstrap race condition (Issue 1). The spec/ADR workflow has been correctly followed. Looking forward to seeing the tasks document and MSSQL backend implementation.

Reviewed by Claude Sonnet 4.6

[claude]

PR Review: Database Utilities for Boxes (ADR 0053 + Spec 0023)

This is a documentation-only PR (ADR, requirements, and tasks) - no implementation code yet. The overall design is well-reasoned and clearly written.

Strengths

• Clear problem statement. The three gaps (no migration support, inconsistent APIs, sample-only orchestration) are concrete and well-evidenced with the builder parameter comparison table.
• Solid role decomposition. Separating IAmABoxProvisioner from IAmABoxMigrationRunner follows Responsibility-Driven Design and keeps the interfaces minimal.
• BoxTableState record is excellent. Encoding the three scenarios (fresh install, bootstrap, normal) in a typed record eliminates ambiguity that would live as implicit conventions inside the runner.
• Concurrency control is handled per-backend with an honest table documenting each backend locking mechanism, the Spanner failure window, and the PostgreSQL hash collision risk.
• Forward-only migration rationale is sound and consistent with how Brighter handles misconfiguration elsewhere.
• TryAddEnumerable for hosted service registration correctly prevents duplicates and mirrors the UseOutboxSweeper pattern.

Design Questions and Concerns

1. IAmARelationalDatabaseConfiguration.SchemaName is a breaking change (Task 0.1)

Adding SchemaName to a public interface breaks any implementors outside this repo. The ADR acknowledges this but notes there are no known external implementors - that is a difficult claim to verify for a library in the wild. Consider adding it as a default interface member to avoid breaking existing implementors. The risk should also appear in the ADR Consequences section.

2. BoxProvisioningHostedService may affect readiness probes

IHostedService.StartAsync blocks the host from completing startup. If a provisioner waits on an advisory lock timeout (default 30s), health check endpoints may report unhealthy during that window. Worth documenting in Consequences - operators on Kubernetes will need to tune initialDelaySeconds appropriately.

3. PostgreSQL advisory lock - hash collision risk is understated

The ADR notes hashtext returns 32-bit integers and collisions cause spurious contention. However, if two box tables hash to the same value they will serialize against each other at startup - in deployments with many services starting simultaneously this could cause cascading slow starts. The two-argument form pg_advisory_lock(constant, hashtext(name)) with a Brighter-specific namespace constant is mentioned as an alternative but dismissed as overly complex. For library infrastructure code, the collision-resistant form is worth the small extra complexity.

4. MSSQL: single-transaction semantics for lock and migrations is underdocumented

The ADR correctly notes MSSQL uses a single transaction for both lock and all migrations. This means a failed migration N rolls back migrations 1..N-1 applied in the same run - different from the per-migration transaction semantics in other backends. When multiple migrations are pending (upgrading several versions at once), this is a meaningful difference. The ADR should document this explicitly to avoid surprises for contributors implementing the MSSQL runner.

5. Payload mode validation ambiguity

Tasks 2.5 and 2.6 describe payload mode mismatch detection as part of DetectTableStateAsync, but this information does not flow through BoxTableState. The ADR should clarify whether payload validation is a distinct step (e.g., a ValidateSchemaAsync method) or an internal implementation detail of each provisioner. As written it is ambiguous.

6. MigrationLockTimeout is captured at registration time

The timeout is captured from BoxProvisioningOptions when options.Add() is called, before DI resolution. Changing the timeout after calling AddMsSqlOutbox() has no effect. This is correct behaviour but will surprise users. A note in the XML doc for MigrationLockTimeout would help.

7. Multiple UseBoxProvisioning calls silently create independent option instances

TryAddEnumerable prevents duplicate BoxProvisioningHostedService registrations, but each UseBoxProvisioning call creates a new BoxProvisioningOptions instance. A user calling UseBoxProvisioning twice expecting shared options will be silently surprised. Worth a note in the UseBoxProvisioning XML doc.

8. Spanner: idempotency contract for UpScript should be explicit

The ADR says the Spanner runner must catch column-already-exists errors when DDL was applied but the history row was not written. This shifts responsibility onto migration authors to write idempotent UpScript values. Consider encoding this formally in IAmABoxMigration or in the Spanner runner documentation.

Minor and Style Notes

• Aspire out-of-scope is inconsistent. requirements.md and ADR section 8 say Aspire integration is out of scope, but the connectionName overload in section 3 (resolving from IConfiguration at DI resolution time) is the Aspire integration path. What is deferred is deeper Aspire hosting integration. Clarifying this distinction would avoid confusion.

• tidy-first concern in Task 2.6. The task includes a structural refactor (extract shared payload validation logic) within a behaviour task. Per the project guidelines, structural changes should be separate. Consider a dedicated structural task between 2.5 and 2.6.

• Verified: SpannerOutboxBuilder is indeed missing DataRef and SpecVersion - checked against the source. Task 0.2 is correctly identified.

• Verified: IAmARelationalDatabaseConfiguration does not expose SchemaName - checked against the source. Task 0.1 is correctly identified as a prerequisite.

Summary

The design is solid and the ADR is thorough. Main items worth addressing before implementation begins:

1. Document the breaking change risk of adding SchemaName to the interface in the ADR Consequences section (or use default interface members).
2. Prefer the two-argument pg_advisory_lock form for PostgreSQL to avoid hash collisions in library infrastructure.
3. Clarify MSSQL single-transaction-for-all-migrations semantics vs per-migration transactions in other backends.
4. Align the Aspire out-of-scope statement - the connectionName overload is already the Aspire path and is in scope for this ADR.

The task breakdown in tasks.md is well-structured with clear TDD commands, test file names, and approval gates. It will be straightforward to implement incrementally.

[claude]

PR Review: Database Utilities for Boxes (Spec 0023 / ADR 0053). This is a spec/design-only PR. See full review posted separately.

[claude]

PR Review: Database Utilities for Boxes (Spec 0023 / ADR 0053)

This is a spec/design-only PR (no code implementation yet), so the review covers the ADR, requirements, and task plan quality.

Overall: Excellent and thorough design document. The architecture is well-reasoned, consistent with existing Brighter patterns, and trade-offs are clearly articulated. A few issues worth addressing before implementation begins.

[claude]

Issues

1. PostgreSQL Namespace Constant Derivation is Inaccurate

The ADR states the constant 74726 is derived from the ASCII values of 'Br' (B=66, r=114). However none of the common derivations produce that value (66x1000+114=66114, 66x256+114=17010). 74726 appears to be an arbitrary constant. Either correct the derivation with the actual calculation, or describe it as an application-specific constant reserved for Brighter. The value itself does not matter — it just needs to be stable and accurately documented.

2. MySQL ALTER TABLE ADD COLUMN IF NOT EXISTS Availability

Section 7 mentions using IF NOT EXISTS on ALTER TABLE ADD COLUMN as a safety net for MySQL. This syntax is only available in MariaDB (since 10.0.2) and MySQL 8.0.2+. MySQL 5.7.x does not support it. The ADR should either state a minimum MySQL version requirement, or describe an alternative guard (e.g. query INFORMATION_SCHEMA.COLUMNS before issuing ALTER TABLE). The concern is real: if DetectCurrentVersionAsync returns v1 as a fallback for a table already at v2, the runner would try to add existing columns and fail on MySQL 5.7.

3. MSSQL All-or-Nothing Semantics Belongs in Risks/Mitigations

Section 5 explains MSSQL all-or-nothing migration behaviour clearly but only in prose. Since this meaningfully differs from all other backends — a failed migration N rolls back migrations 1..N-1 in the same run — it deserves a dedicated entry in the Risks and Mitigations section. Operators troubleshooting a failed multi-version upgrade on MSSQL could be confused when intermediate migrations disappear from the history table.

[claude]

4. PostgreSQL Retry Loop Must Respect CancellationToken

Section 5 describes the pg_try_advisory_lock retry loop bounded by MigrationLockTimeout, but does not mention CancellationToken propagation within the inter-attempt sleep. If the host signals shutdown while the runner is waiting for the lock, the retry loop should exit promptly (e.g. via Task.Delay(delay, cancellationToken)). Section 5 already calls out that tokens must flow to every DbCommand.ExecuteNonQueryAsync — the same principle applies to the retry sleep.

5. MigrationLockTimeout Ordering Sensitivity

BoxProvisioningOptions documents that setting MigrationLockTimeout after calling a backend extension method has no effect on previously-registered provisioners. This is a subtle footgun. Consider documenting a required pattern (timeout set before any Add* calls), or accepting it as a direct parameter on UseBoxProvisioning to make the ordering constraint explicit and less error-prone.

[claude]

Minor Observations

• Task 2.5a numbering: The 'a' suffix is unusual in the task list. Consider renaming to Task 2.5.1 for consistency.
• SchemaName DEFAULT in __BrighterMigrationHistory: The MSSQL DDL hardcodes DEFAULT 'dbo' for SchemaName. Each backend runner should use its appropriate default ('public' for PostgreSQL, no schema concept for MySQL/SQLite). Worth confirming in task notes for Tasks 2.2 and 3.1.
• RelationalBoxMigrationRunner name in section 5: Section 5 implies one shared class, but section 9 shows per-backend runners (MsSqlBoxMigrationRunner, etc.). A clarifying sentence would prevent contributor confusion.
• Spanner GetExistsQuery replacement: The requirements note SpannerInboxBuilder is entirely missing GetExistsQuery. The tasks do not explicitly state that the provisioner's DoesTableExistAsync is the replacement. A brief note in Task 6.1 would help.

What's Working Well

• The BoxTableState record cleanly captures the three provisioning scenarios (fresh install, bootstrap, normal) in the type system, eliminating conditional branching scattered through the runner.
• Reusing existing builder DDL for v1 migrations avoids schema duplication and drift.
• The TryAddEnumerable guard on hosted service registration is correct; the explanation of why multiple UseBoxProvisioning calls remain safe is well-documented.
• Fail-fast ConfigurationException wrapping is consistent with Brighter's existing startup error strategy.
• The Spanner failure window (DDL outside transactions) is acknowledged and the catch-already-exists mitigation is pragmatic.
• No DownScript is explicitly justified rather than just omitted — good design rationale.
• tasks.md enforces the mandatory /test-first TDD workflow with approval gates, consistent with CLAUDE.md requirements.

Generated with Claude Code

[claude]

test - please ignore

[claude]

PR Review: Database Utilities for our boxes (Spec 0023 / ADR 0053)

This is a well-crafted design PR. The ADR is thorough, the rationale for alternatives rejected is solid, and the Responsibility-Driven Design structure maps cleanly to Brighter existing patterns. A few observations below.

Strengths

• BoxTableState record - making the three provisioning scenarios (fresh install, bootstrap, normal migration) explicit in the type system is exactly right. Eliminates a whole class of ambiguous integer-semantics bugs.
• TryAddEnumerable guard - preventing double registration of BoxProvisioningHostedService when UseBoxProvisioning is called multiple times is a nice defensive touch.
• migrationLockTimeout as a first-class parameter on UseBoxProvisioning - the ADR correctly identifies the ordering footgun (setting MigrationLockTimeout after Add* calls is silently ignored) and mitigates it well.
• Forward-only migrations with no DownScript - the rationale is sound for an additive-only library shipped in a NuGet package.
• Bootstrap path - detecting pre-migration installations via column introspection and inserting synthetic history rows is the correct approach; treating it as a separate concern from the runner's normal path keeps each path readable.
• Alternatives Considered section - the FluentMigrator, EF Core, single-package, and extend-builders alternatives are all worth documenting and the rejections are well-argued.

[claude]

Issues and Questions

1. BoxType on IAmABoxProvisioner leaks a hosting concern into the domain interface

The only consumer of BoxType is BoxProvisioningHostedService.StartAsync, which uses it to sort provisioners. Ordering is a hosting/coordinator concern, not a provisioner capability. An IAmABoxProvisioner that knows how to provision a box should not need to advertise its type to drive ordering in its host.

Consider whether ordering should be expressed differently - e.g. ordering could come from registration order (outbox extensions register before inbox), or the hosted service could sort by a separate attribute. Leaving BoxType on the interface is not wrong, but it is worth a deliberate call.

2. PostgreSQL advisory lock namespace constant needs clearer provenance

The ADR states 74726 is an application-specific constant reserved for Brighter but does not explain how it was chosen or where it is defined. This should be a named constant with a comment explaining its origin and stability contract. The ADR should state which file/class owns this constant so implementors know where to find it. This value must remain stable across versions - changing it would break lock coordination between instances running different Brighter versions during a rolling deploy.

3. MSSQL all-or-nothing rollback deserves a line in Consequences -> Negative

The ADR documents this well in the Risks section, but it is also an observable negative consequence: operators upgrading across multiple Brighter versions on MSSQL will see that if migration N fails, the history table shows no applied migrations for that run - which differs from PostgreSQL/MySQL/SQLite where 1..N-1 succeed and are recorded. A one-liner in the Consequences section alongside the existing negatives would help operators surface this earlier.

4. Concurrent Task.WhenAll test (Task 2.9) may be timing-sensitive in CI

The concurrency test launches two provisioners concurrently and asserts no duplicate rows. On a fast CI host, both instances may complete sequentially without ever actually contending on the lock, so the test passes without exercising the lock path. Consider whether the test needs an injection point (e.g. an artificial delay hook in the migration runner) to force contention, or whether it should be documented as an integration smoke test rather than a true concurrency proof.

[claude]

5. DetectCurrentVersionAsync for Spanner column introspection dialect

Section 7 says backend provisioners must implement DetectCurrentVersionAsync via INFORMATION_SCHEMA.COLUMNS. The ADR specifies pragma_table_info for SQLite but does not confirm the exact query form for Spanner. Since Spanner SQL dialect differs in some places, the ADR should explicitly call out what query the Spanner provisioner will use for column introspection (similar to how the SQLite form is called out).

6. tasks.md is included but README.md status still shows Tasks unchecked

The checklist in README.md shows Tasks as unchecked, but tasks.md is added in this PR. Either the checkbox should be ticked, or the convention is that it only gets ticked when tasks are complete (not when the file is created). A clarifying update to the README would remove ambiguity.

7. sp_getapplock resource name length limit

MSSQL sp_getapplock has a 255-character limit on the Resource parameter. The resource name used is BrighterMigration_{tableName}. If a caller configures a very long table name, the resource string could exceed 255 characters and cause sp_getapplock to error silently or behave unexpectedly. Consider a guard in the runner that validates the combined length.

Minor Nits

• binaryMessagePayload not on the connectionName inbox overload: The outbox connectionName overload has this parameter; the inbox overload does not. If binary inbox is intentionally unsupported, a comment on the method to that effect would prevent a future question about the asymmetry.
• Task 2.5a ordering: Good to see the structural refactor separated per tidy-first. Worth confirming the intended sequence: 2.5 (implement) then 2.5a (refactor) then 2.6 (reuse helper).
• UpScript vs Script: The Up prefix is retained for convention consistency, which is reasonable given there is no Down. Not a blocking concern.

Summary

This is a solid, well-considered design that has clearly been through substantial review iteration (12 commits of refinement). The issues above are mostly design questions and implementation guard-rails rather than blockers. Happy to approve once the PostgreSQL namespace constant provenance and the README status nit are addressed.

[claude]

(Note: the first short comment above was a test - please disregard it)

[claude]

PR Review: Database Utilities for Boxes (Spec 0023 + ADR 0053)

This is a well-structured spec and architecture PR. The role-based design is solid and consistent with Brighter's existing conventions. Below is a detailed review of each artifact.

---

ADR 0053 — Box Database Migration

Strengths

• The BoxTableState record cleanly externalises the three detection scenarios (fresh install / bootstrap / normal) into the type system, eliminating implicit integer semantics.
• Forward-only migrations (no DownScript) is exactly the right call for production DDL; the rationale is clearly documented.
• The MigrationLockTimeout footgun mitigation (passing it on UseBoxProvisioning() before the configure delegate runs) is a nice ergonomic fix.
• Fail-fast via ConfigurationException is consistent with Brighter convention and the rationale is well-explained.
• TryAddEnumerable guard against double-registration of the hosted service is a good defensive touch.

Issues

1. Race window: advisory lock acquired after detection (medium)

The DetectTableStateAsync code snippet shows the provisioner opening its own connection and introspecting the schema before the migration runner acquires the advisory lock. Two concurrently starting instances could both detect TableExists: false, both acquire the lock (serially), and both attempt to run the v1 migration — relying entirely on the migration runner's idempotency to save them.

The ADR says "single DbConnection strategy with advisory lock held for duration of all migrations" but the code splits detection (provisioner) and migration (runner) across what appear to be separate connection lifetimes. These are in tension.

Suggested clarification (or fix): either (a) the advisory lock is acquired at the top of ProvisionAsync, wrapping the full detect+migrate sequence on one connection, or (b) the ADR explicitly states that detection is inherently TOCTOU-tolerant because the runner's idempotency absorbs the race. Option (b) is fine if intentional, but it needs to be stated so implementors don't accidentally break it.

2. __BrighterMigrationHistory table — who creates it, and when? (minor)

The bootstrap path inserts "synthetic history rows" into __BrighterMigrationHistory, and the normal path reads from it. But the ADR doesn't specify when the history table itself is created. Is it:

• Part of the v1 migration DDL (so it is always created alongside the box table)?
• Created lazily by the runner on first use?
• A prerequisite that the runner bootstraps independently?

The tasks.md (Task 1.1) defers this to the implementation, but since all three MSSQL tasks (2.2–2.4) test against the history table, the creation timing is load-bearing. A sentence in the ADR Decision section would prevent each backend implementor from making a different choice.

3. PostgreSQL hashtext() stability (minor)

The advisory lock uses pg_try_advisory_lock(74736, hashtext(schemaName || '.' || tableName)). hashtext() is a PostgreSQL-internal function; its output is not guaranteed stable across major versions (it changed between PG 12 and PG 13 for some inputs). If an upgrade changes the hash, the lock namespace shifts and concurrent processes could bypass serialisation. Consider using a deterministic alternative (e.g. ('x' || substr(md5(schemaName || '.' || tableName), 1, 8))::bit(32)::int) or documenting the version dependency explicitly.

4. Spanner history + DDL failure window (minor)

The ADR acknowledges the window where Spanner DDL succeeds but the history transaction fails. The mitigation ("idempotency contract: re-running DDL on a table that already exists must not error") is correct, but the history bootstrap for Spanner should be clarified: if the provisioner detects the table exists but no history row, does it run DetectCurrentVersionAsync and insert synthetic rows, exactly as other backends do? The Spanner section of the ADR reads as if it may skip this step.

5. Missing test scenario: double-call of UseBoxProvisioning (minor)

Task 1.5 covers the happy-path registration, but there is no test task for the TryAddEnumerable guard (calling UseBoxProvisioning twice should register BoxProvisioningHostedService only once). This is a correctness invariant worth an explicit test, especially since the ADR calls it out in a comment.

---

Requirements (specs/0023/requirements.md)

Clean and complete. The split of FR-4 to update WebAPI samples (rather than Aspire hosting) is the right scope decision. The acceptance criteria are testable and specific. No issues.

---

Tasks (specs/0023/tasks.md)

• The TDD task structure is well-formed: each behavioral task has a /test-first command, and structural changes are correctly separated into Task 2.5a (tidy-first). This follows the project's mandatory TDD workflow correctly.
• The prerequisite-before-phase ordering is correct and the dependency graph is clear.
• Suggestion: add a Task 1.6 (or extend 1.5) to test the double-registration guard described above.
• The three-scenario test coverage per backend (create → idempotent → bootstrap) builds well — each test is independently verifiable.

---

Summary

The design is sound and the documentation is thorough. The main item worth resolving before implementation begins is item 1 above (the advisory lock / connection lifecycle gap), since it determines a correctness invariant that each backend implementor will need to get right. Items 2–5 are minor but worth a clarifying sentence each to prevent ambiguity during implementation.

[codescene-delta-analysis]

No application code in the PR — skipped Code Health checks.

See analysis details in CodeScene

Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

[claude]

PR Review: Database Utilities for our boxes. This is a documentation/spec-only PR. Overall the ADR and task list are high quality - full review details follow in subsequent comments.

[claude]

PR Review: Database Utilities for our boxes. Full review below in follow-up comments.

[claude]

OVERALL: This is a documentation/spec-only PR (ADR + requirements + tasks). No production code is changed. The design is thorough, following Responsibility-Driven Design with correct use of /test-first commands throughout the task list.

[claude]

KEY CORRECTNESS ISSUE - Race condition between state detection and advisory lock acquisition: The provisioner detects BoxTableState before the runner acquires the advisory lock. Two instances starting simultaneously both detect TableExists=false. Instance A acquires lock, creates table, inserts __BrighterMigrationHistory row, commits, releases. Instance B then acquires lock and re-runs all migrations with stale state. CREATE TABLE IF NOT EXISTS passes silently, but INSERT INTO __BrighterMigrationHistory fails with PRIMARY KEY violation (A already inserted the row). Startup fails. The ADR claims concurrent instances will find no outstanding migrations after waiting, but the runner uses the pre-passed BoxTableState without re-checking inside the lock. Fix options: (a) runner re-reads state inside the lock before applying, (b) history row inserts use INSERT IF NOT EXISTS or MERGE semantics, or (c) detection moves inside the lock. This must be resolved before implementation begins.

[claude]

OTHER CORRECTNESS ITEMS: (1) Schema name missing from MSSQL advisory lock resource - lock resource is BrighterMigration_{tableName}; if two schemas have tables with the same name (e.g. dbo.Outbox and reporting.Outbox), provisioners contend unnecessarily - suggest BrighterMigration_{schemaName}_{tableName}. (2) Spanner DetectCurrentVersionAsync not specified - the ADR describes Spanner DDL semantics but does not specify how to introspect column existence. Spanner has INFORMATION_SCHEMA but the syntax differs from MSSQL/PostgreSQL/MySQL. Should be addressed before Spanner backend tasks are written.

[claude]

DESIGN OBSERVATIONS: (1) MigrationLockTimeout ordering footgun - the ADR documents that setting MigrationLockTimeout after Add* methods has no effect on already-registered provisioners. Consider making the setter internal or init-only, forcing callers through the safe UseBoxProvisioning(configure, migrationLockTimeout) overload. (2) Two connections per ProvisionAsync call - provisioner opens one for detection, runner opens a second. A brief comment explaining this is intentional (advisory lock lifetime requires a dedicated connection held across all migration steps) would help future maintainers. (3) pg_try_advisory_lock parameter types - hashtext() returns int4 but the two-argument form takes bigint; PostgreSQL widens implicitly so this works, but worth noting in implementation comments.

[claude]

REQUIREMENTS AND TASKS FEEDBACK: (1) Missing AC for concurrency - AC-1 through AC-6 do not cover concurrent startup. Suggested AC-7: Given multiple application instances start simultaneously, when each runs box provisioning, the database schema is in a consistent state with no duplicate tables or history rows. (2) Task 2.9 placement - listed after 2.8 but the dependency graph shows it can run in parallel with 2.3-2.8 after 2.2 completes (sp_getapplock is introduced in 2.2). (3) MySQL missing explicit idempotency task - MSSQL has Task 2.3, PostgreSQL has Task 3.4; MySQL only has the concurrent test (4.2) which covers it implicitly. (4) SQLite and Spanner phases lack payload validation tasks - MSSQL has Tasks 2.5/2.6 for payload mode validation; if mismatch detection prevents silent data corruption it should appear for all backends supporting binary/text variants.

[claude]

MINOR POINTS AND SUMMARY: Minor: (1) BoxProvisioningOptions.Add() is public but intended for backend extension packages - a doc comment clarifying the intended caller would help. (2) The constant 74726 is described as reserved for Brighter but there is no external reservation mechanism; 'chosen for Brighter' is more accurate. (3) The spec README shows Tasks as unchecked but tasks.md is included in this PR - worth checking before merge. Summary: The race condition (concurrent state detection outside the lock) is the key issue to resolve before implementation begins, as it breaks the core concurrency invariant the design relies on. Items 2 and 3 under correctness should be addressed in the ADR before the corresponding backend tasks are written. The remaining items are enhancements or clarifications. Reviewed with Claude Code.