diff --git a/.env.ci b/.env.ci index aad58d9..807e963 100644 --- a/.env.ci +++ b/.env.ci @@ -6,10 +6,9 @@ # # PDS 3001 | PLC 3002 | AppView 8081 | Postgres 5435 (appview) / 5434 (test) / 5436 (plc) # -# Keeping those ports identical is what lets the test suite's hardcoded -# "http://localhost:8081" and "localhost:3001" literals (tests/e2e/*.go, and the -# getTestPDSURL/TEST_DATABASE_URL fallbacks in tests/integration) resolve to the -# CI stack without editing a single test. It also means CI config cannot quietly +# Keeping those ports identical is what lets the test suite's endpoint defaults +# (testkit.Endpoints() in tests/testkit/testkit.go, which is the only place a +# base URL is constructed) resolve to the CI stack without editing a single test. It also means CI config cannot quietly # drift away from dev config. # # Lines that differ from .env.dev are marked "CI:" with the reason. @@ -32,7 +31,7 @@ POSTGRES_PASSWORD=dev_password DATABASE_URL=postgres://dev_user:dev_password@localhost:5435/coves_dev?sslmode=disable # ============================================================================= -# PostgreSQL — test database (read directly by tests/integration) +# PostgreSQL — test database (read by tests/testkit; T1 integration tier) # ============================================================================= POSTGRES_TEST_DB=coves_test POSTGRES_TEST_USER=test_user diff --git a/Makefile b/Makefile index 65a63c0..ec41ad0 100644 --- a/Makefile +++ b/Makefile @@ -120,13 +120,17 @@ db-reset: ## Reset database (delete all data and re-run migrations) ##@ Testing -test: ## T0 unit tier - no Docker, no database, no network. The inner loop. +test: ## T0 unit tier - no Docker, no database, no public network. The inner loop. @# Untagged `go test` is the unit tier by construction: every test that @# needs something out of process carries a build tag (integration/e2e/live) @# and is therefore not in this build at all. That is what lets this target @# start no containers and wait for nothing — and it is checked, not hoped @# for: the tier is verified by running this selection with the network @# switched off entirely. + @# + @# "No network" means nothing out of process: in-process httptest servers on + @# loopback are used freely here, and they are why `--network none` is the + @# honest check rather than an unreachable ideal. @echo "$(GREEN)Running the unit tier (untagged)...$(RESET)" @go test ./cmd/... ./internal/... ./tests/... @echo "$(GREEN)✓ Unit tier complete$(RESET)" @@ -281,7 +285,7 @@ test-db-reset: ## Reset test database test-db-prepare: ## Create or refresh the template database that testkit.DB clones per test @./scripts/test-db-prepare.sh -test-audit: ## Count test-suite invariant violations (warn only; -v for file:line) +test-audit: ## Test-suite invariant audit - hard gate, any violation fails (-v for file:line) @./scripts/test-audit.sh test-db-stop: ## Stop test database diff --git a/PROJECT_STRUCTURE.md b/PROJECT_STRUCTURE.md index 2519904..4fd314e 100644 --- a/PROJECT_STRUCTURE.md +++ b/PROJECT_STRUCTURE.md @@ -19,6 +19,8 @@ Coves/ │ ├── backfill-profiles/ # One-off maintenance: backfill actor profiles │ ├── reindex-votes/ # One-off maintenance: rebuild vote counts │ ├── tools/ # generate-oauth-key +│ ├── ci-report/ # CI gate: a skip is a failure unless allowlisted +│ ├── contract-manifest/ # CI gate: every consumed collection has an e2e contract │ ├── validate-lexicon/ # Lexicon schema validation │ └── validate-live/ # Validation against a live instance │ @@ -46,11 +48,27 @@ Coves/ │ ├── static/ # Static web assets ├── scripts/ # Development and deployment scripts -├── tests/ # Integration and e2e tests (require live infra) +├── tests/ # Only the cross-cutting tiers (see "Tests" below) +│ ├── testkit/ # The shared harness every tier imports +│ ├── e2e/ # T2 pipeline contracts (//go:build e2e) +│ ├── live/ # T3 opt-in tests against the public internet +│ ├── fixtures/ # Shared record/blob builders +│ ├── ci/ # Skip allowlist and contract-manifest inputs +│ └── lexicon-test-data/ # Example records for lexicon validation ├── docs/ # Additional documentation └── aggregators/ # Aggregator bot examples ``` +## Tests + +Tiers are selected by build tag, never by directory or filename. T0 unit tests +(no tag) and T1 integration tests (`//go:build integration`) live **in-package**, +next to the code they test, so `internal/…` and `cmd/…` hold the bulk of the +suite. `tests/` keeps only what is genuinely cross-cutting. + +`docs/TEST_ARCHITECTURE.md` is the canonical reference for the tiers, the make +targets, and the gates `make ci` enforces. + ## Server startup `cmd/server` is split by concern rather than being one long `main`: diff --git a/TESTING_SUMMARY.md b/TESTING_SUMMARY.md deleted file mode 100644 index f3791ed..0000000 --- a/TESTING_SUMMARY.md +++ /dev/null @@ -1,407 +0,0 @@ -# Coves Testing Guide - -This document explains how testing works in Coves, including setup, running tests, and understanding the test infrastructure. - -## Overview - -Coves uses a unified testing approach with: -- **Single configuration file**: [.env.dev](.env.dev) for all environments (dev + test) -- **Isolated test database**: PostgreSQL on port 5434 (separate from dev on 5433) -- **Makefile commands**: Simple `make test` command handles everything -- **Docker Compose profiles**: Test database spins up automatically - -## Quick Start - -```bash -# Run all tests (starts test DB, runs migrations, executes tests) -make test - -# Reset test database (clean slate) -make test-db-reset - -# Stop test database -make test-db-stop -``` - -## Test Infrastructure - -### Configuration (.env.dev) - -All test configuration lives in [.env.dev](.env.dev): - -```bash -# Test Database Configuration -POSTGRES_TEST_DB=coves_test -POSTGRES_TEST_USER=test_user -POSTGRES_TEST_PASSWORD=test_password -POSTGRES_TEST_PORT=5434 -``` - -**No separate `.env.test` file needed!** Everything is in `.env.dev`. - -### Test Database - -The test database runs in Docker via [docker-compose.dev.yml](docker-compose.dev.yml): - -- **Service**: `postgres-test` (profile: `test`) -- **Port**: 5434 (separate from dev database on 5433) -- **Automatic startup**: The Makefile handles starting/stopping -- **Isolated data**: Completely separate from development database - -### Running Tests Manually - -If you need to run tests without the Makefile: - -```bash -# 1. Load environment variables -set -a && source .env.dev && set +a - -# 2. Start test database -docker-compose -f docker-compose.dev.yml --env-file .env.dev --profile test up -d postgres-test - -# 3. Wait for it to be ready -sleep 3 - -# 4. Run migrations -goose -dir internal/db/migrations postgres \ - "postgresql://$POSTGRES_TEST_USER:$POSTGRES_TEST_PASSWORD@localhost:$POSTGRES_TEST_PORT/$POSTGRES_TEST_DB?sslmode=disable" up - -# 5. Run tests -go test ./... -v - -# 6. Stop test database (optional) -docker-compose -f docker-compose.dev.yml --env-file .env.dev --profile test stop postgres-test -``` - -**Note**: The Makefile automatically loads `.env.dev` variables, so `make test` is simpler than running manually. - -## Test Types - -### 1. Unit Tests - -Test individual components in isolation. - -**Note**: Unit tests will be added as needed. Currently focusing on integration tests. - -```bash -# Run unit tests for a specific package (when available) -go test -v ./internal/core/users/... -``` - -### 2. Integration Tests - -Test full request/response flows with a real database. - -**Location**: [tests/integration/](tests/integration/) - -**How they work**: -- Start test database -- Run migrations -- Execute HTTP requests against real handlers -- Verify responses - -**Example**: [tests/integration/integration_test.go](tests/integration/integration_test.go) - -```bash -# Run integration tests -make test -# or -go test -v ./tests/integration/... -``` - -### 3. Lexicon Validation Tests - -Validate AT Protocol Lexicon schemas and test data. - -**Components**: -- **Schemas**: [internal/atproto/lexicon/](internal/atproto/lexicon/) - 57 lexicon schema files -- **Test Data**: [tests/lexicon-test-data/](tests/lexicon-test-data/) - Example records for validation -- **Validator**: [cmd/validate-lexicon/](cmd/validate-lexicon/) - Validation tool -- **Library**: [internal/validation/](internal/validation/) - Validation helpers - -**Running validation**: - -```bash -# Full validation (schemas + test data) -go run cmd/validate-lexicon/main.go - -# Schemas only (skip test data) -go run cmd/validate-lexicon/main.go --schemas-only - -# Verbose output -go run cmd/validate-lexicon/main.go -v - -# Strict mode -go run cmd/validate-lexicon/main.go --strict -``` - -**Test data naming convention**: -- `*-valid*.json` - Should pass validation -- `*-invalid-*.json` - Should fail validation (tests error detection) - -**Current coverage** (as of last update): -- ✅ social.coves.actor.profile -- ✅ social.coves.community.profile -- ✅ social.coves.post.record -- ✅ social.coves.interaction.vote -- ✅ social.coves.moderation.ban - -## Database Migrations - -Migrations are managed with [goose](https://github.com/pressly/goose) and stored in [internal/db/migrations/](internal/db/migrations/). - -### Running Migrations - -```bash -# Development database -make db-migrate - -# Test database (automatically run by `make test`) -make test-db-reset -``` - -### Creating Migrations - -```bash -# Create a new migration -goose -dir internal/db/migrations create migration_name sql - -# This creates: -# internal/db/migrations/YYYYMMDDHHMMSS_migration_name.sql -``` - -### Migration Best Practices - -- **Always test migrations** on test database first -- **Write both Up and Down** migrations -- **Keep migrations atomic** - one logical change per migration -- **Test rollback** - verify the Down migration works -- **Don't modify old migrations** - create new ones instead - -## Test Database Management - -### Fresh Start - -```bash -# Complete reset (deletes all data) -make test-db-reset -``` - -### Connecting to Test Database - -```bash -# Using psql -PGPASSWORD=test_password psql -h localhost -p 5434 -U test_user -d coves_test - -# Using docker exec -docker exec -it coves-test-postgres psql -U test_user -d coves_test -``` - -### Inspecting Test Data - -```sql --- List all tables -\dt - --- View table schema -\d table_name - --- Query data -SELECT * FROM users; - --- Check migrations -SELECT * FROM goose_db_version; -``` - -## Writing Tests - -### Integration Test Template - -```go -package integration - -import ( - "testing" - // ... imports -) - -func TestYourFeature(t *testing.T) { - db := setupTestDB(t) - defer db.Close() - - // Wire up dependencies - repo := postgres.NewYourRepository(db) - service := yourpackage.NewYourService(repo) - - // Create test router - r := chi.NewRouter() - r.Mount("/api/path", routes.YourRoutes(service)) - - // Make request - req := httptest.NewRequest("GET", "/api/path", nil) - w := httptest.NewRecorder() - r.ServeHTTP(w, req) - - // Assert - if w.Code != http.StatusOK { - t.Errorf("Expected 200, got %d", w.Code) - } -} -``` - -### Test Helper: setupTestDB - -The `setupTestDB` function (in [tests/integration/integration_test.go](tests/integration/integration_test.go)): -- Reads config from environment variables (set by `.env.dev`) -- Connects to test database -- Runs migrations -- Cleans up test data -- Returns ready-to-use `*sql.DB` - -## Common Test Commands - -```bash -# Run all tests -make test - -# Run specific test package -go test -v ./internal/core/users/... - -# Run specific test -go test -v ./tests/integration/... -run TestCreateUser - -# Run with coverage -go test -v -cover ./... - -# Run with race detector -go test -v -race ./... - -# Verbose output -go test -v ./... - -# Clean and run -make test-db-reset && make test -``` - -## Troubleshooting - -### Test database won't start - -```bash -# Check if port 5434 is in use -lsof -i :5434 - -# Check container logs -docker logs coves-test-postgres - -# Nuclear reset -docker-compose -f docker-compose.dev.yml --profile test down -v -make test-db-reset -``` - -### Migrations failing - -```bash -# Load environment and check migration status -set -a && source .env.dev && set +a -goose -dir internal/db/migrations postgres \ - "postgresql://$POSTGRES_TEST_USER:$POSTGRES_TEST_PASSWORD@localhost:$POSTGRES_TEST_PORT/$POSTGRES_TEST_DB?sslmode=disable" status - -# Reset and retry -make test-db-reset -``` - -### Tests can't connect to database - -```bash -# Verify test database is running -docker ps | grep coves-test-postgres - -# Verify environment variables -set -a && source .env.dev && set +a && echo "Test DB Port: $POSTGRES_TEST_PORT" - -# Test connection manually -PGPASSWORD=test_password psql -h localhost -p 5434 -U test_user -d coves_test -c "SELECT 1" -``` - -### Lexicon validation errors - -```bash -# Check schema syntax -cat internal/atproto/lexicon/path/to/schema.json | jq . - -# Validate specific schema -go run cmd/validate-lexicon/main.go -v - -# Check test data format -cat tests/lexicon-test-data/your-test.json | jq . -``` - -## Best Practices - -### Test Organization -- ✅ Unit tests live next to the code they test (`*_test.go`) -- ✅ Integration tests live in `tests/integration/` -- ✅ Test data lives in `tests/lexicon-test-data/` -- ✅ One test file per feature/endpoint - -### Test Data -- ✅ Use `.test` handles for test users (e.g., `alice.test`) (auto-cleaned by setupTestDB) -- ✅ Clean up data in tests (or rely on setupTestDB cleanup) -- ✅ Don't rely on specific test execution order -- ✅ Each test should be independent - -### Database Tests -- ✅ Always use the test database (port 5434) -- ✅ Never connect to development database (port 5433) in tests -- ✅ Use transactions for fast test cleanup (where applicable) -- ✅ Test with realistic data sizes - -### Lexicon Tests -- ✅ Create both valid and invalid test cases -- ✅ Cover all required fields -- ✅ Test edge cases (empty strings, max lengths, etc.) -- ✅ Update tests when schemas change - -## CI/CD Integration - -When setting up CI/CD, the test pipeline should: - -```yaml -# Example GitHub Actions workflow -steps: - - name: Start test database - run: | - docker-compose -f docker-compose.dev.yml --env-file .env.dev --profile test up -d postgres-test - sleep 5 - - - name: Run migrations - run: make test-db-migrate # (add this to Makefile if needed) - - - name: Run tests - run: make test - - - name: Cleanup - run: docker-compose -f docker-compose.dev.yml --profile test down -v -``` - -## Related Documentation - -- [Makefile](Makefile) - All test commands -- [.env.dev](.env.dev) - Test configuration -- [docker-compose.dev.yml](docker-compose.dev.yml) - Test database setup -- [LOCAL_DEVELOPMENT.md](docs/LOCAL_DEVELOPMENT.md) - Full development setup -- [CLAUDE.md](CLAUDE.md) - Build guidelines - -## Getting Help - -If tests are failing: -1. Check [Troubleshooting](#troubleshooting) section above -2. Verify `.env.dev` has correct test database config -3. Run `make test-db-reset` for a clean slate -4. Check test database logs: `docker logs coves-test-postgres` - -For questions about: -- **Test infrastructure**: This document -- **AT Protocol testing**: [ATPROTO_GUIDE.md](ATPROTO_GUIDE.md) -- **Development setup**: [LOCAL_DEVELOPMENT.md](docs/LOCAL_DEVELOPMENT.md) diff --git a/cmd/ci-report/main_test.go b/cmd/ci-report/main_test.go index c77d1b4..c1378e3 100644 --- a/cmd/ci-report/main_test.go +++ b/cmd/ci-report/main_test.go @@ -71,7 +71,11 @@ func TestGateOutcomes(t *testing.T) { name: "an unapproved skip fails the gate", stream: []string{ "pkg/a|TestOne|pass", - "pkg/a|TestPDS|skip| blob_test.go:49: PDS not running at http://localhost:3001", + // A verbatim skip message from the era this gate exists to end. + // It is input text for the parser, not an address: ci-report + // never dials anything, and blunting the sample would make the + // fixture less like the output it has to survive. + "pkg/a|TestPDS|skip| blob_test.go:49: PDS not running at http://localhost:3001", // coves:allow-host-literal: quoted skip message parsed as text; ci-report opens no connections }, allowlist: "# nothing tolerated\n", wantCode: 1, diff --git a/cmd/server/jobs_test.go b/cmd/server/jobs_test.go index 6dce76d..37ea0ca 100644 --- a/cmd/server/jobs_test.go +++ b/cmd/server/jobs_test.go @@ -2,26 +2,14 @@ package main import ( "context" + "fmt" "sync" "sync/atomic" "testing" "time" -) -// waitFor polls until cond holds or the deadline passes. Ticker-driven jobs -// are inherently timing-dependent, so tests poll rather than sleep a fixed -// amount and hope. -func waitFor(t *testing.T, timeout time.Duration, cond func() bool) bool { - t.Helper() - deadline := time.Now().Add(timeout) - for time.Now().Before(deadline) { - if cond() { - return true - } - time.Sleep(time.Millisecond) - } - return cond() -} + "Coves/tests/testkit" +) // The reason recovery lives per cycle rather than around the whole goroutine: // a job that dies on its first panic takes aggregator token refresh with it, @@ -40,9 +28,12 @@ func TestRunTicker_SurvivesAPanickingCycle(t *testing.T) { } }) - if !waitFor(t, 5*time.Second, func() bool { return cycles.Load() >= 3 }) { - t.Fatalf("job ran %d cycles after a panic; it should have kept going", cycles.Load()) - } + testkit.WaitFor(t, 5*time.Second, func() (bool, error) { + return cycles.Load() >= 3, nil + }, testkit.WithDescription("the panicky job to keep cycling after its first cycle panicked"), + testkit.WithDiagnostics(func() string { + return fmt.Sprintf("cycles completed: %d", cycles.Load()) + })) cancel() wg.Wait() @@ -58,9 +49,9 @@ func TestRunTicker_StopsOnContextCancel(t *testing.T) { cycles.Add(1) }) - if !waitFor(t, 5*time.Second, func() bool { return cycles.Load() >= 2 }) { - t.Fatal("job never ran") - } + testkit.WaitFor(t, 5*time.Second, func() (bool, error) { + return cycles.Load() >= 2, nil + }, testkit.WithDescription("the counter job to run at least two cycles")) cancel() @@ -77,12 +68,17 @@ func TestRunTicker_StopsOnContextCancel(t *testing.T) { t.Fatal("job did not exit after context cancellation") } - // No further cycles once the WaitGroup has been released. + // No further cycles once the WaitGroup has been released. This is a + // stays-true claim, and the window is worth several hundred ticks of the + // 1ms interval — a job that ignored cancellation would be caught on the + // first poll after the window opens. settled := cycles.Load() - time.Sleep(20 * time.Millisecond) - if got := cycles.Load(); got != settled { - t.Errorf("job ran %d more cycles after exiting", got-settled) - } + testkit.Holds(t, 200*time.Millisecond, func() (bool, error) { + return cycles.Load() == settled, nil + }, testkit.WithDescription("the cycle count to stay at %d after the job exited", settled), + testkit.WithDiagnostics(func() string { + return fmt.Sprintf("cycles now: %d", cycles.Load()) + })) } // Work receives a live, deadline-bounded context derived from the job's own, diff --git a/docker-compose.ci.yml b/docker-compose.ci.yml index 359ef97..f6eb694 100644 --- a/docker-compose.ci.yml +++ b/docker-compose.ci.yml @@ -530,7 +530,7 @@ services: # --------------------------------------------------------------------------- # Built from the working tree every run. This is the difference that matters - # most versus `make test-all`, which tests whatever binary a long-running + # most versus `make test-e2e-dev`, which tests whatever binary a long-running # `make run` started in another terminal — potentially several edits stale. appview: build: diff --git a/docs/COMMENT_SYSTEM_IMPLEMENTATION.md b/docs/COMMENT_SYSTEM_IMPLEMENTATION.md index 18459a5..3b374d3 100644 --- a/docs/COMMENT_SYSTEM_IMPLEMENTATION.md +++ b/docs/COMMENT_SYSTEM_IMPLEMENTATION.md @@ -688,7 +688,7 @@ log.Println(" - Updating: Post comment counts and comment reply counts atomical ### Test Suite -**File:** `tests/integration/comment_consumer_test.go` +**File:** `internal/core/comments/comment_consumer_test.go` **Test Coverage:** 6 test suites, 18 test cases, **100% passing** @@ -1384,28 +1384,25 @@ The implementation provides a solid foundation for building rich threaded discus ### Run Tests +The comment tests below are T1: they carry `//go:build integration` and need +Postgres, which `make test-integration` starts. All of them live in +`internal/core/comments`, next to the code they test. + **Phase 1 - Indexing Tests:** ```bash -TEST_DATABASE_URL="postgres://test_user:test_password@localhost:5434/coves_test?sslmode=disable" \ - go test -v ./tests/integration/comment_consumer_test.go \ - ./tests/integration/user_test.go \ - ./tests/integration/helpers.go \ +go test -tags integration -v ./internal/core/comments/... \ -run "TestCommentConsumer" -timeout 60s ``` **Phase 2A - Query API Tests:** ```bash -TEST_DATABASE_URL="postgres://test_user:test_password@localhost:5434/coves_test?sslmode=disable" \ - go test -v ./tests/integration/comment_query_test.go \ - ./tests/integration/user_test.go \ - ./tests/integration/helpers.go \ +go test -tags integration -v ./internal/core/comments/... \ -run "TestCommentQuery" -timeout 120s ``` **Phase 2B - Voting Tests:** ```bash -TEST_DATABASE_URL="postgres://test_user:test_password@localhost:5434/coves_test?sslmode=disable" \ - go test -v ./tests/integration/ \ +go test -tags integration -v ./internal/core/comments/... \ -run "TestCommentVote" -timeout 60s ``` @@ -1429,14 +1426,10 @@ go test -v ./internal/core/comments/... -run TestValidateGetCommentsRequest **All Comment Tests (Integration + Unit):** ```bash -# Integration tests (requires database) -TEST_DATABASE_URL="postgres://test_user:test_password@localhost:5434/coves_test?sslmode=disable" \ - go test -v ./tests/integration/comment_*.go \ - ./tests/integration/user_test.go \ - ./tests/integration/helpers.go \ - -timeout 120s - -# Unit tests (no database) +# T1 integration tests (requires database) +go test -tags integration -v ./internal/core/comments/... -timeout 120s + +# T0 unit tests (no database) go test -v ./internal/core/comments/... ``` diff --git a/docs/COMMUNITY_FEEDS.md b/docs/COMMUNITY_FEEDS.md index 3ad22bc..bb7160e 100644 --- a/docs/COMMUNITY_FEEDS.md +++ b/docs/COMMUNITY_FEEDS.md @@ -434,7 +434,7 @@ Total: 8 test cases, 12 sub-tests - ✅ Security (cursor injection, SQL injection attempts) - ✅ Edge cases (empty feeds, zero/negative limits) -**Location:** `tests/integration/feed_test.go` +**Location:** `internal/db/postgres/community_feed_test.go` --- diff --git a/docs/E2E_TESTING.md b/docs/E2E_TESTING.md deleted file mode 100644 index e6888be..0000000 --- a/docs/E2E_TESTING.md +++ /dev/null @@ -1,174 +0,0 @@ -# End-to-End Testing Guide - -## Overview - -Coves supports full E2E testing with a local atProto stack: - -``` -Third-party Client → Coves XRPC → PDS → Jetstream → Coves AppView → PostgreSQL -``` - -**Why Jetstream?** -- PDS emits raw CBOR-encoded firehose (binary, hard to parse) -- Jetstream converts CBOR → clean JSON (same format as production) -- Tests exactly match production behavior - ---- - -## Quick Start - -### 1. Start Development Stack - -```bash -make dev-up -``` - -This starts: -- **PostgreSQL** (port 5433) - Coves database -- **PDS** (port 3001) - Local atProto server -- **Jetstream** (port 6008) - CBOR → JSON converter (always runs for read-forward) - -> **Note:** Jetstream is now part of `dev-up` since read-forward architecture requires it - -### 2. Start AppView - -```bash -# In another terminal -make run # Starts AppView (auto-runs migrations) -``` - -AppView will connect to `ws://localhost:6008/subscribe` (configured in `.env.dev`) - -### 3. Run Automated E2E Tests - -```bash -make e2e-test -``` - -This runs the full test suite: -- Creates accounts via XRPC endpoint -- Verifies PDS account creation -- Validates Jetstream indexing -- Confirms database storage - -### 4. Manual Testing (Optional) - -#### Create User via Coves XRPC - -```bash -curl -X POST http://localhost:8081/xrpc/social.coves.actor.signup \ - -H "Content-Type: application/json" \ - -d '{ - "handle": "alice.local.coves.dev", - "email": "alice@test.com", - "password": "test1234" - }' -``` - -**Response:** -```json -{ - "did": "did:plc:xyz123...", - "handle": "alice.local.coves.dev", - "accessJwt": "eyJ...", - "refreshJwt": "eyJ..." -} -``` - -**What happens:** -1. Coves XRPC handler receives signup request -2. Calls PDS `com.atproto.server.createAccount` -3. PDS creates account → emits to firehose -4. Jetstream converts event → JSON -5. AppView receives JSON → indexes user -6. User appears in PostgreSQL `users` table - -### 5. Verify Indexing - -Check AppView logs: -``` -2025/01/15 12:00:00 Identity event: did:plc:xyz123 → alice.local.coves.dev -2025/01/15 12:00:00 Indexed new user: alice.local.coves.dev (did:plc:xyz123) -``` - -Query via API: -```bash -curl "http://localhost:8081/xrpc/social.coves.actor.getProfile?actor=alice.local.coves.dev" -``` - -Expected response: -```json -{ - "did": "did:plc:xyz123...", - "profile": { - "handle": "alice.local.coves.dev", - "createdAt": "2025-01-15T12:00:00Z" - } -} -``` - ---- - -## Workflow Summary - -### Daily Development -```bash -make dev-up # Start PDS + PostgreSQL + Jetstream (once) -make run # Start AppView (in another terminal) -make test # Run fast tests during development -``` - -### Before Commits/PRs -```bash -make e2e-test # Run full E2E test suite -``` - -### Reset Everything -```bash -make dev-reset # Nuclear option - deletes all data -make dev-up # Start fresh -``` - ---- - -## Testing Scenarios - -### Automated E2E Test Suite - -```bash -make e2e-test -``` - -This tests: -- ✅ Single account creation via XRPC -- ✅ Idempotent duplicate event handling -- ✅ Multiple concurrent user indexing - -### Manual: User Registration via XRPC - -```bash -curl -X POST http://localhost:8081/xrpc/social.coves.actor.signup \ - -H "Content-Type: application/json" \ - -d '{"handle":"bob.local.coves.dev","email":"bob@test.com","password":"pass1234"}' -``` - -### Manual: Federated User (Direct PDS) - -```bash -# Simulates a user on another PDS -curl -X POST http://localhost:3001/xrpc/com.atproto.server.createAccount \ - -H "Content-Type: application/json" \ - -d '{"handle":"charlie.local.coves.dev","email":"charlie@test.com","password":"pass1234"}' - -# Coves AppView will still index via Jetstream (read-forward) -``` - ---- - -## Next Steps - -1. ✅ E2E testing infrastructure complete -2. ✅ Automated E2E test suite implemented -3. ✅ XRPC signup endpoint for third-party clients -4. 🔨 TODO: Add handle update support -5. 🔨 TODO: Add CI/CD E2E tests diff --git a/docs/FEED_SYSTEM_IMPLEMENTATION.md b/docs/FEED_SYSTEM_IMPLEMENTATION.md index a6c3b05..a59c090 100644 --- a/docs/FEED_SYSTEM_IMPLEMENTATION.md +++ b/docs/FEED_SYSTEM_IMPLEMENTATION.md @@ -156,28 +156,31 @@ Both repositories include: - `internal/atproto/lexicon/social/coves/feed/getTimeline.json` - Updated with sort/timeframe - `internal/atproto/lexicon/social/coves/feed/getDiscover.json` - New lexicon -### Integration Tests - -- `tests/integration/timeline_test.go` - 6 test scenarios (400+ lines) - - Basic feed (subscription filtering) - - Hot sorting - - Pagination - - Empty when no subscriptions - - Unauthorized access - - Limit validation - -- `tests/integration/discover_test.go` - 5 test scenarios (270+ lines) - - Shows all communities - - No auth required - - Hot sorting - - Pagination - - Limit validation +### Tests + +Tests live next to the code they cover, not in a `tests/` catch-all — see +`docs/TEST_ARCHITECTURE.md` for the tier rules. The feed tests are: + +- `internal/core/timeline/timeline_feed_test.go` - T1 (`//go:build integration`), + handler-through-Postgres feed behaviour +- `internal/core/timeline/service_test.go` - T0, service validation and response shaping +- `internal/core/discover/discover_feed_test.go` - T1, the same for the public feed, + including hot-sort ranking edge cases and viewer vote state +- `internal/core/discover/service_test.go` - T0 +- `internal/core/timeline/harness_test.go`, `internal/core/discover/harness_test.go` - + each package's `TestMain`, declaring its Postgres requirement +- `internal/db/postgres/timeline_repo_block_test.go`, + `internal/db/postgres/discover_repo_block_test.go`, + `internal/db/postgres/feed_repo_block_test.go` - T1 repo-level block filtering + +Scenario-by-scenario coverage is listed under [Testing](#testing) below. ### Test Helpers -- `tests/integration/helpers.go` - Added shared test helpers: - - `createFeedTestCommunity()` - Create test communities - - `createTestPost()` - Create test posts with custom scores/timestamps +Shared fixtures come from the `tests/fixtures` package — `fixtures.User()` and +`fixtures.Community()` replace the old `createFeedTestCommunity()`. Databases come +from `testkit.DB(t)`, which clones a migrated template per test, and identities from +`testkit.UniqueID(t)`. ## Files Modified @@ -330,38 +333,48 @@ score = upvotes / (age_in_hours + 2)^1.5 ### Test Coverage -**Timeline Tests:** `tests/integration/timeline_test.go` +**Timeline Tests:** `internal/core/timeline/timeline_feed_test.go` 1. ✅ Basic feed - Shows posts from subscribed communities only 2. ✅ Hot sorting - Time-decay ranking across communities 3. ✅ Pagination - Cursor-based, no overlap 4. ✅ Empty feed - When user has no subscriptions 5. ✅ Unauthorized - Returns 401 without auth 6. ✅ Limit validation - Rejects limit > 50 +7. ✅ Multi-community - An unsubscribed community stays excluded under all three sorts -**Discover Tests:** `tests/integration/discover_test.go` +**Discover Tests:** `internal/core/discover/discover_feed_test.go` 1. ✅ Shows all communities - No subscription filter 2. ✅ No auth required - Works without JWT 3. ✅ Hot sorting - Time-decay across all posts -4. ✅ Pagination - Cursor-based -5. ✅ Limit validation - Rejects limit > 50 +4. ✅ Hot sorting, log damping - A high-vote bridged post must not bury fresh organic ones +5. ✅ Hot sorting, negative scores - The cursor formula matches the live ORDER BY across the sign boundary +6. ✅ Hot sorting, future-dated post - `GREATEST(age, 0)` holds against a hostile or skewed `created_at` +7. ✅ Pagination - Cursor-based +8. ✅ Limit validation - Rejects limit > 50 +9. ✅ Viewer vote state - Authenticated callers see their own votes +10. ✅ No viewer state without auth - Anonymous callers see none + +Service-layer validation and response shaping are T0 (no database): +`internal/core/timeline/service_test.go` and `internal/core/discover/service_test.go`. ### Running Tests +These are T1 integration tests: they carry `//go:build integration` and need +Postgres. `make test-integration` starts the test database and runs the whole +tier; to narrow it to the feeds: + ```bash -# Reset test database (clean slate) +# Reset the test database (clean slate) make test-db-reset # Run timeline tests -TEST_DATABASE_URL="postgres://test_user:test_password@localhost:5434/coves_test?sslmode=disable" \ - go test -v ./tests/integration/timeline_test.go ./tests/integration/user_test.go ./tests/integration/helpers.go -timeout 60s +go test -tags integration -v ./internal/core/timeline/... -timeout 60s # Run discover tests -TEST_DATABASE_URL="postgres://test_user:test_password@localhost:5434/coves_test?sslmode=disable" \ - go test -v ./tests/integration/discover_test.go ./tests/integration/user_test.go ./tests/integration/helpers.go -timeout 60s +go test -tags integration -v ./internal/core/discover/... -timeout 60s -# Run all integration tests -TEST_DATABASE_URL="postgres://test_user:test_password@localhost:5434/coves_test?sslmode=disable" \ - go test ./tests/integration/... -v -timeout 180s +# Run the whole T1 tier +make test-integration ``` All tests passing ✅ @@ -778,6 +791,6 @@ All feeds support: For implementation details, see the source code: - Timeline: `internal/core/timeline/`, `internal/db/postgres/timeline_repo.go` - Discover: `internal/core/discover/`, `internal/db/postgres/discover_repo.go` -- Tests: `tests/integration/timeline_test.go`, `tests/integration/discover_test.go` +- Tests: `internal/core/timeline/timeline_feed_test.go`, `internal/core/discover/discover_feed_test.go` For architecture decisions, see this document's "Architecture Decisions" section. diff --git a/docs/LOCAL_DEVELOPMENT.md b/docs/LOCAL_DEVELOPMENT.md index c7844b8..05aeb99 100644 --- a/docs/LOCAL_DEVELOPMENT.md +++ b/docs/LOCAL_DEVELOPMENT.md @@ -15,19 +15,16 @@ Complete guide for setting up and running the Coves atProto development environm ## Quick Start ```bash -# 1. Start the PostgreSQL database -make dev-db-up - -# 2. Start the PDS +# 1. Start the stack (PostgreSQL + PDS + Jetstream + PLC Directory) make dev-up -# 3. View logs +# 2. View logs make dev-logs -# 4. Check status +# 3. Check status make dev-status -# 5. When done +# 4. When done make dev-down ``` @@ -96,8 +93,8 @@ Your Production PDS (:3000) ← Runs independently The PostgreSQL database must be running first: ```bash -# Start the database -make dev-db-up +# Start the stack; PostgreSQL comes up with it +make dev-up # Verify it's running make dev-status @@ -181,21 +178,41 @@ make dev-reset # Nuclear option - remove all data and volumes ### Database Commands ```bash -make dev-db-up # Start PostgreSQL database -make dev-db-down # Stop PostgreSQL database -make dev-db-reset # Reset database (delete all data) -make db-shell # Open psql shell to the database +make db-shell # Open psql shell to the development database +make db-migrate # Run migrations against the development database +make db-migrate-down # Roll back the last migration +make db-reset # Reset the database (delete all data, re-run migrations) ``` +**Creating a migration.** Migrations are [goose](https://github.com/pressly/goose) +files in [internal/db/migrations/](../internal/db/migrations/), embedded into the +binary: + +```bash +goose -dir internal/db/migrations create migration_name sql +# → internal/db/migrations/YYYYMMDDHHMMSS_migration_name.sql +``` + +- Write both `Up` and `Down`, and verify the rollback works +- Keep migrations atomic — one logical change each +- Never modify a migration that has already been applied; add a new one +- Test against the test database (`make test-db-reset`) before the dev one + ### Testing Commands ```bash -make test # Run all tests (starts test DB, runs migrations, executes tests) +make test # T0 unit tier - no Docker, no database, no network +make test-integration # T1 - needs Postgres; starts postgres-test itself +make test-e2e # T2 pipeline tier - brings up the hermetic stack and runs inside it +make test-live # T3 - opt-in, deliberately hits the public internet +make ci # The merge gate: hermetic stack, T0+T1+T2, egress-blocked + make test-db-reset # Reset test database (clean slate) make test-db-stop # Stop test database ``` -**See [TESTING_SUMMARY.md](../TESTING_SUMMARY.md) for complete testing documentation.** +**See [TEST_ARCHITECTURE.md](TEST_ARCHITECTURE.md) for the canonical description of +the tiers, the gates, and how to write a test in each one.** ### Workflow Commands @@ -306,11 +323,11 @@ kill -9 **Solution:** ```bash -# Ensure database is running -make dev-db-up +# Ensure the stack (and its database) is running +make dev-up # Check database logs -cd internal/db/local_dev_db_compose && docker-compose logs +docker-compose -f docker-compose.dev.yml --env-file .env.dev logs postgres # Verify connection manually PGPASSWORD=dev_password psql -h localhost -p 5433 -U dev_user -d coves_dev @@ -359,13 +376,10 @@ curl -i -N -H "Connection: Upgrade" -H "Upgrade: websocket" \ ```bash # Manually clean everything docker-compose -f docker-compose.dev.yml down -v -cd internal/db/local_dev_db_compose && docker-compose down -v docker volume prune -f docker network prune -f # Then start fresh -make dev-db-up -sleep 2 make dev-up ``` @@ -436,12 +450,13 @@ LOG_LEVEL=debug 1. **Build the Firehose Subscriber** - Create the AppView component that subscribes to the relay 2. **Define Custom Lexicons** - Create Coves-specific schemas in `internal/atproto/lexicon/social/coves/` 3. **Implement XRPC Handlers** - Build the API endpoints for Coves features -4. **Create Integration Tests** - Use Testcontainers to test the full stack +4. **Cover it at the right tier** - see [TEST_ARCHITECTURE.md](TEST_ARCHITECTURE.md) ## Additional Resources - [ATPROTO_GUIDE.md](../ATPROTO_GUIDE.md) - Comprehensive atProto implementation guide - [PROJECT_STRUCTURE.md](../PROJECT_STRUCTURE.md) - Project organization +- [TEST_ARCHITECTURE.md](TEST_ARCHITECTURE.md) - The test suite: tiers, targets, gates ## Getting Help diff --git a/docs/PRD_ALPHA_GO_LIVE.md b/docs/PRD_ALPHA_GO_LIVE.md index 13864ff..72a8a9f 100644 --- a/docs/PRD_ALPHA_GO_LIVE.md +++ b/docs/PRD_ALPHA_GO_LIVE.md @@ -127,7 +127,7 @@ This document tracks the remaining work required to launch Coves alpha with real - **Security Model**: Matches Bluesky (DNS/HTTPS authority + bidirectional binding) - **Performance**: Bounded LRU cache (1000 entries), rate limiting (10 req/s), 24h TTL - **Impact**: AppView indexing and federation trust (not community creation API) -- **Tests**: `tests/integration/community_hostedby_security_test.go` +- **Tests**: `internal/atproto/jetstream/community_hostedby_verification_test.go` **Actual Effort**: 3 hours (implementation + testing) **Risk**: ✅ Low (complete and tested) @@ -335,7 +335,7 @@ This document tracks the remaining work required to launch Coves alpha with real - [x] Graceful fallback for CI/CD environments **Actual Time**: ~3 hours (agent-implemented) -**Test Location**: `tests/integration/user_journey_e2e_test.go` +**Test Location**: `tests/e2e/journey_test.go` (`TestUserJourney`) #### 2. Blob Upload E2E Test ✅ COMPLETE **What**: Test image upload and display in posts @@ -352,7 +352,7 @@ This document tracks the remaining work required to launch Coves alpha with real - [x] Actual JPEG format testing (not just PNG with different MIME types) **Actual Time**: ~2-3 hours (agent-implemented) -**Test Location**: `tests/integration/blob_upload_e2e_test.go` +**Test Location**: `internal/core/blobs/blob_upload_integration_test.go`, plus the avatar blob steps of `tests/e2e/user_contract_test.go` #### 3. Multi-Community Timeline Test ✅ COMPLETE **What**: Test timeline feed with multiple community subscriptions @@ -368,7 +368,7 @@ This document tracks the remaining work required to launch Coves alpha with real - [x] Verify record schema compliance across communities **Actual Time**: ~2 hours -**Test Location**: `/tests/integration/timeline_test.go::TestGetTimeline_MultiCommunity_E2E` +**Test Location**: `internal/core/timeline/timeline_feed_test.go::TestGetTimeline_MultiCommunity` #### 4. Concurrent User Scenarios ✅ COMPLETE **What**: Test system behavior with simultaneous users @@ -384,7 +384,7 @@ This document tracks the remaining work required to launch Coves alpha with real - [x] Concurrent subscribe/unsubscribe (20 users) **Actual Time**: ~3 hours (agent-implemented) + 1 hour (race condition verification added) -**Test Location**: `tests/integration/concurrent_scenarios_test.go` +**Test Location**: `internal/db/postgres/concurrent_writes_test.go` **Finding**: NO RACE CONDITIONS DETECTED - all tests pass with full database verification #### 5. Rate Limiting Tests ✅ COMPLETE diff --git a/docs/PRD_BACKLOG.md b/docs/PRD_BACKLOG.md index 48441a6..dd37234 100644 --- a/docs/PRD_BACKLOG.md +++ b/docs/PRD_BACKLOG.md @@ -276,12 +276,13 @@ if err != nil { 3. ✅ **Phase 3 (Beta):** Fix block endpoints - COMPLETE (2025-11-16) - Updated block/unblock handlers to use `ResolveCommunityIdentifier()` - Accepts handles (`@gaming.community.coves.social`), DIDs, and scoped format (`!gaming@coves.social`) - - Added comprehensive tests: [block_handle_resolution_test.go](../tests/integration/block_handle_resolution_test.go) + - Added comprehensive tests: [service_identifier_resolution_test.go](../internal/core/communities/service_identifier_resolution_test.go) - All 7 test cases passing **Files Modified (Phase 3 - Block Endpoints):** - `internal/api/handlers/community/block.go` - Added `ResolveCommunityIdentifier()` calls -- `tests/integration/block_handle_resolution_test.go` - Comprehensive test coverage +- `internal/core/communities/service_identifier_resolution_test.go` - Comprehensive test coverage +- `internal/api/handlers/community/block_test.go` - Block/unblock endpoints at the HTTP boundary **Existing Infrastructure:** ✅ `ResolveCommunityIdentifier()` already implemented at [service.go:852](../internal/core/communities/service.go#L852) @@ -353,7 +354,7 @@ if err != nil { **Files Created:** - [internal/core/communities/token_utils.go](../internal/core/communities/token_utils.go) - JWT parsing utilities - [internal/core/communities/token_refresh.go](../internal/core/communities/token_refresh.go) - Refresh and re-auth logic -- [tests/integration/token_refresh_test.go](../tests/integration/token_refresh_test.go) - Integration tests +- [internal/core/communities/token_expiration_test.go](../internal/core/communities/token_expiration_test.go), [service_credentials_test.go](../internal/core/communities/service_credentials_test.go) - Integration tests **Files Modified:** - [internal/core/communities/service.go](../internal/core/communities/service.go) - Added `ensureFreshToken` + concurrency control @@ -389,7 +390,7 @@ if err != nil { - Consumer: [community_consumer.go](../internal/atproto/jetstream/community_consumer.go) ✅ Extracts and indexes - Repository: [community_repo_subscriptions.go](../internal/db/postgres/community_repo_subscriptions.go) ✅ All queries updated - Migration: [008_add_content_visibility_to_subscriptions.sql](../internal/db/migrations/008_add_content_visibility_to_subscriptions.sql) ✅ Schema changes -- Tests: [subscription_indexing_test.go](../tests/integration/subscription_indexing_test.go) ✅ Comprehensive coverage +- Tests: [community_consumer_test.go](../internal/atproto/jetstream/community_consumer_test.go) ✅ Comprehensive coverage **Documentation:** See [IMPLEMENTATION_SUBSCRIPTION_INDEXING.md](../docs/IMPLEMENTATION_SUBSCRIPTION_INDEXING.md) for full details @@ -427,7 +428,7 @@ When comments arrive before their parent post is indexed (common with cross-repo **Solution Implemented:** - ✅ Post consumer reconciliation logic WAS already implemented at [post_consumer.go:210-226](../internal/atproto/jetstream/post_consumer.go#L210-L226) - ✅ Reconciliation query counts pre-existing comments when indexing new posts -- ✅ Comprehensive test suite added: [post_consumer_test.go](../tests/integration/post_consumer_test.go) +- ✅ Comprehensive test suite added: [consumer_comment_count_test.go](../internal/core/posts/consumer_comment_count_test.go) - Single comment before post - Multiple comments before post - Mixed before/after ordering @@ -452,7 +453,7 @@ _, reconcileErr := tx.ExecContext(ctx, reconcileQuery, post.URI, postID) **Files Modified:** - `internal/atproto/jetstream/comment_consumer.go` - Updated documentation -- `tests/integration/post_consumer_test.go` - Added comprehensive test coverage +- `internal/core/posts/consumer_comment_count_test.go` - Added comprehensive test coverage **Impact:** ✅ Post comment counters are now accurate regardless of Jetstream event ordering @@ -709,7 +710,7 @@ Document: did:plc choice, pgcrypto encryption, Jetstream vs firehose, write-forw **Files Created:** - [internal/core/communities/token_utils.go](../internal/core/communities/token_utils.go) - [internal/core/communities/token_refresh.go](../internal/core/communities/token_refresh.go) -- [tests/integration/token_refresh_test.go](../tests/integration/token_refresh_test.go) +- [internal/core/communities/token_expiration_test.go](../internal/core/communities/token_expiration_test.go) **Files Modified:** - [internal/core/communities/service.go](../internal/core/communities/service.go) - Added `ensureFreshToken` method @@ -737,7 +738,7 @@ Document: did:plc choice, pgcrypto encryption, Jetstream vs firehose, write-forw - [internal/atproto/auth/jwt.go](../internal/atproto/auth/jwt.go) - JWT parsing with atProto compatibility - [internal/api/middleware/auth.go](../internal/api/middleware/auth.go) - Auth middleware - [internal/api/handlers/community/](../internal/api/handlers/community/) - All handlers updated -- [tests/integration/community_e2e_test.go](../tests/integration/community_e2e_test.go) - OAuth E2E tests +- [internal/atproto/oauth/oauth_integration_test.go](../internal/atproto/oauth/oauth_integration_test.go) - OAuth session tests; [tests/e2e/community_contract_test.go](../tests/e2e/community_contract_test.go) - community pipeline contract **Related:** Also implemented `hostedByDID` auto-population for security (see P1 item above) diff --git a/docs/PRD_COMMUNITIES.md b/docs/PRD_COMMUNITIES.md index ab20000..69f4e8c 100644 --- a/docs/PRD_COMMUNITIES.md +++ b/docs/PRD_COMMUNITIES.md @@ -157,7 +157,7 @@ Hosted By: did:web:coves.social (instance manages credentials) - Consumer: [internal/atproto/jetstream/community_consumer.go](internal/atproto/jetstream/community_consumer.go) - Connector: [internal/atproto/jetstream/community_jetstream_connector.go](internal/atproto/jetstream/community_jetstream_connector.go) - Migration: [internal/db/migrations/008_add_content_visibility_to_subscriptions.sql](internal/db/migrations/008_add_content_visibility_to_subscriptions.sql) - - Tests: [tests/integration/subscription_indexing_test.go](tests/integration/subscription_indexing_test.go) + - Tests: [internal/atproto/jetstream/community_consumer_test.go](internal/atproto/jetstream/community_consumer_test.go), [tests/e2e/subscription_contract_test.go](tests/e2e/subscription_contract_test.go) ### Critical Security (High Priority) - [x] **OAuth Authentication:** ✅ COMPLETE - User access tokens flow end-to-end diff --git a/docs/TEST_ARCHITECTURE.md b/docs/TEST_ARCHITECTURE.md index ec87f2e..8a58dbe 100644 --- a/docs/TEST_ARCHITECTURE.md +++ b/docs/TEST_ARCHITECTURE.md @@ -1,9 +1,12 @@ # Coves Test Architecture -**Status: PROPOSED — this document is the spec for the test-suite refactor on branch `worktree-test-refactor`.** -Revision 2: incorporates an external design review (OpenAI Codex, `gpt-5.6-sol`, 2026-07-28) which surfaced several structural blind spots — most importantly that the original black-box E2E design could pass with a dead firehose for synchronously-indexed domains (§3.4), that per-package `TestMain`s race on template-DB creation (§3.3), and that the phase ordering broke its own green-boundary invariant (§4). +**Status: CANONICAL — this describes the test suite as it is built. It is the reference for writing or changing any test in this repo.** -This document replaces `TESTING_SUMMARY.md` and `docs/E2E_TESTING.md`, both of which describe a system that no longer exists (wrong ports, references to files that were never written, no mention of the real gate `make ci`). Delete both when Phase 6 lands. +It began life as a spec, written across two revisions before the work existed (revision 2 incorporated an external design review — OpenAI Codex `gpt-5.6-sol`, 2026-07-28 — which caught that the original black-box E2E design could pass with a dead firehose for synchronously-indexed domains (§3.4), that per-package `TestMain`s race on template-DB creation (§3.3), and that the phase ordering broke its own green-boundary invariant (§4)). The refactor then ran to completion across twenty iterations, and at the end every claim below was reconciled against the shipped tree. Where the plan and reality diverged, **reality won and the divergence is named** — see §4's per-phase state, and §6, which collects the limits that still stand. + +Two documents it replaces, `TESTING_SUMMARY.md` and `docs/E2E_TESTING.md`, have been deleted: both described a system that no longer exists (wrong ports, references to files that were never written, no mention of the real gate `make ci`). + +Read §3.1 and §3.5 before writing a test; §3.6 is what will fail you if you don't. --- @@ -13,13 +16,13 @@ We work agentically. The test suite is the thing that lets an agent (or a human) 1. **Green means deployable.** A passing run must certify the whole pipeline — endpoint → PDS → firehose → consumer → Postgres → serving endpoint — not just the parts whose infrastructure happened to be up. 2. **Red means a regression.** No flakes. A test that fails 1-in-20 runs trains agents and humans to retry instead of investigate, which is worse than not having the test. -3. **Skips are failures unless explicitly allowlisted.** Today there are 449 skip sites and `make test-all` counts every one as a pass. Stop the PDS and the suite still prints green. `cmd/ci-report` already inverts this — it becomes the only skip mechanism. +3. **Skips are failures unless explicitly allowlisted.** The suite this replaced had 449 skip sites and a `make test-all` that counted every one as a pass: stop the PDS and it still printed green. `cmd/ci-report` inverts that, and it is the only skip mechanism there is. 4. **Fast feedback tiers.** An agent iterating on a service should get signal in seconds (unit), on a repo in ~a minute (integration), and full pipeline certification in minutes (e2e) — each tier runnable independently. 5. **Hermetic means hermetic: tests NEVER touch public atProto infrastructure.** No `plc.directory`, no Bluesky relays/Jetstreams, no public PDSes — everything routes through our self-hosted Docker services (PLC directory, PDS, relay, Jetstream). This is a hard rule, enforced mechanically (3.7), with exactly one exception: the opt-in T3 `live` tier, which exists precisely so that reality checks are explicit instead of accidental. -## 2. Where we are (July 2026 survey) +## 2. Where we started (as surveyed, July 2026) -The full survey lives in the PR description for this branch. Counts below are approximate and were sampled at spec-writing time; the migration's real accounting is the violation audit script (§3.6), not this prose. The load-bearing facts: +**Historical.** Every count in this section is a snapshot of the suite *before* the refactor, sampled at spec-writing time and kept because the design decisions in §3 and §5 only make sense against it. None of it describes the tree today: `setupTestDB` and `tests/integration` no longer exist, `-short` is gone, and the numbers below are the problem, not the state. For what is true now, read §3 and §4; for the live accounting, run `make test-audit`. - **~84k LOC of tests** (41k in `tests/integration` alone), essentially no `t.Parallel()`, essentially no `require.Eventually`, **zero** build tags. The only tier selector is `-short`, which silently skips ~161 guarded sites — `make test` runs almost none of the integration suite while printing green. - **Taxonomy is inverted.** `tests/e2e/` contains two pure unit tests of the rate limiter (`httptest.NewRecorder`, no infra); `tests/integration/` contains the most end-to-end test in the repo (`user_journey_e2e_test.go`). 15 files named `*_e2e_test.go` live in `tests/integration`. Directory, filename, and function name each imply a different tier and none binds to what the test does. @@ -29,68 +32,80 @@ The full survey lives in the PR description for this branch. Counts below are ap - **Coverage is inverted relative to risk.** `internal/core/communities` (7 src files), `internal/core/votes` (6), `internal/atproto/identity` (7), `internal/api/routes` (17), `timeline`, `discover`, `communityFeeds`: **zero unit tests** — verified only through the slowest, most-skipped tier. `internal/db/postgres`: 16 repos, 3 test files. - **The good news:** the hermetic CI harness (`make ci` → `scripts/ci.sh` → `docker-compose.ci.yml` → `scripts/ci-runner.sh` → `cmd/ci-report` + `tests/ci/allowed_skips.txt`) is the highest-quality artifact in the tree: hermetic stack, skip-inversion, staleness enforcement, mandatory skip reasons. The refactor builds *on* it, not around it. -## 3. Target architecture +## 3. The architecture ### 3.1 Four tiers, mechanically enforced by build tags | Tier | Tag | Lives in | Talks to | Wall-clock budget | Parallel | |---|---|---|---|---|---| | **T0 Unit** | *(none)* | in-package (`internal/…`, `cmd/…`) | nothing out-of-process | < 60 s total | yes, default | -| **T1 Integration** | `//go:build integration` | in-package, next to the code it tests | Postgres only | < 3 min total | yes (see 3.3) | +| **T1 Integration** | `//go:build integration` | in-package, next to the code it tests | Postgres; a handful of packages also the PDS | < 3 min total | within a package (see 3.3) | | **T2 Pipeline (E2E)** | `//go:build e2e` | `tests/e2e/` | full hermetic stack incl. running AppView | < 10 min total | **no — serial** (see 3.4) | | **T3 Live** | `//go:build live` | `tests/live/` | public internet (Bluesky API, unfurl targets, real PLC) | opt-in only | — | +As built: **105** files carry `integration`, **16** carry `e2e`, **5** carry `live`, and everything else is T0. `make test` (T0 alone, no Docker) takes ~4 s warm and ~10 s with a cold test cache — comfortably inside its 60 s budget. + Rules that give the tiers teeth: -- **Build tags are the only tier mechanism.** `-short`/`testing.Short()` is deleted everywhere. You cannot accidentally run (or accidentally *not* run) a tier. -- **Tags classify files, so files must be single-tier.** Several current files mix tiers in one file (`identity_resolution_test.go`: DB cache tests + live-PLC tests; `bluesky_post_test.go`: pure URL parsing + live Bluesky API + DB; `post_unfurl_test.go`: local fixtures + third-party targets). These get **split by test function** during Phase 2, tracked in a migration manifest — tagging them in place is not possible and pretending otherwise was a hole in revision 1. -- **Missing infrastructure is a FAILURE, not a skip.** If you invoked `-tags integration`, you asked for Postgres; if it's not there, `t.Fatal`. All 34 copy-pasted "PDS not running → t.Skipf" preambles are deleted. The harness (3.3) does one connectivity check per package setup and fails fast with a message that says exactly which `make` target brings the stack up. The same applies to T3: an explicitly-invoked `make test-live` with missing config **fails**, it does not skip — skips are for the merge gate, and `live` is never on the merge path. -- **`t.Skip` is banned in test bodies.** The only legitimate skips are environmental gates owned by the harness, and every one must appear in `tests/ci/allowed_skips.txt` with a reason or `ci-report` fails the run. The 6 "DEBT — never run" entries in the current allowlist get deleted or implemented in Phase 2 — not carried forward. The 8 allowlisted "defs-only lexicon" subtest skips are fixed at the source in Phase 2: the validator stops *generating* subtests for defs-only lexicon files instead of generating-then-skipping them. -- **`tests/integration/` and `tests/unit/` are dissolved** by the end of the migration. Repo/service/consumer tests move next to the code they test (T1); pipeline sagas become T2 contracts; the two rate-limiter "e2e" files become T0 tests in `internal/api/middleware`. +- **Build tags are the only tier mechanism.** `-short`/`testing.Short()` is gone from the tree — all 161 guarded sites — and the audit (§3.6.3) scans production sources as well as tests to keep it that way. You cannot accidentally run, or accidentally *not* run, a tier. +- **Tags classify files, so files must be single-tier.** Files that mixed tiers were **split by test function**, not tagged in place: `identity_resolution_test.go` (DB cache tests + live-PLC tests), `bluesky_post_test.go` (pure URL parsing + live Bluesky API + DB), `post_unfurl_test.go` (local fixtures + third-party targets), and two `jetstream` files. Pretending a mixed file could be tagged whole was a hole in revision 1. A mixed *package* is still fine and common: its infra-gated `TestMain` lives in a tagged file and its shared fakes and helpers in an untagged sibling (`tests/testkit/harness_test.go` / `harness_support_test.go` is the worked example). Tag sets are additive, so a tagged half may use untagged helpers — never the reverse. +- **Missing infrastructure is a FAILURE, not a skip.** If you invoked `-tags integration`, you asked for Postgres; if it isn't there, the run fails. All 34 copy-pasted "PDS not running → `t.Skipf`" preambles are deleted; `testkit.Main(m, testkit.RequirePostgres, testkit.RequirePDS, …)` does one connectivity probe per package and fails fast naming the `make` target that brings the stack up. The same applies to T3: `make test-live` with missing config or no egress **fails** — the build tag is the opt-in, and `live` is never on the merge path, so nobody reaches it by accident and "could not reach reality" is an answer the caller needs to see in red. +- **`t.Skip` is banned in test bodies, and the tree contains none.** The one sanctioned skip mechanism is `tests/ci/allowed_skips.txt` + `cmd/ci-report`, and **the allowlist is empty** — all 25 entries went: 11 left with the public-network tests when they moved to `tests/live/`, the 6 "DEBT — never run" entries were deleted or implemented, and the 8 "defs-only lexicon" subtest skips were fixed at the source (the validator stopped *generating* subtests for defs-only lexicon files, which turned out to be a coverage gain: 43 fragment resolutions that previously asserted nothing now do). The audit deliberately offers no exemption marker for this category — two mechanisms for one rule is how the first one rots. +- **`tests/integration/` and `tests/unit/` are gone.** `tests/` now holds only what is genuinely cross-cutting: `e2e/` (contracts, the saga, the reliability suite), `testkit/`, `fixtures/`, `live/`, `ci/`, and the lexicon validation harness. Repo, service and consumer tests live next to the code they test as T1 — 20 such packages carry their own `TestMain`, plus three under `tests/`. ### 3.2 What each tier is *for* -**T0 — logic.** Validation, transforms, cursor encoding, error mapping, service logic against interface fakes. This is where behavioral breadth lives: every edge case, every error path. Packages currently at zero (communities, votes, identity, routes, timeline, discover, communityFeeds) get their matrix coverage here, not in 1,400-LOC integration files. +**T0 — logic.** Validation, transforms, cursor encoding, error mapping, service logic against interface fakes. This is where behavioral breadth lives: every edge case, every error path. The seven packages that were at zero coverage (communities, votes, identity, routes, timeline, discover, communityFeeds) got their matrix coverage here, not in 1,400-LOC integration files. **No package in `internal/core` is without tests** — though one, `communitysuggestions`, is covered at T1 only. T0 reaches no *out-of-process* service: it uses loopback `httptest` servers freely, and transport-error paths use an injected failing transport rather than a real remote dial. + +**T1 — the seams.** Two seams carry most of the weight and both terminate at Postgres: +- *Repo tests*: real SQL against a real schema (`internal/db/postgres`, 16 repos, 3 tested at the survey, now at 83% statement coverage). +- *Consumer tests*: synthetic `jetstream.JetstreamEvent` → `consumer.HandleEvent` → assert rows. This was the old suite's "Strategy A" and it is legitimate — it just isn't E2E and no longer claims to be. Idempotency, out-of-order delivery, rev-gating, malformed records: all here, cheap and deterministic. -**T1 — the seams.** Two seams matter and both terminate at Postgres: -- *Repo tests*: real SQL against a real schema (`internal/db/postgres`, 16 repos, currently 3 tested). -- *Consumer tests*: synthetic `jetstream.JetstreamEvent` → `consumer.HandleEvent` → assert rows. This is today's "Strategy A" and it is legitimate — it just isn't E2E and stops being named that. Idempotency, out-of-order delivery, rev-gating, malformed records: all here, cheap and deterministic. +Feed/timeline/discover sorting-and-cursor tests are DB-fixture-driven with no PDS, so they are T1 repo/service tests and live with their packages. -Feed/timeline/discover sorting-and-cursor tests (today's `feed_test.go`, `timeline_test.go`, `discover_test.go`, `comment_query_test.go` — DB-fixture-driven, no PDS) are T1 repo/service tests and move accordingly. +Six packages need a real PDS as well, and declare it in their own `TestMain`: these are the **write-forward** tests, which assert the *shape of the record the AppView writes* against a PDS that will reject a malformed one. They are the tier that proves authenticated write behaviour, which T2 structurally cannot (§3.4b) — so "Postgres only" is the common case, not a rule. -**T2 — the pipeline contracts.** See 3.4. Narrow and deep, not broad: ingestion proof per consumed collection, API contracts per client-facing write surface, plus a small pipeline-reliability suite. +**T2 — the pipeline contracts.** See 3.4. Narrow and deep, not broad: ingestion proof per consumed collection, API contracts per client-facing write surface, the cross-domain saga, and a pipeline-reliability suite. -**T3 — reality checks.** Live Bluesky handle resolution, real unfurl targets (YouTube/Reddit/Streamable/Kagi), real PLC. Valuable (real data catches what fixtures don't) but never on the merge path. Run nightly/pre-release via `make test-live`; failures notify, don't block. +**T3 — reality checks.** Live Bluesky handle resolution, real unfurl targets (YouTube/Reddit/Streamable), real PLC. Valuable (real data catches what fixtures don't) but never on the merge path — `make test-live` is opt-in, and failures notify rather than block. + +T3 has one rule worth stating on its own, because it is the opposite of the intuition: **a live test that cannot reach the internet must fail.** The tier used to be full of network-tolerant skips, so a live run with no egress printed green — the precise "green means nothing" failure this refactor exists to remove. They are all conversions to failure now, and the failure text says what was unreachable, notes that `make ci` blocks egress by design so the test cannot have come from inside the CI stack, and — for the assertions pinned to third-party content — names the fixture variable and the shape a replacement must have. Third-party content genuinely goes stale, and the resulting red is the point: the cost is a notification, the benefit is that "the shape we believe Bluesky emits" gets re-verified on every run instead of silently lapsing. ### 3.3 One harness: `tests/testkit` -A single package imported by all tiers. Everything below already exists in embryonic form somewhere in the tree — the harness is mostly consolidation, and it kills the current 3×`setupTestDB` + 10×`subscribeToJetstream*` + 5×image-fixture + 4×PDS-client-factory duplication (the four drifted `*PasswordAuthPDSClientFactory` adapters, both `createPDSAccount` definitions, and the hand-rolled XRPC clients all collapse into `pds.go`). +A single package imported by all tiers. Most of it was consolidation rather than invention, and it killed the 4×`setupTestDB` + 10×`subscribeToJetstream*` + 5×image-fixture + 5×PDS-client-factory duplication (the drifted `*PasswordAuthPDSClientFactory` adapters, both `createPDSAccount` definitions, and the hand-rolled XRPC clients all collapsed into `pds.go`). It ships with its own test suite — 125 test functions, 61 of them untagged, clean under `-race` and `-shuffle`. ``` tests/testkit/ - testkit.go // package setup helper: env, log silencing, infra fail-fast probe - db.go // Postgres isolation (below) - pds.go // account/session/record helpers (absorbs helpers.go:84-277) - firehose.go // cursor-gated subscribe, ONE generic implementation (T1/T3 debugging aid) + testkit.go // package setup: testkit.Main + Require* infra probes, env, log silencing + db.go // Postgres isolation (below), endpoint loading, concurrency budget + pds.go // account/session/record helpers + firehose.go // cursor-gated subscribe, ONE generic implementation (T1/debugging aid) wait.go // the only wait primitives allowed in tests fixtures.go // UniqueID, PNG/JPEG bytes, generic record builders appview.go // T2 only: XRPC client against the running AppView ``` -**Dependency direction (hard rule, prevents import cycles).** `testkit` imports **no** `internal/core/*` domain package — only infra-level packages (`jetstream` event types, DB driver, migrations). This matters because T1 tests are *in-package*: if `testkit` imported `internal/core/communities`, then `communities`' own test file importing `testkit` would be an import cycle. Domain-specific builders that need domain types live either in the domain's own `_test.go` files or in small leaf `test` packages — never in the shared kit. +**Dependency direction (hard rule, prevents import cycles).** `testkit` imports **no** `internal/core/*` domain package — only infra-level packages — in practice the DB driver and the embedded migrations, since even the `jetstream` event type has to be duplicated (see below). This matters because T1 tests are *in-package*: if `testkit` imported `internal/core/communities`, then `communities`' own test file importing `testkit` would be an import cycle. Domain-specific builders that need domain types live either in the domain's own `_test.go` files or in small leaf `test` packages — never in the shared kit. + +The rule is **transitive**, which is sharper than it sounds and was learned the hard way: `internal/atproto/pds` imports `internal/core/blobs`, so testkit could not use the obvious PDS client and had to reimplement it. Where testkit duplicates a type for this reason, `package testkit_test` (an external test package, which *may* import the internal one) pins the duplicate against the original — `firehose_pin_test.go` is the example to copy. **DB isolation — template-clone-per-test, harness-provisioned.** Template creation/migration does **not** live in per-package `TestMain`s: `go test ./...` runs each package as a separate binary, several in parallel, and N processes racing to create-and-migrate `coves_test_template` is a built-in flake. Instead: - The **Make/CI harness** provisions the template once before `go test` runs (`make test-integration` and `ci-runner.sh` both call the same `scripts/test-db-prepare.sh`: create template if absent, run migrations, stamp it with a hash of the migrations dir; re-provision on hash mismatch). - As a belt-and-suspenders for direct `go test -tags integration ./internal/foo` invocations, `testkit`'s package-setup path takes a **Postgres advisory lock** around a verify-or-provision of the template, so ad-hoc runs are safe too. - `testkit.DB(t)` executes `CREATE DATABASE … TEMPLATE coves_test_template` with a sanitized unique name, returns the pool, and `t.Cleanup` force-drops it (`WITH (FORCE)`, so leaked connections can't wedge teardown). Panic-orphaned clones are swept by `test-db-prepare.sh` on the next run (name prefix + age). -- **Connection budgets are explicit.** Cloned-per-test pools multiply fast: pool size defaults to 2–3 per test DB, `-parallel` is capped by a computed budget (`max_connections` ÷ pool size, with headroom for the AppView), and the CI Postgres sets `max_connections` accordingly. This is configured once in testkit + compose, not per test. +- **Connection budgets are explicit.** Cloned-per-test pools multiply fast: pool size defaults to 2–3 per test DB, `-parallel` is capped by a computed budget (`max_connections` ÷ pool size, minus headroom for Postgres' superuser reserve, a developer's `psql` and a concurrent `test-db-prepare` — *not* for the AppView, which talks to a different server), and the CI Postgres sets `max_connections` accordingly. This is configured once in testkit + compose, not per test. + +A clone costs ~30 ms on local NVMe — well under the 100–200 ms the plan budgeted — and ~133 ms per test end-to-end in CI once pool setup and the force-drop are counted. It was bought back comfortably: all 243 `setupTestDB` call sites across 60 files (the §2 survey undercounted at 222/50 — it missed four hand-rolled clones of the helper), all four `setupTestDB` definitions, 82 distinct unscoped `DELETE FROM` statements and every per-file `cleanup*` function are deleted, and tests cannot see each other by construction. A full `-shuffle=on` integration run was green the first time it was tried, which is the evidence that the wipes had become dead weight. `testkit.DB(t)` is now the only way a test in this tree gets a database, grep-verified. `goose` survives in exactly one place — testkit's own template provisioning, through `NewProvider` rather than the package globals — and in no test body. + +**Parallelism is earned, not assumed.** DB isolation removed the *biggest* blocker to `t.Parallel()`, not every blocker: the tree also had ~100 sites of process-global mutation (`t.Setenv` — which Go itself rejects under `t.Parallel()` — plus `os.Setenv`, logger and default-HTTP-client fiddling). Those were audited and classified before any parallelism was enabled; the tree now has **1,127 `t.Parallel()` calls** and runs clean under `-race` and `-shuffle`, with a small number of sites left deliberately serial and annotated as such. -Cost is ~100–200 ms per test on local NVMe — trivially bought back: all 222 `setupTestDB` call sites, all unscoped `DELETE FROM` wipes, and every per-file `cleanup*` function are deleted; tests cannot see each other by construction. +**`-p 1` survives, and not for the reason it originally existed.** The plan was to delete it at the end of phase 3. It is still there, because dropping it surfaced a shared resource one level up from the database: **Jetstream `account` and `identity` events bypass the consumers' wanted-collection filter entirely**, so a burst of parallel signups is visible to every subscriber on the stream and starves them (measured: the full tagged run failed 2 of 4 times at `-p 2`, and 0 of 4 at `-p 1`). The constraint is the *stream*, not the DB, and the flag now carries that reason in `tests/testkit/db.go` next to `packageParallelism`. Within a package, `-parallel` is set from a computed connection budget rather than a guess. See §6. -**Parallelism is earned, not assumed.** DB isolation removes the *biggest* blocker to `t.Parallel()`, not every blocker: the tree also has ~100 sites of process-global mutation (`t.Setenv` — which Go itself rejects under `t.Parallel()` — plus `os.Setenv`, logger and default-HTTP-client fiddling). Phase 3 therefore audits and classifies these first (inject env/clock/client via testkit instead of mutating globals), enables `t.Parallel()` only on proven-safe tests, and runs the race detector as part of the phase's exit criteria. `-p 1` is deleted at the end of that phase, not the start. +**Firehose helper — T1 and debugging scope only.** One generic, cursor-gated subscriber replaced the 10 copies: capture the cursor *before* the write and replay from it, so the subscribe-after-write race is unrepresentable through the API, and the gorilla/websocket consecutive-timeout guard is written exactly once. **T2 contracts never dial websockets** — the AppView's own consumers do the consuming (3.4) — and outside testkit's one generic subscriber — plus the `RequireJetstream` liveness probe — no test in the tree dials one any more. -**Firehose helper — T1/T3 scope only.** One generic, cursor-gated subscriber replaces the 10 copies (capture cursor *before* the write, replay from it — the subscribe-after-write race becomes unrepresentable through this API; the gorilla/websocket `maxConsecutiveTimeouts` guard is written exactly once). But note its narrow role: **T2 contracts never dial websockets** — the AppView's own consumers do the consuming (3.4). The helper exists for T1-adjacent consumer plumbing tests and debugging, not as the E2E mechanism. +Writing that guard once mattered more than the deduplication did. All ten hand-rolled copies shared a latent bug: a gorilla connection is **corrupt after a read deadline expires**, so a loop that `continue`s past a timeout silently gives up at ~5 s regardless of the 30 s it advertised. Years of "Timeout: No Jetstream event received" failures were often that, not slow indexing. testkit's version re-dials on every read error (only an undecodable frame is terminal), counts events discarded for predating the cursor as a clock-skew diagnostic, and fails on pending-buffer overflow rather than dropping. **Waiting — three primitives, everything else is banned:** @@ -103,9 +118,9 @@ fh.Await(t, match) // firehose delivery (T1/debugging scope, s Probes return `(done bool, err error)` where a non-nil `err` is **terminal** — a 401 or 500 from a serving endpoint fails immediately with that response attached, instead of being retried into an opaque timeout (agents debugging a timeout with no context is exactly the failure mode this tier exists to avoid). On timeout, `WaitFor` reports the last observation, and in T2 additionally snapshots the AppView's consumer-health endpoint (cursor positions, dead-letter counts) into the failure message. -`time.Sleep` in `*_test.go` fails CI via the lint gate. All sleeps, hand-rolled `select`/`time.After` blocks, and bare retry loops migrate to these. +`time.Sleep` in test code **fails the gate** (§3.6.3). Two survive in the whole tree, both inside `WaitFor`/`Holds` themselves — a poll loop sleeps — and both carry a per-line `coves:allow-sleep` marker. testkit is *scanned* rather than exempted as a directory, which is a deliberate narrowing: "the package that implements waiting" was licensing its own tests too, and the last one hiding behind that (a 150 ms settle in `appview_test.go` racing a health probe against the scheduler) became a probe count instead. Every sleep, hand-rolled `select`/`time.After` block and bare retry loop migrated to these primitives, including the ones that looked immovable: a circuit breaker's open window is crossed by ageing its stored `lastFailure` rather than by waiting out the window; a rate limiter's window is crossed through an injected clock; an `httptest` handler that needed to be slow blocks on a channel the test releases instead of sleeping. Two traps worth knowing before converting one yourself — **a probe with a side effect can prevent its own success** (polling a TTL cache through the read API that touches mtime keeps the entry permanently young: the wait times out and the code is fine), and a **negative** claim needs `Holds`, never `WaitFor`, because an eventually-check is satisfied by an asynchronous fix on its way to the right answer and then passes forever against corrected code. -**Identity — `testkit.UniqueID(t)`** is the only handle/ID generator: a per-run random prefix (seeded once per process) + atomic counter, ≤18-char PDS-safe (per the PDS local-label cap). The counter-only design of revision 1 was insufficient: counters reset across processes and local PDS state persists across runs, so two `make test-e2e` invocations in the same second could collide. The surviving `time.Now().Unix()` handle-collision sites (`user_signup_test.go:69,110,153`, `community_e2e_test.go:177,380` — one inside a loop) are migrated in Phase 1. Hermetic-stack runs get fresh PDS volumes; for kept stacks (`COVES_CI_KEEP_STACK`) accumulation is accepted and documented — the run-scoped prefix makes it harmless. +**Identity — `testkit.UniqueID(t)`** is the only handle/ID generator: a per-run random prefix (seeded once per process) + atomic counter, ≤18-char PDS-safe (per the PDS local-label cap). The counter-only design of revision 1 was insufficient: counters reset across processes and local PDS state persists across runs, so two `make test-e2e` invocations in the same second could collide. Hermetic-stack runs get fresh PDS volumes; for kept stacks (`COVES_CI_KEEP_STACK`) accumulation is accepted — the run-scoped prefix makes it harmless. Because the prefix is process-scoped, `-count=N` never exercises prefix-keyed code: assert structure, not substrings. **Assertions — testify everywhere.** New/touched code uses `require` (fail fast) / `assert` (accumulate); no new bare `t.Errorf`. @@ -124,85 +139,147 @@ Record written DIRECTLY to the PDS (bypassing the AppView entirely) → destructive steps additionally verified with Holds (deletes STAY deleted) ``` -Because the AppView never saw the write, firehose delivery is the *only* way the data can appear — a dead consumer cannot false-pass. Shape: create → visible; update → visible; delete → gone *and stays gone*. This is honest **direct-PDS-path** testing, not federation (the record still lives on the one PDS the stack fronts) — true federation-path testing needs a second, independently-addressed PDS and a relay in the hermetic stack, which is Phase 5 scope, and the spec stops calling single-PDS direct writes "federation". +Because the AppView never saw the write, firehose delivery is the *only* way the data can appear — a dead consumer cannot false-pass. Shape: create → visible; update → visible; delete → gone *and stays gone*. (Jetstream is in that chain rather than the raw firehose because the PDS emits CBOR-encoded commits and Jetstream converts them to the same JSON the production consumers read; the tier consumes exactly what production consumes.) **All ten consumed collections have one**, enforced by the manifest check below. -**The contract inventory is generated, not hand-curated.** Revision 1's hand-written list was already wrong (it missed `social.coves.actor.block` and `social.coves.community.block`, and collapsed `aggregator.service`/`aggregator.authorization` — two distinct record types). The source of truth is `jetstream.WantedCollections` (the map in `internal/atproto/jetstream/feeds.go`): a CI check walks every consumed collection and fails if any lacks a `//coves:ingestion-contract ` marker in `tests/e2e/`, or if a marker points at a collection no longer consumed. Adding a collection to a consumer without a contract breaks the build — mechanically, not by review vigilance. +This much is honest **direct-PDS-path** testing but it is not federation: the record still lives on the PDS the stack fronts. Federation is now built as well, and is a distinct thing: + +**Federation-path contracts (delivered).** The hermetic stack carries a **second PDS** (`pds2` — its own hostname, volume and PLC registration, no AppView credentials and no configuration naming it) and a **hermetic relay**, with Jetstream re-pointed through the relay so the topology is prod-shaped. Four contracts promote the highest-value collections to the federated path — post, comment, vote, and remote blob fetch — and a fifth pins the identity limitation described below. In a promoted contract the record is written to the *non-fronted* PDS, discovered and indexed purely via the firehose, and remote blob fetching is proven un-fakeable (the blob is asserted absent locally first). Two production defects were reproduced hermetically this way. + +What federation did **not** deliver is as important, because it is structural rather than unfinished: **federated authors are unindexable.** The users consumer default-denies identities from untrusted hosts, so a remote author never gets a row — federated comments, votes and blobs work precisely because they have no author foreign key. Turning the trust config on was considered and rejected: it makes `pds2` identities resolvable, which walks straight into the `handle.invalid` UNIQUE-squat defect and leaves the stack non-re-runnable. This is a documented limit (§6), not a gap someone forgot. + +**The contract inventory is generated, not hand-curated.** Revision 1's hand-written list was already wrong (it missed `social.coves.actor.block` and `social.coves.community.block`, and collapsed `aggregator.service`/`aggregator.authorization` — two distinct record types). The source of truth is the consumer table in `internal/atproto/jetstream/feeds.go`, read through `jetstream.ConsumedCollections()`: `cmd/contract-manifest` walks every consumed collection and fails if any lacks a `// coves:ingestion-contract ` marker in `tests/e2e/`, or if a marker points at a collection no longer consumed. Markers only count in files that actually build under `-tags e2e` (checked with `go/build`'s `MatchFile`, not a grep), and a marker file may not import `websocket`/`jetstream` or call `testkit.DB` — an AST check, because a "contract" that reaches around the pipeline is worse than none. Adding a collection to a consumer without a contract breaks the build, mechanically, not by review vigilance. + +The burn-down file `tests/ci/pending_contracts.txt` let phase 4 defer a collection by naming the task that owed it a contract. **It is empty and the ratchet is closed**: both entry points pass `-allow-pending=false`, so a line in that file now fails the gate rather than buying time. **(b) API contracts — the client-facing surface. One per write endpoint family.** Client-path through the AppView's XRPC write endpoints, exactly as the mobile app calls them, asserting the *response* and the *synchronous* effects (session issued, record URI returned, synchronously-indexed rows present, blob accepted). These verify what third-party clients experience — including precisely the synchronous-indexing behavior that makes them unsuitable as pipeline proof. Avatars/blob uploads are covered here as steps of the `user` and `community` API contracts (they are blob-path cases of those record types, not record types of their own). -**Known limitation (July 2026, discovered writing the community contract): T2 cannot authenticate a write.** `OAuthAuthMiddleware.RequireAuth` accepts exactly one credential — a *sealed* session token naming a row in the OAuth session store — and the only thing that mints one is `/oauth/callback`, at the end of the browser authorization-code flow against the PDS' own HTML login pages. `social.coves.actor.signup` returns the PDS' `accessJwt`, which is not sealed and is rejected; `/oauth/refresh` requires a sealed token to begin with. The integration tier sidesteps this in-process (`store.SaveSession` + `client.SealSession`), which T2 cannot do without writing to the AppView's own database — the one thing §3.4's rules forbid. So until that changes, an API contract covers **the auth boundary** (every write NSID answers 401 to a session-less client — which also catches a route registered without the middleware, something a handler test structurally cannot see) and **the read surface** (a record indexed through the pipeline is served back by every identifier form a client may use), while authenticated write *behaviour* is proven at T1: handler tests against a mock service, plus write-forward tests that assert the record shape against a real PDS. A hard-gated, test-only session-minting path in the AppView would close the gap and is Phase-5 pre-work, not something a contract may improvise. +**Standing limitation (discovered writing the community contract; still true as shipped): T2 cannot authenticate a write.** `OAuthAuthMiddleware.RequireAuth` accepts exactly one credential — a *sealed* session token naming a row in the OAuth session store — and the only thing that mints one is `/oauth/callback`, at the end of the browser authorization-code flow against the PDS' own HTML login pages. `social.coves.actor.signup` returns the PDS' `accessJwt`, which is not sealed and is rejected; `/oauth/refresh` requires a sealed token to begin with. The integration tier sidesteps this in-process (`store.SaveSession` + `client.SealSession`), which T2 cannot do without writing to the AppView's own database — the one thing §3.4's rules forbid. So until that changes, an API contract covers **the auth boundary** (every write NSID answers 401 to a session-less client — which also catches a route registered without the middleware, something a handler test structurally cannot see) and **the read surface** (a record indexed through the pipeline is served back by every identifier form a client may use), while authenticated write *behaviour* is proven at T1: handler tests against a mock service, plus write-forward tests that assert the record shape against a real PDS. A hard-gated, test-only session-minting path in the AppView would close the gap; it remains unbuilt. See §6. + +The unauthenticated read surface has its own reachable edge, worth knowing before you try to contract something that cannot be contracted: **anything viewer-scoped is invisible at T2.** Personalised `getTimeline` is `RequireAuth`, so subscription fan-out cannot be observed from this tier at all (`getDiscover` deliberately does not filter, `communityFeed` filters by community, and `community.list?subscribed=true` 401s); block enforcement is likewise entirely viewer-scoped, so the block contracts observe **consumer-health measurement windows** — snapshot the counters, write the block, bound on a same-repo visible event, re-read — rather than a serving endpoint. Both are covered at T1 and both would be unlocked by the session mint. **(c) Pipeline-reliability suite — the failure modes CRUD never touches.** -The production ingestion path has machinery that steady-state contracts cannot exercise: persisted cursors, reconnect-and-replay, rev-gating (stale events must not resurrect or regress records), duplicate delivery, dead-letter capture, multi-feed consumers. One small suite covers it end-to-end: restart the AppView mid-stream and verify cursor resume; write during a Jetstream outage and verify replay indexes exactly once; deliver a stale rev after a delete and verify no resurrection (`Holds`); poison a record and verify dead-letter capture + consumer health reporting. CI's single self-feed topology differs from prod's multi-feed setup, so this suite also runs one overlapping two-feed configuration to exercise the rev-gating overlap path. +The production ingestion path has machinery that steady-state contracts cannot exercise: persisted cursors, reconnect-and-replay, rev-gating (stale events must not resurrect or regress records), duplicate delivery, dead-letter capture, multi-feed consumers. Five scenarios cover it end to end and all five prove a production property rather than a test-harness one: restart the AppView mid-stream and verify cursor resume; take the AppView down and back up, and verify the connector's cursor rewind replays without double-indexing — counted in *rows*, not counters; stage a dead letter and verify a stale rev cannot resurrect it; recover a dead letter through the **boot-time redrive pass**, i.e. the production path, with no test-only knob; and run one overlapping **two-feed** configuration on fresh keys, asserting the connector *names*, because CI's single self-feed topology differs from production's multi-feed setup and the rev-gating overlap path is exactly what that difference hides. + +The suite needs to restart and partition containers, which a test inside the network namespace cannot do. Rather than mount the Docker socket, it drives a **file-based control channel**: five argument-less verbs written to a project-scoped path, watched by a host-side process with a pidfile. The watcher is absent from the production image. A `TestMain` guard (`requireSingleFeedTopology`) refuses to run against a stack left in the two-feed configuration by a previous run, because a poisoned kept stack silently invalidates every measurement window in the tier. Rules that hold across all three classes: -1. **T2 tests never instantiate consumers and never dial websockets.** The AppView container consumes, exactly as deployed; tests observe via serving endpoints (plus the consumer-health endpoint for reliability assertions). This deletes the entire `subscribeToJetstream*` family from the E2E tier and finally exercises `cmd/server`'s wiring. -2. **T2 runs serially.** All contracts share one AppView, one `coves_dev` DB, one PDS, one PLC, one cursor stream — template-cloning isolates T1, not this tier. ~10–15 serial contracts fit the 10-minute budget; if it ever grows past that, the answer is stack-sharding, not interleaving writers into a shared eventually-consistent namespace. All identities are run-scoped via `UniqueID`. -3. **Behavioral breadth is out of scope.** "Does `sort=alphabetical` work" (17 of `community_e2e_test.go`'s 20 subtests) is a T1 service/repo test. If a T2 contract fails, the answer to "which layer broke?" should be "the pipeline," not one of 20 endpoint behaviors. This is how the 1,820-LOC and 1,001-LOC god-files get decomposed rather than moved. +1. **T2 tests never instantiate consumers and never dial websockets.** The AppView container consumes, exactly as deployed; tests observe via serving endpoints (plus the consumer-health endpoint for reliability assertions). This deleted the entire `subscribeToJetstream*` family from the E2E tier and finally exercises `cmd/server`'s wiring. +2. **T2 runs serially** (`-p 1 -parallel 1`, written down exactly once, in `scripts/lib/runner-ready.sh`). All contracts share one AppView, one DB, one PLC, one cursor stream (and the two PDSes of §3.4a's federation topology) — template-cloning isolates T1, not this tier. If the tier ever outgrows its budget, the answer is stack-sharding, not interleaving writers into a shared eventually-consistent namespace. All identities are run-scoped via `UniqueID`, and each contract gets a synthetic client IP: the AppView's rate limiter is **one bucket per IP across the whole shared namespace** and it outlives the run on a kept stack. +3. **Behavioral breadth is out of scope.** "Does `sort=alphabetical` work" (17 of the old `community_e2e_test.go`'s 20 subtests) is a T1 service/repo test. If a T2 contract fails, the answer to "which layer broke?" should be "the pipeline," not one of 20 endpoint behaviors. This is how the 1,820-LOC and 1,001-LOC god-files were decomposed rather than moved. 4. **Sequential steps within a contract are legitimate here and only here** — eventual consistency is a pipeline, and the pipeline is what's under test. -5. **One cross-domain saga survives**: `user_journey` (signup → community → post → comment → vote → timeline), rebuilt on testkit, as the single "does the whole product hold together" smoke test. +5. **One cross-domain saga survives**: `user_journey` (signup → community → post → comment → vote → timeline), rebuilt on testkit as the single "does the whole product hold together" smoke test — 3 actors, 3 repos, 4 read paths. The file it replaced had 2 genuinely-end-to-end steps out of 11, with direct SQL inserts standing in for the rest. +6. **Negative bounds must be intra-repo.** Jetstream parallelises across repos, so a "the bad record did not appear" claim bounded by a good record in a *different* repo is topology luck. Write both into the same repo, and bound on an observable that the same consumer subscription delivers — never on a counter that unfiltered `identity`/`account` events also advance. +7. **A pin is not an assertion of intent.** Where a contract documents a known defect, the assertion message names the issue file and says what a failure means ("if this failed, the defect is fixed"), the wrong-but-current value is pinned with `Holds` as well as `WaitFor`, and any production comment next to the defect carries a KNOWN DEFECT note — otherwise the next reader "cleans up" the comment and the pin quietly becomes a specification. ### 3.5 Command surface ``` -make test # T0. No Docker. The inner loop. <60s. -make test-integration # T1. Provisions template DB, starts postgres-test if needed. Parallel. +make test # T0. No Docker, no database, no public network — in-process + # httptest loopback is fine (§3.2). The inner loop. <10s. +make test-integration # T1. Provisions the template DB, starts postgres-test if needed. make test-e2e # T2. Runs INSIDE the hermetic stack via the compose runner (see below). +make test-e2e-dev # T2 against the long-lived dev stack. Debugging only; skips the + # reliability and federation contracts, which need the CI topology. make test-live # T3. Opt-in, hits the internet. Missing config = failure, not skip. -make ci # THE GATE: hermetic stack, T0+T1+T2, ci-report skip inversion. +make ci # THE GATE: hermetic stack, T0+T1+T2, egress-blocked, ci-report verdict. +make test-audit # The violation audit on its own (§3.6.3). Hard gate; -v for file:line. +make test-db-prepare # Create or refresh the template database testkit.DB clones per test. ``` -- **`make test-e2e` runs through the compose runner, not from the host.** The hermetic stack publishes no host ports (by design, see 3.7), so a host-run `go test -tags e2e` cannot reach it; revision 1's "requires the ci stack or dev stack up" was incoherent. The target brings up the stack (or reuses a `COVES_CI_KEEP_STACK` one) and executes the e2e binary inside the runner's network namespace — the same path `make ci` uses, so there is exactly one way T2 executes. Debugging against the long-lived dev stack stays possible via an explicitly-named `make test-e2e-dev` escape hatch. -- `make ci` stays exactly what it is today (build from tree → staged compose up → bootstrap → run → `ci-report`), except the runner invokes tiers by tag: T0+T1 with `-tags integration` in parallel, then T2 with `-tags e2e` serially against the composed AppView. -- `make test-all` is deleted. It is a slower `make ci` with weaker guarantees (any-one-service-up counts as "infra ready", skips count as passes, `./internal/...` runs without `-p 1` — a live race today). One gate, not two. -- Silent-failure fixes ride along: `goose up || true` in `make test` loses the `|| true`; `sleep 3` becomes `pg_isready` polling; dead targets (`create-test-account`, `verify-stack` — scripts that don't exist) are removed. +- **`make test-e2e` runs through the compose runner, not from the host.** The hermetic stack publishes no host ports (by design, see 3.7), so a host-run `go test -tags e2e` cannot reach it; revision 1's "requires the ci stack or dev stack up" was incoherent. The target brings up the stack (or reuses a `COVES_CI_KEEP_STACK` one) and executes the e2e binary inside the runner's network namespace — the same path `make ci` uses, so there is exactly one way T2 executes. The `make test-e2e-dev` escape hatch is explicitly named as such. +- `make ci` is what it always was (build from tree → staged compose up → bootstrap → run → `ci-report`), with the runner invoking tiers by tag: T0+T1 with `-tags integration`, then T2 with `-tags e2e` serially against the composed AppView. +- **`go test`'s exit code is not the verdict; `ci-report` is** — but a mismatch between them fails the gate. That rule closed a hole the harness had since birth: a runner OOM-killed at 137 while its report said "ok" used to pass silently, because a truncated stream contains no failures. Exit statuses are captured and reconciled now, by two rules written down beside the check in `scripts/ci-runner.sh`: a `go test` status above 1 fails the gate regardless of the report, and a nonzero status with an "ok" report means a truncated capture and also fails. The rules are shell, not Go, and have no automated test of their own — `ci-report`'s table-driven tests cover its gate outcomes, not this cross-check. +- `make test-all` is deleted. It was a slower `make ci` with weaker guarantees (any-one-service-up counted as "infra ready", skips counted as passes, `./internal/...` ran without `-p 1`). One gate, not two. +- Silent-failure fixes rode along: `goose up || true` in `make test` lost the `|| true`, `sleep 3` became a real connection to the published port (an in-container `pg_isready` would prove only that Postgres is up on its own loopback, so a wrong or already-claimed port would sail through), and the dead targets (`create-test-account`, `verify-stack` — scripts that never existed) are gone. +- `make test-integration` **fails loudly without the dev stack** rather than skipping green. Each package declares its own floor in its `TestMain` — most want only `RequirePostgres`, six add `RequirePDS`, and `RequireJetstream` appears only in testkit's own harness — so the target as a whole needs the full dev stack while an individual `go test -tags integration ./internal/foo` needs only what that package asked for. ### 3.6 Enforcement (so it doesn't rot back) -The suite got here by drift, so the invariants get mechanical guards, all inside `make ci`: +The suite got here by drift, so the invariants have mechanical guards. **All four are hard gates inside `make ci`.** Each of the three ratchets was probed before being called done — a violation introduced, the red confirmed, the violation reverted; `tests/ci/pending_contracts.txt`'s header records the contract-manifest probe in both directions, which is the one that left an artefact: -1. `ci-report` (exists): skip ⇒ failure unless allowlisted-with-reason; stale allowlist entries fail. -2. **Contract manifest check** (new, Phase 4): every collection in `jetstream.WantedCollections` has a live ingestion-contract marker in `tests/e2e/`, and no marker is stale (3.4a). -3. **Violation audit script** (`scripts/test-audit.sh`, new in Phase 1): counts `time.Sleep` in tests, `t.Skip` outside testkit, `testing.Short()`, `websocket.DefaultDialer` outside `testkit/firehose.go`, hardcoded endpoint literals outside testkit, and public atProto hostnames outside `tests/live/`. It is both the lint gate (warn during migration → hard-fail at Phase 6) and the migration's progress meter — every count must be assigned to a phase and reach zero, so "the final lint flip will pass" is tracked continuously instead of hoped for. These greps are **tripwires, not proof** — they catch drift cheaply and are bypassable by construction (concatenation, IP literals); the *guarantee* is layer 3 of 3.7. -4. `go vet ./...` with each tag set — tagged files that don't compile are otherwise invisible. +1. **`cmd/ci-report` — skip inversion.** A skip is a failure unless `tests/ci/allowed_skips.txt` allowlists it with a reason; an allowlist entry that did *not* skip this run fails as stale. The allowlist is currently **empty**, and its header argues that the next person to add a line should first check the answer is not "fix the test instead". +2. **`cmd/contract-manifest` — the contract manifest.** Every collection the consumer table declares (`jetstream.ConsumedCollections()`) has a live ingestion-contract marker in `tests/e2e/`, and no marker is stale (3.4a). Run with `-allow-pending=false` at both entry points; the burn-down file is empty. +3. **`scripts/test-audit.sh` — the violation audit.** Counts `time.Sleep` in test code (testkit included), `t.Skip` outside testkit, `testing.Short()` anywhere, client-side websocket dials in test code (`DefaultDialer`, a hand-rolled `websocket.Dialer`, `NewClient` — not the server-side `Upgrader`, which the fake-Jetstream test servers legitimately use), hardcoded endpoint literals, and public atProto hostnames outside `tests/live/`. It ran in warn mode through phases 0–5 as the migration's progress meter — 911 violations at the phase-1 baseline, every count assigned to a phase — and **now fails the build**. All six categories are at zero. The residue that is legitimately not zero is declared in the source with a reason marker (`coves:allow-sleep`, `coves:allow-host-literal`, `coves:allow-public-host`, plus a `-file:` form for files whose entire subject matter *is* the literal), and file-scope exemptions print their reason on every run so a broad one cannot go quiet. There is deliberately no marker for `t.Skip`. These greps are **tripwires, not proof** — bypassable by construction, via concatenation or an IP literal; the *guarantee* is layer 3 of 3.7. +4. **`go vet` with each tag set** — `./...` untagged, `-tags integration ./...`, `-tags e2e ./tests/e2e/...`, `-tags live ./tests/live/...`. Tagged files that don't compile are otherwise invisible, and the `live` pass is the load-bearing one because that tier never executes on the merge path. ### 3.7 Network isolation: never the public network Three layers, from convention to physics: -1. **All endpoints come from testkit, all defaults point at the Docker stack.** `testkit.Endpoints()` reads `PDS_URL`, `PLC_DIRECTORY_URL`, `JETSTREAM_URL`, `APPVIEW_URL`, `POSTGRES_TEST_*` once, defaulting to the compose-stack addresses. No other test code constructs a base URL. (This also retires the 3 leftover `localhost:2583` references — a stale upstream-PDS default from a different topology.) -2. **Lint tripwire** (3.6.3): public atProto hostnames in test/testkit code outside `tests/live/` fail CI. Scope-aware, not a blanket ban — hostname literals in *fixture data* (e.g. validating production config parsing) are fine and get an explicit exemption comment; the tripwire targets endpoint construction. -3. **Egress-blocked CI network (the actual guarantee)**: the hermetic stack's networks become `internal: true`, so an accidental public call — however constructed — fails at connect time instead of silently resolving against real infrastructure. Two prerequisites discovered in review, both handled in Phase 0 *after* the public-network tests are moved out (see ordering in §4): (a) the runner image downloads Go modules at runtime, so a cold cache + no egress = broken build — `test-db-prepare`/`ci.sh` populate the module cache volume *before* attaching the runner to the internal network (and `make ci-clean` notes that the next run re-populates); (b) a cold-cache, egress-blocked `make ci` run is an explicit Phase 0 acceptance check, not an assumption. +1. **All endpoints come from testkit, all defaults point at the Docker stack.** `testkit.Endpoints()` reads `POSTGRES_TEST_*`, `PDS_URL`, `PDS2_URL`, the two `*_SERVICE_HANDLE_DOMAINS`, `PLC_DIRECTORY_URL`, `JETSTREAM_TEST_URL` and `APPVIEW_URL` (falling back to `APPVIEW_PUBLIC_URL`) once per process, defaulting to the compose-stack addresses. The Jetstream variable is deliberately not `JETSTREAM_URL`: that one is the server's own config, and reusing it would couple the tests' view of the stack to the AppView's. No other test code constructs a base URL. `PDS2` — the federated PDS — is deliberately *not* defaulted: it exists only in the CI stack, so a default would make a dev-stack run dial a dead port. Instead `RequireFederatedPDS` returns cleanly when `PDS2_URL` is unset — keeping the dev escape hatch coherent — and `NewFederatedPDS` fails by name if a federation contract is actually reached without one. +2. **Lint tripwire** (3.6.3): public atProto hostnames in test code outside `tests/live/` fail the build. Scope-aware, not a blanket ban — only hostnames inside a URL with a scheme are counted, which drops ~92 pure-data matches on the pattern alone, and what remains and is still legitimate (a URL parser's inputs, a config test asserting the production default) carries a declared exemption. The tripwire targets endpoint construction. +3. **Egress-blocked CI network (the actual guarantee)**: the hermetic stack's network is `internal: true`, so an accidental public call — however constructed — fails at connect time instead of silently resolving against real infrastructure. Two prerequisites, both handled: (a) the runner image downloads Go modules at runtime, so a cold cache with no egress is a broken build — `ci.sh` populates the module-cache volume *before* attaching the runner to the internal network, and `GOPROXY=off` is set on the runner *service* only, since the prefetch runs the same image outside the stack; (b) a cold-cache, egress-blocked `make ci` is a run we actually perform, not an assumption. + +The egress block earned its keep immediately: turning it on found **four** hidden runtime dependencies where the survey had predicted one — a hardcoded Turnstile verification URL, a healthcheck that redirected to a public web page, three tests dialling `public.api.bsky.app`, and a DNS-dependent 404 case. No grep finds a healthcheck redirect. Expect it to keep finding things, and note the failure mode it creates: egress failures are *fast* (DNS resolves to nothing in milliseconds), so a fallback-tolerant test does not hang or skip — it silently takes its fallback path and passes. -The env-gated real-handle tests (`TEST_REAL_HANDLES=1`) hit the real PLC by design — they become T3 `live` tests, where that is explicit and opt-in rather than an env var buried in a conditional. +The env-gated real-handle tests (`TEST_REAL_HANDLES=1`) hit the real PLC by design. They are T3 `live` tests now, where that is explicit and opt-in rather than an env var buried in a conditional. -## 4. Migration plan +## 4. The migration, as executed -Each phase lands as its own commit(s) on `worktree-test-refactor`, and **`make ci` must be green at every phase boundary**. Phases are sized to be a focused agent session each, except Phase 4 (multi-session, per-domain). LOC figures are rough deltas (added − deleted). +**All six phases are complete.** Each landed as its own commit(s) on `worktree-test-refactor` with `make ci` green at every phase boundary. The plan is kept below because the *ordering* rationale is still the useful part; each phase carries its as-built state. -**Green-at-boundary is necessary but not sufficient for the destructive phases.** A green run proves the surviving tests pass, not that coverage survived. Phases 2 and 4 therefore follow a **strangler rule**: before an old test file is deleted, its distinct *behaviors* (not its LOC) are inventoried in the phase's migration manifest and mapped to their new home (T0/T1 test or T2 contract), and the mapping is reviewed in the PR. The net-LOC reduction below is an *expectation*, not a target — an agent optimizing for deletion is exactly the failure mode the manifest exists to catch. +**Green-at-boundary was necessary but not sufficient for the destructive phases.** A green run proves the surviving tests pass, not that coverage survived. Phases 2 and 4 followed a **strangler rule**: before an old test file was deleted, its distinct *behaviors* — not its LOC — were inventoried and mapped to their new home, and the mapping was reviewed. This caught real losses. It is also why the LOC figures below are expectations rather than targets: an agent optimising for deletion is exactly the failure mode the inventory exists to catch. -- **Phase 0 — Make the gate real.** *(~200–500 LOC touched, net ~0)* Order matters here (revision 1 had it backwards): **(1)** get `make ci` green as imported (baseline commit `aa57ccc`); **(2)** relocate the public-network test files (`bluesky_post_test.go`, live-target unfurl tests, `TEST_REAL_HANDLES` identity tests) to `tests/live/` with the `live` tag — move-only, no rewrite; **(3)** flip the stack networks to `internal: true` with module-cache pre-population (3.7.3); **(4)** prove a cold-cache egress-blocked run is green and record timing + allowlist as the baseline. *Exit: green hermetic run with no public egress from a cold cache; timings captured.* -- **Phase 1 — testkit + audit script.** *(net +2,500: ~1,500 harness + ~800 harness tests + audit script)* Build `tests/testkit` (db/pds/firehose/wait/fixtures/appview) with the dependency-direction rule; `scripts/test-db-prepare.sh` (template provisioning + advisory-lock fallback + orphan sweep); `scripts/test-audit.sh` wired into CI as warnings with every count assigned to a phase; fix the 5 surviving handle-collision sites. No mass migration yet — testkit ships with its own tests. *Exit: testkit exists and is tested; audit baseline recorded with per-phase burn-down.* -- **Phase 2 — Kill the lies, split the mixed files, tag everything.** *(net −800)* Delete the 6 never-run debt tests. Fix the lexicon validator to not generate defs-only subtests (retiring those 8 allowlist entries). Move the 2 rate-limiter "e2e" files to `internal/api/middleware` as T0; fold `tests/unit/community_service_test.go` toward `internal/core/communities`. **Split multi-tier files by test function** (manifest-tracked) so every file is single-tier, then add build tags in place; retarget the Makefile to tags and delete `-short`. *Exit: tiers mechanically selectable; `make test` honest; every file single-tier; allowlist ≤ ~5 entries.* -- **Phase 3 — DB isolation, then parallelism.** *(net −1,800)* Migrate `setupTestDB`'s 222 call sites to `testkit.DB(t)`; delete all unscoped `DELETE FROM`s and per-file cleanups. Then the global-state pass: audit `t.Setenv`/`os.Setenv`/logger/http-default mutation sites, convert to testkit injection, enable `t.Parallel()` on proven-safe tests, set the connection budgets (3.3), run `-race` clean. Drop `-p 1` last. *Exit: parallel suite green under `-race`; wall-clock vs Phase 0 recorded.* -- **Phase 4 — Pipeline contracts.** *(net −8,000 to −12,000 — multi-session, one domain at a time: post, comment, community, vote, user, blocks, aggregators…)* Per domain, strangler-style: inventory the old files' behaviors → add the missing T0/T1 tests they imply → build the ingestion contract (3.4a) and API contract (3.4b) on testkit → run old + new together → delete the old file after reviewed parity. Build the reliability suite (3.4c) and the contract-manifest CI check. Delete the 10 `subscribeToJetstream*` copies as their callers migrate. `tests/integration/` ends empty and is removed. *Exit: every consumed collection has an ingestion contract (CI-enforced); reliability suite green; `tests/integration` gone.* -- **Phase 5 — Coverage debt + real federation topology.** *(net +7,500, additive)* Unit tests for the zero-coverage core packages (communities, votes, identity, routes, timeline, discover, communityFeeds — in that order); repo tests for the 13 untested repos. Then the topology work revision 1 hand-waved: add a **second PDS** (own hostname, storage, PLC registration) and a hermetic **relay** to the CI stack, and promote the highest-value ingestion contracts (post, comment, vote — cf. the known vote-federation gap) to true federation-path: record written to the *non-fronted* PDS, discovered and indexed purely via the firehose, remote identity resolution and blob fetching asserted explicitly. *Exit: no zero-test package in `internal/core`; federation contracts for the big three running against the two-PDS topology.* -- **Phase 6 — Enforcement flip + docs.** *(net −400)* Audit-script counts must be zero; flip it from warn to fail. This doc moves from PROPOSED to CANONICAL; `TESTING_SUMMARY.md` + `docs/E2E_TESTING.md` deleted; CLAUDE.md testing section points here. +- **Phase 0 — Make the gate real.** Order mattered here (revision 1 had it backwards): **(1)** get `make ci` green as imported; **(2)** relocate the public-network test files to `tests/live/` with the `live` tag, move-only; **(3)** flip the stack networks to `internal: true` with module-cache pre-population (3.7.3); **(4)** prove a cold-cache egress-blocked run green. **DONE** — the gate was green on the first run (3,307 tests, 19 skips against a 21-entry allowlist, 2.2 min warm); cold egress-blocked came in at 2:21. The egress flip found four hidden network dependencies, not the one predicted. +- **Phase 1 — testkit + audit script.** Build `tests/testkit` with the dependency-direction rule; `scripts/test-db-prepare.sh`; `scripts/test-audit.sh` in warn mode with every count assigned to a phase. **DONE** — 117 testkit tests, `-race`/`-shuffle` clean; clone cost measured at ~30 ms, well under the estimate; audit baseline 911. Building the firehose helper is what uncovered the corrupt-after-deadline bug in all ten legacy subscribers (§3.3). +- **Phase 2 — Kill the lies, split the mixed files, tag everything.** Delete the never-run debt tests; stop the lexicon validator generating defs-only subtests; move the two rate-limiter "e2e" files to `internal/api/middleware` as T0; split multi-tier files by test function; tag everything; retarget the Makefile; delete `-short` and `make test-all`. **DONE** — allowlist reached **zero entries**, 161 `Short` guards deleted, and the untagged suite passed under `--network none` on the first attempt, which is the honesty check that `make test` really is hermetic. `tests/unit` turned out to be 100% fake (servers never dialled, literals asserted against themselves) and was deleted wholesale rather than ported. +- **Phase 3 — DB isolation, then parallelism.** Migrate every `setupTestDB` call site to `testkit.DB(t)`; delete the wipes and cleanups; then the global-state pass, `t.Parallel()` on proven-safe tests, connection budgets, `-race` clean. **DONE with one exception**: `-p 1` was *not* dropped. Removing it exposed three concurrency bugs it had been masking — a template-destruction race, the legacy firehose's 5-s-behind-a-30-s-promise, and the Jetstream `account`/`identity` bypass — of which the first two were fixed and the third is a property of the stream, not the suite. `-p 1` stays, with the new reason written next to it. See §6. +- **Phase 4 — Pipeline contracts.** Per domain, strangler-style: inventory the old file's behaviors → add the T0/T1 tests they imply → build the ingestion and API contracts on testkit → run old and new together → delete the old file after reviewed parity. Build the reliability suite and the contract-manifest check. **DONE** — all 10 collections contracted, `tests/integration` removed (38 files: 20 relocated, 18 deleted with citations), the last hand-rolled subscriber deleted. Roughly 14,000 LOC of test code went; coverage went up. Nine production defects were found and filed along the way, several of them the kind only a real pipeline surfaces (a vote arriving before its subject is lost *and* its later delete subtracts a real vote; deleted posts served in full to anonymous callers; `community.block` indexed but never enforced). +- **Phase 5 — Coverage debt + real federation topology.** Unit tests for the seven zero-coverage core packages and the untested repos; then the second PDS, the hermetic relay, and federated ingestion contracts. **DONE** — +885 tests. Coverage: votes 33→96%, identity 59→96%, routes 16→94%, `internal/db/postgres` 40→83%, timeline/discover/communityFeeds to 100%. Twenty-one further defects were found, the sharpest being a `%f` cursor truncation that silently kills hot-comment pagination on months-old threads. Federation landed as described in §3.4a, including the structural limit on federated authors. +- **Phase 6 — Enforcement flip + docs.** Drive the audit to its floor and flip it to a hard failure; promote this document to CANONICAL; delete `TESTING_SUMMARY.md` and `docs/E2E_TESTING.md`; point CLAUDE.md here. **DONE** — this document. -Aggregate expectation: the suite lands around **~78k LOC (from ~84k) with materially more coverage** — duplication currently dwarfs the gaps, so the refactor is net-negative even while filling seven zero-coverage packages. Measured done-criteria (via `test-audit.sh`, not prose): 0 `time.Sleep` in tests · 0 `t.Skip` outside testkit · 0 `testing.Short()` · allowlist ≤ ~5 entries, all `~`-conditional · 0 packages in `internal/core` without tests · `-p 1` gone · every consumed collection contract-covered · `make test` < 60 s · `make ci` green at ≤ Phase-0 wall-clock. +### Done-criteria, measured + +The criteria were deliberately written to be checkable by a tool rather than by prose. Measured at the close: + +| Criterion | State | +|---|---| +| 0 `time.Sleep` in tests | **met, with two declared exemptions** — 0 unmarked across all test code, testkit included (the scan covers it rather than skipping the directory, so a sleep added there counts). The two that remain are the poll interval inside `WaitFor` and `Holds` — a poll loop sleeps — each carrying a `coves:allow-sleep` marker the audit reads and reports in its EXEMPT column. | +| 0 `t.Skip` outside testkit | **met** — 0, and the allowlist mechanism has no second door | +| 0 `testing.Short()` | **met** — 0, scanned across production sources too | +| Allowlist ≤ ~5 entries, all conditional | **beaten** — 0 entries | +| 0 packages in `internal/core` without tests | **met** | +| Every consumed collection contract-covered | **met** — 10/10, CI-enforced, burn-down empty | +| `make test` < 60 s | **beaten** — ~4 s warm, ~10 s cold, no Docker | +| `make ci` green | **met** — and green twice in a row at the close | +| **`-p 1` gone** | **NOT met.** The Jetstream `account`/`identity` bypass makes package-parallel test binaries starve each other's subscribers. The flag is now justified by a measured cause rather than inherited from the shared-database era; removing it needs a change to what the consumers subscribe to, not to the suite. | +| `make ci` at ≤ Phase-0 wall-clock | **NOT met**, and deliberately so. Phase 0 was 2.2 min; the gate is now ~6.5 min. The additions are all tier coverage that did not previously exist — the T2 contracts (~2 min), the reliability suite, and the two-PDS federation topology (~11 s of stack, ~41 s of contracts) — against a T2 budget of 10 minutes. The trade was made knowingly each time and the per-phase cost is recorded above. | + +One aggregate expectation was simply **wrong** and is worth correcting rather than quietly dropping: the plan predicted the suite would shrink to ~78k LOC from ~84k. It is **~106k**. Phase 4 did delete on the predicted scale, but phase 5's coverage work — seven zero-coverage packages, thirteen untested repos, 885 tests — added far more than the +7,500 estimated. Fewer lies, more lines. ## 5. Decisions & rationale (short form) -- **Build tags over directories-as-tiers**: the current tree proves naming conventions don't survive contact with velocity. Tags are compile-time, greppable, and un-forgettable — with the corollary (learned in review) that files must be split to single-tier first, because tags classify files, not tests. -- **T1 colocated with code, not in `tests/`**: repo tests next to repos get maintained when the repo changes; a 41k-LOC `tests/integration` catch-all demonstrably doesn't. `tests/` keeps only what is genuinely cross-cutting: e2e contracts, live tests, testkit, ci config. -- **Template-clone-per-test over shared-DB-with-cleanup**: cleanup-discipline is exactly what failed (331 DELETE statements). Isolation by construction is the only version that survives agents writing tests at speed — provisioned by the harness, not racing `TestMain`s. +- **Build tags over directories-as-tiers**: the pre-refactor tree proved naming conventions don't survive contact with velocity. Tags are compile-time, greppable, and un-forgettable — with the corollary (learned in review) that files must be split to single-tier first, because tags classify files, not tests. +- **T1 colocated with code, not in `tests/`**: repo tests next to repos get maintained when the repo changes; a 41k-LOC `tests/integration` catch-all demonstrably didn't. `tests/` keeps only what is genuinely cross-cutting: e2e contracts, live tests, testkit, ci config. +- **Template-clone-per-test over shared-DB-with-cleanup**: cleanup-discipline is exactly what failed (331 DELETE *executions* per run, across 82 written statements). Isolation by construction is the only version that survives agents writing tests at speed — provisioned by the harness, not racing `TestMain`s. - **Direct-PDS ingestion contracts over client-path-only E2E**: synchronous indexing on the client path means endpoint-in/endpoint-out tests can't prove the pipeline. Bypassing the AppView on the write side makes firehose delivery the only possible explanation for a passing read — un-fakeable by construction. -- **Serial T2 over parallel**: one shared AppView/PDS/DB namespace makes interleaved eventually-consistent writers a flake factory. Ten serial contracts inside a 10-minute budget is the boring, correct call. +- **Serial T2 over parallel**: one shared AppView/PDS/DB namespace makes interleaved eventually-consistent writers a flake factory. Serial contracts inside a 10-minute budget is the boring, correct call; the tier has grown to 32 test functions and still fits. - **Black-box T2 over test-instantiated consumers**: tests the wiring that actually ships, deletes the largest duplication cluster, and makes the E2E tier readable as product documentation. - **Skips-as-failures stays and expands**: `cmd/ci-report` is the best idea already in the tree. The refactor makes the allowlist *shrink* to near-zero rather than institutionalizing it. -- **Generated contract manifest over reviewed inventory**: the hand-written inventory was wrong on day one (missed two collections). Deriving it from `WantedCollections` makes "every collection is pipeline-tested" a build invariant instead of a review habit. +- **Generated contract manifest over reviewed inventory**: the hand-written inventory was wrong on day one (missed two collections). Deriving it from the consumer table makes "every collection is pipeline-tested" a build invariant instead of a review habit. + +## 6. Known limitations (as shipped) + +These are the standing gaps. They are collected here rather than left scattered so that nobody has to rediscover one by writing a test that cannot work. Each is a *structural* limit with a known unlock — none is a task somebody forgot. + +**1. T2 cannot authenticate a write** (§3.4b). `RequireAuth` accepts only a *sealed* session token, and the only thing that mints one is `/oauth/callback` at the end of a browser authorization-code flow against the PDS' own HTML login pages. T2 therefore covers the auth boundary (every write NSID answers 401 to a session-less client) and the read surface; authenticated write *behaviour* is proven at T1 with handler tests plus write-forward tests against a real PDS. **Unlock:** a hard-gated, test-only session-minting path in the AppView. Everything in item 2 falls out of it. + +**2. Viewer-scoped behaviour is unreachable from T2.** Subscription fan-out (`getTimeline`), personalised feeds, and block enforcement are all `RequireAuth` or viewer-scoped, so the block contracts observe consumer-health measurement windows rather than a serving endpoint, and fan-out is asserted only at T1. Direct consequence of item 1. + +**3. Federated authors cannot be indexed** (§3.4a). The users consumer default-denies identities from untrusted hosts, so a record authored on the second PDS indexes fine (comments, votes, blobs have no author foreign key) but its *author* never gets a row. Enabling the trust config was tried and rejected: it walks into the `handle.invalid` UNIQUE-squat defect and leaves the stack non-re-runnable. **Unlock:** fix the identity-cache defect that stores the `handle.invalid` sentinel — it is the upstream cause of two filed production bugs as well. + +**4. `hostedBy` DID verification is off in CI** (`SKIP_DID_WEB_VERIFICATION`). Community contracts therefore prove field *transport*, not verification, and say so in their doc comments. **Unlock:** an injectable HTTP client on the community service so verification can be exercised against a stub. + +**5. `-p 1` is still required** (§3.3, §4). Jetstream `account` and `identity` events bypass the consumers' wanted-collection filter, so parallel package binaries starve each other's subscribers. **Unlock:** change what the consumers subscribe to; the suite side is already ready (`packageParallelism` in `tests/testkit/db.go` is a single constant, and the per-package budget is computed). + +**6. `tests/fixtures`' import rule is a convention, not a compiler constraint.** The dependency-direction rule in §3.3 *is* compiler-enforced for `testkit` — a violation is an import cycle. The same discipline in `tests/fixtures` is only a convention, and nothing fails if someone breaks it. + +**7. The audit's greps are tripwires, not proofs** (§3.6.3). A URL built by concatenation, an IP literal, a sleep behind a helper: all invisible. The guarantee against public-network access is the egress-blocked network, not the grep. Treat a green audit as "no obvious drift", never as "no violation". + +**8. One flake vector is known and documented rather than fixed.** A Jetstream reconnect within the 5-second cursor rewind can double-count a *permanent* dead letter, because the row dedupes on conflict while the counter increments unconditionally. It is called out in `requireRejected` where an assertion could trip over it. + +**9. Cross-feed ordering of identity events is unenforced, and the tests pin that rather than assert it.** Jetstream `identity` events are not repo commits and carry no `rev`, so the per-record rev gate that orders commits across overlapping feeds cannot see them: `handleIdentityEvent` (`internal/atproto/jetstream/user_consumer.go`) applies whatever arrives, whenever it arrives, and under the multi-feed topology a lagging feed's stale event can transiently revert a handle until the next identity event for that DID lands. This is the one limitation on this list that a test actively holds in place: `internal/core/users/user_identity_consumer_test.go`'s "Out-of-order identity events are last-write-wins, not seq-ordered" asserts the wrong-but-current handle and its failure message says the fix has landed — §3.4's rule 7 applied to a production defect rather than a contract. **Unlock:** re-resolve the DID against PLC, the source of truth, on each identity event; or give identity events an ordering key the rev gate can read. Note the interaction with item 5 — the same unfiltered `identity`/`account` stream is what keeps `-p 1`. diff --git a/internal/api/handlers/community/block_test.go b/internal/api/handlers/community/block_test.go index ff58c45..8854313 100644 --- a/internal/api/handlers/community/block_test.go +++ b/internal/api/handlers/community/block_test.go @@ -119,7 +119,7 @@ func createBlockTestOAuthSession(did string) *oauth.ClientSessionData { return &oauth.ClientSessionData{ AccountDID: parsedDID, SessionID: "test-session", - HostURL: "http://localhost:3001", + HostURL: testSessionPDSHostURL, AccessToken: "test-access-token", } } diff --git a/internal/api/handlers/community/list_test.go b/internal/api/handlers/community/list_test.go index 7776ca9..3df40f2 100644 --- a/internal/api/handlers/community/list_test.go +++ b/internal/api/handlers/community/list_test.go @@ -210,7 +210,7 @@ func createListTestOAuthSession(did string) *oauth.ClientSessionData { return &oauth.ClientSessionData{ AccountDID: parsedDID, SessionID: "test-session", - HostURL: "http://localhost:3001", + HostURL: testSessionPDSHostURL, AccessToken: "test-access-token", } } diff --git a/internal/api/handlers/community/subscribe_test.go b/internal/api/handlers/community/subscribe_test.go index 5d2489c..1a35da2 100644 --- a/internal/api/handlers/community/subscribe_test.go +++ b/internal/api/handlers/community/subscribe_test.go @@ -16,13 +16,25 @@ import ( "github.com/bluesky-social/indigo/atproto/syntax" ) +// testSessionPDSHostURL is the PDS the mock OAuth sessions in this package +// claim to come from. +// +// Every handler test here drives a mock community service, so the session is +// carried as far as the authorisation check and no further: nothing in this +// package resolves the host, opens a connection to it, or asserts on it. It is +// a plausible-looking field value, not an endpoint, which is why it is a +// literal instead of testkit.Endpoints() — these tests need no stack at all, +// and taking a dependency on one to fill in an unused field would be a lie +// about what they require. +const testSessionPDSHostURL = "http://localhost:3001" // coves:allow-host-literal: inert HostURL field on a mock OAuth session; no handler test in this package dials it + // createTestOAuthSession creates a mock OAuth session for testing func createTestOAuthSession(did string) *oauth.ClientSessionData { parsedDID, _ := syntax.ParseDID(did) return &oauth.ClientSessionData{ AccountDID: parsedDID, SessionID: "test-session", - HostURL: "http://localhost:3001", + HostURL: testSessionPDSHostURL, AccessToken: "test-access-token", } } diff --git a/internal/api/middleware/ratelimit.go b/internal/api/middleware/ratelimit.go index f8bcbb2..88df96a 100644 --- a/internal/api/middleware/ratelimit.go +++ b/internal/api/middleware/ratelimit.go @@ -12,8 +12,13 @@ import ( // RateLimiter is a simple in-memory token-bucket-style limiter keyed by client IP. // For multi-replica deploys, swap in a Redis-backed implementation. type RateLimiter struct { - clients map[string]*clientLimit - stop chan struct{} + clients map[string]*clientLimit + stop chan struct{} + // now reads the wall clock. Production always gets time.Now; the seam + // exists because a rate-limit window is a deadline, and a test that waits + // out a real one is slow at best and flaky at worst. See + // newRateLimiterWithClock. + now func() time.Time requests int window time.Duration name string // used in security-event logs to distinguish global vs per-route @@ -35,9 +40,22 @@ func NewRateLimiter(requests int, window time.Duration) *RateLimiter { // NewNamedRateLimiter creates a rate limiter that tags its 429 logs with name. // Pass something route-distinct like "signupToken" or "global". func NewNamedRateLimiter(name string, requests int, window time.Duration) *RateLimiter { + return newRateLimiterWithClock(name, requests, window, func() time.Time { + return time.Now().UTC() + }) +} + +// newRateLimiterWithClock is NewNamedRateLimiter with the clock supplied. +// Test-only: the exported constructors are the production entry points and +// always pass time.Now. +// +// now must be safe for concurrent use — allow() calls it on the request path +// and the cleanup goroutine calls it on its own schedule. +func newRateLimiterWithClock(name string, requests int, window time.Duration, now func() time.Time) *RateLimiter { rl := &RateLimiter{ clients: make(map[string]*clientLimit), stop: make(chan struct{}), + now: now, requests: requests, window: window, name: name, @@ -84,7 +102,7 @@ func (rl *RateLimiter) allow(clientID string) bool { rl.mu.Lock() defer rl.mu.Unlock() - now := time.Now().UTC() + now := rl.now() client, exists := rl.clients[clientID] if !exists { @@ -128,7 +146,7 @@ func (rl *RateLimiter) cleanup() { return case <-ticker.C: rl.mu.Lock() - now := time.Now().UTC() + now := rl.now() for clientID, client := range rl.clients { if now.After(client.resetTime) { delete(rl.clients, clientID) diff --git a/internal/api/middleware/ratelimit_http_test.go b/internal/api/middleware/ratelimit_http_test.go index c26c41b..789de93 100644 --- a/internal/api/middleware/ratelimit_http_test.go +++ b/internal/api/middleware/ratelimit_http_test.go @@ -28,6 +28,16 @@ func newHTTPTestLimiter(t *testing.T, requests int, window time.Duration) *RateL return rl } +// newHTTPTestLimiterWithClock is newHTTPTestLimiter for the window-expiry +// tests, which drive time themselves rather than waiting for it. testClock is +// defined in ratelimit_test.go. +func newHTTPTestLimiterWithClock(t *testing.T, requests int, window time.Duration, clock *testClock) *RateLimiter { + t.Helper() + rl := newRateLimiterWithClock("default", requests, window, clock.Now) + t.Cleanup(rl.Stop) + return rl +} + // okHandler is the terminal handler for these tests: it proves the request // reached past the limiter. func okHandler() http.Handler { @@ -255,11 +265,13 @@ func TestRateLimiter_HTTP_NoRateLimitHeaders(t *testing.T) { } // TestRateLimiter_HTTP_WindowReset covers budget recovery once a window -// expires. The limiter reads time.Now directly, so these waits are real; they -// are kept to a few hundred milliseconds. +// expires. The limiter's clock is injected here, so the window boundary is +// crossed by advancing time rather than by waiting for it: no wall clock is +// spent and there is no margin to tune. func TestRateLimiter_HTTP_WindowReset(t *testing.T) { t.Run("budget returns after the window expires", func(t *testing.T) { - handler := newHTTPTestLimiter(t, 2, 100*time.Millisecond).Middleware(okHandler()) + clock := newTestClock() + handler := newHTTPTestLimiterWithClock(t, 2, 1*time.Minute, clock).Middleware(okHandler()) clientIP := "192.168.1.130:12345" for i := 0; i < 2; i++ { @@ -276,7 +288,7 @@ func TestRateLimiter_HTTP_WindowReset(t *testing.T) { handler.ServeHTTP(rr, req) assert.Equal(t, http.StatusTooManyRequests, rr.Code) - time.Sleep(150 * time.Millisecond) + clock.Advance(1*time.Minute + time.Second) req = httptest.NewRequest("GET", "/test", nil) req.RemoteAddr = clientIP @@ -290,7 +302,8 @@ func TestRateLimiter_HTTP_WindowReset(t *testing.T) { // is set when the bucket opens and is never pushed back by later // requests. Three requests spread over 100ms of a 200ms window still // exhaust the budget, and the budget returns on the original schedule. - handler := newHTTPTestLimiter(t, 3, 200*time.Millisecond).Middleware(okHandler()) + clock := newTestClock() + handler := newHTTPTestLimiterWithClock(t, 3, 200*time.Millisecond, clock).Middleware(okHandler()) clientIP := "192.168.1.131:12345" for i := 0; i < 3; i++ { @@ -299,7 +312,7 @@ func TestRateLimiter_HTTP_WindowReset(t *testing.T) { rr := httptest.NewRecorder() handler.ServeHTTP(rr, req) assert.Equal(t, http.StatusOK, rr.Code, "Request %d should succeed", i+1) - time.Sleep(50 * time.Millisecond) + clock.Advance(50 * time.Millisecond) } req := httptest.NewRequest("GET", "/test", nil) @@ -308,7 +321,11 @@ func TestRateLimiter_HTTP_WindowReset(t *testing.T) { handler.ServeHTTP(rr, req) assert.Equal(t, http.StatusTooManyRequests, rr.Code, "4th request should be blocked") - time.Sleep(100 * time.Millisecond) + // 150ms into the window when the 4th request was rejected; another + // 100ms puts the clock at 250ms, past the 200ms reset the FIRST request + // set. If later requests had pushed the reset back, this would still be + // inside the window and still be a 429. + clock.Advance(100 * time.Millisecond) req = httptest.NewRequest("GET", "/test", nil) req.RemoteAddr = clientIP diff --git a/internal/api/middleware/ratelimit_test.go b/internal/api/middleware/ratelimit_test.go index f491eca..47507b4 100644 --- a/internal/api/middleware/ratelimit_test.go +++ b/internal/api/middleware/ratelimit_test.go @@ -1,6 +1,7 @@ package middleware import ( + "fmt" "net/http" "net/http/httptest" "sync" @@ -10,12 +11,37 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "Coves/tests/testkit" ) // allow() is the load-bearing primitive — every per-route rate limit and the // global limit go through it. These tests pin the contract directly so we don't // rely on flaky end-to-end timing. +// testClock is a hand-advanced clock for the limiter's injected now(). Window +// expiry is a deadline the limiter itself computed, so a test can cross it by +// moving the clock instead of waiting for it — deterministic and free. +// +// Concurrency-safe by construction: the limiter's cleanup goroutine reads the +// clock on its own schedule while the test writes it, so the timestamp lives in +// an atomic rather than a plain field. +type testClock struct { + nanos atomic.Int64 +} + +func newTestClock() *testClock { + c := &testClock{} + // An arbitrary fixed instant. Nothing depends on its value, only on the + // deltas the test applies, but a fixed start keeps failures reproducible. + c.nanos.Store(time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC).UnixNano()) + return c +} + +func (c *testClock) Now() time.Time { return time.Unix(0, c.nanos.Load()).UTC() } + +func (c *testClock) Advance(d time.Duration) { c.nanos.Add(int64(d)) } + func TestRateLimiter_Allow_UnderLimit(t *testing.T) { rl := NewNamedRateLimiter("test", 3, time.Minute) t.Cleanup(rl.Stop) @@ -51,14 +77,17 @@ func TestRateLimiter_Allow_PerClientIsolation(t *testing.T) { } func TestRateLimiter_Allow_WindowResets(t *testing.T) { - // Tiny window so the test runs fast. - rl := NewNamedRateLimiter("test", 1, 50*time.Millisecond) + // The clock is injected, so the window boundary is crossed by moving time + // rather than by outlasting it. That also lets the window be the realistic + // minute production configures instead of an artificially tiny one. + clock := newTestClock() + rl := newRateLimiterWithClock("test", 1, time.Minute, clock.Now) t.Cleanup(rl.Stop) require.True(t, rl.allow("ip-a")) require.False(t, rl.allow("ip-a")) - time.Sleep(80 * time.Millisecond) + clock.Advance(time.Minute + time.Second) assert.True(t, rl.allow("ip-a"), "window expired — budget should reset") } @@ -175,17 +204,26 @@ func TestRateLimiter_TreatsClientWithVaryingProxyChainAsSameBucket(t *testing.T) } func TestRateLimiter_Cleanup_RemovesExpiredEntries(t *testing.T) { - // Tiny window: cleanup ticker also fires every `window`, so sleeping past - // 2× the window guarantees at least one tick has cleared expired entries. - rl := NewNamedRateLimiter("test", 1, 50*time.Millisecond) + // Eviction is work a background goroutine does, so this waits for the + // eviction itself rather than for a duration guessed to contain it. The + // clock makes the entry expired the instant it is advanced, so the very + // next cleanup tick must remove it; the window doubles as the ticker + // interval, so a small one keeps that tick close. + clock := newTestClock() + rl := newRateLimiterWithClock("test", 1, 20*time.Millisecond, clock.Now) t.Cleanup(rl.Stop) require.True(t, rl.allow("ip-a")) require.Equal(t, 1, rl.clientCount(), "entry should be tracked immediately") - time.Sleep(150 * time.Millisecond) + clock.Advance(time.Minute) - assert.Equal(t, 0, rl.clientCount(), "cleanup goroutine must evict expired entries") + testkit.WaitFor(t, 5*time.Second, func() (bool, error) { + return rl.clientCount() == 0, nil + }, testkit.WithDescription("the cleanup goroutine to evict the expired bucket"), + testkit.WithDiagnostics(func() string { + return fmt.Sprintf("tracked clients: %d", rl.clientCount()) + })) } func TestRateLimiter_AllowIsConcurrencySafe(t *testing.T) { diff --git a/internal/atproto/jetstream/connector_test.go b/internal/atproto/jetstream/connector_test.go index 966b2b8..027a388 100644 --- a/internal/atproto/jetstream/connector_test.go +++ b/internal/atproto/jetstream/connector_test.go @@ -15,6 +15,8 @@ import ( "time" "github.com/gorilla/websocket" + + "Coves/tests/testkit" ) // --- Test doubles --- @@ -340,16 +342,15 @@ func testEventJSON(t *testing.T, timeUS int64) []byte { return data } +// waitFor adapts the harness primitive to the (description, condition) shape +// these twenty call sites use. The poll interval is tightened from the harness +// default because this package's fixtures are deliberately fast — a 20ms +// reconnect delay is not worth observing at 100ms granularity. func waitFor(t *testing.T, timeout time.Duration, description string, condition func() bool) { t.Helper() - deadline := time.Now().Add(timeout) - for time.Now().Before(deadline) { - if condition() { - return - } - time.Sleep(5 * time.Millisecond) - } - t.Fatalf("timed out waiting for: %s", description) + testkit.WaitFor(t, timeout, func() (bool, error) { return condition(), nil }, + testkit.WithDescription("%s", description), + testkit.WithPollInterval(5*time.Millisecond)) } // fastConnectorOptions keeps test runtimes low. diff --git a/internal/atproto/jetstream/feeds_test.go b/internal/atproto/jetstream/feeds_test.go index 822a0ea..78a10d3 100644 --- a/internal/atproto/jetstream/feeds_test.go +++ b/internal/atproto/jetstream/feeds_test.go @@ -1,5 +1,7 @@ package jetstream +// coves:allow-public-host-file: this file tests the JETSTREAM_FEEDS spec parser, and the spec production actually runs names the public Bluesky Jetstream — asserting on any other string would test a topology we do not deploy. ParseFeeds and SubscribeURL are pure string functions: nothing here opens a socket. + import ( "testing" @@ -16,10 +18,10 @@ func TestParseFeeds_TwoFeeds_OrderPreserved(t *testing.T) { } func TestParseFeeds_SingleFeedWithWhitespaceAndTrailingSemicolon(t *testing.T) { - feeds, err := ParseFeeds(" self = ws://localhost:6008 ; ") + feeds, err := ParseFeeds(" self = ws://localhost:6008 ; ") // coves:allow-host-literal: spec text under test; the padding around the URL is the point, so it stays inline. require.NoError(t, err) require.Len(t, feeds, 1) - assert.Equal(t, Feed{Key: "self", BaseURL: "ws://localhost:6008"}, feeds[0]) + assert.Equal(t, Feed{Key: "self", BaseURL: "ws://localhost:6008"}, feeds[0]) // coves:allow-host-literal: the trimmed form the parser must produce from the padded input above. } func TestParseFeeds_Rejections(t *testing.T) { diff --git a/internal/atproto/jetstream/user_consumer_test.go b/internal/atproto/jetstream/user_consumer_test.go index b2065fe..e008d44 100644 --- a/internal/atproto/jetstream/user_consumer_test.go +++ b/internal/atproto/jetstream/user_consumer_test.go @@ -12,6 +12,15 @@ import ( "github.com/stretchr/testify/require" ) +// bskySocialPDS is the pds_url carried by the already-indexed users these +// tests hand to the consumer. It is a RECORD FIELD, not an endpoint: the +// consumer compares it (bridge trust) and re-indexes it, and the only two +// collaborators here are a mock user service and a mock resolver, so nothing +// in this file can reach it. A user whose PDS is Bluesky is the realistic +// shape for a profile arriving over the firehose, which is why it is not +// invented. +const bskySocialPDS = "https://bsky.social" // coves:allow-public-host: fixture pds_url on mock user records; no HTTP client exists in this test. + // mockUserService is a test double for users.UserService type mockUserService struct { users map[string]*users.User @@ -234,7 +243,7 @@ func TestUserConsumer_HandleProfileCommit(t *testing.T) { mockService.users["did:plc:testuser"] = &users.User{ DID: "did:plc:testuser", Handle: "testuser.bsky.social", - PDSURL: "https://bsky.social", + PDSURL: bskySocialPDS, } mockResolver := &mockIdentityResolverForUser{} consumer := NewUserEventConsumer(mockService, mockResolver) @@ -276,7 +285,7 @@ func TestUserConsumer_HandleProfileCommit(t *testing.T) { mockService.users["did:plc:testuser"] = &users.User{ DID: "did:plc:testuser", Handle: "testuser.bsky.social", - PDSURL: "https://bsky.social", + PDSURL: bskySocialPDS, } mockResolver := &mockIdentityResolverForUser{} consumer := NewUserEventConsumer(mockService, mockResolver) @@ -318,7 +327,7 @@ func TestUserConsumer_HandleProfileCommit(t *testing.T) { mockService.users["did:plc:testuser"] = &users.User{ DID: "did:plc:testuser", Handle: "testuser.bsky.social", - PDSURL: "https://bsky.social", + PDSURL: bskySocialPDS, } mockResolver := &mockIdentityResolverForUser{} consumer := NewUserEventConsumer(mockService, mockResolver) @@ -365,7 +374,7 @@ func TestUserConsumer_HandleProfileCommit(t *testing.T) { mockService.users["did:plc:testuser"] = &users.User{ DID: "did:plc:testuser", Handle: "testuser.bsky.social", - PDSURL: "https://bsky.social", + PDSURL: bskySocialPDS, } mockResolver := &mockIdentityResolverForUser{} consumer := NewUserEventConsumer(mockService, mockResolver) @@ -412,7 +421,7 @@ func TestUserConsumer_HandleProfileCommit(t *testing.T) { mockService.users["did:plc:testuser"] = &users.User{ DID: "did:plc:testuser", Handle: "testuser.bsky.social", - PDSURL: "https://bsky.social", + PDSURL: bskySocialPDS, } mockResolver := &mockIdentityResolverForUser{} consumer := NewUserEventConsumer(mockService, mockResolver) @@ -476,7 +485,7 @@ func TestUserConsumer_HandleProfileCommit(t *testing.T) { mockService.users["did:plc:testuser"] = &users.User{ DID: "did:plc:testuser", Handle: "testuser.bsky.social", - PDSURL: "https://bsky.social", + PDSURL: bskySocialPDS, DisplayName: "Existing Name", Bio: "Existing Bio", AvatarCID: "existingavatar", @@ -528,7 +537,7 @@ func TestUserConsumer_HandleProfileCommit(t *testing.T) { mockService.users["did:plc:testuser"] = &users.User{ DID: "did:plc:testuser", Handle: "testuser.bsky.social", - PDSURL: "https://bsky.social", + PDSURL: bskySocialPDS, DisplayName: "Old Name", } mockResolver := &mockIdentityResolverForUser{} @@ -627,7 +636,7 @@ func TestUserConsumer_HandleProfileCommit(t *testing.T) { mockService.users["did:plc:testuser"] = &users.User{ DID: "did:plc:testuser", Handle: "testuser.bsky.social", - PDSURL: "https://bsky.social", + PDSURL: bskySocialPDS, } mockResolver := &mockIdentityResolverForUser{} consumer := NewUserEventConsumer(mockService, mockResolver) @@ -658,7 +667,7 @@ func TestUserConsumer_HandleProfileCommit(t *testing.T) { mockService.users["did:plc:testuser"] = &users.User{ DID: "did:plc:testuser", Handle: "testuser.bsky.social", - PDSURL: "https://bsky.social", + PDSURL: bskySocialPDS, } mockResolver := &mockIdentityResolverForUser{} consumer := NewUserEventConsumer(mockService, mockResolver) @@ -714,7 +723,7 @@ func TestUserConsumer_PropagatesUpdateProfileError(t *testing.T) { mockService.users["did:plc:testuser"] = &users.User{ DID: "did:plc:testuser", Handle: "testuser.bsky.social", - PDSURL: "https://bsky.social", + PDSURL: bskySocialPDS, } mockService.updateError = errors.New("database write error") mockResolver := &mockIdentityResolverForUser{} diff --git a/internal/atproto/lexicon/social/coves/richtext/facet_test.go b/internal/atproto/lexicon/social/coves/richtext/facet_test.go index b88f13b..b7399c1 100644 --- a/internal/atproto/lexicon/social/coves/richtext/facet_test.go +++ b/internal/atproto/lexicon/social/coves/richtext/facet_test.go @@ -166,104 +166,6 @@ func TestUTF8ByteCounting(t *testing.T) { } } -// TestOverlappingFacets tests validation of overlapping facet ranges -func TestOverlappingFacets(t *testing.T) { - tests := []struct { - name string - description string - facets []map[string]interface{} - expectError bool - }{ - { - name: "non-overlapping facets", - facets: []map[string]interface{}{ - { - "index": map[string]int{ - "byteStart": 0, - "byteEnd": 5, - }, - }, - { - "index": map[string]int{ - "byteStart": 10, - "byteEnd": 15, - }, - }, - }, - expectError: false, - description: "Facets with non-overlapping ranges should be valid", - }, - { - name: "exact same range", - facets: []map[string]interface{}{ - { - "index": map[string]int{ - "byteStart": 5, - "byteEnd": 10, - }, - }, - { - "index": map[string]int{ - "byteStart": 5, - "byteEnd": 10, - }, - }, - }, - expectError: false, - description: "Multiple facets on the same range are allowed (e.g., bold + italic)", - }, - { - name: "nested ranges", - facets: []map[string]interface{}{ - { - "index": map[string]int{ - "byteStart": 0, - "byteEnd": 20, - }, - }, - { - "index": map[string]int{ - "byteStart": 5, - "byteEnd": 15, - }, - }, - }, - expectError: false, - description: "Nested facet ranges are allowed", - }, - { - name: "partial overlap", - facets: []map[string]interface{}{ - { - "index": map[string]int{ - "byteStart": 0, - "byteEnd": 10, - }, - }, - { - "index": map[string]int{ - "byteStart": 5, - "byteEnd": 15, - }, - }, - }, - expectError: false, - description: "Partially overlapping facets are allowed", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // For now, we're not implementing overlap validation - // as it's allowed in AT Protocol - // This test documents the expected behavior - if tt.expectError { - t.Skip("Overlap validation not implemented - all overlaps are currently allowed") - } - }) - } -} - // TestFacetFeatureTypes tests all supported facet feature types func TestFacetFeatureTypes(t *testing.T) { featureTypes := []struct { diff --git a/internal/atproto/oauth/handlers_security_test.go b/internal/atproto/oauth/handlers_security_test.go index 827f57f..00049ff 100644 --- a/internal/atproto/oauth/handlers_security_test.go +++ b/internal/atproto/oauth/handlers_security_test.go @@ -10,6 +10,21 @@ import ( "github.com/stretchr/testify/require" ) +// Loopback redirect URIs the mobile allowlist must REJECT. +// +// These are fixture inputs to a pure string check (isAllowedRedirectURI / +// BuildAllowedRedirectURIs) and to the assertions about its answers. Nothing +// here is ever dialled, and none of them is a test-stack address on purpose: +// :5173 is Vite's dev server and :3000 a generic dev front end, which is +// exactly what a developer would reach for and exactly what the allowlist +// exists to refuse. Naming them is the test; reading them from testkit would +// make the assertion tautological. +const ( + viteDevRedirectURI = "http://localhost:5173/callback" // coves:allow-host-literal: rejected-URI fixture for the mobile redirect allowlist, never dialled + loopbackHostRedirectURI = "http://localhost:3000/callback" // coves:allow-host-literal: rejected-URI fixture for the mobile redirect allowlist, never dialled + loopbackIPRedirectURI = "http://127.0.0.1:3000/callback" // coves:allow-host-literal: rejected-URI fixture for the mobile redirect allowlist, never dialled +) + // TestExtractScheme tests the scheme extraction function func TestExtractScheme(t *testing.T) { tests := []struct { @@ -352,7 +367,7 @@ func TestBuildAllowedRedirectURIs(t *testing.T) { // Should reject URIs not in the list assert.False(t, allowed["http://evil.com/callback"], "should reject evil.com") - assert.False(t, allowed["http://localhost:5173/callback"], "should reject localhost") + assert.False(t, allowed[viteDevRedirectURI], "should reject localhost") assert.False(t, allowed["evil://steal"], "should reject evil scheme") }) @@ -417,8 +432,8 @@ func TestOAuthHandler_isAllowedRedirectURI(t *testing.T) { // These URIs should be rejected rejectedURIs := []string{ - "http://localhost:5173/callback", // Localhost (use Vite proxy instead) - "http://localhost:3000/callback", // Localhost + viteDevRedirectURI, // Localhost (use Vite proxy instead) + loopbackHostRedirectURI, // Localhost "http://evil.com/callback", // Evil domain "https://example.com/oauth", // Random HTTPS "https://coves.social/wrong/path", // Right domain, wrong path @@ -458,7 +473,7 @@ func TestHandleMobileLogin_MobileURIs(t *testing.T) { handler := createTestOAuthHandler(t) req := httptest.NewRequest(http.MethodGet, - "/oauth/mobile/login?handle=test.user&redirect_uri=http://localhost:5173/callback", nil) + "/oauth/mobile/login?handle=test.user&redirect_uri="+viteDevRedirectURI, nil) rec := httptest.NewRecorder() handler.HandleMobileLogin(rec, req) @@ -497,9 +512,9 @@ func TestMobileURIs_OnlyMobileAllowed(t *testing.T) { "mobile Universal Link should work") // Localhost URIs should NOT work (use Vite proxy for dev) - assert.False(t, handler.isAllowedRedirectURI("http://localhost:5173/callback"), + assert.False(t, handler.isAllowedRedirectURI(viteDevRedirectURI), "localhost should be rejected") - assert.False(t, handler.isAllowedRedirectURI("http://127.0.0.1:3000/callback"), + assert.False(t, handler.isAllowedRedirectURI(loopbackIPRedirectURI), "127.0.0.1 should be rejected") }) } diff --git a/internal/atproto/oauth/handlers_test.go b/internal/atproto/oauth/handlers_test.go index cbab9de..9d7abc7 100644 --- a/internal/atproto/oauth/handlers_test.go +++ b/internal/atproto/oauth/handlers_test.go @@ -440,7 +440,11 @@ func TestOAuthEndpointsNoConflict(t *testing.T) { // TestConfidentialClientWithDevMode verifies confidential client works in dev mode func TestConfidentialClientWithDevMode(t *testing.T) { config := &OAuthConfig{ - PublicURL: "http://127.0.0.1:8081", + // The loopback address a dev-mode client derives its client_id and + // callback URL from. This test asserts on the SHAPE of what the client + // builds out of it (confidential, private_key_jwt, "http://" client_id) + // and never dials it, so the address is the fixture, not an endpoint. + PublicURL: "http://127.0.0.1:8081", // coves:allow-host-literal: dev-mode loopback PublicURL fixture; the client_id is asserted on, the address is never dialled Scopes: []string{"atproto"}, PLCURL: testPLCURL, DevMode: true, // Dev mode enabled diff --git a/internal/atproto/oauth/oauth_helpers_test.go b/internal/atproto/oauth/oauth_helpers_test.go index 9921538..c7aad15 100644 --- a/internal/atproto/oauth/oauth_helpers_test.go +++ b/internal/atproto/oauth/oauth_helpers_test.go @@ -8,16 +8,31 @@ import ( "crypto/rand" "database/sql" "encoding/base64" - "strings" "testing" oauthlib "github.com/bluesky-social/indigo/atproto/auth/oauth" "github.com/stretchr/testify/require" ) -// SetupOAuthTestClient creates an OAuth client configured for testing with a PDS -// When PDS_URL starts with https://, production mode is used (DevMode=false) -// Otherwise, dev mode is used for localhost testing +// testPDSURL is the PDS this package's OAuth sessions belong to: the address a +// ClientSessionData records as its HostURL and the issuer a callback carries. +// +// It comes from testkit rather than a literal so a relocated stack moves the +// tests with it (docs/TEST_ARCHITECTURE.md §3.7, layer 1). +func testPDSURL() string { + return testkit.Endpoints().PDS.BaseURL +} + +// SetupOAuthTestClient creates an OAuth client configured for testing against +// the test stack's PDS. +// +// The client is ALWAYS in dev mode. There used to be a second branch here that +// switched to production mode and the public plc.directory whenever PDS_URL +// began with "https://", and no run could ever select it: the stack's PDS is a +// local HTTP service in both the dev and the hermetic CI compose files, and +// CI's network is egress-blocked (docs/TEST_ARCHITECTURE.md §3.7, layer 3), so +// a run that somehow did select it would fail at connect time rather than +// "resolve DIDs read-only" as its comment claimed. It is deleted, not exempted. func SetupOAuthTestClient(t *testing.T, store oauthlib.ClientAuthStore) *oauth.OAuthClient { t.Helper() @@ -26,37 +41,19 @@ func SetupOAuthTestClient(t *testing.T, store oauthlib.ClientAuthStore) *oauth.O _, err := rand.Read(sealSecret) require.NoError(t, err, "Failed to generate seal secret") - sealSecretB64 := base64.StdEncoding.EncodeToString(sealSecret) - - // Detect if we're testing against a production (HTTPS) PDS - pdsURL := testkit.Endpoints().PDS.BaseURL - isProductionPDS := strings.HasPrefix(pdsURL, "https://") + endpoints := testkit.Endpoints() - // Configure based on PDS type - var config *oauth.OAuthConfig - if isProductionPDS { - // Production mode: HTTPS PDS, use real PLC directory - config = &oauth.OAuthConfig{ - PublicURL: "http://localhost:3000", // Test server callback URL - SealSecret: sealSecretB64, // For sealing mobile tokens - Scopes: []string{"atproto"}, - DevMode: false, // Production mode for HTTPS PDS - AllowPrivateIPs: false, // No private IPs in production mode - PLCURL: "https://plc.directory", // READ-ONLY: resolving DIDs that already exist on the production directory - } - t.Logf("🌐 OAuth client configured for production PDS: %s", pdsURL) - } else { - // Dev mode: localhost PDS with HTTP - config = &oauth.OAuthConfig{ - PublicURL: "http://localhost:3000", // Match the callback URL expected by PDS - SealSecret: sealSecretB64, // For sealing mobile tokens - Scopes: []string{"atproto"}, - DevMode: true, // Enable dev mode for localhost testing - AllowPrivateIPs: true, // Allow private IPs for local testing - PLCURL: testkit.Endpoints().PLC.BaseURL, // Use local PLC directory for DID resolution - } - t.Logf("🔧 OAuth client configured for local PDS: %s", pdsURL) + config := &oauth.OAuthConfig{ + // PublicURL is the OAuth client's OWN public address — Coves, not the + // PDS — which is what the callback URL and client_id are built from. + PublicURL: endpoints.AppView.BaseURL, + SealSecret: base64.StdEncoding.EncodeToString(sealSecret), // For sealing mobile tokens + Scopes: []string{"atproto"}, + DevMode: true, // Loopback client: the stack's PDS speaks HTTP + AllowPrivateIPs: true, // Allow private IPs for local testing + PLCURL: endpoints.PLC.BaseURL, // Local PLC directory for DID resolution } + t.Logf("🔧 OAuth client configured for local PDS: %s", endpoints.PDS.BaseURL) client, err := oauth.NewOAuthClient(config, store) require.NoError(t, err, "Failed to create OAuth client") diff --git a/internal/atproto/oauth/oauth_integration_test.go b/internal/atproto/oauth/oauth_integration_test.go index 63ceafa..deff136 100644 --- a/internal/atproto/oauth/oauth_integration_test.go +++ b/internal/atproto/oauth/oauth_integration_test.go @@ -81,7 +81,7 @@ func testOAuthComponentsWithMockedSession(t *testing.T, ctx context.Context, _ i testSession := oauthlib.ClientSessionData{ AccountDID: parsedDID, SessionID: fmt.Sprintf("localhost-test-%d", time.Now().UnixNano()), - HostURL: "http://localhost:3001", + HostURL: testPDSURL(), AccessToken: "mocked-access-token", Scopes: []string{"atproto"}, } @@ -155,7 +155,7 @@ func TestOAuthE2E_TokenExpiration(t *testing.T) { testSession := oauthlib.ClientSessionData{ AccountDID: did, SessionID: "expired-session", - HostURL: "http://localhost:3001", + HostURL: testPDSURL(), AccessToken: "expired-token", Scopes: []string{"atproto"}, } @@ -298,21 +298,21 @@ func TestOAuthE2E_MultipleSessionsPerUser(t *testing.T) { { AccountDID: did, SessionID: "session-1-web", - HostURL: "http://localhost:3001", + HostURL: testPDSURL(), AccessToken: "token-1", Scopes: []string{"atproto"}, }, { AccountDID: did, SessionID: "session-2-mobile", - HostURL: "http://localhost:3001", + HostURL: testPDSURL(), AccessToken: "token-2", Scopes: []string{"atproto"}, }, { AccountDID: did, SessionID: "session-3-tablet", - HostURL: "http://localhost:3001", + HostURL: testPDSURL(), AccessToken: "token-3", Scopes: []string{"atproto"}, }, @@ -380,10 +380,10 @@ func TestOAuthE2E_AuthRequestStorage(t *testing.T) { PKCEVerifier: "test-pkce-verifier", DPoPPrivateKeyMultibase: "test-dpop-key", DPoPAuthServerNonce: "test-nonce", - AuthServerURL: "http://localhost:3001", - RequestURI: "http://localhost:3001/authorize", - AuthServerTokenEndpoint: "http://localhost:3001/oauth/token", - AuthServerRevocationEndpoint: "http://localhost:3001/oauth/revoke", + AuthServerURL: testPDSURL(), + RequestURI: testPDSURL() + "/authorize", + AuthServerTokenEndpoint: testPDSURL() + "/oauth/token", + AuthServerRevocationEndpoint: testPDSURL() + "/oauth/revoke", Scopes: []string{"atproto"}, } @@ -427,7 +427,7 @@ func TestOAuthE2E_AuthRequestStorage(t *testing.T) { oldAuthRequest := oauthlib.AuthRequestData{ State: oldState, PKCEVerifier: "old-verifier", - AuthServerURL: "http://localhost:3001", + AuthServerURL: testPDSURL(), Scopes: []string{"atproto"}, } @@ -482,10 +482,10 @@ func TestOAuthE2E_TokenRefresh(t *testing.T) { initialSession := oauthlib.ClientSessionData{ AccountDID: did, SessionID: "refresh-session-1", - HostURL: "http://localhost:3001", - AuthServerURL: "http://localhost:3001", - AuthServerTokenEndpoint: "http://localhost:3001/oauth/token", - AuthServerRevocationEndpoint: "http://localhost:3001/oauth/revoke", + HostURL: testPDSURL(), + AuthServerURL: testPDSURL(), + AuthServerTokenEndpoint: testPDSURL() + "/oauth/token", + AuthServerRevocationEndpoint: testPDSURL() + "/oauth/revoke", AccessToken: "initial-access-token", RefreshToken: "initial-refresh-token", DPoPPrivateKeyMultibase: "test-dpop-key", @@ -727,9 +727,9 @@ func TestOAuthE2E_SessionUpdate(t *testing.T) { originalSession := oauthlib.ClientSessionData{ AccountDID: did, SessionID: "update-session-1", - HostURL: "http://localhost:3001", - AuthServerURL: "http://localhost:3001", - AuthServerTokenEndpoint: "http://localhost:3001/oauth/token", + HostURL: testPDSURL(), + AuthServerURL: testPDSURL(), + AuthServerTokenEndpoint: testPDSURL() + "/oauth/token", AccessToken: "original-access-token", RefreshToken: "original-refresh-token", DPoPPrivateKeyMultibase: "original-dpop-key", @@ -814,13 +814,19 @@ func TestOAuthE2E_RefreshTokenRotation(t *testing.T) { {"access-token-v3", "refresh-token-v3"}, } + // Each rotation must land as a distinct write, so its row carries a strictly + // later updated_at than the one before. That is the observable the loop's + // old `time.Sleep(10 * time.Millisecond)` was guessing at — the sleep was + // commented "ensure timestamp differences" and then nothing checked one. + var previousUpdate time.Time + for i, tokenPair := range tokens { session := oauthlib.ClientSessionData{ AccountDID: did, SessionID: sessionID, - HostURL: "http://localhost:3001", - AuthServerURL: "http://localhost:3001", - AuthServerTokenEndpoint: "http://localhost:3001/oauth/token", + HostURL: testPDSURL(), + AuthServerURL: testPDSURL(), + AuthServerTokenEndpoint: testPDSURL() + "/oauth/token", AccessToken: tokenPair.access, RefreshToken: tokenPair.refresh, Scopes: []string{"atproto"}, @@ -839,8 +845,17 @@ func TestOAuthE2E_RefreshTokenRotation(t *testing.T) { assert.Equal(t, tokenPair.refresh, retrieved.RefreshToken, "Refresh token should match iteration %d", i+1) - // Small delay to ensure timestamp differences - time.Sleep(10 * time.Millisecond) + var thisUpdate time.Time + testkit.WaitFor(t, 5*time.Second, func() (bool, error) { + if err := db.QueryRowContext(ctx, + "SELECT updated_at FROM oauth_sessions WHERE did = $1 AND session_id = $2", + did.String(), sessionID).Scan(&thisUpdate); err != nil { + return false, err + } + return thisUpdate.After(previousUpdate), nil + }, testkit.WithDescription( + "rotation %d to advance oauth_sessions.updated_at past %s", i+1, previousUpdate)) + previousUpdate = thisUpdate } t.Logf("✅ Refresh token rotation verified through %d cycles", len(tokens)) diff --git a/internal/atproto/oauth/oauth_session_fixation_test.go b/internal/atproto/oauth/oauth_session_fixation_test.go index 289dd2e..e501600 100644 --- a/internal/atproto/oauth/oauth_session_fixation_test.go +++ b/internal/atproto/oauth/oauth_session_fixation_test.go @@ -63,7 +63,7 @@ func TestOAuth_SessionFixationAttackPrevention(t *testing.T) { testSession := oauthlib.ClientSessionData{ AccountDID: parsedDID, SessionID: sessionID, - HostURL: "http://localhost:3001", + HostURL: testPDSURL(), AccessToken: "test-access-token", Scopes: []string{"atproto"}, } @@ -75,7 +75,7 @@ func TestOAuth_SessionFixationAttackPrevention(t *testing.T) { // Step 2: Attacker planted a mobile_redirect_uri cookie (without binding) // This simulates the cookie being planted earlier by attacker attackerRedirectURI := "evil://steal" - req := httptest.NewRequest("GET", "/oauth/callback?code=test&state=test&iss=http://localhost:3001", nil) + req := httptest.NewRequest("GET", "/oauth/callback?code=test&state=test&iss="+testPDSURL(), nil) // Plant the attacker's cookie (URL escaped as it would be in real scenario) req.AddCookie(&http.Cookie{ @@ -118,7 +118,7 @@ func TestOAuth_SessionFixationAttackPrevention(t *testing.T) { testSession := oauthlib.ClientSessionData{ AccountDID: parsedDID, SessionID: sessionID, - HostURL: "http://localhost:3001", + HostURL: testPDSURL(), AccessToken: "mobile-access-token", Scopes: []string{"atproto"}, } @@ -131,7 +131,7 @@ func TestOAuth_SessionFixationAttackPrevention(t *testing.T) { // Use Universal Link URI that's in the allowlist legitRedirectURI := "https://coves.social/app/oauth/callback" csrfToken := "valid-csrf-token-for-mobile" - req := httptest.NewRequest("GET", "/oauth/callback?code=test&state=test&iss=http://localhost:3001", nil) + req := httptest.NewRequest("GET", "/oauth/callback?code=test&state=test&iss="+testPDSURL(), nil) // Add mobile redirect URI cookie req.AddCookie(&http.Cookie{ @@ -179,7 +179,7 @@ func TestOAuth_SessionFixationAttackPrevention(t *testing.T) { testSession := oauthlib.ClientSessionData{ AccountDID: parsedDID, SessionID: sessionID, - HostURL: "http://localhost:3001", + HostURL: testPDSURL(), AccessToken: "binding-test-token", Scopes: []string{"atproto"}, } @@ -190,7 +190,7 @@ func TestOAuth_SessionFixationAttackPrevention(t *testing.T) { // Attacker tries to plant evil redirect with a binding from different URI attackerRedirectURI := "evil://steal" attackerCSRF := "attacker-csrf-token" - req := httptest.NewRequest("GET", "/oauth/callback?code=test&state=test&iss=http://localhost:3001", nil) + req := httptest.NewRequest("GET", "/oauth/callback?code=test&state=test&iss="+testPDSURL(), nil) req.AddCookie(&http.Cookie{ Name: "mobile_redirect_uri", @@ -235,7 +235,7 @@ func TestOAuth_SessionFixationAttackPrevention(t *testing.T) { testSession := oauthlib.ClientSessionData{ AccountDID: parsedDID, SessionID: sessionID, - HostURL: "http://localhost:3001", + HostURL: testPDSURL(), AccessToken: "csrf-test-token", Scopes: []string{"atproto"}, } @@ -259,7 +259,7 @@ func TestOAuth_SessionFixationAttackPrevention(t *testing.T) { // But attacker managed to change the CSRF cookie attackerCSRF := "attacker-replaced-csrf" - req := httptest.NewRequest("GET", "/oauth/callback?code=test&state=test&iss=http://localhost:3001", nil) + req := httptest.NewRequest("GET", "/oauth/callback?code=test&state=test&iss="+testPDSURL(), nil) req.AddCookie(&http.Cookie{ Name: "mobile_redirect_uri", diff --git a/internal/atproto/oauth/seal_test.go b/internal/atproto/oauth/seal_test.go index f845d16..e18b7bb 100644 --- a/internal/atproto/oauth/seal_test.go +++ b/internal/atproto/oauth/seal_test.go @@ -60,22 +60,23 @@ func TestSealSession_ExpirationValidation(t *testing.T) { did := "did:plc:abc123" sessionID := "session-xyz" - ttl := 2 * time.Second // Short TTL (must be >= 1 second due to Unix timestamp granularity) - // Seal the session - token, err := client.SealSession(did, sessionID, ttl) + // A token inside its TTL unseals. + live, err := client.SealSession(did, sessionID, 1*time.Hour) require.NoError(t, err) - // Should work immediately - session, err := client.UnsealSession(token) + session, err := client.UnsealSession(live) require.NoError(t, err) assert.Equal(t, did, session.DID) - // Wait well past expiration - time.Sleep(2500 * time.Millisecond) + // Expiry is stamped at seal time as now+ttl and compared against the wall + // clock at unseal, so a negative TTL produces exactly the state a real + // token reaches by ageing — without spending 2.5s of wall clock reaching + // it. Both paths run the same `ExpiresAt <= now` branch in UnsealSession. + expired, err := client.SealSession(did, sessionID, -1*time.Second) + require.NoError(t, err) - // Should fail after expiration - session, err = client.UnsealSession(token) + session, err = client.UnsealSession(expired) assert.Error(t, err) assert.Nil(t, session) assert.Contains(t, err.Error(), "token expired") diff --git a/internal/config/config_test.go b/internal/config/config_test.go index ede2376..4a5f649 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,5 +1,12 @@ package config +// This file tests Load(), which reads environment variables and returns a +// struct. It resolves nothing and dials nothing, so every URL below is either +// an input string or an expected output string. +// +// coves:allow-host-literal-file: the dev defaults ARE localhost:3001/3002/3003/6008, and the production guard's whole job is to reject localhost — sourcing either from testkit would assert the parser against itself and delete the test's meaning. +// coves:allow-public-host-file: the production defaults this parser must produce name plc.directory and the public Bluesky Jetstream; an assertion on any other value would be asserting a deployment we do not ship. + import ( "bytes" "encoding/base64" diff --git a/internal/core/aggregators/apikey_service_test.go b/internal/core/aggregators/apikey_service_test.go index 93a2d23..836a5a2 100644 --- a/internal/core/aggregators/apikey_service_test.go +++ b/internal/core/aggregators/apikey_service_test.go @@ -5,11 +5,14 @@ import ( "crypto/sha256" "encoding/hex" "errors" + "fmt" "testing" "time" "github.com/bluesky-social/indigo/atproto/auth/oauth" "github.com/bluesky-social/indigo/atproto/syntax" + + "Coves/tests/testkit" ) // ptrTime returns a pointer to a time.Time (current time) @@ -1124,13 +1127,15 @@ func TestAPIKeyService_FailedLastUsedUpdates_IncrementsOnError(t *testing.T) { t.Fatal("timeout waiting for async UpdateAPIKeyLastUsed call") } - // Give a moment for the counter to be incremented - time.Sleep(10 * time.Millisecond) - - // Counter should now be 1 - if got := service.GetFailedLastUsedUpdates(); got != 1 { - t.Errorf("GetFailedLastUsedUpdates() after failure = %d, want 1", got) - } + // The mock signals from a defer, so it is reached BEFORE the goroutine that + // called it has looked at the returned error — the counter increment is + // still in flight at this point. Wait for the increment itself. + testkit.WaitFor(t, 5*time.Second, func() (bool, error) { + return service.GetFailedLastUsedUpdates() == 1, nil + }, testkit.WithDescription("the failed-last_used counter to record the database error"), + testkit.WithDiagnostics(func() string { + return fmt.Sprintf("counter: %d", service.GetFailedLastUsedUpdates()) + })) } // ============================================================================= diff --git a/internal/core/blueskypost/circuit_breaker_test.go b/internal/core/blueskypost/circuit_breaker_test.go index 479b389..dff2499 100644 --- a/internal/core/blueskypost/circuit_breaker_test.go +++ b/internal/core/blueskypost/circuit_breaker_test.go @@ -7,6 +7,26 @@ import ( "time" ) +// rewindLastFailure moves a provider's recorded failure timestamp further into +// the past, which is precisely what the passage of time does to it. +// +// The open window is a deadline the breaker computes from that timestamp +// (`time.Since(lastFailure) > openDuration`), so a test crosses the deadline by +// moving the timestamp rather than by outliving it: the same production branch +// runs, with no wall clock spent and no margin to tune. It also lets these +// tests keep the real five-minute openDuration instead of shrinking it to +// something a sleep can afford. +func rewindLastFailure(t *testing.T, cb *circuitBreaker, provider string, d time.Duration) { + t.Helper() + cb.mu.Lock() + defer cb.mu.Unlock() + last, ok := cb.lastFailure[provider] + if !ok { + t.Fatalf("provider %q has no recorded failure to rewind", provider) + } + cb.lastFailure[provider] = last.Add(-d) +} + func TestCircuitBreaker_InitialState(t *testing.T) { cb := newCircuitBreaker() @@ -62,8 +82,6 @@ func TestCircuitBreaker_StaysClosedBelowThreshold(t *testing.T) { func TestCircuitBreaker_TransitionsToHalfOpenAfterTimeout(t *testing.T) { cb := newCircuitBreaker() - // Set a very short open duration for testing - cb.openDuration = 10 * time.Millisecond provider := "test-provider" testErr := errors.New("test error") @@ -78,8 +96,8 @@ func TestCircuitBreaker_TransitionsToHalfOpenAfterTimeout(t *testing.T) { t.Fatal("Circuit should be open after threshold failures") } - // Wait for the open duration to pass - time.Sleep(cb.openDuration + 5*time.Millisecond) + // Put the open duration behind us. + rewindLastFailure(t, cb, provider, cb.openDuration+time.Second) // Circuit should transition to half-open and allow attempt canAttempt, err = cb.canAttempt(provider) @@ -93,7 +111,6 @@ func TestCircuitBreaker_TransitionsToHalfOpenAfterTimeout(t *testing.T) { func TestCircuitBreaker_ClosesOnSuccessAfterHalfOpen(t *testing.T) { cb := newCircuitBreaker() - cb.openDuration = 10 * time.Millisecond provider := "test-provider" testErr := errors.New("test error") @@ -102,8 +119,8 @@ func TestCircuitBreaker_ClosesOnSuccessAfterHalfOpen(t *testing.T) { cb.recordFailure(provider, testErr) } - // Wait for half-open - time.Sleep(cb.openDuration + 5*time.Millisecond) + // Age the failure past the open window, so the next attempt is half-open. + rewindLastFailure(t, cb, provider, cb.openDuration+time.Second) // Verify we can attempt canAttempt, _ := cb.canAttempt(provider) @@ -267,7 +284,6 @@ func TestCircuitBreaker_MultipleProvidersThreadSafety(t *testing.T) { func TestCircuitBreaker_StateTransitions(t *testing.T) { cb := newCircuitBreaker() - cb.openDuration = 10 * time.Millisecond provider := "test-provider" testErr := errors.New("test error") @@ -289,9 +305,9 @@ func TestCircuitBreaker_StateTransitions(t *testing.T) { } cb.mu.RUnlock() - // Wait for half-open transition - time.Sleep(cb.openDuration + 5*time.Millisecond) - _, _ = cb.canAttempt(provider) // Trigger state check + // Age past the open window, then trigger the state check. + rewindLastFailure(t, cb, provider, cb.openDuration+time.Second) + _, _ = cb.canAttempt(provider) cb.mu.RLock() state := cb.getState(provider) @@ -337,7 +353,6 @@ func TestCircuitBreaker_ErrorMessage(t *testing.T) { func TestCircuitBreaker_HalfOpenFailureReopens(t *testing.T) { cb := newCircuitBreaker() - cb.openDuration = 10 * time.Millisecond provider := "test-provider" testErr := errors.New("test error") @@ -346,8 +361,8 @@ func TestCircuitBreaker_HalfOpenFailureReopens(t *testing.T) { cb.recordFailure(provider, testErr) } - // Wait for half-open - time.Sleep(cb.openDuration + 5*time.Millisecond) + // Age past the open window and take the half-open transition. + rewindLastFailure(t, cb, provider, cb.openDuration+time.Second) _, _ = cb.canAttempt(provider) // Record another failure in half-open state @@ -394,15 +409,15 @@ func TestCircuitBreaker_CustomThresholdAndDuration(t *testing.T) { t.Error("Circuit should be open after 5 failures") } - // Should not transition to half-open before 20ms - time.Sleep(10 * time.Millisecond) + // Half of the custom 20ms window has passed: still open. + rewindLastFailure(t, cb, provider, 10*time.Millisecond) canAttempt, _ = cb.canAttempt(provider) if canAttempt { t.Error("Circuit should still be open before timeout") } - // Should transition after 20ms - time.Sleep(15 * time.Millisecond) + // 25ms total, past the custom window: half-open. + rewindLastFailure(t, cb, provider, 15*time.Millisecond) canAttempt, _ = cb.canAttempt(provider) if !canAttempt { t.Error("Circuit should be half-open after timeout") diff --git a/internal/core/blueskypost/repository_test.go b/internal/core/blueskypost/repository_test.go index 273d219..68d65f5 100644 --- a/internal/core/blueskypost/repository_test.go +++ b/internal/core/blueskypost/repository_test.go @@ -31,8 +31,11 @@ func TestValidateATURI(t *testing.T) { wantErr: true, }, { - name: "http URL instead of AT-URI", - atURI: "https://bsky.app/profile/user.bsky.social/post/abc123", + name: "http URL instead of AT-URI", + // The case exists BECAUSE this string is a plausible thing to pass + // where an AT-URI belongs; validateATURI does a prefix check on it + // and returns an error. + atURI: "https://bsky.app/profile/user.bsky.social/post/abc123", // coves:allow-public-host: rejection input for a string validator. wantErr: true, }, } diff --git a/internal/core/blueskypost/service_test.go b/internal/core/blueskypost/service_test.go index 4171d00..d5950ed 100644 --- a/internal/core/blueskypost/service_test.go +++ b/internal/core/blueskypost/service_test.go @@ -3,12 +3,19 @@ package blueskypost import ( "context" "errors" + "fmt" "net/http" "net/http/httptest" "testing" "time" ) +// alicePostURL is the one bsky.app permalink these tests parse. IsBlueskyURL +// and ParseBlueskyURL are regex-and-split over the string — no request is made +// to derive the AT-URI — and every test here that does issue a request goes +// through newStubbedService, which points the fetcher at an httptest server. +const alicePostURL = "https://bsky.app/profile/alice.bsky.social/post/abc123" // coves:allow-public-host: parser input; the only host this file dials is the local stub server. + // mockRepository implements Repository for testing type mockRepository struct { storage map[string]*BlueskyPostResult @@ -75,7 +82,7 @@ func TestService_IsBlueskyURL(t *testing.T) { }{ { name: "valid bsky.app URL", - url: "https://bsky.app/profile/alice.bsky.social/post/abc123", + url: alicePostURL, expected: true, }, { @@ -113,7 +120,7 @@ func TestService_ParseBlueskyURL(t *testing.T) { }{ { name: "valid URL", - url: "https://bsky.app/profile/alice.bsky.social/post/abc123", + url: alicePostURL, expectedURI: "at://did:plc:alice123/app.bsky.feed.post/abc123", wantErr: false, }, @@ -341,7 +348,11 @@ func TestService_DefaultAPITarget(t *testing.T) { // response parsing has no merge-path coverage at all: it was previously only // exercised by the live-tier tests. func TestService_ResolvePost_ParsesAPIResponse(t *testing.T) { - const goldenResponse = `{ + // The avatar is spliced in rather than written inline because it is the one + // public hostname in the fixture, and a JSON line cannot carry a Go comment + // saying so. + const goldenAvatar = "https://cdn.bsky.app/img/avatar/alice.jpg" // coves:allow-public-host: a field in a canned API response body; decoded by the stub handler's client, never fetched. + goldenResponse := fmt.Sprintf(`{ "posts": [ { "uri": "at://did:plc:alice123/app.bsky.feed.post/abc123", @@ -350,7 +361,7 @@ func TestService_ResolvePost_ParsesAPIResponse(t *testing.T) { "did": "did:plc:alice123", "handle": "alice.bsky.social", "displayName": "Alice", - "avatar": "https://cdn.bsky.app/img/avatar/alice.jpg" + "avatar": %q }, "record": { "text": "hello from the golden fixture", @@ -362,7 +373,7 @@ func TestService_ResolvePost_ParsesAPIResponse(t *testing.T) { "indexedAt": "2026-07-01T12:00:05Z" } ] - }` + }`, goldenAvatar) repo := newMockRepository() var requestedURI string @@ -611,7 +622,7 @@ func TestService_IntegrationFlow(t *testing.T) { ctx := context.Background() // Step 1: Check URL - url := "https://bsky.app/profile/alice.bsky.social/post/abc123" + url := alicePostURL if !svc.IsBlueskyURL(url) { t.Fatalf("IsBlueskyURL(%q) should return true", url) } diff --git a/internal/core/blueskypost/url_parser_test.go b/internal/core/blueskypost/url_parser_test.go index f1fbf9f..9fb3d9f 100644 --- a/internal/core/blueskypost/url_parser_test.go +++ b/internal/core/blueskypost/url_parser_test.go @@ -1,5 +1,7 @@ package blueskypost +// coves:allow-public-host-file: this file IS the bsky.app URL parser's test — every hostname in it is a parser INPUT or an expected reject, matched against a regex and never dialled, and per-line markers on ~19 table rows would bury the table they annotate. + import ( "Coves/internal/atproto/identity" "context" diff --git a/internal/core/communities/service_provisioning_test.go b/internal/core/communities/service_provisioning_test.go index 1e151dd..9758ef5 100644 --- a/internal/core/communities/service_provisioning_test.go +++ b/internal/core/communities/service_provisioning_test.go @@ -20,9 +20,9 @@ import ( // What creating a community actually produces on the PDS. // -// tests/integration/community_service_integration_test.go already covers the -// service's own view of provisioning — the returned DID, handle, record URI and -// the credentials landing encrypted in Postgres. What it does NOT check, and +// service_credentials_test.go, next to this file, already covers the service's +// own view of provisioning — the returned DID, handle, record URI and the +// credentials landing encrypted in Postgres. What it does NOT check, and // what tests/integration/community_e2e_test.go's deleted queryPDSAccount step // did, is the binding on the OTHER side: that the handle the service reports is // a handle the PDS will actually resolve, to this community's DID. diff --git a/internal/core/communities/service_writeforward_test.go b/internal/core/communities/service_writeforward_test.go index cf32d96..e6e43f3 100644 --- a/internal/core/communities/service_writeforward_test.go +++ b/internal/core/communities/service_writeforward_test.go @@ -24,9 +24,9 @@ import ( // resulting record from the PDS, and hand-fed a synthetic event to a consumer. // Two of the three now have better homes — handler behaviour is // internal/api/handlers/community's subscribe_test.go and block_test.go, and -// consumer behaviour is tests/integration's subscription_indexing_test.go and -// community_blocking_test.go, both of which cover more cases than the deleted -// file did. What was left, and is here, is the write-forward itself: the record +// consumer behaviour is internal/atproto/jetstream's community_consumer_test.go +// and community_consumer_block_test.go, both of which cover more cases than the +// deleted file did. What was left, and is here, is the write-forward itself: the record // the service puts in the user's repo. // // # WHY THE COLLECTION NAME IS THE POINT diff --git a/internal/core/communitysuggestions/community_suggestion_integration_test.go b/internal/core/communitysuggestions/community_suggestion_integration_test.go index 2348cf5..b65cad0 100644 --- a/internal/core/communitysuggestions/community_suggestion_integration_test.go +++ b/internal/core/communitysuggestions/community_suggestion_integration_test.go @@ -9,6 +9,8 @@ import ( "Coves/tests/fixtures" "Coves/tests/testkit" "bytes" + "context" + "database/sql" "encoding/json" "fmt" "net/http" @@ -205,6 +207,15 @@ func updateStatusRequest(t *testing.T, router http.Handler, token string, sugges // Returns the router, the OAuthMiddleware (for adding users), and a cleanup function. func setupSuggestionTestRouter(t *testing.T, adminDIDs []string) (http.Handler, *fixtures.OAuthMiddleware) { t.Helper() + router, auth, _ := setupSuggestionTestRouterWithDB(t, adminDIDs) + return router, auth +} + +// setupSuggestionTestRouterWithDB additionally hands back the isolated database +// behind the router, for the one test that has to reach past the HTTP surface +// and age a row's created_at. +func setupSuggestionTestRouterWithDB(t *testing.T, adminDIDs []string) (http.Handler, *fixtures.OAuthMiddleware, *sql.DB) { + t.Helper() db := testkit.DB(t) @@ -219,7 +230,31 @@ func setupSuggestionTestRouter(t *testing.T, adminDIDs []string) (http.Handler, r := chi.NewRouter() routes.RegisterCommunitySuggestionRoutes(r, service, e2eAuth.OAuthAuthMiddleware, adminDIDs) - return r, e2eAuth + return r, e2eAuth, db +} + +// backdateSuggestion moves a suggestion's created_at into the past. +// +// The "new" sort is `ORDER BY created_at DESC` with no tiebreak, so two rows +// stamped inside the same clock tick may come back in either order. Backdating +// makes the gap a fact about the data rather than a bet on how long a write +// takes — which is what a sleep between the two writes was. +func backdateSuggestion(t *testing.T, db *sql.DB, suggestionID int64, by time.Duration) { + t.Helper() + + res, err := db.ExecContext(context.Background(), + `UPDATE community_suggestions SET created_at = created_at - make_interval(secs => $1) WHERE id = $2`, + by.Seconds(), suggestionID) + if err != nil { + t.Fatalf("Failed to backdate suggestion %d: %v", suggestionID, err) + } + affected, err := res.RowsAffected() + if err != nil { + t.Fatalf("Failed to read rows affected backdating suggestion %d: %v", suggestionID, err) + } + if affected != 1 { + t.Fatalf("Backdating suggestion %d touched %d rows, want 1", suggestionID, affected) + } } // TestCommunitySuggestionE2E is the comprehensive E2E integration test for the @@ -445,13 +480,15 @@ func TestCommunitySuggestionE2E(t *testing.T) { // Test: List Suggestions - Sort by New // ===================================================================== t.Run("List suggestions - sort by new", func(t *testing.T) { - listRouter, listAuth := setupSuggestionTestRouter(t, []string{adminDID}) + listRouter, listAuth, listDB := setupSuggestionTestRouterWithDB(t, []string{adminDID}) listUserToken := listAuth.AddUser(userDID) - // Create two suggestions with a small delay to ensure different timestamps - _ = mustCreateTestSuggestion(t, listRouter, listUserToken, + // Two suggestions an hour apart. The gap is applied to the row rather + // than waited out between the writes, so the ordering under test is + // decided by the data and not by how fast the first request returned. + s1 := mustCreateTestSuggestion(t, listRouter, listUserToken, "Older Community", "Created first") - time.Sleep(10 * time.Millisecond) + backdateSuggestion(t, listDB, s1.ID, time.Hour) s2 := mustCreateTestSuggestion(t, listRouter, listUserToken, "Newer Community", "Created second") diff --git a/internal/core/imageproxy/cache_test.go b/internal/core/imageproxy/cache_test.go index c396280..ca55985 100644 --- a/internal/core/imageproxy/cache_test.go +++ b/internal/core/imageproxy/cache_test.go @@ -6,8 +6,27 @@ import ( "path/filepath" "testing" "time" + + "Coves/tests/testkit" ) +// entryIsGone reports whether a cache file has been removed from disk. It is +// the probe the cleanup-job tests wait on, and it deliberately does not go +// through DiskCache.Get: a hit touches the entry's mtime, which is the very +// value the TTL sweep is judging, so polling through Get would keep the entry +// alive forever. +func entryIsGone(path string) (bool, error) { + _, err := os.Stat(path) + if errors.Is(err, os.ErrNotExist) { + return true, nil + } + if err != nil { + // Anything other than "not there" is a broken probe, not a delay. + return false, err + } + return false, nil +} + // mustNewDiskCache is a test helper that creates a DiskCache or fails the test // Uses 0 for TTL (disabled) by default for backward compatibility func mustNewDiskCache(t *testing.T, basePath string, maxSizeGB int) *DiskCache { @@ -562,10 +581,18 @@ func TestDiskCache_StartCleanupJob(t *testing.T) { cancel := cache.StartCleanupJob(50 * time.Millisecond) defer cancel() - // Wait for at least one cleanup cycle - time.Sleep(100 * time.Millisecond) - - // Expired file should be gone + // The eviction is work the background goroutine does, so wait for the + // eviction rather than for a duration guessed to contain a cycle. + // + // The probe stats the file instead of calling Get: a cache hit TOUCHES the + // entry's mtime, and mtime is what the TTL sweep reads. Polling through Get + // would keep resetting the age of the very entry it is waiting to see + // expire, and the wait would never finish. + testkit.WaitFor(t, 10*time.Second, func() (bool, error) { + return entryIsGone(expiredPath) + }, testkit.WithDescription("the background cleanup job to evict the expired entry")) + + // And it is gone through the cache API too, not merely off the disk. if _, found, _ := cache.Get("avatar", "did:plc:expired", "cid_expired"); found { t.Error("Expired entry should have been cleaned up by background job") } @@ -586,13 +613,33 @@ func TestDiskCache_StartCleanupJob_ZeroInterval(t *testing.T) { func TestDiskCache_StartCleanupJob_GracefulShutdown(t *testing.T) { tmpDir := t.TempDir() - cache := mustNewDiskCache(t, tmpDir, 1) + // 1-day TTL, so the entry seeded below is expirable and the job's first + // cycle has something observable to do. + cache, err := NewDiskCache(tmpDir, 1, 1) + if err != nil { + t.Fatalf("NewDiskCache failed: %v", err) + } + + data := make([]byte, 100) + if err := cache.Set("avatar", "did:plc:expired", "cid_expired", data); err != nil { + t.Fatalf("Set failed: %v", err) + } + expiredPath := cache.cachePath("avatar", "did:plc:expired", "cid_expired") + oldTime := time.Now().Add(-48 * time.Hour) + if err := os.Chtimes(expiredPath, oldTime, oldTime); err != nil { + t.Fatalf("Chtimes failed: %v", err) + } // Start cleanup job cancel := cache.StartCleanupJob(10 * time.Millisecond) - // Let it run briefly - time.Sleep(30 * time.Millisecond) + // Cancelling a job that never started running would prove nothing, so wait + // until a cycle has demonstrably run — the expired entry disappearing is + // that evidence — and only then shut it down mid-flight. See the note in + // TestDiskCache_StartCleanupJob for why the probe stats rather than Gets. + testkit.WaitFor(t, 10*time.Second, func() (bool, error) { + return entryIsGone(expiredPath) + }, testkit.WithDescription("the cleanup job to complete a cycle before it is cancelled")) // Cancel should not hang or panic done := make(chan struct{}) diff --git a/internal/core/imageproxy/fetcher_test.go b/internal/core/imageproxy/fetcher_test.go index f3dc869..7bfa537 100644 --- a/internal/core/imageproxy/fetcher_test.go +++ b/internal/core/imageproxy/fetcher_test.go @@ -56,12 +56,20 @@ func TestPDSFetcher_Fetch_NotFound(t *testing.T) { } func TestPDSFetcher_Fetch_Timeout(t *testing.T) { + // A PDS that never answers. Blocking is strictly better than sleeping + // here: the handler ends the instant the client gives up, so the test + // costs exactly the 50ms timeout it asserts on and there is no margin to + // guess at. release is the backstop for a cancellation that never reaches + // the server — without it, Close would wait on the request forever. + release := make(chan struct{}) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Sleep longer than the timeout - time.Sleep(200 * time.Millisecond) - w.WriteHeader(http.StatusOK) + select { + case <-r.Context().Done(): + case <-release: + } })) defer server.Close() + defer close(release) // Use a very short timeout fetcher := NewPDSFetcher(50*time.Millisecond, 10) @@ -77,20 +85,29 @@ func TestPDSFetcher_Fetch_NetworkError(t *testing.T) { fetcher := NewPDSFetcher(5*time.Second, 10) ctx := context.Background() - // Use an invalid URL that will cause a network error - _, err := fetcher.Fetch(ctx, "http://localhost:99999", "did:plc:test123", "bafyreicid123") + // Port 99999 is outside the valid range, so this is a malformed address + // rather than an endpoint: the dialer rejects it before any packet leaves + // the process, which is exactly the "the PDS URL is unusable" path under + // test. + _, err := fetcher.Fetch(ctx, "http://localhost:99999", "did:plc:test123", "bafyreicid123") // coves:allow-host-literal: invalid port, rejected by the dialer — never a reachable endpoint if !errors.Is(err, ErrPDSFetchFailed) { t.Errorf("expected ErrPDSFetchFailed, got: %v", err) } } func TestPDSFetcher_Fetch_ContextCancellation(t *testing.T) { + // The server must not be what ends this call — the already-cancelled + // context is. So it never answers, and if a request somehow reaches it the + // handler blocks instead of racing the cancellation with a sleep. + release := make(chan struct{}) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Sleep to allow context cancellation - time.Sleep(100 * time.Millisecond) - w.WriteHeader(http.StatusOK) + select { + case <-r.Context().Done(): + case <-release: + } })) defer server.Close() + defer close(release) fetcher := NewPDSFetcher(5*time.Second, 10) ctx, cancel := context.WithCancel(context.Background()) diff --git a/internal/core/imageproxy/service_test.go b/internal/core/imageproxy/service_test.go index b3e3a5b..01c9658 100644 --- a/internal/core/imageproxy/service_test.go +++ b/internal/core/imageproxy/service_test.go @@ -3,9 +3,12 @@ package imageproxy import ( "context" "errors" + "fmt" "sync" "testing" "time" + + "Coves/tests/testkit" ) // MockCache implements Cache for testing @@ -220,13 +223,14 @@ func TestImageProxyService_GetImage_CacheMiss(t *testing.T) { t.Errorf("expected processor to be called once, got %d calls", processor.Calls()) } - // Wait a bit for async cache write - time.Sleep(50 * time.Millisecond) - - // Verify cache was written - if cache.SetCalls() < 1 { - t.Errorf("expected cache to be written, got %d set calls", cache.SetCalls()) - } + // The cache write happens on its own goroutine, so wait for the write + // itself rather than for a duration guessed to contain it. + testkit.WaitFor(t, 5*time.Second, func() (bool, error) { + return cache.SetCalls() >= 1, nil + }, testkit.WithDescription("the asynchronous cache write to land"), + testkit.WithDiagnostics(func() string { + return fmt.Sprintf("cache Set calls: %d", cache.SetCalls()) + })) // Verify the correct data was cached setData, found := cache.GetSetData("avatar", "did:plc:test123", "bafyreicid123") @@ -312,13 +316,13 @@ func TestImageProxyService_GetImage_CacheWriteIsAsync(t *testing.T) { t.Logf("warning: GetImage took %v, expected faster response", elapsed) } - // Wait for async cache write to complete - time.Sleep(100 * time.Millisecond) - - // Now verify cache was written - if cache.SetCalls() < 1 { - t.Errorf("expected cache to be written asynchronously, got %d set calls", cache.SetCalls()) - } + // The write still has to happen — asynchronous must not mean dropped. + testkit.WaitFor(t, 5*time.Second, func() (bool, error) { + return cache.SetCalls() >= 1, nil + }, testkit.WithDescription("the cache write to complete after GetImage returned"), + testkit.WithDiagnostics(func() string { + return fmt.Sprintf("cache Set calls: %d", cache.SetCalls()) + })) } func TestImageProxyService_GetImage_EmptyPreset(t *testing.T) { diff --git a/internal/core/posts/blob_transform_test.go b/internal/core/posts/blob_transform_test.go index 97aa199..75b5571 100644 --- a/internal/core/posts/blob_transform_test.go +++ b/internal/core/posts/blob_transform_test.go @@ -7,12 +7,24 @@ import ( "github.com/stretchr/testify/require" ) +// testPDSBaseURL is the base URL the transforms under test concatenate onto. +// +// It is data, not an endpoint. TransformBlobRefsToURLs and transformThumbToURL +// take the PDS base off the record (CommunityRef.PDSURL) or as a parameter and +// build a getBlob path from it; there is no HTTP client in this file and +// nothing here is dialled. Reading the base from testkit.Endpoints() — the +// same place the serving code reads it — would make the expected strings +// tautological, so it is written down once here instead, which also keeps the +// assertions focused on the part the function actually builds: the path and +// its query. +const testPDSBaseURL = "http://localhost:3001" // coves:allow-host-literal: expected-output fixture for a pure string transform; never dialled + func TestTransformBlobRefsToURLs(t *testing.T) { t.Run("transforms external embed thumb from blob to URL", func(t *testing.T) { post := &PostView{ Community: &CommunityRef{ DID: "did:plc:testcommunity", - PDSURL: "http://localhost:3001", + PDSURL: testPDSBaseURL, }, Embed: map[string]interface{}{ "$type": "social.coves.embed.external", @@ -44,7 +56,7 @@ func TestTransformBlobRefsToURLs(t *testing.T) { thumbURL, ok := external["thumb"].(string) require.True(t, ok, "thumb should be a string URL") assert.Equal(t, - "http://localhost:3001/xrpc/com.atproto.sync.getBlob?did=did:plc:testcommunity&cid=bafyreib6tbnql2ux3whnfysbzabthaj2vvck53nimhbi5g5a7jgvgr5eqm", + testPDSBaseURL+"/xrpc/com.atproto.sync.getBlob?did=did:plc:testcommunity&cid=bafyreib6tbnql2ux3whnfysbzabthaj2vvck53nimhbi5g5a7jgvgr5eqm", thumbURL) }) @@ -52,7 +64,7 @@ func TestTransformBlobRefsToURLs(t *testing.T) { post := &PostView{ Community: &CommunityRef{ DID: "did:plc:testcommunity", - PDSURL: "http://localhost:3001", + PDSURL: testPDSBaseURL, }, Embed: map[string]interface{}{ "$type": "social.coves.embed.external", @@ -74,11 +86,11 @@ func TestTransformBlobRefsToURLs(t *testing.T) { }) t.Run("handles already-transformed URL thumb", func(t *testing.T) { - expectedURL := "http://localhost:3001/xrpc/com.atproto.sync.getBlob?did=did:plc:test&cid=bafytest" + expectedURL := testPDSBaseURL + "/xrpc/com.atproto.sync.getBlob?did=did:plc:test&cid=bafytest" post := &PostView{ Community: &CommunityRef{ DID: "did:plc:testcommunity", - PDSURL: "http://localhost:3001", + PDSURL: testPDSBaseURL, }, Embed: map[string]interface{}{ "$type": "social.coves.embed.external", @@ -104,7 +116,7 @@ func TestTransformBlobRefsToURLs(t *testing.T) { post := &PostView{ Community: &CommunityRef{ DID: "did:plc:testcommunity", - PDSURL: "http://localhost:3001", + PDSURL: testPDSBaseURL, }, Embed: nil, } @@ -184,7 +196,7 @@ func TestTransformBlobRefsToURLs(t *testing.T) { post := &PostView{ Community: &CommunityRef{ DID: "did:plc:testcommunity", - PDSURL: "http://localhost:3001", + PDSURL: testPDSBaseURL, }, Embed: map[string]interface{}{ "$type": "social.coves.embed.external", @@ -213,7 +225,7 @@ func TestTransformBlobRefsToURLs(t *testing.T) { post := &PostView{ Community: &CommunityRef{ DID: "did:plc:testcommunity", - PDSURL: "http://localhost:3001", + PDSURL: testPDSBaseURL, }, Embed: map[string]interface{}{ "$type": "social.coves.embed.images", @@ -256,23 +268,23 @@ func TestTransformThumbToURL(t *testing.T) { }, } - transformThumbToURL(external, "did:plc:test", "http://localhost:3001") + transformThumbToURL(external, "did:plc:test", testPDSBaseURL) thumbURL, ok := external["thumb"].(string) require.True(t, ok, "thumb should be a string URL") assert.Equal(t, - "http://localhost:3001/xrpc/com.atproto.sync.getBlob?did=did:plc:test&cid=bafyreib6tbnql2ux3whnfysbzabthaj2vvck53nimhbi5g5a7jgvgr5eqm", + testPDSBaseURL+"/xrpc/com.atproto.sync.getBlob?did=did:plc:test&cid=bafyreib6tbnql2ux3whnfysbzabthaj2vvck53nimhbi5g5a7jgvgr5eqm", thumbURL) }) t.Run("does not transform if thumb is already string", func(t *testing.T) { - expectedURL := "http://localhost:3001/xrpc/com.atproto.sync.getBlob?did=did:plc:test&cid=bafytest" + expectedURL := testPDSBaseURL + "/xrpc/com.atproto.sync.getBlob?did=did:plc:test&cid=bafytest" external := map[string]interface{}{ "uri": "https://example.com", "thumb": expectedURL, } - transformThumbToURL(external, "did:plc:test", "http://localhost:3001") + transformThumbToURL(external, "did:plc:test", testPDSBaseURL) thumbURL, ok := external["thumb"].(string) require.True(t, ok, "thumb should still be a string") @@ -284,7 +296,7 @@ func TestTransformThumbToURL(t *testing.T) { "uri": "https://example.com", } - transformThumbToURL(external, "did:plc:test", "http://localhost:3001") + transformThumbToURL(external, "did:plc:test", testPDSBaseURL) _, hasThumb := external["thumb"] assert.False(t, hasThumb, "thumb should not be added") @@ -301,7 +313,7 @@ func TestTransformThumbToURL(t *testing.T) { }, } - transformThumbToURL(external, "did:plc:test", "http://localhost:3001") + transformThumbToURL(external, "did:plc:test", testPDSBaseURL) // Verify thumb is unchanged thumb, ok := external["thumb"].(map[string]interface{}) diff --git a/internal/core/posts/embed_conversion_test.go b/internal/core/posts/embed_conversion_test.go index 0305381..1127c73 100644 --- a/internal/core/posts/embed_conversion_test.go +++ b/internal/core/posts/embed_conversion_test.go @@ -11,6 +11,20 @@ import ( "github.com/stretchr/testify/require" ) +// blueskyPostURL builds the permalink that goes in an external embed's `uri` +// field. It exists so the one public hostname in this file is written once, +// where it can be explained, instead of nine times in nine table rows. +// +// The string is inert here. Every test below drives tryConvertBlueskyURLToPostEmbed +// with a mockBlueskyService whose IsBlueskyURL, ParseBlueskyURL and ResolvePost +// all ignore their argument and return canned values — the URL is never parsed +// and certainly never fetched. The handles vary only to name the scenario the +// mock is configured for. +func blueskyPostURL(handle, rkey string) string { + const bskyAppOrigin = "https://bsky.app" // coves:allow-public-host: fixture origin for embed `uri` fields; the Bluesky service is mocked, so nothing resolves or dials it. + return bskyAppOrigin + "/profile/" + handle + "/post/" + rkey +} + // mockBlueskyService implements blueskypost.Service for testing type mockBlueskyService struct { isBlueskyURLResult bool @@ -41,7 +55,7 @@ func TestTryConvertBlueskyURLToPostEmbed(t *testing.T) { } external := map[string]interface{}{ - "uri": "https://bsky.app/profile/test.bsky.social/post/abc123", + "uri": blueskyPostURL("test.bsky.social", "abc123"), } postRecord := &PostRecord{} @@ -137,7 +151,7 @@ func TestTryConvertBlueskyURLToPostEmbed(t *testing.T) { } external := map[string]interface{}{ - "uri": "https://bsky.app/profile/nonexistent.bsky.social/post/abc123", + "uri": blueskyPostURL("nonexistent.bsky.social", "abc123"), } postRecord := &PostRecord{} @@ -158,7 +172,7 @@ func TestTryConvertBlueskyURLToPostEmbed(t *testing.T) { } external := map[string]interface{}{ - "uri": "https://bsky.app/profile/test.bsky.social/post/abc123", + "uri": blueskyPostURL("test.bsky.social", "abc123"), } postRecord := &PostRecord{} @@ -182,7 +196,7 @@ func TestTryConvertBlueskyURLToPostEmbed(t *testing.T) { } external := map[string]interface{}{ - "uri": "https://bsky.app/profile/deleted.bsky.social/post/deleted123", + "uri": blueskyPostURL("deleted.bsky.social", "deleted123"), } postRecord := &PostRecord{} @@ -204,7 +218,7 @@ func TestTryConvertBlueskyURLToPostEmbed(t *testing.T) { } external := map[string]interface{}{ - "uri": "https://bsky.app/profile/test.bsky.social/post/abc123", + "uri": blueskyPostURL("test.bsky.social", "abc123"), } postRecord := &PostRecord{} @@ -225,7 +239,7 @@ func TestTryConvertBlueskyURLToPostEmbed(t *testing.T) { } external := map[string]interface{}{ - "uri": "https://bsky.app/profile/test.bsky.social/post/abc123", + "uri": blueskyPostURL("test.bsky.social", "abc123"), } postRecord := &PostRecord{} @@ -249,7 +263,7 @@ func TestTryConvertBlueskyURLToPostEmbed(t *testing.T) { } external := map[string]interface{}{ - "uri": "https://bsky.app/profile/test.bsky.social/post/abc123", + "uri": blueskyPostURL("test.bsky.social", "abc123"), } postRecord := &PostRecord{} @@ -273,7 +287,7 @@ func TestTryConvertBlueskyURLToPostEmbed(t *testing.T) { } external := map[string]interface{}{ - "uri": "https://bsky.app/profile/test.bsky.social/post/abc123", + "uri": blueskyPostURL("test.bsky.social", "abc123"), } postRecord := &PostRecord{} @@ -302,7 +316,7 @@ func TestTryConvertBlueskyURLToPostEmbed(t *testing.T) { } external := map[string]interface{}{ - "uri": "https://bsky.app/profile/test.bsky.social/post/xyz789", + "uri": blueskyPostURL("test.bsky.social", "xyz789"), } postRecord := &PostRecord{} diff --git a/internal/core/posts/record_lexicon_validation_test.go b/internal/core/posts/record_lexicon_validation_test.go index 6abeeef..b179a3f 100644 --- a/internal/core/posts/record_lexicon_validation_test.go +++ b/internal/core/posts/record_lexicon_validation_test.go @@ -55,7 +55,7 @@ func TestPostRecord_LinkCarryingPostsAreAcceptedByTheRepo(t *testing.T) { "community": communityDID, "author": author.DID, "title": "Post with Bluesky Link", - "content": "Check out this Bluesky post: https://bsky.app/profile/jay.bsky.team/post/3l7bsovn5rz2n", + "content": "Check out this Bluesky post: https://bsky.app/profile/jay.bsky.team/post/3l7bsovn5rz2n", // coves:allow-public-host: the URL is post body TEXT written to the local test PDS — the whole point is that it survives the commit as text and is not resolved. "createdAt": time.Now().UTC().Format(time.RFC3339), } diff --git a/internal/core/unfurl/circuit_breaker_test.go b/internal/core/unfurl/circuit_breaker_test.go index bbd4783..a78cf6f 100644 --- a/internal/core/unfurl/circuit_breaker_test.go +++ b/internal/core/unfurl/circuit_breaker_test.go @@ -6,6 +6,24 @@ import ( "time" ) +// rewindLastFailure moves a provider's recorded failure timestamp further into +// the past, which is exactly what the passage of time does to it. +// +// The open window is a deadline the breaker derives from that timestamp +// (`time.Since(lastFailure) > openDuration`), so a test crosses it by moving +// the timestamp instead of waiting the window out: the same production branch +// runs, deterministically and for free. +func rewindLastFailure(t *testing.T, cb *circuitBreaker, provider string, d time.Duration) { + t.Helper() + cb.mu.Lock() + defer cb.mu.Unlock() + last, ok := cb.lastFailure[provider] + if !ok { + t.Fatalf("provider %q has no recorded failure to rewind", provider) + } + cb.lastFailure[provider] = last.Add(-d) +} + func TestCircuitBreaker_Basic(t *testing.T) { t.Parallel() cb := newCircuitBreaker() @@ -73,7 +91,6 @@ func TestCircuitBreaker_RecoveryAfterSuccess(t *testing.T) { func TestCircuitBreaker_HalfOpenTransition(t *testing.T) { t.Parallel() cb := newCircuitBreaker() - cb.openDuration = 100 * time.Millisecond // Short duration for testing provider := "half-open-provider" // Open the circuit @@ -87,8 +104,9 @@ func TestCircuitBreaker_HalfOpenTransition(t *testing.T) { t.Error("Expected circuit to be open") } - // Wait for open duration - time.Sleep(150 * time.Millisecond) + // Put the open duration behind us. The real five-minute window is used + // here, since nothing waits it out. + rewindLastFailure(t, cb, provider, cb.openDuration+time.Second) // Should transition to half-open and allow one attempt canAttempt, err := cb.canAttempt(provider) diff --git a/internal/core/unfurl/kagi_test.go b/internal/core/unfurl/kagi_test.go index 8e70bbb..7ba17f0 100644 --- a/internal/core/unfurl/kagi_test.go +++ b/internal/core/unfurl/kagi_test.go @@ -141,11 +141,22 @@ func TestFetchKagiKite_HTTPError(t *testing.T) { func TestFetchKagiKite_Timeout(t *testing.T) { t.Parallel() + + // The server never answers. Blocking beats sleeping here: the handler ends + // the moment the client gives up, so the test costs only the 100ms timeout + // it is actually asserting on, and there is no "is two seconds long enough" + // margin to get wrong. release is the backstop for the case where the + // client's cancellation never reaches the server — without it, Close would + // block on the in-flight request forever. + release := make(chan struct{}) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - time.Sleep(2 * time.Second) - w.WriteHeader(http.StatusOK) + select { + case <-r.Context().Done(): + case <-release: + } })) defer server.Close() + defer close(release) ctx := context.Background() diff --git a/internal/core/unfurl/opengraph_test.go b/internal/core/unfurl/opengraph_test.go index d3b8a5f..8a03e9e 100644 --- a/internal/core/unfurl/opengraph_test.go +++ b/internal/core/unfurl/opengraph_test.go @@ -191,11 +191,22 @@ func TestFetchOpenGraph_HTTPError(t *testing.T) { func TestFetchOpenGraph_Timeout(t *testing.T) { t.Parallel() + + // The server never answers. Blocking beats sleeping here: the handler ends + // the moment the client gives up, so the test costs only the 100ms timeout + // it is actually asserting on, and there is no "is two seconds long enough" + // margin to get wrong. release is the backstop for the case where the + // client's cancellation never reaches the server — without it, Close would + // block on the in-flight request forever. + release := make(chan struct{}) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - time.Sleep(2 * time.Second) - w.WriteHeader(http.StatusOK) + select { + case <-r.Context().Done(): + case <-release: + } })) defer server.Close() + defer close(release) ctx := context.Background() result, err := fetchOpenGraph(ctx, server.URL, 100*time.Millisecond, "CovesBot/1.0") diff --git a/internal/core/userblocks/service_test.go b/internal/core/userblocks/service_test.go index 9d1fdea..869122c 100644 --- a/internal/core/userblocks/service_test.go +++ b/internal/core/userblocks/service_test.go @@ -128,8 +128,16 @@ func (m *mockPDSClient) DID() string { return "did:plc:mock" } +// mockHostURL is the PDS address the mock client and the fake session report. +// +// Nothing here connects to it: the service under test is built with +// NewServiceWithPDSFactory and a factory that hands back mockPDSClient, so +// every record write is a method call on a struct in this file. The value only +// has to be a well-formed host URL for the session to look real. +const mockHostURL = "http://localhost:3001" // coves:allow-host-literal: value reported by a mock pds.Client and a fake OAuth session; no client is constructed from it. + func (m *mockPDSClient) HostURL() string { - return "http://localhost:3001" + return mockHostURL } // --- Helper to create a test session --- @@ -139,7 +147,7 @@ func testSession(did string) *oauth.ClientSessionData { return &oauth.ClientSessionData{ AccountDID: parsedDID, AccessToken: "test-token", - HostURL: "http://localhost:3001", + HostURL: mockHostURL, } } diff --git a/internal/core/users/profile_backfill_test.go b/internal/core/users/profile_backfill_test.go index 5a8c2f2..5a135dd 100644 --- a/internal/core/users/profile_backfill_test.go +++ b/internal/core/users/profile_backfill_test.go @@ -14,10 +14,16 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + + "Coves/tests/testkit" ) -// backfillSettle is how long the skip-path tests wait before asserting that no -// backfill fetch happened — long enough to catch a regressed async fetch. +// backfillSettle is the window over which the skip-path tests hold their +// "no backfill fetch happened" claim. These are negative assertions about an +// asynchronous path, so they are checked continuously across the window rather +// than once at the end of it: a regressed goroutine that fires late still gets +// caught, and one that fires early is reported the moment it does instead of +// being indistinguishable from one that fired late. const backfillSettle = 100 * time.Millisecond // waitForBackfill waits for the async backfill goroutine to hit the fake PDS @@ -358,9 +364,14 @@ func TestIndexUser_SkipsBackfillWhenProfilePopulated(t *testing.T) { require.NoError(t, err) // The emptiness check is synchronous (no goroutine spawns for populated - // profiles), but settle briefly so a regressed async fetch would be caught. - time.Sleep(backfillSettle) - assert.Equal(t, int64(0), atomic.LoadInt64(&hits)) + // profiles); hold the claim across a window so a regressed async fetch is + // caught whenever in it the fetch lands. + testkit.Holds(t, backfillSettle, func() (bool, error) { + return atomic.LoadInt64(&hits) == 0, nil + }, testkit.WithDescription("the PDS to stay untouched for a user that already has profile data")) + + // Cumulative over the whole window above: a write at any point in it is + // still on the mock's call list now. mockRepo.AssertNotCalled(t, "UpdateProfile", mock.Anything, mock.Anything, mock.Anything) } @@ -385,10 +396,11 @@ func TestIndexUser_BackfillDisabledByDefault(t *testing.T) { err := service.IndexUser(context.Background(), testDID, "nobackfill.test", srv.URL) require.NoError(t, err) - // Backfill disabled → no goroutine spawns; settle briefly to catch a - // regressed async fetch before asserting. - time.Sleep(backfillSettle) - assert.Equal(t, int64(0), atomic.LoadInt64(&hits)) + // Backfill disabled → no goroutine spawns; hold the claim across a window + // so a regressed async fetch is caught wherever in it it lands. + testkit.Holds(t, backfillSettle, func() (bool, error) { + return atomic.LoadInt64(&hits) == 0, nil + }, testkit.WithDescription("the PDS to stay untouched when backfill was never enabled")) } // TestIndexUser_BackfillFailureDoesNotFailIndexing verifies backfill is best-effort: @@ -416,9 +428,16 @@ func TestIndexUser_BackfillFailureDoesNotFailIndexing(t *testing.T) { require.Eventually(t, func() bool { return atomic.LoadInt64(&hits) == 1 }, 5*time.Second, 10*time.Millisecond, "backfill goroutine never fetched from the PDS") - // Settle so the goroutine's post-fetch path (which must bail on the error) - // has finished before asserting no write happened. - time.Sleep(backfillSettle) + + // The fetch failed, so the goroutine must bail: exactly one attempt, and + // nothing after it. Held across a window rather than sampled once, which is + // what catches a retry loop. + testkit.Holds(t, backfillSettle, func() (bool, error) { + return atomic.LoadInt64(&hits) == 1, nil + }, testkit.WithDescription("the failed backfill to stay at a single attempt")) + + // Cumulative over that window: a write at any point in it would be on the + // mock's call list now. mockRepo.AssertNotCalled(t, "UpdateProfile", mock.Anything, mock.Anything, mock.Anything) } diff --git a/internal/core/users/turnstile_test.go b/internal/core/users/turnstile_test.go index b784296..34e2271 100644 --- a/internal/core/users/turnstile_test.go +++ b/internal/core/users/turnstile_test.go @@ -55,9 +55,14 @@ func TestNewCloudflareTurnstile_SiteverifyURL(t *testing.T) { assert.Equal(t, defaultTurnstileSiteverifyURL, v.siteverifyURL) }) + // The value is opaque to the option: this subtest proves WithSiteverifyURL + // stores what it is given, so the string is the fixture AND the expected + // output. No request is made, and nothing listens on this address. + const stubSiteverifyURL = "http://localhost:3003/stub" // coves:allow-host-literal: opaque fixture for the option setter; asserted on, never dialled + t.Run("override redirects verification", func(t *testing.T) { - v := NewCloudflareTurnstile("s", WithSiteverifyURL("http://localhost:3003/stub")).(*cloudflareTurnstile) - assert.Equal(t, "http://localhost:3003/stub", v.siteverifyURL) + v := NewCloudflareTurnstile("s", WithSiteverifyURL(stubSiteverifyURL)).(*cloudflareTurnstile) + assert.Equal(t, stubSiteverifyURL, v.siteverifyURL) }) // The override is plumbed from an env var that is empty in every diff --git a/internal/core/users/user_integration_test.go b/internal/core/users/user_integration_test.go index e4306f0..e7879dd 100644 --- a/internal/core/users/user_integration_test.go +++ b/internal/core/users/user_integration_test.go @@ -24,6 +24,16 @@ import ( "github.com/go-chi/chi/v5" ) +// testPDSURL is the PDS these tests attribute their users to: the default +// users.NewUserService is constructed with, and the PDSURL every user record +// created here carries. +// +// It comes from testkit rather than a literal so a relocated stack moves the +// tests with it (docs/TEST_ARCHITECTURE.md §3.7, layer 1). +func testPDSURL() string { + return testkit.Endpoints().PDS.BaseURL +} + // testUserRouteOptions returns route options with a dummy PDS client factory. // Use this for tests that register user routes but don't actually call updateProfile. func testUserRouteOptions() *routes.UserRouteOptions { @@ -41,7 +51,7 @@ func TestUserCreationAndRetrieval(t *testing.T) { // Wire up dependencies userRepo := postgres.NewUserRepository(db) resolver := identity.NewResolver(db, identity.DefaultConfig()) - userService := users.NewUserService(userRepo, resolver, "http://localhost:3001", nil, "") + userService := users.NewUserService(userRepo, resolver, testPDSURL(), nil, "") ctx := context.Background() @@ -50,7 +60,7 @@ func TestUserCreationAndRetrieval(t *testing.T) { req := users.CreateUserRequest{ DID: "did:plc:test123456", Handle: "alice.test", - PDSURL: "http://localhost:3001", + PDSURL: testPDSURL(), } user, err := userService.CreateUser(ctx, req) @@ -107,14 +117,14 @@ func TestGetProfileEndpoint(t *testing.T) { // Wire up dependencies userRepo := postgres.NewUserRepository(db) resolver := identity.NewResolver(db, identity.DefaultConfig()) - userService := users.NewUserService(userRepo, resolver, "http://localhost:3001", nil, "") + userService := users.NewUserService(userRepo, resolver, testPDSURL(), nil, "") // Create test user directly in service ctx := context.Background() _, err := userService.CreateUser(ctx, users.CreateUserRequest{ DID: "did:plc:endpoint123", Handle: "bob.test", - PDSURL: "http://localhost:3001", + PDSURL: testPDSURL(), }) if err != nil { t.Fatalf("Failed to create test user: %v", err) @@ -206,14 +216,14 @@ func TestDuplicateCreation(t *testing.T) { userRepo := postgres.NewUserRepository(db) resolver := identity.NewResolver(db, identity.DefaultConfig()) - userService := users.NewUserService(userRepo, resolver, "http://localhost:3001", nil, "") + userService := users.NewUserService(userRepo, resolver, testPDSURL(), nil, "") ctx := context.Background() // Create first user _, err := userService.CreateUser(ctx, users.CreateUserRequest{ DID: "did:plc:duplicate123", Handle: "duplicate.test", - PDSURL: "http://localhost:3001", + PDSURL: testPDSURL(), }) if err != nil { t.Fatalf("Failed to create first user: %v", err) @@ -224,7 +234,7 @@ func TestDuplicateCreation(t *testing.T) { user, err := userService.CreateUser(ctx, users.CreateUserRequest{ DID: "did:plc:duplicate123", Handle: "different.test", // Different handle, same DID - PDSURL: "http://localhost:3001", + PDSURL: testPDSURL(), }) // Should return existing user, not error if err != nil { @@ -242,7 +252,7 @@ func TestDuplicateCreation(t *testing.T) { _, err := userService.CreateUser(ctx, users.CreateUserRequest{ DID: "did:plc:different456", Handle: "duplicate.test", - PDSURL: "http://localhost:3001", + PDSURL: testPDSURL(), }) if err == nil { @@ -427,7 +437,7 @@ func TestProfileStats(t *testing.T) { // Wire up dependencies userRepo := postgres.NewUserRepository(db) resolver := identity.NewResolver(db, identity.DefaultConfig()) - userService := users.NewUserService(userRepo, resolver, "http://localhost:3001", nil, "") + userService := users.NewUserService(userRepo, resolver, testPDSURL(), nil, "") ctx := context.Background() @@ -435,7 +445,7 @@ func TestProfileStats(t *testing.T) { _, err := userService.CreateUser(ctx, users.CreateUserRequest{ DID: testDID, Handle: fmt.Sprintf("statsuser%d.test", uniqueSuffix), - PDSURL: "http://localhost:3001", + PDSURL: testPDSURL(), }) if err != nil { t.Fatalf("Failed to create test user: %v", err) @@ -569,7 +579,7 @@ func TestProfileStats_CommentCount(t *testing.T) { userRepo := postgres.NewUserRepository(db) resolver := identity.NewResolver(db, identity.DefaultConfig()) - userService := users.NewUserService(userRepo, resolver, "http://localhost:3001", nil, "") + userService := users.NewUserService(userRepo, resolver, testPDSURL(), nil, "") ctx := context.Background() @@ -577,7 +587,7 @@ func TestProfileStats_CommentCount(t *testing.T) { _, err := userService.CreateUser(ctx, users.CreateUserRequest{ DID: testDID, Handle: fmt.Sprintf("commentuser%d.test", uniqueSuffix), - PDSURL: "http://localhost:3001", + PDSURL: testPDSURL(), }) if err != nil { t.Fatalf("Failed to create test user: %v", err) @@ -660,7 +670,7 @@ func TestProfileStats_CommunityCount(t *testing.T) { userRepo := postgres.NewUserRepository(db) resolver := identity.NewResolver(db, identity.DefaultConfig()) - userService := users.NewUserService(userRepo, resolver, "http://localhost:3001", nil, "") + userService := users.NewUserService(userRepo, resolver, testPDSURL(), nil, "") ctx := context.Background() @@ -668,7 +678,7 @@ func TestProfileStats_CommunityCount(t *testing.T) { _, err := userService.CreateUser(ctx, users.CreateUserRequest{ DID: testDID, Handle: fmt.Sprintf("subuser%d.test", uniqueSuffix), - PDSURL: "http://localhost:3001", + PDSURL: testPDSURL(), }) if err != nil { t.Fatalf("Failed to create test user: %v", err) @@ -717,7 +727,7 @@ func TestGetProfile_NonExistentDID(t *testing.T) { userRepo := postgres.NewUserRepository(db) resolver := identity.NewResolver(db, identity.DefaultConfig()) - userService := users.NewUserService(userRepo, resolver, "http://localhost:3001", nil, "") + userService := users.NewUserService(userRepo, resolver, testPDSURL(), nil, "") ctx := context.Background() @@ -765,7 +775,7 @@ func TestProfileStatsEndpoint(t *testing.T) { // Wire up dependencies userRepo := postgres.NewUserRepository(db) resolver := identity.NewResolver(db, identity.DefaultConfig()) - userService := users.NewUserService(userRepo, resolver, "http://localhost:3001", nil, "") + userService := users.NewUserService(userRepo, resolver, testPDSURL(), nil, "") // Create test user testDID := "did:plc:endpointstats123" @@ -773,7 +783,7 @@ func TestProfileStatsEndpoint(t *testing.T) { _, err := userService.CreateUser(ctx, users.CreateUserRequest{ DID: testDID, Handle: "endpointstats.test", - PDSURL: "http://localhost:3001", + PDSURL: testPDSURL(), }) if err != nil { t.Fatalf("Failed to create test user: %v", err) @@ -857,7 +867,7 @@ func TestHandleValidation(t *testing.T) { userRepo := postgres.NewUserRepository(db) resolver := identity.NewResolver(db, identity.DefaultConfig()) - userService := users.NewUserService(userRepo, resolver, "http://localhost:3001", nil, "") + userService := users.NewUserService(userRepo, resolver, testPDSURL(), nil, "") ctx := context.Background() testCases := []struct { @@ -872,21 +882,21 @@ func TestHandleValidation(t *testing.T) { name: "Valid handle with hyphen", did: "did:plc:valid1", handle: "alice-bob.test", - pdsURL: "http://localhost:3001", + pdsURL: testPDSURL(), shouldError: false, }, { name: "Valid handle with dots", did: "did:plc:valid2", handle: "alice.bob.test", - pdsURL: "http://localhost:3001", + pdsURL: testPDSURL(), shouldError: false, }, { name: "Invalid: no dot (not domain-like)", did: "did:plc:invalid8", handle: "alice", - pdsURL: "http://localhost:3001", + pdsURL: testPDSURL(), shouldError: true, errorMsg: "invalid handle", }, @@ -894,14 +904,14 @@ func TestHandleValidation(t *testing.T) { name: "Valid: consecutive hyphens (allowed per atProto spec)", did: "did:plc:valid3", handle: "alice--bob.test", - pdsURL: "http://localhost:3001", + pdsURL: testPDSURL(), shouldError: false, }, { name: "Invalid: starts with hyphen", did: "did:plc:invalid2", handle: "-alice.test", - pdsURL: "http://localhost:3001", + pdsURL: testPDSURL(), shouldError: true, errorMsg: "invalid handle", }, @@ -909,7 +919,7 @@ func TestHandleValidation(t *testing.T) { name: "Invalid: ends with hyphen", did: "did:plc:invalid3", handle: "alice-.test", - pdsURL: "http://localhost:3001", + pdsURL: testPDSURL(), shouldError: true, errorMsg: "invalid handle", }, @@ -917,7 +927,7 @@ func TestHandleValidation(t *testing.T) { name: "Invalid: special characters", did: "did:plc:invalid4", handle: "alice!bob.test", - pdsURL: "http://localhost:3001", + pdsURL: testPDSURL(), shouldError: true, errorMsg: "invalid handle", }, @@ -925,7 +935,7 @@ func TestHandleValidation(t *testing.T) { name: "Invalid: spaces", did: "did:plc:invalid5", handle: "alice bob.test", - pdsURL: "http://localhost:3001", + pdsURL: testPDSURL(), shouldError: true, errorMsg: "invalid handle", }, @@ -933,7 +943,7 @@ func TestHandleValidation(t *testing.T) { name: "Invalid: too long", did: "did:plc:invalid6", handle: strings.Repeat("a", 254) + ".test", - pdsURL: "http://localhost:3001", + pdsURL: testPDSURL(), shouldError: true, errorMsg: "invalid handle", }, @@ -941,7 +951,7 @@ func TestHandleValidation(t *testing.T) { name: "Invalid: missing DID prefix", did: "plc:invalid7", handle: "valid.test", - pdsURL: "http://localhost:3001", + pdsURL: testPDSURL(), shouldError: true, errorMsg: "must start with 'did:'", }, @@ -983,7 +993,7 @@ func TestAccountDeletion_Integration(t *testing.T) { // Wire up dependencies userRepo := postgres.NewUserRepository(db) resolver := identity.NewResolver(db, identity.DefaultConfig()) - userService := users.NewUserService(userRepo, resolver, "http://localhost:3001", nil, "") + userService := users.NewUserService(userRepo, resolver, testPDSURL(), nil, "") ctx := context.Background() @@ -991,7 +1001,7 @@ func TestAccountDeletion_Integration(t *testing.T) { _, err := userService.CreateUser(ctx, users.CreateUserRequest{ DID: testDID, Handle: testHandle, - PDSURL: "http://localhost:3001", + PDSURL: testPDSURL(), }) if err != nil { t.Fatalf("Failed to create test user: %v", err) diff --git a/internal/db/postgres/vote_repo_test.go b/internal/db/postgres/vote_repo_test.go index 26eaab7..81dedb1 100644 --- a/internal/db/postgres/vote_repo_test.go +++ b/internal/db/postgres/vote_repo_test.go @@ -22,7 +22,7 @@ func createTestUser(t *testing.T, db *sql.DB, handle, did string) { VALUES ($1, $2, $3, NOW()) ON CONFLICT (did) DO NOTHING ` - _, err := db.Exec(query, did, handle, "https://bsky.social") + _, err := db.Exec(query, did, handle, testkit.Endpoints().PDS.BaseURL) require.NoError(t, err, "Failed to create test user") } diff --git a/loop_state.md b/loop_state.md index cf93604..c714f1b 100644 --- a/loop_state.md +++ b/loop_state.md @@ -37,6 +37,8 @@ ALL work happens in that worktree — every worker agent must cd there first. Statuses: pending → in-progress → review → done (or blocked: ). Stop the loop when every task is done, or on any blocked task. +**LOOP COMPLETE 2026-07-31 — all 20 tasks done. 30 production defects found+filed. make ci green 4384/0. Branch worktree-test-refactor ready for merge review.** + ## Task table | # | Task | Phase | R | Status | Commit | Notes | @@ -60,7 +62,7 @@ Stop the loop when every task is done, or on any blocked task. | 17 | Unit-coverage debt A: communities, votes, identity (repo tests for their repos too) | 5 | S | done | (see git log) | +375 tests for +3s gate. Coverage (tagged): communities 64→79.6%, votes 33→95.7%, identity 58.5→95.8%, postgres 40→47.4%. TWO MORE DEFECTS (tally 12), mutation-proven: p2 vote-cache expiry REVIVAL (deadline dropped, map kept, SetVote extends → stale map republished for full TTL → vote attempt reported as WITHDRAWAL); p3 Search count-vs-page mismatch (ILIKE count vs trgm-filtered page). Also pinned the July-23 vote-cache-desync backlog issue end-to-end (finally has a test). Recording-fake discipline: reviews found 19 kills / 0 vacuous in 20 mutation samples. Polish batch caught a REAL DATA RACE in a test fake (-race-only). pagination×block-filter gap CLOSED (property holds, now asserted). make ci GREEN 3872/0 @5:21; audit 235 | | 18 | Unit-coverage debt B: routes, timeline, discover, communityFeeds + remaining untested repos | 5 | S | done | (see git log) | +510 tests (gate 4382/0). Routes 15.5→94.1% via the BEHAVIORAL CLASSIFIER (chi.Walk middleware chains + sentinel wrapping → mechanical 401 matrix + EXACT rate budgets for all 57 routes); timeline/discover/communityFeeds → 100% T0; postgres 47.4→83.1%. 148 mutations, 147 kills (1 proven-equivalent). SEVENTEEN new defects (tally 29; honest recount from claimed 18/30) — headline p1: hot-comment cursor %f truncation kills pagination on months-old threads (post feeds already solved it — port feed_repo_base shape); GetByURI post-half extends the task-12 leak issue; nanoseconds-as-seconds refresh window (year 116106). REPORT-VS-REALITY INCIDENT: worker claimed defects "filed" — both reviewers found ZERO issue files; batch forced 9 files + 32 pin↔issue wirings, verified by me directly. Arithmetic overstated 7-10x, corrected. Audit 235 | | 19 | Federation topology: 2nd PDS + hermetic relay in compose; promote post/comment/vote contracts to true federation-path | 5 ⛩ | S | done | (see git log) | REAL FEDERATION: pds2 (own host/vol/PLC) + BigSky relay + relay-postgres; Jetstream RE-POINTED through the relay (prod-shaped, one feed preserved so all task-15/16 windows survive). SPIKE TRUTH: federated comments/votes/blobs WORK (no author FK); federated AUTHORS structurally unindexable (users consumer default-denies untrusted hosts; documented limit, not fixed). 5 federation contracts; remote blob fetch un-fakeable (requireBlobAbsentLocally); DID-served-as-handle degradation asserted. NEW ORDERING RULE (both reviewers verified vs code): intra-repo order holds only within ONE consumer subscription (own socket+filter) — all existing bounds are same-collection/same-consumer, safe. Trust-config left UNSET (endorsed — trusting pds2 → handle.invalid UNIQUE squat, non-re-runnable). 2 prod defects reproduced hermetically + defect #30 filed (identity cache stores handle.invalid sentinel — upstream cause of both siblings). Review (Codex fair + Opus): 1 real bug — identity negative concluded early on eventsProcessed; FRESH FIXER rewrote it to a same-repo malformed-block dead-letter bound, MUTATION-PROVEN (old bound would have passed the trust mutation, new catches it). make ci GREEN 4389/0 @6:30 (+11s topology, +41s contracts); cold-cache egress-blocked green | -| 20 | Enforcement flip: audit counts → 0, warn → fail; docs (spec → CANONICAL, delete TESTING_SUMMARY.md + docs/E2E_TESTING.md, CLAUDE.md pointer) | 6 ⛩ | S | in-progress | | S not M — audit has real residue (sleeps/skips) + spec §3.4a/§3.7 refresh | +| 20 | Enforcement flip: audit→0 warn→fail; spec CANONICAL; delete stale docs; CLAUDE.md pointer | 6 ⛩ | S | done | (see git log) | LOOP COMPLETE. Audit HARD-GATED at true zero (0/0/0/0 + 21 host + 37 public-host exemptions, all reason-annotated; testkit no longer blanket-excluded — 2 marked wait.go sleeps); 3 ratchets confirmed hard + probed both ways (audit, contract-manifest -allow-pending=false 10/0, ci-report skip-inversion). Spec → CANONICAL, reconciled: worker self-caught 15 false claims, final review caught 6 MORE honesty gaps (Codex 5/6 vs Opus over-cert — testkit-sleep wording, T0 no-network overclaim, websocket tripwire only DefaultDialer, stale FEED_SYSTEM pointers, verbose crash bash 3.2, §6 incomplete) — fresh fixer closed all: appview sleep was a RACE (converted to probe-counter, stronger), tripwire broadened + 4-form planted-dial proven, §6 item 9 added. TESTING_SUMMARY.md + docs/E2E_TESTING.md deleted, pointers rewired. make ci GREEN ×2 4384/0 @~6:15. Two done-criteria honestly NOT met + documented: -p 1 stays (account/identity events bypass wantedCollections — measured), make ci 6min > phase-0 2.2min (all added-coverage). Suite ~106k LOC (plan predicted 78k — coverage exceeded estimate). Defect tally 30. ## Cross-iteration notes for future iterations (parent + workers append surprises, interface changes, deferred TODOs) diff --git a/scripts/ci-runner.sh b/scripts/ci-runner.sh index 26189d0..4ce665e 100755 --- a/scripts/ci-runner.sh +++ b/scripts/ci-runner.sh @@ -68,13 +68,18 @@ echo " ✓ -tags live" echo # --------------------------------------------------------------------------- -# 1d. Violation audit (advisory) +# 1d. Violation audit (hard gate) # --------------------------------------------------------------------------- -# docs/TEST_ARCHITECTURE.md §3.6.3. The suite is mid-migration and every count -# below is scheduled against a phase, so this reports and never judges — the -# `|| true` is belt-and-braces on top of the script's own exit 0. It becomes the -# hard lint gate in the final phase, when the counts are zero. -bash /src/scripts/test-audit.sh || true +# docs/TEST_ARCHITECTURE.md §3.6.3. This reported and never judged through +# phases 0-5, while every count was scheduled against a phase; phase 6 drove +# them to their floor and flipped it. The `|| true` that used to sit here is +# gone, and so is the script's own `exit 0` — a new sleep, skip, endpoint +# literal or public hostname fails the gate here, before anything is executed. +# +# Cheap and infrastructure-free, so it runs alongside the type-check rather than +# after the suite: a violation is knowable at second zero and there is no reason +# to learn about it twenty minutes later. +bash /src/scripts/test-audit.sh # --------------------------------------------------------------------------- # 1e. Test template database diff --git a/scripts/test-audit.sh b/scripts/test-audit.sh index 6d75d36..e2c3e71 100755 --- a/scripts/test-audit.sh +++ b/scripts/test-audit.sh @@ -1,36 +1,55 @@ #!/usr/bin/env bash -# Counts test-suite invariant violations. Progress meter first, lint gate second. +# Counts test-suite invariant violations. HARD GATE — a nonzero count fails. # # WHAT THIS IS FOR # # docs/TEST_ARCHITECTURE.md §3.6.3. Every category below is something the -# refactor is removing, and each has a phase that must drive it to zero. Running -# this after every change turns "will the final enforcement flip pass?" into a -# number you can watch instead of a hope. +# refactor removed, and this is what stops it coming back. It ran in warn mode +# through phases 0-5 as the migration's progress meter (911 violations at the +# phase-1 baseline); phase 6 flipped it to a failure, which is only honest +# because the counts reached their floor first. # -# WARN MODE. This always exits 0. It prints a table and nothing else, so it can -# sit in the CI pipeline from day one without failing builds for violations -# that are scheduled to be fixed three phases from now. The final phase flips it -# to a hard failure, at which point every count must already be zero. +# THE FLOOR IS ZERO — with exemptions that are declared, not assumed. # -# THESE ARE TRIPWIRES, NOT PROOFS. Every check here is a grep, and a grep is -# bypassable by construction — a URL built by concatenation, a bare IP literal, -# a sleep hidden behind a helper. They catch drift cheaply. The actual guarantee -# that tests never reach the public network is the egress-blocked CI network -# (docker-compose.ci.yml, `internal: true`). +# Every check here is a grep over text, and text has legitimate reasons to +# contain the thing being counted: a URL-parser test's inputs are Bluesky URLs, +# a config test's assertions are the production defaults it parses, and a test +# that proves a cache entry expires has to let the deadline pass. Those are +# exempted individually, in the source, with a reason — so each one is a +# decision someone made and can be found by grepping for the marker: +# +# someCall(x) // coves:allow-sleep: +# someCall(x) // coves:allow-host-literal: +# someCall(x) // coves:allow-public-host: +# +# and, where per-line markers would be noise because the whole file's subject +# matter IS the literal (the Bluesky URL parser; the config parser), a +# file-scope form declared once near the top of the file: +# +# // coves:allow-public-host-file: +# +# File-scope exemptions are printed on EVERY run with their reason and hit +# count, so a broad exemption cannot go quiet. Adding either kind is a visible +# edit in review; that is the whole enforcement model — the greps stop drift, +# the annotations record the exceptions, and neither pretends to be a proof. +# +# THESE ARE TRIPWIRES, NOT PROOFS. A grep is bypassable by construction — a URL +# built by concatenation, a bare IP literal, a sleep hidden behind a helper. +# They catch drift cheaply. The actual guarantee that tests never reach the +# public network is the egress-blocked CI network (docker-compose.ci.yml, +# `internal: true`). # # SCOPE. "test code" means every *_test.go file under cmd/, internal/ and # tests/, plus every .go file under tests/ — shared helpers that do not end in # _test.go (tests/testkit, tests/fixtures) are test code too, and historically -# some of the worst offenders lived in exactly those files. -# -# Whole-line comments are not counted. This tree explains itself at length, and -# a comment that mentions localhost:5434 while describing the stack is not a -# hardcoded endpoint. It is a deliberate undercount at the margin: a violation -# trailing a comment on the same line still counts, because the code is there. +# some of the worst offenders lived in exactly those files. Production sources +# are out of scope for every category except testing.Short(), which nothing in +# this tree may call: production code legitimately dials websockets and names +# public hosts, and auditing it here would mean exempting the AppView from a +# rule written for its tests. # # Usage: -# scripts/test-audit.sh summary table +# scripts/test-audit.sh summary table; exits nonzero on any violation # scripts/test-audit.sh -v table plus every offending file:line set -uo pipefail @@ -45,6 +64,7 @@ fi CYAN='\033[36m' YELLOW='\033[33m' GREEN='\033[32m' +RED='\033[31m' RESET='\033[0m' # --------------------------------------------------------------------------- @@ -69,17 +89,23 @@ all_go_files() { TOTAL=0 declare -a ROWS=() declare -a DETAILS=() +declare -a EXEMPT_FILES=() -# scan