Memory Wiki

I'll start by reading all four build briefs, the Module 4 PB, the past faults log, and exploring the actual code base.

Now let me examine the actual codebase to verify the claims in the briefs.

Let me examine the key source files to verify the claims about FAULT fixes and imports.

Now let me check the MailQueue module code.

Now let me check EmailTemplates and EmailAdmin code.

Let me also verify the test files and the exception files to complete the review.

Let me do a final check confirming the missing files don't exist and check the test method counts in key files.

Let me check the duplicate exception files' namespaces and count remaining test methods across all modules.

Now I have all the data needed for a comprehensive review. Let me compile the final verdict. **Actual test counts:** - T21: 21 tests (exactly as claimed) - T22: 17 tests (exactly as claimed) - T23: 27 tests (exactly as claimed) - T24: 18 tests (exactly as claimed) **Exception file duplication confirmed** in all 4 modules. **WpMailOverride.php** — not found (confirmed). **V002__SeedEmailTemplates.php** — not found (confirmed). **V002__seed_email_templates.sql** — not found (confirmed). Here is my complete review: --- ### T21: REVISE (minor) **FAULT-013 (Webhook signature):** Correctly identifies the security gap. The current `BrevoServiceModule::handleBrevoWebhook()` (line 180) calls `$service->handleWebhook($payload)` without any signature check. The proposed fix — extract `brevo-signature` header, load `brevo.webhook_secret` from ConfigManager, call `BrevoApiClient::verifyWebhookSignature()` — is the right approach. `verifyWebhookSignature()` already exists and uses `hash_hmac('sha256', ...)` correctly. **APPROVE approach.** **FAULT-017 (Duplicate exception):** Both files confirmed: - `src/BrevoService/BrevoServiceException.php` (namespace `Profe\BrevoService`) - `src/BrevoService/Exception/BrevoServiceException.php` (namespace `Profe\BrevoService\Exception`) Fix approach (delete top-level, keep `Exception/` subdir) is correct. **HOWEVER**, the plan says "Update imports in BrevoService.php (line 17) and BrevoApiClient.php (line 16)" — these files ALREADY import from `Profe\BrevoService\Exception\BrevoServiceException`. No import updates are needed; only the file deletion is required. This overstates the work needed. **Interface contract:** Matches actual code exactly. All methods present: `sendTransactionalEmail`, `sendCampaignEmail`, `getAccountInfo`, `isAvailable`, `getContact`, `syncContact`, `createList`, `handleWebhook`, `isDuplicateEvent`. **PASS.** **Dependencies:** Plan lists `DbMigrationInterface` and `CsrfMiddlewareInterface` as dependencies; actual `getDependencies()` omits them (accessed via global container). This is consistent with the codebase pattern. **PASS.** **Test count:** 21 actual tests + 4 new = 25. Meets FAULT-016 minimum (≥12). **PASS.** **FAULT-011 audit:** Included in build steps. **PASS.** **One found issue:** `BrevoApiClient.php` line 22 declares `private string $cacheTtl;` but the constructor parameter is `int $cacheTtl`. With `declare(strict_types=1)` this will fail at runtime. This is an existing code bug, not introduced by the plan, but the plan could flag it. --- ### T22: APPROVE **FAULT-014 (wp_mail override):** Correctly identifies the gap. `WpMailOverride.php` does not exist (confirmed). The proposed `phpmailer_init` hook approach is correct per FAULT-014 prevention rules — cleaner than pluggable function override. The implementation code handles: - `clearAllRecipients()` to prevent dual-send - Config disable as fallback - Graceful exception handling with fallback to default WordPress behavior - Registration via `static::register()` in `boot()` **APPROVE approach and implementation code.** **FAULT-017 (Duplicate exception):** Both files confirmed at `src/MailQueue/MailQueueException.php` and `src/MailQueue/Exception/MailQueueException.php`. Fix approach correct. Same minor note as T21 about imports already pointing to `Exception/` subdir. **Interface contract:** Actual `MailQueueInterface` matches plan exactly. Methods: `enqueue`, `enqueueToEmail`, `processQueue`, `getStatus`, `cancel`, `getQueueStats`. **PASS.** **Dependencies:** Plan's list matches actual `getDependencies()` (all 5 interfaces). The plan also lists `CsrfMiddlewareInterface` and `DbMigrationInterface` as used-by-the-module dependencies (accessed via global container) — accurate. **PASS.** **Test count:** 17 actual tests + 5 new = 22. Exceeds FAULT-016 minimum. **PASS.** **FAULT-011 audit:** Included in build steps. **PASS.** --- ### T23: APPROVE **FAULT-015 (Template seed data):** Correctly identifies the gap. `V002__SeedEmailTemplates.php`, `V002__seed_email_templates.sql` do not exist (confirmed). The plan proposes a PHP migration class that loads SQL, plus a separate SQL file with 27 INSERT statements. The 27 template IDs match PB Appendix B exactly (15 transactional + 5 campaign + 7 system). **APPROVE approach.** **FAULT-017 (Duplicate exception):** Both files confirmed at `src/EmailTemplates/EmailTemplateException.php` and `src/EmailTemplates/Exception/EmailTemplateException.php`. Fix approach correct. **Interface contract:** Actual `EmailTemplatesInterface` matches plan exactly. All 7 methods present: `render`, `getTemplateIds`, `getTemplate`, `saveTemplate`, `deleteTemplate`, `validateRequiredVars`, `getTemplatesByCategory`. **PASS.** **Dependencies:** Actual `getDependencies()` lists `MailQueueInterface`, `EventBusInterface`, `DiContainerInterface`. Plan additionally lists `CsrfMiddlewareInterface` and `DbMigrationInterface` which are used via global container. Accurate. **PASS.** **Test count:** 27 actual tests + 4 new = 31. Exceeds FAULT-016 minimum. **PASS.** **FAULT-011 audit:** Included in build steps. **PASS.** --- ### T24: APPROVE **FAULT-017 (Duplicate exception):** Both files confirmed at `src/EmailAdmin/EmailAdminException.php` and `src/EmailAdmin/Exception/EmailAdminException.php`. Fix approach correct. **FAULT-002 workaround documentation:** The plan correctly includes a step to verify FAULT-002 workaround comments exist in `EmailAdminRepository.php` for direct `profe_users` JOIN queries. The actual `EmailAdminService` delegates to the repository; the plan includes a verification test. **APPROVE.** **Interface contract:** Actual `EmailAdminInterface` matches plan. All 6 methods present: `listEmailLog`, `getEmailDetail`, `sendTestEmail`, `previewTemplate`, `getEmailDashboardStats`, `listWebhookEvents`. **PASS.** **Dependencies:** Actual `getDependencies()` lists 7 interfaces including `CsrfMiddlewareInterface` — matches plan exactly. This is the only Module 4 module where `CsrfMiddlewareInterface` is properly declared in `getDependencies()`. **PASS.** **Test count:** 18 actual tests + 2 new = 20. Exceeds FAULT-016 minimum. **PASS.** **FAULT-011 audit:** Included in build steps. **PASS.** --- ### Cross-cutting Issues (applies to all 4 plans): 1. **FAULT-017 import updates overstated:** All 4 modules already use the correct `Exception/` subdir imports in their source files. The build plans say "Update imports" but no import changes are actually needed — only the top-level duplicate files need deletion. This is a minor inaccuracy in each plan but does not affect correctness. 2. **`DbMigrationInterface` and `CsrfMiddlewareInterface` in dependency tables:** The plans declare these as dependencies, but in T21/T22/T23 they are not in the actual `getDependencies()` array (accessed via `global $profeContainer`). This is consistent with how other modules in the codebase handle cross-cutting concerns. Not a blocker. 3. **Backwards compatibility:** All plans explicitly state they are adding features (wp_mail override, seed migration, signature verification) and fixing structural issues (exception consolidation) — no breaking changes to existing interfaces. **PASS.** --- ### VVerdict: REVISE (T21 only, minor), APPROVE (T22, T23, T24)