fix(03): revise plans based on checker feedback

This commit is contained in:
2026-03-24 09:04:19 +01:00
parent e8e0731adc
commit b6b7ca44dc
2 changed files with 38 additions and 37 deletions

View File

@@ -18,8 +18,9 @@ must_haves:
truths: truths:
- "PostgresStore implements all 9 Store interface methods with PostgreSQL SQL syntax" - "PostgresStore implements all 9 Store interface methods with PostgreSQL SQL syntax"
- "PostgreSQL baseline migration creates the same 3 tables as SQLite (updates, tags, tag_assignments)" - "PostgreSQL baseline migration creates the same 3 tables as SQLite (updates, tags, tag_assignments)"
- "RunMigrations is renamed to RunSQLiteMigrations for symmetry; RunPostgresMigrations exists for PostgreSQL" - "RunMigrations is renamed to RunSQLiteMigrations in migrate.go; RunPostgresMigrations exists for PostgreSQL"
- "Existing SQLite migration path is unchanged (backward compatible)" - "Existing SQLite migration path is unchanged (backward compatible)"
- "Application compiles and all existing tests pass after adding PostgreSQL support code"
artifacts: artifacts:
- path: "pkg/diunwebhook/postgres_store.go" - path: "pkg/diunwebhook/postgres_store.go"
provides: "PostgresStore struct implementing Store interface" provides: "PostgresStore struct implementing Store interface"
@@ -139,16 +140,12 @@ Import alias: `sqlitemigrate "github.com/golang-migrate/migrate/v4/database/sqli
- pkg/diunwebhook/migrate.go (current RunMigrations implementation to rename) - pkg/diunwebhook/migrate.go (current RunMigrations implementation to rename)
- pkg/diunwebhook/migrations/sqlite/0001_initial_schema.up.sql (schema to translate) - pkg/diunwebhook/migrations/sqlite/0001_initial_schema.up.sql (schema to translate)
- pkg/diunwebhook/migrations/sqlite/0001_initial_schema.down.sql (down migration to copy) - pkg/diunwebhook/migrations/sqlite/0001_initial_schema.down.sql (down migration to copy)
- pkg/diunwebhook/export_test.go (calls RunMigrations - must update call site)
- cmd/diunwebhook/main.go (calls RunMigrations - must update call site)
- go.mod (current dependencies) - go.mod (current dependencies)
</read_first> </read_first>
<files> <files>
pkg/diunwebhook/migrations/postgres/0001_initial_schema.up.sql, pkg/diunwebhook/migrations/postgres/0001_initial_schema.up.sql,
pkg/diunwebhook/migrations/postgres/0001_initial_schema.down.sql, pkg/diunwebhook/migrations/postgres/0001_initial_schema.down.sql,
pkg/diunwebhook/migrate.go, pkg/diunwebhook/migrate.go,
pkg/diunwebhook/export_test.go,
cmd/diunwebhook/main.go,
go.mod, go.mod,
go.sum go.sum
</files> </files>
@@ -201,6 +198,7 @@ Import alias: `sqlitemigrate "github.com/golang-migrate/migrate/v4/database/sqli
4. Rewrite `pkg/diunwebhook/migrate.go`: 4. Rewrite `pkg/diunwebhook/migrate.go`:
- Rename `RunMigrations` to `RunSQLiteMigrations` (per RESEARCH.md recommendation) - Rename `RunMigrations` to `RunSQLiteMigrations` (per RESEARCH.md recommendation)
- IMPORTANT: Only rename the function definition in migrate.go itself. Do NOT touch cmd/diunwebhook/main.go or pkg/diunwebhook/export_test.go — those call-site renames are handled in Plan 02.
- Add a second `//go:embed migrations/postgres` directive for `var postgresMigrations embed.FS` - Add a second `//go:embed migrations/postgres` directive for `var postgresMigrations embed.FS`
- Add `RunPostgresMigrations(db *sql.DB) error` using `pgxmigrate "github.com/golang-migrate/migrate/v4/database/pgx/v5"` as the database driver - Add `RunPostgresMigrations(db *sql.DB) error` using `pgxmigrate "github.com/golang-migrate/migrate/v4/database/pgx/v5"` as the database driver
- The pgx migrate driver name string for `migrate.NewWithInstance` is `"pgx5"` (NOT "pgx" or "postgres" -- this is the registration name used by golang-migrate's pgx/v5 sub-package) - The pgx migrate driver name string for `migrate.NewWithInstance` is `"pgx5"` (NOT "pgx" or "postgres" -- this is the registration name used by golang-migrate's pgx/v5 sub-package)
@@ -221,14 +219,12 @@ Import alias: `sqlitemigrate "github.com/golang-migrate/migrate/v4/database/sqli
``` ```
- RunPostgresMigrations body follows the exact same pattern as RunSQLiteMigrations but uses `postgresMigrations`, `"migrations/postgres"`, `pgxmigrate.WithInstance`, and `"pgx5"` as the database name - RunPostgresMigrations body follows the exact same pattern as RunSQLiteMigrations but uses `postgresMigrations`, `"migrations/postgres"`, `pgxmigrate.WithInstance`, and `"pgx5"` as the database name
5. Update the two call sites that reference `RunMigrations`: 5. Because migrate.go renames `RunMigrations` to `RunSQLiteMigrations` but the call sites in main.go and export_test.go still reference the old name, the build will break temporarily. This is expected — Plan 02 (wave 2) updates those call sites. To verify this plan in isolation, the verify command uses `go build ./pkg/diunwebhook/` (package only, not `./...`) and `go vet ./pkg/diunwebhook/`.
- `cmd/diunwebhook/main.go` line 29: change `diun.RunMigrations(db)` to `diun.RunSQLiteMigrations(db)`
- `pkg/diunwebhook/export_test.go` line 12 and line 24: change `RunMigrations(db)` to `RunSQLiteMigrations(db)`
6. Run `go mod tidy` to clean up go.sum. 6. Run `go mod tidy` to clean up go.sum.
</action> </action>
<verify> <verify>
<automated>cd /home/jean-luc-makiola/Development/projects/DiunDashboard && go build ./... && go test -v -count=1 ./pkg/diunwebhook/ -run TestWebhookHandler 2>&1 | head -30</automated> <automated>cd /home/jean-luc-makiola/Development/projects/DiunDashboard && go build ./pkg/diunwebhook/ && go vet ./pkg/diunwebhook/</automated>
</verify> </verify>
<acceptance_criteria> <acceptance_criteria>
- pkg/diunwebhook/migrations/postgres/0001_initial_schema.up.sql contains `SERIAL PRIMARY KEY` - pkg/diunwebhook/migrations/postgres/0001_initial_schema.up.sql contains `SERIAL PRIMARY KEY`
@@ -241,13 +237,11 @@ Import alias: `sqlitemigrate "github.com/golang-migrate/migrate/v4/database/sqli
- pkg/diunwebhook/migrate.go contains `//go:embed migrations/postgres` - pkg/diunwebhook/migrate.go contains `//go:embed migrations/postgres`
- pkg/diunwebhook/migrate.go contains `pgxmigrate "github.com/golang-migrate/migrate/v4/database/pgx/v5"` - pkg/diunwebhook/migrate.go contains `pgxmigrate "github.com/golang-migrate/migrate/v4/database/pgx/v5"`
- pkg/diunwebhook/migrate.go contains `"pgx5"` (driver name in NewWithInstance call) - pkg/diunwebhook/migrate.go contains `"pgx5"` (driver name in NewWithInstance call)
- cmd/diunwebhook/main.go contains `RunSQLiteMigrations` (not RunMigrations)
- pkg/diunwebhook/export_test.go contains `RunSQLiteMigrations` (not RunMigrations)
- go.mod contains `github.com/jackc/pgx/v5` - go.mod contains `github.com/jackc/pgx/v5`
- `go build ./...` exits 0 - `go build ./pkg/diunwebhook/` exits 0
- `go test -count=1 ./pkg/diunwebhook/` exits 0 (all existing SQLite tests still pass) - `go vet ./pkg/diunwebhook/` exits 0
</acceptance_criteria> </acceptance_criteria>
<done>PostgreSQL migration files exist with correct dialect. RunMigrations renamed to RunSQLiteMigrations. RunPostgresMigrations added. pgx/v5 dependency in go.mod. All existing tests pass unchanged.</done> <done>PostgreSQL migration files exist with correct dialect. RunMigrations renamed to RunSQLiteMigrations in migrate.go. RunPostgresMigrations added. pgx/v5 dependency in go.mod. Package builds and vets cleanly.</done>
</task> </task>
<task type="auto"> <task type="auto">
@@ -378,7 +372,7 @@ func NewPostgresStore(db *sql.DB) *PostgresStore {
**IMPORTANT: No mutex.Lock/Unlock anywhere in PostgresStore** (per D-05). No `sync.Mutex` field in the struct. **IMPORTANT: No mutex.Lock/Unlock anywhere in PostgresStore** (per D-05). No `sync.Mutex` field in the struct.
</action> </action>
<verify> <verify>
<automated>cd /home/jean-luc-makiola/Development/projects/DiunDashboard && go build ./... && go vet ./pkg/diunwebhook/</automated> <automated>cd /home/jean-luc-makiola/Development/projects/DiunDashboard && go build ./pkg/diunwebhook/ && go vet ./pkg/diunwebhook/</automated>
</verify> </verify>
<acceptance_criteria> <acceptance_criteria>
- pkg/diunwebhook/postgres_store.go contains `type PostgresStore struct` - pkg/diunwebhook/postgres_store.go contains `type PostgresStore struct`
@@ -398,7 +392,7 @@ func NewPostgresStore(db *sql.DB) *PostgresStore {
- pkg/diunwebhook/postgres_store.go contains `SetMaxOpenConns(25)` (constructor pool config) - pkg/diunwebhook/postgres_store.go contains `SetMaxOpenConns(25)` (constructor pool config)
- pkg/diunwebhook/postgres_store.go does NOT contain `sync.Mutex` (no mutex for PostgreSQL) - pkg/diunwebhook/postgres_store.go does NOT contain `sync.Mutex` (no mutex for PostgreSQL)
- pkg/diunwebhook/postgres_store.go does NOT contain `mu.Lock` (no mutex) - pkg/diunwebhook/postgres_store.go does NOT contain `mu.Lock` (no mutex)
- `go build ./...` exits 0 - `go build ./pkg/diunwebhook/` exits 0
- `go vet ./pkg/diunwebhook/` exits 0 - `go vet ./pkg/diunwebhook/` exits 0
</acceptance_criteria> </acceptance_criteria>
<done>PostgresStore implements all 9 Store interface methods with PostgreSQL-native SQL. No mutex. Pool settings configured. CreateTag uses RETURNING id. AssignTag uses ON CONFLICT DO UPDATE. Code compiles and passes vet.</done> <done>PostgresStore implements all 9 Store interface methods with PostgreSQL-native SQL. No mutex. Pool settings configured. CreateTag uses RETURNING id. AssignTag uses ON CONFLICT DO UPDATE. Code compiles and passes vet.</done>
@@ -407,18 +401,18 @@ func NewPostgresStore(db *sql.DB) *PostgresStore {
</tasks> </tasks>
<verification> <verification>
1. `go build ./...` succeeds (both stores compile, migrate.go compiles with both drivers) 1. `go build ./pkg/diunwebhook/` succeeds (both stores compile, migrate.go compiles with both drivers)
2. `go test -v -count=1 ./pkg/diunwebhook/` passes (all existing SQLite tests unchanged) 2. `go vet ./pkg/diunwebhook/` clean
3. `go vet ./pkg/diunwebhook/` clean 3. PostgresStore has all 9 methods matching Store interface (compiler enforces this)
4. PostgresStore has all 9 methods matching Store interface (compiler enforces this) 4. Migration files exist in both `migrations/sqlite/` and `migrations/postgres/`
5. Migration files exist in both `migrations/sqlite/` and `migrations/postgres/` 5. Note: `go build ./...` and full test suite will fail until Plan 02 updates call sites in main.go and export_test.go that still reference the old `RunMigrations` name. This is expected.
</verification> </verification>
<success_criteria> <success_criteria>
- PostgresStore compiles and implements Store interface (go build succeeds) - PostgresStore compiles and implements Store interface (go build ./pkg/diunwebhook/ succeeds)
- All existing SQLite tests pass (RunMigrations rename did not break anything)
- PostgreSQL migration creates identical table structure to SQLite (3 tables: updates, tags, tag_assignments) - PostgreSQL migration creates identical table structure to SQLite (3 tables: updates, tags, tag_assignments)
- pgx/v5 is in go.mod as a direct dependency - pgx/v5 is in go.mod as a direct dependency
- migrate.go exports both RunSQLiteMigrations and RunPostgresMigrations
</success_criteria> </success_criteria>
<output> <output>

View File

@@ -21,6 +21,7 @@ must_haves:
- "Docker Compose with --profile postgres activates a PostgreSQL service" - "Docker Compose with --profile postgres activates a PostgreSQL service"
- "Default docker compose (no profile) remains SQLite-only" - "Default docker compose (no profile) remains SQLite-only"
- "Duplicate tag creation returns 409 on both SQLite and PostgreSQL" - "Duplicate tag creation returns 409 on both SQLite and PostgreSQL"
- "Existing SQLite users can upgrade to this version with zero configuration changes and no data loss"
artifacts: artifacts:
- path: "cmd/diunwebhook/main.go" - path: "cmd/diunwebhook/main.go"
provides: "DATABASE_URL branching logic" provides: "DATABASE_URL branching logic"
@@ -55,7 +56,7 @@ must_haves:
<objective> <objective>
Wire PostgresStore into the application and deployment infrastructure. Wire PostgresStore into the application and deployment infrastructure.
Purpose: Connects the PostgresStore (built in Plan 01) to the startup path, adds Docker Compose profiles for PostgreSQL deployments, creates build-tagged integration test helpers, and fixes the UNIQUE constraint detection to work across both database backends. Purpose: Connects the PostgresStore (built in Plan 01) to the startup path, adds Docker Compose profiles for PostgreSQL deployments, creates build-tagged integration test helpers, and fixes the UNIQUE constraint detection to work across both database backends. Also updates all call sites that still reference the old `RunMigrations` name (renamed to `RunSQLiteMigrations` in Plan 01).
Output: Updated main.go with DATABASE_URL branching, compose files with postgres profiles, build-tagged test helper, cross-dialect error handling fix. Output: Updated main.go with DATABASE_URL branching, compose files with postgres profiles, build-tagged test helper, cross-dialect error handling fix.
</objective> </objective>
@@ -106,18 +107,19 @@ func NewServer(store Store, webhookSecret string) *Server
<tasks> <tasks>
<task type="auto"> <task type="auto">
<name>Task 1: Wire DATABASE_URL branching in main.go and fix cross-dialect UNIQUE detection</name> <name>Task 1: Wire DATABASE_URL branching in main.go, update call sites, and fix cross-dialect UNIQUE detection</name>
<read_first> <read_first>
- cmd/diunwebhook/main.go (current SQLite-only startup to add branching) - cmd/diunwebhook/main.go (current SQLite-only startup to rewrite with branching)
- pkg/diunwebhook/diunwebhook.go (TagsHandler line 172 - UNIQUE detection to fix) - pkg/diunwebhook/diunwebhook.go (TagsHandler - UNIQUE detection to fix)
- pkg/diunwebhook/export_test.go (calls RunMigrations - must rename to RunSQLiteMigrations)
- pkg/diunwebhook/postgres_store.go (verify NewPostgresStore exists from Plan 01) - pkg/diunwebhook/postgres_store.go (verify NewPostgresStore exists from Plan 01)
- pkg/diunwebhook/migrate.go (verify RunSQLiteMigrations and RunPostgresMigrations exist from Plan 01) - pkg/diunwebhook/migrate.go (verify RunSQLiteMigrations and RunPostgresMigrations exist from Plan 01)
</read_first> </read_first>
<files>cmd/diunwebhook/main.go, pkg/diunwebhook/diunwebhook.go</files> <files>cmd/diunwebhook/main.go, pkg/diunwebhook/diunwebhook.go, pkg/diunwebhook/export_test.go</files>
<action> <action>
**1. Update `cmd/diunwebhook/main.go`** to branch on `DATABASE_URL` per D-07, D-08, D-09. **1. Rewrite `cmd/diunwebhook/main.go`** with DATABASE_URL branching per D-07, D-08, D-09.
Replace the current database setup block (lines 18-33) with DATABASE_URL branching. The full main function should: Replace the current database setup block with DATABASE_URL branching. The full main function should:
```go ```go
package main package main
@@ -174,13 +176,17 @@ func main() {
Key changes: Key changes:
- Add blank import `_ "github.com/jackc/pgx/v5/stdlib"` to register "pgx" driver name - Add blank import `_ "github.com/jackc/pgx/v5/stdlib"` to register "pgx" driver name
- `DATABASE_URL` present -> `sql.Open("pgx", databaseURL)` -> `RunPostgresMigrations` -> `NewPostgresStore` - `DATABASE_URL` present -> `sql.Open("pgx", databaseURL)` -> `RunPostgresMigrations` -> `NewPostgresStore`
- `DATABASE_URL` absent -> existing SQLite path with `RunSQLiteMigrations` (renamed in Plan 01) - `DATABASE_URL` absent -> existing SQLite path with `RunSQLiteMigrations` (renamed from `RunMigrations` in Plan 01)
- Log `"Using PostgreSQL database"` or `"Using SQLite database at %s"` per D-09 - Log `"Using PostgreSQL database"` or `"Using SQLite database at %s"` per D-09
- Keep all existing code after the store setup unchanged (secret, server, mux, httpSrv, shutdown) - Keep all existing code after the store setup unchanged (secret, server, mux, httpSrv, shutdown)
**2. Fix cross-dialect UNIQUE constraint detection in `pkg/diunwebhook/diunwebhook.go`.** **2. Update `pkg/diunwebhook/export_test.go`** to use the renamed function.
In the `TagsHandler` method, line 172, change: Change all occurrences of `RunMigrations(db)` to `RunSQLiteMigrations(db)` in export_test.go. This completes the rename that Plan 01 started in migrate.go.
**3. Fix cross-dialect UNIQUE constraint detection in `pkg/diunwebhook/diunwebhook.go`.**
In the `TagsHandler` method, change:
```go ```go
if strings.Contains(err.Error(), "UNIQUE") { if strings.Contains(err.Error(), "UNIQUE") {
``` ```
@@ -203,12 +209,13 @@ Why: SQLite errors contain uppercase "UNIQUE" (e.g., `UNIQUE constraint failed:
- cmd/diunwebhook/main.go contains `log.Printf("Using SQLite database at %s", dbPath)` - cmd/diunwebhook/main.go contains `log.Printf("Using SQLite database at %s", dbPath)`
- cmd/diunwebhook/main.go contains `_ "github.com/jackc/pgx/v5/stdlib"` - cmd/diunwebhook/main.go contains `_ "github.com/jackc/pgx/v5/stdlib"`
- cmd/diunwebhook/main.go contains `diun.RunSQLiteMigrations(db)` (not RunMigrations) - cmd/diunwebhook/main.go contains `diun.RunSQLiteMigrations(db)` (not RunMigrations)
- pkg/diunwebhook/export_test.go contains `RunSQLiteMigrations` (not RunMigrations)
- pkg/diunwebhook/diunwebhook.go contains `strings.Contains(strings.ToLower(err.Error()), "unique")` - pkg/diunwebhook/diunwebhook.go contains `strings.Contains(strings.ToLower(err.Error()), "unique")`
- pkg/diunwebhook/diunwebhook.go does NOT contain `strings.Contains(err.Error(), "UNIQUE")` (old pattern removed) - pkg/diunwebhook/diunwebhook.go does NOT contain `strings.Contains(err.Error(), "UNIQUE")` (old pattern removed)
- `go build ./...` exits 0 - `go build ./...` exits 0
- `go test -count=1 ./pkg/diunwebhook/` exits 0 - `go test -v -count=1 ./pkg/diunwebhook/` exits 0 (full test suite passes)
</acceptance_criteria> </acceptance_criteria>
<done>main.go branches on DATABASE_URL to select PostgreSQL or SQLite. pgx/v5/stdlib is blank-imported to register the driver. Startup log identifies the active backend. UNIQUE detection is case-insensitive for cross-dialect compatibility. All existing tests pass.</done> <done>main.go branches on DATABASE_URL to select PostgreSQL or SQLite. pgx/v5/stdlib is blank-imported to register the driver. Startup log identifies the active backend. export_test.go updated with RunSQLiteMigrations. UNIQUE detection is case-insensitive for cross-dialect compatibility. All existing tests pass.</done>
</task> </task>
<task type="auto"> <task type="auto">
@@ -349,7 +356,7 @@ func NewTestPostgresServer() (*Server, error) {
This file is in the `diunwebhook` package (internal, same as export_test.go pattern). The `//go:build postgres` tag ensures it only compiles when explicitly requested with `go test -tags postgres`. Without the tag, `go test ./pkg/diunwebhook/` skips this file entirely -- no pgx import, no PostgreSQL dependency. This file is in the `diunwebhook` package (internal, same as export_test.go pattern). The `//go:build postgres` tag ensures it only compiles when explicitly requested with `go test -tags postgres`. Without the tag, `go test ./pkg/diunwebhook/` skips this file entirely -- no pgx import, no PostgreSQL dependency.
</action> </action>
<verify> <verify>
<automated>cd /home/jean-luc-makiola/Development/projects/DiunDashboard && go build ./... && docker compose config --quiet 2>&1; echo "exit: $?"</automated> <automated>cd /home/jean-luc-makiola/Development/projects/DiunDashboard && go build ./... && go test -v -count=1 ./pkg/diunwebhook/ 2>&1 | tail -5</automated>
</verify> </verify>
<acceptance_criteria> <acceptance_criteria>
- compose.yml contains `profiles:` under the postgres service - compose.yml contains `profiles:` under the postgres service
@@ -371,7 +378,7 @@ This file is in the `diunwebhook` package (internal, same as export_test.go patt
- pkg/diunwebhook/postgres_test.go contains `NewPostgresStore(db)` - pkg/diunwebhook/postgres_test.go contains `NewPostgresStore(db)`
- pkg/diunwebhook/postgres_test.go contains `TEST_DATABASE_URL` - pkg/diunwebhook/postgres_test.go contains `TEST_DATABASE_URL`
- `go build ./...` exits 0 (postgres_test.go is not compiled without build tag) - `go build ./...` exits 0 (postgres_test.go is not compiled without build tag)
- `go test -count=1 ./pkg/diunwebhook/` exits 0 (SQLite tests still pass, postgres_test.go skipped) - `go test -v -count=1 ./pkg/diunwebhook/` exits 0 (full SQLite test suite passes, postgres_test.go skipped)
</acceptance_criteria> </acceptance_criteria>
<done>Docker Compose files support optional PostgreSQL via profiles. Default deploy remains SQLite-only. Build-tagged test helper exists for PostgreSQL integration testing. Dockerfile needs no changes (pgx/v5 is pure Go).</done> <done>Docker Compose files support optional PostgreSQL via profiles. Default deploy remains SQLite-only. Build-tagged test helper exists for PostgreSQL integration testing. Dockerfile needs no changes (pgx/v5 is pure Go).</done>
</task> </task>