# Plan review corrections — binding addendum **Date:** 2026-08-06 **Status:** BINDING. Where this document conflicts with the design spec or any of the three plans, **this document wins**. Every implementer must read it before touching code. Produced by five independent pre-implementation reviewers (architecture/data-model, product/AI-logic, backend/security, frontend/UX, test/performance) whose findings were then verified against the repository by the orchestrator. Only verified findings appear here; each correction states the evidence that justified it. --- ## 0. Verified environment facts every implementer needs These were checked in the repository. Do not re-derive them, and do not assume otherwise. **0.1 — Feature flag semantics. CORRECTED 2026-08-06 after measuring it on the running app; the original text here was wrong and said the opposite.** Flags in this project are **presence-based**. Measured, not reasoned: ``` env('FEATURE_ZOOM_ACTION_PLAN_DEMO') → string(5) "false" (bool) that value → bool(true) ← the flag is ON env('ABSENT_VAR', false) → bool(false) ← off ``` `config/features.php` does `(bool) env('FEATURE_X', false)`, and PHP evaluates the non-empty string `"false"` as `true`. The repository is `Dotenv\Repository\AdapterRepository`, which hands back the raw string rather than coercing it. So: **an ABSENT variable is off. `FEATURE_X=false` turns the feature ON.** To disable a flag, delete the line or comment it out — never set it to `false`. "Committed default false" still holds, because `config/features.php` carries the literal `false` and no committed file sets these variables; a production host that simply does not define them is correctly off. The hazard is narrower and sharper than the original text: an operator who *thinks* they are disabling a demo by writing `=false` enables it instead. `.env` is git-ignored. In this workspace it is a SYMLINK to `~/Documents/GitHub/petav3/.env`, shared across several checkouts — editing it affects all of them, so restore anything you change. The original claim (that `Env::getOption()` maps `"false"` to boolean `false`) came from reading the framework source rather than running it. It is left recorded here because the mistake is instructive: on a production-safety fact, measure it. **0.2 — Flag/role gating goes in `prepareForValidation()`, never only the controller body.** Laravel validates a type-hinted `FormRequest` *before* the controller method runs, so a check placed only in the controller leaks the field contract through 422 validation errors while the flag is off. Copy the shipped pattern verbatim (`app/Http/Requests/Manage/Zoom/ActionPlans/UpdateRequest.php:24-28`): ```php protected function prepareForValidation(): void { abort_unless(config('features.'), 404); abort_unless($this->user()->isSuperAdmin(), 403); } public function authorize(): bool { return true; } ``` The controller repeats both aborts as defence in depth. This applies to **every** new request class: Plan A Tasks 6 and 8, Plan B Tasks 3 and 4, and any Plan C request class. **0.3 — Routes are registered unconditionally.** `routes/web.php` warns three times against wrapping a route in `if (config(...))`, because a cached route table bakes the flag in. Gate inside the controller/FormRequest only. **0.4 — `LeadVisibility::apply()` is a no-op for Super Admin.** `LEVEL_ALL => $query` returns the query untouched (`src/Auth/Support/LeadVisibility.php`). "Lead-visibility scoped" is therefore **not** a data-minimisation control for a Super Admin surface. Any claim that it is must be replaced by an explicit field-level decision. **0.5 — The prompt-injection control is `JSON_HEX_TAG`, and it is hand-copied per controller.** There is no shared trait. Every new AI endpoint must reproduce it: ```php "\n" . json_encode($snapshot, JSON_UNESCAPED_UNICODE | JSON_HEX_TAG) . "\n\n" ``` plus the untrusted-data caveat sentence. `JSON_HEX_TAG` escapes `<` and `>` so customer or reviewer text cannot forge the closing delimiter. A plan step saying only "treat as untrusted" is insufficient. Reference: `ActionPlanChatController::grounding()`. **0.6 — Frontend test tooling is Vitest + happy-dom. `@vue/test-utils` is NOT installed.** Component tests hand-roll a harness with `createApp`/`h`/`nextTick` from `vue` plus `querySelector`/`dispatchEvent`. The only precedent is `resources/js/Components/Sales/LeadCell.test.js`. Follow it; do not import a mounting library. **0.7 — The AiClient faking pattern** is an anonymous subclass overriding `chat()`, bound with `$this->app->instance(AiClient::class, $fake)`, exposing `$calls[]` so tests can assert on the **constructed prompt**. Reference: `tests/Feature/Manage/Zoom/ActionPlanChatTest.php:61-92`, and for injection specifically `test_step_and_analysis_text_cannot_forge_the_context_delimiter` (assert on `$this->ai->calls[0]['messages']`, never on the HTTP response). **0.8 — Scratch-database migration procedure.** The plans referred to a "project scratch-database procedure" that is not documented repo-wide. It is, canonically: ```bash mysql -uroot -e "CREATE DATABASE " herd php artisan config:clear DB_DATABASE= herd php artisan tinker --execute="echo DB::connection()->getDatabaseName();" # MUST print , else abort DB_DATABASE= herd php artisan migrate:fresh --force DB_DATABASE= herd php artisan migrate:rollback --step= --force # down() proof mysql -uroot -e "DROP DATABASE " ``` Scratch names for this work: `p3_action_plan_hardening_scratch` (Plan A), `p3_zoom_opportunity_scratch` (Plan B). **Never** target `petav3_testing` (PHPUnit's database) or the development database. All PHP runs through `herd php`. --- ## 1. P0 — `Booking::commissionAt()` is an instance method Design §9 and Plan B Task 2 Step 3 instruct `Booking::commissionAt($rate, $basis)`. Verified: `src/Engagement/Booking.php:227` declares `public function commissionAt($rate, ?int $basis = null): ?string` with **no** `static`. A static call fatals in PHP 8 with `Error: Non-static method ... cannot be called statically`, which would make `EngagementOpportunityValue::calculate()` — the load-bearing commission source for Plans B and C — unable to run at all. **Correction.** Call it on the resolved instance, and use `Engagement::booking()` (a `HasOne` via `latestOfMany()`), not `bookings()`: ```php $commission = $engagement->booking?->commissionAt($project->commission_rate, $project->commission_basis); ``` **Also (P2, same method):** it returns a 2-dp decimal **string** from `number_format()`, but the plan's own test asserts `assertSame(39000.0, ...)`. `"39000.00" !== 39000.0`. `EngagementOpportunityValue` must explicitly `(float)`-cast every value it reads from `commissionAt()`, `price_from` or any `decimal:` cast, matching the money-cast rule in GUIDELINES §14. ## 2. P0 — Plan C must use correlated subqueries, never a JOIN Plan C Task 2 Step 2 says "Join only the confirmed-link/Engagement/Project/Booking rows needed for value." Verified: `Engagement::bookings()` is `HasMany` (`src/Engagement/Engagement.php:202`), and GUIDELINES §14 states plainly that relational columns must use "correlated subqueries, never joins (joins on non-unique/soft-deletable relations inflate the paginator count)". The very file Plan C modifies already implements the correct pattern for `agent_name` in `RecordingsController` and documents why. **Correction.** Strike "Join". Derive the value/sort key with a correlated subquery scoped to the single latest booking (mirroring `Engagement::booking()`'s `latestOfMany()` semantics). Ordering still happens in SQL — the prohibition on sorting a paginated page in PHP stands unchanged. **Mandatory test:** a fixture with one Engagement carrying **two** Booking rows (one current, one historical/cancelled), asserting the meeting appears exactly once and the paginator total is not inflated. Without this fixture the defect is invisible. ## 3. P0 — The concurrency test has no teeth as specified Plan A Task 4 lists "simulated concurrency" as one word. Verified: the existing `ZoomActionPlanApprovalTest::test_unique_index_rejects_a_concurrent_style_duplicate` pre-inserts a colliding row and calls `approve()` once, single-threaded. It asserts the **unique index**, and would still pass with `lockForUpdate()` deleted. **Correction.** Plan A Task 4 must add a genuine lock-contention test: hold the row lock on a second database connection (`BEGIN; SELECT ... FOR UPDATE`), then attempt `approve()` on the first connection with `SET SESSION innodb_lock_wait_timeout = 1`, and assert a lock-wait-timeout `QueryException` (MySQL 1205). That fails if and only if the row lock is present. **Additionally:** add a real mid-loop write-failure test — insert a colliding `(source_action_plan_id, source_step_key)` out of band, run `approve()`, and assert **zero** `LeadActionItem` rows exist afterwards. The pre-validation test alone never reaches the write loop, so it cannot prove atomicity. ## 4. P0 — Five legacy `next_steps` consumers have no test coverage The design promises `meeting_report.next_steps` keeps working for "Calls, F2F, Zoom briefs, queues and chase commands". Verified: `SendCallBrief`, `SendF2fBrief`, `SendZoomMeetingBrief`, `SendZoomMeetingPrepBrief` and `ChaseFollowUps` all call `->actionItems()`, and a search of `tests/` for those class names returns **nothing**. Plan A Task 2 changes the normalizer these depend on, and its regression step runs suites that never touch them. **Correction.** Plan A Task 2 gains a step: add smoke coverage for these five consumers **before** changing the normalizer, asserting each still renders the legacy string steps. This is the only thing standing between the normalizer change and a silent production break in brief delivery. ## 5. P1 — Priority provenance: the AI must not pre-confirm its own suggestion Design §30 promises "a human confirms it" and §42 says the split exists so AI quality can be assessed. But Plan A Task 3 writes `suggested_priority` and `priority` from the *same* AI value, nothing forces a human to touch it, and the checklist then sorts by "confirmed priority". Two consequences, both real: a salesperson works an order they believe a colleague triaged, and any later "was the AI right?" query compares a value against itself and reports perfect agreement. **Correction — three parts, all required.** 1. **Record the act, not the value.** Add `priority_source` to the item contract and to `lead_action_items`: `PRIORITY_SOURCE_AI = 1` (still the AI's default) / `PRIORITY_SOURCE_HUMAN = 2` (a reviewer changed it). Set to human only when the editor actually patched `priority`. Never infer AI quality from `suggested_priority === priority`. 2. **Stop calling it confirmed in the UI.** The selector is labelled **"Priority"**; the badge beside it reads **"AI suggested: High"**. Nothing in the interface claims confirmation that did not happen. 3. Defaulting the selector from the suggestion remains correct and stays. ## 6. P1 — Evidence strings must not launder an AI inference into a record Design §288-290 emits `"Customer interest level was recorded as high"`. Verified: `interest_level` and `buying_stage` are model output from a transcript, and `ConversationAnalysis::enum()` silently substitutes `'none'`/`'unknown'` for anything unparseable — so "recorded as none" can mean "the model returned garbage". Sitting in the same list as `"Pipeline match was confirmed by an admin"` (a genuine human-action fact), the uniform wording lends real credibility to an inference. **Correction.** - `"AI read the customer's interest as High from the meeting"` - `"AI placed the customer at the Consideration stage"` - `"Pipeline match confirmed by on "` — unchanged; it is the one recorded fact, so let it be the only line phrased that way. - `interest_level === 'none'` and `buying_stage === 'unknown'` are **not stated**. They go in `missing` ("No interest level in the analysis"), never in `evidence`. - Plan C Task 1 gains a test asserting no `evidence` entry contains the word "recorded" for an AI-derived field. ## 7. P1 — `data_confidence` must stop wearing the intent badge's clothes Design §43 already decided that "AI priority, opportunity intent, AI confidence and conversion probability … must not share one field or badge." Design §10 and Plan C Task 3 then put `intent: high` and `data_confidence: high` in one payload and one badge row, inheriting the existing green/amber/slate interest ramp. Two honest fields in one uniform assemble the forbidden claim in the reader's head, and Plan C's single disclaimer sits only on the Recordings page while the badges also ship to the Dashboard card and checklist headers. **Correction.** 1. Rename the payload key `data_confidence` → **`data_completeness`** everywhere. 2. Render it as a count with its parts — `2 of 3 · Analysis ✓ · Pipeline match ✓ · Value —` — never as a High/Medium/Low tone badge. Column header: **"Data completeness"**. 3. Label the intent column **"Intent (AI-read)"**. 4. The disclaimer belongs to the signals **component**, not one page, so it ships wherever the block ships: *"Recorded signals to help you choose what to work on. Intent and stage are the AI's reading of the conversation. This is not a prediction that the customer will buy."* 5. Plan B's `suggested_confidence` is the model's self-rating about its own guess; render it as **"AI's own certainty: High (self-rated, not measured)"** or as match words (Strong/Possible/Weak match), never as a bare system-voiced confidence badge. 6. Plan C Task 5 Step 1 gains a grep for `Confidence` appearing in any Vue file that also renders `intent`. ## 8. P1 — Canonical naming: `intent`, not `interest` Design §281 says `"intent"` while §303 says `"interest"` for the same value. Use **`intent`** throughout the payload, SQL `CASE` and Vue; name `customer.interest_level` only when referring to the source field in the analysis. ## 9. P1 — "Apply to all" is unimplementable without an override marker Every row is seeded with an assignee, so the client cannot distinguish "still on the inherited default" from "deliberately set to that same person". "Changing the default alone does not silently overwrite row overrides" (§173) therefore has no mechanism. **Correction.** Add `assignee_overridden` (boolean) to the editor step shape, the HTTP item contract and the stored JSON item. It flips to true the moment a row's assignee select is changed by hand. Defined behaviour, stated once and not left to the implementer: - Changing the **default selector** alone updates only rows where `assignee_overridden === false`. - Pressing **Apply to all** is the explicit escape hatch: it overwrites **every** row, including overridden ones, and resets all `assignee_overridden` to false. The button must say so ("Assign every step to this person"). ## 10. P1 — Do not silently change LeadCell's existing WhatsApp behaviour Plan A Task 9 says to make `LeadCell.vue` "consume the shared contact composable", and the design describes WhatsApp as opening `https://wa.me/{digits}`. Verified: `LeadCell.vue:29,72` today renders an Inertia `` to `/manage/messages?search={digits}` — the **internal Messages inbox** — with no confirm and no logging. The `wa.me` pattern lives elsewhere (`Pages/Manage/Messages/Inbox.vue`). Wiring LeadCell's icon to the new composable would silently switch every existing WhatsApp click in the project-leads and booking-list tables from the internal inbox to an external tab — a behaviour change to working, unrelated UI. **Correction.** The `LeadCell.vue` edit is **Call-only**. Extract the call-intent handoff; leave its WhatsApp `` exactly as it is. The new `wa.me` behaviour exists only on the new checklist task rows. ## 11. P1 — Contact affordance must not depend on an AI guess `action_type` is AI-authored, and Plan A Task 9 makes it decide whether the Call/WhatsApp buttons exist at all. A step reading "Call Mr Tan about the loan margin" typed `send_information` shows no Call button, the salesperson leaves the checklist to find the number, and the `call-clicks` intent log — which the whole Call History feature depends on — never fires. Every legacy row defaults to `TYPE_OTHER` and loses both buttons. **Correction.** Where the viewer may see the Lead's phone, **both** Call and WhatsApp are available on every task. `action_type` decides only which is the *primary* (emphasised) button. The type is never used to hide a contact action. "Do not guess from task prose" stands — the buttons come from the Lead having a visible phone, not from reading the body text. ## 12. P1 — Team Tasks must not carry phone numbers Plan A Task 9 claims phone is included "only after Lead visibility has been applied". Per fact 0.4 that gates nothing for a Super Admin, so Team Tasks — cursor-paginated, no rate limit by design decision — becomes a low-friction bulk phone export for every Lead in the pipeline. The current `DashboardController` carries no phone field at all, so this is a new exposure, not an inherited one. **Correction.** `DashboardController` builds **two distinct row shapes**. `phone` and the Call/WhatsApp actions exist only on **My Tasks**, where the viewer is the actual assignee. Team rows carry no phone. This is a deliberate scope decision, recorded here rather than inherited by reusing one modal component. ## 13. P1 — Specify the revision diff, the row layout, and the modal tab structure Three frontend contracts were under-specified to the point where an implementer must invent them. **13.1 Revision proposal alignment.** Align Current vs Proposed **by step `key`**, classifying each row `matched` / `changed` / `added` / `removed`. Proposals of different length or order are expected (a reviewer asking to "reorder by priority" or "add a follow-up" produces exactly that), so the preview must render added and removed rows explicitly rather than zipping two flat lists. **13.2 Row layout in the review modal.** The steps card is one of two or three grid columns and narrows further when chat is open, so six controls cannot sit on one line. Each step renders as a **stacked mini-card**: line 1 the body input; line 2 the "AI suggested: X" badge plus the Priority, Action type and Assignee selects, wrapping; line 3 the AI reason, truncated to one line and expandable. Approved plans stay read-only as today. **13.3 Checklist tabs.** For a Super Admin the outer axis is **My Tasks | Team Tasks**. *My Tasks* keeps the existing Current Tasks / Completed History toggle inside it. *Team Tasks* is a flat filtered list with no current/completed split, loaded only on first selection. A Sales user sees no outer axis at all — just today's Current Tasks / Completed History. ## 14. P1 — Reproduce the double gate on the shared RecordingDetail `RecordingDetail.vue` is shared by Zoom, Calls and F2F. Its existing Ask-AI tab gates on **both** the feature flag **and** `type === 'zoom'`. Plan B Task 4 states only the flag, leaving isolation to the accident that Call/F2F adapters never populate the key. **Correction.** Gate the opportunity block on the flag **and** `type === 'zoom'`, matching the `canChat` precedent. ## 15. P2 corrections adopted without further argument - **15.1** `zoom_meeting_opportunity_links` combines a unique `zoom_meeting_id` with soft delete. MySQL enforces the unique index across soft-deleted rows, so its repository follows the restore-under-lock pattern already used by `ZoomMeetingActionPlanRepository::replaceDraft()`. - **15.2** `config/features.php` is missing from Plan B's file map. Add it to Plan B Task 1 with the new `zoom_opportunity_demo` key. - **15.3** `tests/Feature/Manage/Zoom/RecordingsOpportunityColumnsTest.php` is **created** by Plan B Task 5 and **modified** (extended, never replaced) by Plan C Task 3. Plan C Task 3 also absorbs Plan B's value columns rather than adding parallel ones — one payload key, not `opportunity` and `opportunity_signals` side by side. - **15.4** A stale per-step assignee (deactivated or no longer Lead-visible) must render as "assignee no longer eligible" in the review UI, not as a silently empty selector. - **15.5** Add `reason_snapshot` to the feedback snapshot set. The reason is what persuaded the reviewer to approve the step, so a rating without it is missing its own context. - **15.6** Label the analysis hash honestly: **"Analysis fingerprint (change detection only — not a prompt version)"**. It cannot answer "which prompt produced this step"; it only detects that the normalized analysis changed, and Plan A Task 2's own normalizer edit will change every pre-existing digest. - **15.7** Plan C Task 4's non-Super visibility rule is `LeadVisibility::allows()` **AND** (meeting agent OR engagement assignment OR open action item). Visibility is the outer gate. - **15.8** Commit a query-count assertion using the repo's existing `DB::enableQueryLog()` / `assertLessThanOrEqual` pattern, rather than a manual EXPLAIN step. - **15.9** Tie-stability test shape: create ≥3 meetings with identical signals; fetch the same page twice and assert identical id sequences; then fetch page 1 + page 2 and assert the concatenation equals a single unpaginated fetch. - **15.10** Add explicit flag-off **prop-absence** assertions (not only endpoint 404s) for every new Dashboard/Recordings payload key, matching `test_flag_off_omits_the_checklist_prop_entirely`. - **15.11** Do not re-derive coverage that already exists and is green. "Unchanged notifier behaviour" and "approved snapshot immutability" are already asserted by `test_approve_never_invokes_the_notifier` and `test_approved_plan_and_generated_items_survive_reanalysis`; extend them if needed, do not duplicate them. --- ## 16. Recorded as scope choices, not domain truths Written down so a later reader does not mistake a simplification for a finding: - **One confirmed engagement per meeting** is a deliberate simplification. If reviewers report meetings covering two projects, drop the unique index and add a primary flag; nothing else depends on the 1:1. - **The default ordering** (intent → stage → value → recency) is a starting point chosen on 2026-08-05, not a measured result. It is expected to change once staff report which order matched reality. Ship the column-header sorts so Value and Intent are one click apart, and state the active order in the UI ("Sorted by: Intent, then stage, then value"). - **A `reason` cannot be verified** against the recording. The prompt asks the model to omit actions it cannot ground; the server can only check shape. Label the field **"AI's reason — not checked against the recording"** and treat every reason as unverified. - **`suggested_confidence` is uncalibrated.** After the first 30 reviews, compare confirm-rate per confidence band; if High and Low confirm at the same rate, delete the field.