Memory Wiki

Untitled Session

2026-05-24 08:51 UTC · 57 messages · cli

You2026-05-24 08:53 UTC
Code review T25, T28, T29.
Donna2026-05-24 08:53 UTC
I'll start by reading the past faults log and exploring the code structure for all three modules.
Tool2026-05-24 08:53 UTC
{"content": " 1|# Past Faults Log — Profe Core Modules\n 2|\n 3|> **Purpose:** Every bug, design miss, and edge case discovered during development is recorded here.\n 4|> Future modules (and future TBs) MUST read this log before writing code.\n 5|\n 6|---\n 7|\n 8|## ...
Tool2026-05-24 08:53 UTC
{"total_count": 11, "files": ["/root/profe-modules/crm_contacts/tests/Unit/CrmContactsServiceTest.php", "/root/profe-modules/crm_contacts/tests/Unit/CrmContactsModuleTest.php", "/root/profe-modules/crm_contacts/tests/Unit/CrmContactsRepositoryTest.php", "/root/profe-modules/crm_contacts/tests/Integr...
Tool2026-05-24 08:53 UTC
{"total_count": 12, "files": ["/root/profe-modules/crm_import_export/tests/Integration/CrmImportExportIntegrationTest.php", "/root/profe-modules/crm_import_export/tests/Unit/CsvParserTest.php", "/root/profe-modules/crm_import_export/tests/Unit/CsvWriterTest.php", "/root/profe-modules/crm_import_expo...
Tool2026-05-24 08:53 UTC
{"total_count": 10, "files": ["/root/profe-modules/crm_admin/tests/Unit/CrmAdminServiceTest.php", "/root/profe-modules/crm_admin/tests/Unit/CrmAdminRepositoryTest.php", "/root/profe-modules/crm_admin/tests/Integration/CrmAdminIntegrationTest.php", "/root/profe-modules/crm_admin/tests/Fixture/CrmAdmi...
Donna2026-05-24 08:53 UTC
Now let me read all the source files for each module.
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmContactsModule — ModuleInstanceInterface implementation for crm_contacts\n 4| *\n 5| * FAULT-004: PUT/POST endpoints use checkAdminAccessWithCsrf() calling requireValidNonce().\n 6| * FAULT-01: Factory closure captures $c, never stores ...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmContactsService — CRM Contact Service Implementation\n 4| *\n 5| * HC-03: Lazy initialization of profe_crm_contacts row on first getContact().\n 6| * HC-04: Notes respect max_note_length config.\n 7| * HC-05: Opt-in status validated...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmContactsInterface — CRM Contact Service Contract\n 4| *\n 5| * HC-01: profe_users is the primary contact record — profe_crm_contacts is companion only.\n 6| * HC-02: No duplicate identity columns — profe_crm_contacts has only user_uuid ...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmContactsRepository — Data access for profe_crm_contacts\n 4| *\n 5| * FAULT-002/007: Direct profe_users queries documented as workaround because\n 6| * CoreUsersInterface has no search() or list() method.\n 7| * FAULT-04: Bare table...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CreateCrmContactsMigration — Migration for profe_crm_contacts table\n 4| *\n 5| * FAULT-006: PHP migration class with up/down SQL.\n 6| * HC-08: Migration registered in boot().\n 7| *\n 8| * @package Profe\\CrmContacts\\Migration\n...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmContactsException — Typed exception for CRM Contact operations\n 4| *\n 5| * @package Profe\\CrmContacts\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmContacts;\n 11|\n 12|class CrmContacts...
Donna2026-05-24 08:53 UTC
Now let me read the test files for T25 and the other two modules' source/test files.
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmContactsServiceTest — Unit tests for CrmContactsService\n 4| *\n 5| * Covers: getContact (2), updateContact (1), addNote (1), getContactsByOptInStatus (1)\n 6| *\n 7| * @package Profe\\CrmContacts\\Tests\\Unit\n 8| */\n 9|\n...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmContactsModuleTest — Unit tests for CrmContactsModule\n 4| *\n 5| * @package Profe\\CrmContacts\\Tests\\Unit\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmContacts\\Tests\\Unit;\n 11|\n 12|...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmContactsRepositoryTest — Unit tests for CrmContactsRepository\n 4| *\n 5| * @package Profe\\CrmContacts\\Tests\\Unit\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmContacts\\Tests\\Unit;\n 11|\...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmContactsIntegrationTest — Integration tests for CrmContacts\n 4| *\n 5| * @package Profe\\CrmContacts\\Tests\\Integration\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmContacts\\Tests\\Integratio...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmContactsFixtures — Test fixtures for CrmContacts tests\n 4| *\n 5| * @package Profe\\CrmContacts\\Tests\\Fixture\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmContacts\\Tests\\Fixture;\n 11|\n...
Donna2026-05-24 08:53 UTC
Now let me read the T28 (crm_import_export) and T29 (crm_admin) files.
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmImportExportModule — ModuleInstanceInterface implementation for crm_import_export\n 4| *\n 5| * FAULT-004: POST endpoints use checkAdminAccessWithCsrf() calling requireValidNonce().\n 6| * FAULT-01: Factory closure captures $c, never st...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmImportExportService — CRM Import/Export Service Implementation\n 4| *\n 5| * HC-02: CSV import validates all input via InputSanitizerInterface.\n 6| * HC-03: Import is transactional per-row (partial success supported).\n 7| * HC-06:...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmImportExportInterface — CRM Import/Export Service Contract\n 4| *\n 5| * HC-01: No new tables — reads/writes to existing profe_* tables.\n 6| * HC-02: CSV import validates all input via InputSanitizerInterface.\n 7| * HC-04: Export ...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CsvParser — CSV parsing and validation for import\n 4| *\n 5| * HC-02: CSV import validates all input via InputSanitizerInterface.\n 6| * HC-05: Max file size enforced (configurable).\n 7| * HC-06: Required column: email.\n 8| * HC...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CsvWriter — CSV generation with BOM for Excel compatibility\n 4| *\n 5| * HC-04: Export includes BOM for Excel compatibility.\n 6| *\n 7| * @package Profe\\CrmImportExport\n 8| */\n 9|\n 10|declare(strict_types=1);\n 11|\...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmImportExportException — Typed exception for Import/Export operations\n 4| *\n 5| * @package Profe\\CrmImportExport\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmImportExport;\n 11|\n 12|cla...
Donna2026-05-24 08:53 UTC
Let me read the T28 test files and T29 source/test files.
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmImportExportServiceTest — Unit tests for CrmImportExportService\n 4| *\n 5| * @package Profe\\CrmImportExport\\Tests\\Unit\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmImportExport\\Tests\\Unit;...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmImportExportModuleTest — Unit tests for CrmImportExportModule\n 4| *\n 5| * @package Profe\\CrmImportExport\\Tests\\Unit\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmImportExport\\Tests\\Unit;\n...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CsvParserTest — Unit tests for CsvParser\n 4| *\n 5| * @package Profe\\CrmImportExport\\Tests\\Unit\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmImportExport\\Tests\\Unit;\n 11|\n 12|use PHPU...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CsvWriterTest — Unit tests for CsvWriter\n 4| *\n 5| * @package Profe\\CrmImportExport\\Tests\\Unit\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmImportExport\\Tests\\Unit;\n 11|\n 12|use PHPU...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmImportExportIntegrationTest — Integration tests for CrmImportExport\n 4| *\n 5| * @package Profe\\CrmImportExport\\Tests\\Integration\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmImportExport\\T...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmImportExportFixtures — Test fixtures for CrmImportExport tests\n 4| *\n 5| * @package Profe\\CrmImportExport\\Tests\\Fixture\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmImportExport\\Tests\\Fix...
Donna2026-05-24 08:53 UTC
Now let me read T29 (crm_admin) source and test files.
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmAdminModule — ModuleInstanceInterface implementation for crm_admin\n 4| *\n 5| * FAULT-004: PUT/DELETE/POST endpoints use checkAdminAccessWithCsrf() calling requireValidNonce().\n 6| * FAULT-01: Factory closure captures $c, never stores...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmAdminService — CRM Admin API Service Implementation\n 4| *\n 5| * HC-04: deleteContact does NOT physically delete profe_users row.\n 6| * HC-05: updateContact delegates to CoreUsersInterface/CrmContactsInterface.\n 7| * HC-06: Dashb...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmAdminInterface — CRM Admin API Service Contract\n 4| *\n 5| * HC-01: No new tables — reads/writes via existing CRM interfaces and direct queries.\n 6| * HC-03: listContacts supports sync_status filter.\n 7| * HC-04: deleteContact do...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmAdminRepository — Data access for CRM Admin views\n 4| *\n 5| * FAULT-002/007: Direct profe_users queries documented as workaround because\n 6| * CoreUsersInterface has no search() or list() method that supports JOINs,\n 7| * cross-...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmAdminException — Typed exception for CRM Admin operations\n 4| *\n 5| * @package Profe\\CrmAdmin\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmAdmin;\n 11|\n 12|class CrmAdminException exte...
Donna2026-05-24 08:53 UTC
Now let me read the T29 test files.
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmAdminServiceTest — Unit tests for CrmAdminService\n 4| *\n 5| * @package Profe\\CrmAdmin\\Tests\\Unit\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmAdmin\\Tests\\Unit;\n 11|\n 12|use PHPUni...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmAdminRepositoryTest — Unit tests for CrmAdminRepository\n 4| *\n 5| * @package Profe\\CrmAdmin\\Tests\\Unit\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmAdmin\\Tests\\Unit;\n 11|\n 12|use ...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmAdminModuleTest — Unit tests for CrmAdminModule\n 4| *\n 5| * @package Profe\\CrmAdmin\\Tests\\Unit\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmAdmin\\Tests\\Unit;\n 11|\n 12|use PHPUnit\...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmAdminIntegrationTest — Integration tests for CrmAdmin\n 4| *\n 5| * @package Profe\\CrmAdmin\\Tests\\Integration\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmAdmin\\Tests\\Integration;\n 11|\...
Tool2026-05-24 08:53 UTC
{"content": " 1|<?php\n 2|/**\n 3| * CrmAdminFixtures — Test fixtures for CrmAdmin tests\n 4| *\n 5| * @package Profe\\CrmAdmin\\Tests\\Fixture\n 6| */\n 7|\n 8|declare(strict_types=1);\n 9|\n 10|namespace Profe\\CrmAdmin\\Tests\\Fixture;\n 11|\n 12|class...
Donna2026-05-24 08:53 UTC
Now let me run PHP syntax checks on all files.
Tool2026-05-24 08:53 UTC
{"output": "", "exit_code": 1, "error": null, "exit_code_meaning": "No matches found (not an error)"}
Donna2026-05-24 08:53 UTC
All PHP files pass syntax check. Let me verify the total test counts precisely by running a quick grep.
Tool2026-05-24 08:53 UTC
{"output": "=== T25 CrmContacts ===\n/root/profe-modules/crm_contacts/tests/Unit/CrmContactsServiceTest.php:7\n/root/profe-modules/crm_contacts/tests/Unit/CrmContactsModuleTest.php:2\n/root/profe-modules/crm_contacts/tests/Unit/CrmContactsRepositoryTest.php:5\n/root/profe-modules/crm_contacts/tests/...
Donna2026-05-24 08:53 UTC
I've thoroughly reviewed all three modules. Let me check one more thing about the T25 dependency issue I noticed.
Tool2026-05-24 08:53 UTC
{"total_count": 9, "matches": [{"path": "/root/profe-modules/crm_contacts/src/CrmContacts/CrmContactsModule.php", "line": 19, "content": "use Profe\\ConfigManager\\ConfigManagerInterface;"}, {"path": "/root/profe-modules/crm_contacts/src/CrmContacts/CrmContactsModule.php", "line": 71, "content": " ...
Tool2026-05-24 08:53 UTC
{"total_count": 4, "matches": [{"path": "/root/profe-modules/crm_admin/src/CrmAdmin/Repository/CrmAdminRepository.php", "line": 71, "content": " $where[] = \"cc.`last_synced_at` IS NOT NULL\";"}, {"path": "/root/profe-modules/crm_admin/src/CrmAdmin/Repository/CrmAdminRepository.ph...
Donna2026-05-24 08:53 UTC
Now I have all the data I need. Let me write the review reports.
Tool2026-05-24 08:53 UTC
{"bytes_written": 4251, "dirs_created": true, "lint": {"status": "skipped", "message": "No linter for .md files"}}
Tool2026-05-24 08:53 UTC
{"bytes_written": 3152, "dirs_created": true, "lint": {"status": "skipped", "message": "No linter for .md files"}}
Tool2026-05-24 08:53 UTC
{"bytes_written": 3810, "dirs_created": true, "lint": {"status": "skipped", "message": "No linter for .md files"}}
Donna2026-05-24 08:53 UTC
## Summary ### What I did Code-reviewed three modules (T25 CrmContacts, T28 CrmImportExport, T29 CrmAdmin) against 7 criteria: 1. All 9 past faults fixed 2. CSRF enforced on mutation endpoints 3. ≥12 tests per module 4. Migration classes implement MigrationInterface 5. Direct queries documented 6. PHP syntax check passes ### Verdicts | Module | Verdict | Findings | |--------|---------|----------| | **T25 CrmContacts** | **CONDITIONAL** | Missing `ConfigManagerInterface` in `getDependencies()` (FAULT-003 regression) | | **T28 CrmImportExport** | **GREEN LIGHT** | All criteria met, clean implementation | | **T29 CrmAdmin** | **CONDITIONAL** | Bug: `listContacts()` ORDER BY uses `u.\`last_synced_at\`` but `last_synced_at` is on `cc` table — would cause MySQL error | ### Key Findings 1. **T25 (CrmContacts):** FAULT-003 regression — `ConfigManagerInterface` is injected into the service constructor and used for `max_note_length` config, but not declared in `getDependencies()`. The test `testConstructStoresAllDependencies()` confirms all 6 constructor params including ConfigManagerInterface. 2. **T29 (CrmAdmin):** Real functional bug in `CrmAdminRepository::listContacts()` — when sorting by `last_synced_at`, the query uses `u.\`last_synced_at\`` but this column exists on `cc` (profe_crm_contacts), not `u` (profe_users). Would throw MySQL "Unknown column" error. ### What passed - **CSRF:** All 3 modules enforce CSRF via `checkAdminAccessWithCsrf()` calling `requireValidNonce()` on every POST/PUT/DELETE endpoint ✓ - **Tests:** T25=18, T28=17, T29=18 — all well above the 12 minimum ✓ - **Migrations:** T25 has `CreateCrmContactsMigration` implementing `MigrationInterface`; T28/T29 correctly have none per spec ✓ - **Direct query docs:** All `profe_users` direct queries tagged with `// FAULT-002 workaround:` comments ✓ - **PHP syntax:** All 33 PHP files pass `php -l` ✓ ### Files created - `/root/.hermes/plans/review-ta-code-t25.md` - `/root/.hermes/plans/review-ta-code-t28.md` - `/root/.hermes/plans/review-ta-code-t29.md`