# Peer Review Guide — Harden Event Backbone
**Branch:** `feature/epic2-event-backbone-db`
**Ticket:** Harden Event Backbone: Correct Schema, Audit Log, Pub/Sub Topology, and Domain Model Placement
**LOE:** 13 (XL) | **Risk:** HIGH

This guide gives a second reviewer the context, rationale, and specific checkpoints needed to perform a meaningful peer review. Read each section in order before opening the diff.

---

## 1. Why This Branch Exists

Epic 2 established the Event Backbone but left multiple high-risk gaps vs. the V10 specification:

| Gap | Risk |
|-----|------|
| `events` table used dual-key design (`id` auto-increment + `event_id` UUID) and wrong column names | Breaks deduplication; no safe primary key for publishers |
| `event_audit_log` table missing entirely | Fingerprint-based deduplication (V10 §M7) impossible |
| Pub/Sub topics had no environment suffix; `events.error` topic missing; only 1 of 10 subscriptions provisioned | Sandbox and production traffic mix; unrecoverable failures route nowhere |
| `EventStore` model lived in `app/Models/` (Laravel default, not DDD path); no immutability guard | DDD namespace violation; concurrent processes could corrupt event records |

**Every gap has a cascading effect.** The schema drives the model; the model drives publishers; publishers drive topic naming; topic naming drives subscriptions. Nothing downstream of Epic 2 is safe until all four layers are correct.

---

## 2. What Changed — File-by-File Map

```
config/
  database.php                                         ← event_store connection + clarifying comment
  services.php                                         ← gcp.pubsub_topic_raw key (no more raw env())

database/migrations/
  2026_03_13_154524_create_event_store_table.php       ← original migration (untouched, context only)
  2026_03_13_200000_replace_event_store_with_events_table.php  ← V10 schema replacement
  2026_03_13_200001_create_event_audit_log_table.php   ← new audit log table

src/Domain/Events/Models/
  Event.php                                            ← new DDD model (replaces app/Models/EventStore.php)
  EventAuditLog.php                                    ← new DDD model (net-new)

src/App/Console/Commands/
  TestEventBackboneStress.php                          ← stress test + verification command

scripts/
  provision_gcp_test_env.sh                            ← V10 Pub/Sub topology (8 topics, 10 subs)

phpunit.xml                                            ← event_store SQLite config for tests
tests/Unit/Domain/Events/Models/
  EventStoreTest.php                                   ← 9 unit tests covering both models

docs/ai-rules/
  103-console-commands.md                              ← new rule: dynamic step denominators
  600-eloquent-safe-patterns.md                        ← new Rule 9: TIMESTAMP(6) precision contract

docs/refactoring/
  harden-event-backbone/progress-tracker.md            ← full acceptance criteria log
  epic2-event-backbone/progress-tracker.md             ← bug fix entries appended
```

**Deleted:** `app/Models/EventStore.php` (original non-DDD model — should NOT appear in diff).

---

## 3. Review Checklist by Area

Work through each area in sequence. Each section lists what to look for and what constitutes a pass or fail.

---

### 3.1 Schema Migrations

**Files:**
- `database/migrations/2026_03_13_200000_replace_event_store_with_events_table.php`
- `database/migrations/2026_03_13_200001_create_event_audit_log_table.php`

**What to verify:**

#### `events` table
- [ ] Primary key is `$table->char('id', 36)->primary()` — **no auto-increment `id()` column, no separate `event_id` column**.
- [ ] Column names match V10: `record_type`, `record_id`, `source`, `timestamp`, `base_version`, `changes`, `full_snapshot_ref`, `processed`. **No `aggregate_type`, `aggregate_id`, or `payload` column.**
- [ ] `timestamp` and `created_at` use `dateTimeTz(..., 6)` — TIMESTAMP(6) with microsecond precision.
- [ ] `created_at` uses `->useCurrent()` — set by MySQL default, not Eloquent.
- [ ] `changes` is `json()` not nullable — `NOT NULL` is the V10 contract.
- [ ] `processed` is `tinyInteger()->default(0)` — maps to Laravel boolean cast correctly.
- [ ] Exactly 3 named indexes: `idx_events_aggregate`, `idx_events_unprocessed`, `idx_events_source_time`.
- [ ] `up()` calls `dropIfExists('event_store')` before creating `events` — correct ordering.
- [ ] `down()` has a `DESTRUCTIVE ONE-WAY MIGRATION` comment explaining that `event_store` is NOT recreated and why.

#### `event_audit_log` table
- [ ] `fingerprint` is `char(64)` — SHA-256 is 64 hex chars. **Not `varchar`, not `text`.**
- [ ] `processing_outcome` is nullable — outcomes are only known after processing completes.
- [ ] No FK constraint on `event_id` — intentional. Migration comment must explain why (parallel inserts, application-layer referential integrity).
- [ ] Exactly 3 named indexes: `idx_audit_fingerprint`, `idx_audit_record`, `idx_audit_event`.
- [ ] `down()` correctly drops `event_audit_log` only — reversible.

**Red flags:**
- Any `$table->id()` call in the events migration.
- `nullable()` on `changes` JSON column.
- Missing index names (anonymous indexes break idempotent re-runs).

---

### 3.2 DDD Domain Models

**Files:**
- `src/Domain/Events/Models/Event.php`
- `src/Domain/Events/Models/EventAuditLog.php`

**What to verify:**

#### `Event.php`
- [ ] Namespace is `Domain\Events\Models` — **not** `App\Models`.
- [ ] `protected $connection = 'event_store'` — isolated from `tenant_connection`.
- [ ] `protected $keyType = 'string'` and `public $incrementing = false` — UUID key.
- [ ] `const UPDATED_AT = null` — append-only store, no update tracking.
- [ ] `protected $dateFormat = 'Y-m-d H:i:s.u'` — **this is the microsecond precision fix**. Without this, Laravel's default `Y-m-d H:i:s` silently truncates all TIMESTAMP(6) writes to `.000000`. Verify it is present.
- [ ] `'timestamp' => 'datetime:Y-m-d H:i:s.u'` in `$casts` — explicit format argument; belt-and-suspenders alongside `$dateFormat`.
- [ ] `boot()` contains both `updating` and `deleting` guards that throw `RuntimeException` with the exact messages specified in the ticket acceptance criteria.
- [ ] `$fillable` does NOT include `created_at` — MySQL sets it via DEFAULT.

#### `EventAuditLog.php`
- [ ] `public $timestamps = false` — Laravel must NOT write timestamps; MySQL DEFAULT CURRENT_TIMESTAMP(6) handles `created_at`.
- [ ] `'created_at' => 'datetime:Y-m-d H:i:s.u'` in `$casts` — read-only; format argument ensures microseconds are not truncated on hydration.
- [ ] `event()` relationship uses `belongsTo(Event::class, 'event_id', 'id')` — soft FK, no DB constraint.
- [ ] Same `boot()` immutability guards as `Event.php`.

**Red flags:**
- `$dateFormat` missing from `Event.php` — this is the silent data loss bug.
- `'timestamp' => 'datetime'` without the format argument — incomplete.
- `public $timestamps = true` (default) on `EventAuditLog` — Eloquent would try to write `updated_at` which doesn't exist.
- `protected $connection = 'mysql'` or no connection declared.

---

### 3.3 GCP Provisioning Script

**File:** `scripts/provision_gcp_test_env.sh`

**What to verify:**

#### Bash syntax (two fixed bugs)
- [ ] Lines 33–34 (`echo` header with inline `[[ ]]`): condition is `[[ "$ENV" == "production" ]]` only — **not** `"production" || "sandbox"`. Verify there is a space before `]]`.
- [ ] Line 177 (Cloud SQL settings guard): same — `[[ "$ENV" == "production" ]]` only, space before `]]`.
- [ ] Run `bash -n scripts/provision_gcp_test_env.sh` mentally or literally — must exit 0.

#### Environment logic
- [ ] `sandbox` gets: `AVAILABILITY_TYPE=ZONAL`, `NETWORK_FLAGS=--assign-ip` (public IP), `BACKUP_FLAGS=--backup` (no PITR).
- [ ] `production` gets: `AVAILABILITY_TYPE=REGIONAL`, `NETWORK_FLAGS=--no-assign-ip --network=...` (private IP, requires VPC peering), `BACKUP_FLAGS=--backup --enable-bin-log` (PITR).
- [ ] The header `echo` lines on 33–34 correctly reflect `PUBLIC/ZONAL` for sandbox and `PRIVATE/REGIONAL (HA)` for production.

#### V10 Pub/Sub topology
- [ ] Exactly 4 `create_topic` calls, each using `${ENV}` suffix: `events.raw.${ENV}`, `events.merged.${ENV}`, `events.error.${ENV}`, `events.dlq.${ENV}`.
- [ ] Exactly 5 `create_subscription` calls per environment:
  - `events.raw.${ENV}-merge-sub` → `events.raw.${ENV}`
  - `events.merged.${ENV}-netsuite-sub` → `events.merged.${ENV}`
  - `events.merged.${ENV}-suitex-sub` → `events.merged.${ENV}`
  - `events.error.${ENV}-retry-sub` → `events.error.${ENV}` with `--dead-letter-topic=events.dlq.${ENV} --max-delivery-attempts=5 --min-retry-delay=10s --max-retry-delay=600s`
  - `events.dlq.${ENV}-sub` → `events.dlq.${ENV}` (terminal — no retry flags)
- [ ] **No hardcoded environment strings** in topic names — all use `${ENV}`.
- [ ] `sync_worker` DB user grant is documented (commented SQL block, not executed inline) — correct, since it must run against the specific Cloud SQL instance post-migration.

**Red flags:**
- `"sandbox"&&` or `"sandbox"]]` without a space — bash metacharacter spacing bug.
- `"production" || "sandbox"` in the Cloud SQL settings guard — tautology that forces sandbox into HA mode and VPC peering requirement.
- Any hardcoded `events.raw.sandbox` or `events.raw.production` topic name.
- Missing `events.error.${ENV}` topic — was absent in the original script.

---

### 3.4 Stress Test & Verification Command

**File:** `src/App/Console/Commands/TestEventBackboneStress.php`

**What to verify:**

- [ ] `$hasFailures = false` flag is initialized after the early `FAILURE` return for static isolation check.
- [ ] All three conditions set `$hasFailures = true`: insertion errors (`$errors > 0`), isolation breach (`$tenantQueriesDelta > 0`), and PubSub failure (`$exitCode !== 0`).
- [ ] Return statement is `return $hasFailures ? self::FAILURE : self::SUCCESS` — fail-closed.
- [ ] `DB::connection('tenant_connection')->flushQueryLog()` is called immediately after `enableQueryLog()` — guarantees clean baseline.
- [ ] `$tenantQueriesDelta = count(DB::connection('tenant_connection')->getQueryLog())` — no subtraction from a `$tenantQueriesBefore` variable (was dead code — always zero).
- [ ] PubSub check uses `new Process([...])` with an array argument — **no** `exec()` with string interpolation.
- [ ] `$topic = config('services.gcp.pubsub_topic_raw')` — **no** `env('GCP_PUBSUB_TOPIC_RAW')` which breaks after `config:cache`.
- [ ] `$totalSteps = $this->option('skip-pubsub') ? 3 : 4` declared before step labels — all labels use `/{$totalSteps}`.
- [ ] Comment on the PubSub section notes that this tests `gcloud` CLI auth, not the PHP SDK path.

---

### 3.5 Config Files

**Files:** `config/database.php`, `config/services.php`

- [ ] `config/database.php` has an `event_store` connection entry with `'driver' => env('DB_EVENT_STORE_CONNECTION', 'mysql')`. Verify the inline comment reads: *"Value must be a PDO driver name ('mysql', 'sqlite', 'pgsql'), NOT a Laravel connection name."*
- [ ] `config/services.php` has a `gcp` key: `'pubsub_topic_raw' => env('GCP_PUBSUB_TOPIC_RAW', 'events.raw.test')`.
- [ ] No `env()` calls inside `src/` PHP files for GCP config — all routed through `config('services.gcp.*')`.

---

### 3.6 Tests

**File:** `tests/Unit/Domain/Events/Models/EventStoreTest.php`

- [ ] `phpunit.xml` sets `DB_EVENT_STORE_CONNECTION=sqlite` and `DB_EVENT_STORE_DATABASE=:memory:` — tests run in-memory, no real MySQL needed.
- [ ] `beforeEach` recreates both `events` and `event_audit_log` schemas from scratch on SQLite — schemas must match the migration DDL exactly (column names, types, indexes).
- [ ] Tests cover: connection isolation, no `updated_at`, UUID non-incrementing key, create/retrieve round-trip, immutability guard for `save()`, immutability guard for `delete()`, `EventAuditLog` create/retrieve, `event()` relationship, and audit log immutability.
- [ ] All 9 tests are GREEN: `php artisan test tests/Unit/Domain/Events/Models/EventStoreTest.php`.
- [ ] **Gap to note for the reviewer:** Tests run on SQLite, which has no `TIMESTAMP(6)` type. The microsecond precision of `$dateFormat = 'Y-m-d H:i:s.u'` is not validated in the unit test suite — it would require a MySQL integration test. This is a known gap documented in the PR. The fix is structurally correct (verified by Laravel internals tracing), but microsecond round-trip cannot be asserted here.

---

### 3.7 AI Rules (Governance Documents)

**Files:** `docs/ai-rules/103-console-commands.md`, `docs/ai-rules/600-eloquent-safe-patterns.md`

These are not production code but affect how future AI agents and engineers approach the codebase.

- [ ] `103-console-commands.md` has a new section: **"Dynamic Step Denominators in Multi-Step CLI Commands"** with anti-pattern and correct pattern examples.
- [ ] `600-eloquent-safe-patterns.md` has a new **Rule 9: TIMESTAMP(6) Microsecond Precision Contract** covering: write path failure, read path failure, `->useCurrent()` false safety, `$timestamps = false` special case, and a three-item checklist.

---

## 4. Key Design Decisions to Challenge

These decisions were intentional. If you disagree, raise them — but understand the rationale before doing so.

### 4.1 No FK constraint on `event_id` in `event_audit_log`
**Decision:** `event_id` is indexed but has no `FOREIGN KEY` constraint referencing `events.id`.
**Rationale:** On an append-only event store, a FK constraint imposes INSERT ordering — the `events` row must exist before the `event_audit_log` row. In high-throughput or bulk-insert scenarios, this serializes what could be parallel writes. Referential integrity is guaranteed at the application layer: both rows are inserted in the same transaction.
**Challenge this if:** You believe the serialization cost is acceptable and want the DB to enforce integrity unconditionally.

### 4.2 `down()` of the replace migration does not recreate `event_store`
**Decision:** `down()` drops `events` but does not recreate the old `event_store` table.
**Rationale:** The old `event_store` table was non-V10-compliant. It never held production data (the provisioning script that created it was itself broken — see `provision_gcp_test_env.sh` bash syntax bugs). Recreating it in `down()` would restore a deliberately deprecated, incorrect schema, which would immediately break all V10 application code. The `down()` is documented with a `DESTRUCTIVE ONE-WAY MIGRATION` comment.
**Challenge this if:** You can demonstrate that `event_store` held real data that needs to be recovered.

### 4.3 `gcloud` CLI used for PubSub connectivity check instead of PHP SDK
**Decision:** The stress test command shells out to `gcloud pubsub topics publish` via `Symfony\Component\Process\Process`.
**Rationale:** The project has `google/cloud-storage` in `composer.json` but NOT `google/cloud-pubsub`. No PHP SDK is available. A code comment documents this gap and specifies when to migrate (when `google/cloud-pubsub` is added as a dependency).
**Challenge this if:** You want to add the SDK as part of this PR. Note: that would be a scope expansion.

### 4.4 `$dateFormat` at model level vs. per-cast format argument
**Decision:** `Event.php` uses both `protected $dateFormat = 'Y-m-d H:i:s.u'` AND `'timestamp' => 'datetime:Y-m-d H:i:s.u'` in `$casts`.
**Rationale:** `$dateFormat` fixes both the **write path** (`fromDateTime()`) and the **read path** (`asDateTime()`). The cast format argument alone only fixes the read path — it does NOT affect how Carbon objects are serialized to SQL strings on INSERT. Both are set to make the intent explicit and self-documenting.
**Challenge this if:** You believe the redundancy is confusing. The `$dateFormat` alone would suffice technically, but the explicit cast communicates intent to future maintainers.

---

## 5. What Is NOT in This Branch (Known Gaps)

These items are tracked as open in the acceptance criteria and require a GCP environment to validate:

| Item | Status | Where to Track |
|------|--------|---------------|
| `sync_worker` SELECT+INSERT-only grant validation | Requires Cloud SQL access | `progress-tracker.md` item ☐ |
| Pub/Sub subscription end-to-end test (5-nack DLQ routing) | Requires GCP Pub/Sub | Manual QA after deploy |
| Microsecond round-trip test on MySQL 8.x | SQLite in-memory doesn't support TIMESTAMP(6) | Future integration test |
| `google/cloud-pubsub` SDK migration for PubSub check | Out of scope, SDK not in composer.json | Technical debt comment in command |

---

## 6. How to Run the Tests Locally

```bash
# 1. Run the unit test suite (SQLite — no MySQL required)
php artisan test tests/Unit/Domain/Events/Models/EventStoreTest.php

# 2. Run PHPStan on all changed PHP files
./vendor/bin/phpstan analyse \
  src/Domain/Events/Models/Event.php \
  src/Domain/Events/Models/EventAuditLog.php \
  src/App/Console/Commands/TestEventBackboneStress.php \
  config/database.php \
  config/services.php \
  --level=5

# 3. Bash syntax check on provisioning script
bash -n scripts/provision_gcp_test_env.sh

# 4. Verify sandbox vs. production Cloud SQL flag logic (dry run)
ENV=sandbox bash -c '
  AVAILABILITY_TYPE="ZONAL"; NETWORK_FLAGS="--assign-ip"; BACKUP_FLAGS="--backup"
  if [[ "$ENV" == "production" ]]; then
    AVAILABILITY_TYPE="REGIONAL"; BACKUP_FLAGS="--backup --enable-bin-log"
    NETWORK_FLAGS="--no-assign-ip"
  fi
  echo "sandbox → AVAILABILITY=$AVAILABILITY_TYPE | NETWORK=$NETWORK_FLAGS"
'
```

---

## 7. Bug Fixes Included (Context for Reviewers)

This branch also contains bug fixes identified during implementation review. They are not part of the original ticket scope but were caught during the work and fixed in-place:

| Bug | Severity | Fix |
|-----|----------|-----|
| Stress command exit code ignored isolation breach — CI could pass on a real isolation failure | High | Unified `$hasFailures` flag covering all 3 failure conditions |
| `exec()` with interpolated topic string — shell injection vector | High | Replaced with `Symfony\Component\Process\Process` array args |
| `env('GCP_PUBSUB_TOPIC_RAW')` breaks after `config:cache` in production | Medium | Moved to `config/services.gcp.pubsub_topic_raw` |
| Query log baseline always zero — `flushQueryLog()` missing | Medium | Added `flushQueryLog()` immediately after `enableQueryLog()` |
| Step labels showed `[1/3]` with a 4th step — denominator inconsistent | Low | Dynamic `$totalSteps = option('skip-pubsub') ? 3 : 4` |
| Bash `[[ "sandbox"]]` — missing space before `]]` caused syntax error on every script invocation | High | Added required whitespace — `bash -n` now exits 0 |
| Cloud SQL always-HA: `"production"\|\|"sandbox"` tautology forced sandbox into REGIONAL+private IP, breaking fresh provisioning | High | Condition reduced to `"production"` only |
| `Event.php` missing `$dateFormat = 'Y-m-d H:i:s.u'` — all TIMESTAMP(6) writes stored `.000000` | High | Added `$dateFormat` and explicit cast format |
| `EventAuditLog.php` `created_at` cast used default format — microseconds truncated on read | Medium | Cast updated to `'datetime:Y-m-d H:i:s.u'` |

---

## 8. Acceptance Criteria Pass/Fail Summary

| AC Item | Verifiable Locally? | Status |
|---------|-------------------|--------|
| `events` table UUID PK, correct columns, 3 indexes | Yes — read migration DDL | ✅ |
| `event_audit_log` exists, fingerprint CHAR(64), 3 indexes | Yes — read migration DDL | ✅ |
| 8 Pub/Sub topics with env suffix | Yes — read provision script | ✅ |
| 10 subscriptions with correct routing | Yes — read provision script | ✅ |
| `error` subscription has DLQ backstop after 5 attempts | Yes — read provision script | ✅ |
| `Event` model at DDD path; old `EventStore.php` deleted | Yes — check file existence | ✅ |
| `EventAuditLog` model with UUID key + boot guards | Yes — read model file | ✅ |
| `->save()` throws RuntimeException | Yes — unit test covers this | ✅ |
| `->delete()` throws RuntimeException | Yes — unit test covers this | ✅ |
| `sync_worker` cannot UPDATE/DELETE | No — requires GCP Cloud SQL | ☐ |
| No hardcoded topic names | Yes — grep provision script | ✅ |
| orderingKey contract documented | Yes — model docblock + script output | ✅ |
| Negative test: sandbox cannot publish to production topic | Yes — config-derived naming enforced | ✅ |