Repository navigation
M19 restructure: add Phase 4 Visual Identity Polish; reopen 19.1 for verification pass; renumber phases - #74
Merged
Conversation
…r verification pass; renumber phases - Phase 4 — Visual Identity Polish (new): favicon overhaul (theme-cohesive yet purpose-distinguishable across surfaces), og:image / social previews, and the "open beta" copy refactor (moved here from former Phase 4 / now Phase 5 §5). Slotted at Phase 4 so subsequent phases review their surfaces in their final visual state. No dependency on Phase 3 (LLC). - Phase 1 reopened for end-to-end verification: §1–§4 audit work stays closed and untouched; §5–§7 appended for scenario walkthroughs (incl. time-advanced past-due flows), §6 finding resolution, §7 best-effort stress check. Catch-all alias (`MAIL_ALIAS_ENABLED`) flipped off early in §5.0 so the rest of MS19 emits mail under prod-like routing. Tasks tagged `[Agent]` / `[Subagent]` / `[User]` per the x17.5 convention. - Phases 4–7 → 5–8: Legal Layer (was 4 → 5), Content Promises (5 → 6), Contributor Docs (6 → 7), 2FA (7 → 8). Phase 8 stays positionally last by design. Strategy doc filenames renamed to match. - Phase 3 (LLC) and Phase 5 (Legal Layer) corrected to independent parallel tracks. Stripped the "Prerequisite for Phase 4" assertion from the milestone doc — drafting/UI work has no LLC dependency; only the deploy-time entity-name swap at MS21 needs the formed entity. - `.ics` updated to reflect actual M19 start (2026-05-08), expanded scope (M19 end → 2026-07-04), and downstream cascade (M20: 2026-07-03 → 2026-07-16; M21: 2026-07-15 → 2026-07-28). - Cross-reference updates: `docs/milestones/21_RC_DEPLOYMENT.md` and `docs/strategies/x18.3_SITE_ARCHITECTURE_AND_PUBLISHING.md` updated to point at the new phase numbers. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 400c0232a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+15
to
+16
| DTSTART;VALUE=DATE:20260508 | ||
| DTEND;VALUE=DATE:20260704 |
There was a problem hiding this comment.
The calendar now moves MS19/MS20/MS21 to the July window, but the parent execution doc still says MS19 targets 2026-06-11, MS20 targets 2026-06-23, and MS21 targets 2026-07-05 in docs/PLAN.md (and still describes MS19 as pre-flight). Because this milestone doc links PLAN as the canonical parent execution doc, readers get two conflicting schedules depending on which artifact they open; update PLAN alongside the .ics changes so the active roadmap has one source of truth.
Useful? React with 👍 / 👎.
|
|
||
| ## 4. Verification | ||
|
|
||
| 1. [ ] [Agent] Run `php artisan view:clear` and verify all changed Blade templates render. |
There was a problem hiding this comment.
In this repo, AGENTS.md requires artisan/composer/npm commands to be run through Sail, but this new verification step tells the next agent to run php artisan view:clear directly (the same direct php artisan schedule:run pattern appears in the reopened notification strategy). In the Sail-based setup that can run against the host PHP or simply fail/miss container env, so the checklist should use ./vendor/bin/sail artisan ... for these verification commands.
Useful? React with 👍 / 👎.
Replace dotted notation (### 5.0, ### 5.1, ..., ### 5.10) with restart-at-1 numbering (### 1, ### 2, ..., ### 11) to match the H3 sub-section pattern established in x14.5_BQA_FINDINGS_AND_FIXES.md and x17.1_ISSUER_SWEEP.md. The dotted scheme was unique to my §5 and inconsistent with the rest of the project. Internal back-refs and Exit Criteria parentheticals updated to use prose descriptions instead of dotted-section numbers. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
§5/§6/§7 collapsed to per-sub-section action lists; old §6 (findings collector) retired and prior §7 (stress-readiness) renumbered to §6 with Mailgun free-tier cap recorded. §5.4 carries action items for #75/#76/#77; §5.9 converted from dormant-verification to removal items per #82; §6 references future-stress trigger #81. Top of strategy doc + milestone Status/Phase 1/Active phase lines stripped of "reopened/added/restructured" cruft (state, not history). §3/§4 audit findings filed retroactively as closed Issues #78/#79/#80; in-doc writeups trimmed to pointers. DOC_ROLES: new Issue content conventions section (required problem, optional repro/refs/fix-direction/scope/test plan, never journaling or internal shorthand, always link the strategy doc). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- #75: rewrite issuer subject + heading to "Review payment activity on Invoice …" — replaces ambiguous "Review detected payment …" - #76: tighten client ack body so partial scenario reads clearly as partial. Final wording names the scenario explicitly and reframes the outstanding dollar figure as a post-confirmation value: "This appears to be a partial payment — once confirmations settle, $Y will still be due on this invoice." Per-payment rate lock per PARTIAL_PAYMENTS.md:73-74 makes the figure stable across the confirmation interval. - #77: per-invoice delivery log defaults to client-facing rows (filter by recipient, not by type enumeration, so new *_issuer types stay hidden automatically). "Include emails sent to me" checkbox toggles the unfiltered view; section is anchored so reload returns to the log instead of the page top. - Restructure §5.4 of the strategy doc to nest verifications under each finding's ship item. Concision pass on completed-item notes. Fixes #75 Fixes #76 Fixes #77 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…mails
Reframes the post-confirmation alert mails so they're clearly
distinguishable from the detection-time acknowledgment. The
detection-time ack reads "Bitcoin payment detected … we'll follow
up if anything needs your attention"; the confirmation-time alert
was reading like a duplicate of that promise rather than the
status update it actually is.
All four templates (`InvoiceUnderpaymentClientMail`,
`InvoiceUnderpaymentIssuerMail`, `InvoiceOverpaymentClientMail`,
`InvoiceOverpaymentIssuerMail`) now carry a consistent
"Payment confirmed — …" subject prefix and a body opener that
explicitly names the confirmation event ("Your payment is now
confirmed on the network …" / "The client's payment is now
confirmed on the network …") so the reader can't mistake this for
a duplicate of the detection-time ack.
Also restructures §5.4 of the strategy doc so ship items nest as
sub-items of the QA action that surfaced them, rather than sitting
as top-level siblings.
Fixes #84
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The past-due alert pipeline fires three escalation slots (1d / 7d / 14d) on independent context keys (past_due_1/_2/_3), but the rendered mail carried no slot indicator — three nudges arrived looking identical and the system's graduated escalation never surfaced to the recipient who's supposed to act on it. Now both Mailables read `$delivery->context_key` and render slot-aware subject + body across client + issuer audiences: - Slot 1 stays "Reminder" framing — one-day-late is often an oversight; calling it the first notice would treat soft contact as already-failed. - Slots 2/3 surface "2nd past-due notice" / "3rd past-due notice" anchored to past-due specifically (not counting any pre-due contact), plus a "1 week overdue" / "2 weeks overdue" time anchor. - Body copy escalates with the count — slot 2 opens with a follow- up acknowledgment; slot 3 names the delinquency timeline and hints at escalation outside the platform (the platform is watch-only; it can't pursue payment for the issuer). Also carries §5.6 (overpay alert) verification checkoffs — agent- side flow + cooldown regression all clean; user-side eyeball QA signed off. No code changes for §5.6. Fixes #85 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Branding preview Mailable fires via NotificationSettingsController's direct Mail::to path; no invoice_deliveries row is created (bypass per spec §5.14.4.1 confirmed). Rendered preview lands at the owner's inbox and reads as expected. No code changes — verification-only. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The InvoicePartialWarning{Client,Issuer}Mail family was wired into
DeliverInvoiceMail's type map but never queued by service code —
dormant Mailables held around under a "preserve for historical
delivery-log rendering" premise. Pre-launch the database holds only
seed/test data, so that premise no longer applies.
Removed:
- app/Mail/InvoicePartialWarning{Client,Issuer}Mail.php (deleted)
- resources/views/mail/invoice-partial-warning-{client,issuer}.blade.php (deleted)
- DeliverInvoiceMail: type-map entries, imports, and the partial-
warning skip-handler branch in skipReason()
- InvoiceDelivery::typeLabel: the two partial-warning entries
- InvoiceAlertService::skipInvalidQueuedDeliveries: the partial-
warning skip branch (no longer reachable)
- Invoice::shouldWarnAboutPartialPayments() (no remaining callers)
- Invoice $fillable + $casts: last_partial_warning_sent_at
- Schema: dropped invoices.last_partial_warning_sent_at; deleted
*_partial_warning rows from invoice_deliveries
- Spec NOTIFICATIONS.md: §5.12.3 (legacy-rows-render clause) and
the two matrix rows in Coverage & Status
- Test surgery:
- InvoicePaymentCorrectionTest: fixture rows substituted with
overpay-alert types (still in skipInvalidQueuedDeliveries
scope); test method renamed accordingly
- WatchPaymentsCommandTest: deleted the two regression tests
asserting absence of removed types
§4.4.3 of the spec stays — useful historical context for why the
underpay alert exists in place of the partial-warning family.
Full suite green: 335 passed / 1639 assertions / 184s.
Fixes #82
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
§5.10 walk surfaced one finding: InvoiceAlertService::sendPastDueAlerts correctly early-returns on `paid` or `void` status, but checkPaymentThresholds did not — voiding an invoice that was still in `partial` state and then triggering any caller (payment sync, correction action, manual service call) would queue and send underpay/overpay alerts to live recipients on a defunct invoice. Fix: mirror the existing past-due gate in checkPaymentThresholds. Gate only on `void` — paid is still allowed since overpay alerts legitimately fire post-paid when overpaymentPercent exceeds the threshold. Verified: voided invoice #100 (which had previously leaked the alerts) now correctly produces zero new rows on a re-fired checkPaymentThresholds call. Full suite 335/335. Also: - §5.11 (mail-client rendering spot-check) signed off. - §5.12 added to track #87 (settlement mails — client receipt + issuer paid notice — drop on-chain evidence for multi-payment invoices). Doc-only addition; the ship lands in a separate commit. Fixes #86 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Settlement mails are accounting documents — they should list every confirmed on-chain payment that supports the "paid" claim. Both fell short for multi-payment invoices: - Client receipt previously rendered only $invoice->txid (the latest confirmed payment per refreshPaymentLedger), silently dropping any earlier confirmed payments that contributed to settlement. - Issuer paid notice rendered no txid at all. Both Mailables now query confirmed, non-ignored, non-adjustment payments via the existing payments() relation and pass them to the view. Each payment renders with its locked-at-detection fiat_amount (per docs/specs/PARTIAL_PAYMENTS.md:73-74), sats received, and confirmation time (client side) — full on-chain evidence in one place. Single-payment invoices render one entry without awkward "1 of 1" framing; multi-payment renders the list with an "across N on-chain payments" anchor in the receipt opener. New tests in tests/Feature/SettlementMailEvidenceTest.php verify both Mailables across multi-payment + single-payment scenarios. Full suite green: 338 passed / 1662 assertions. Fixes #87 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
MS18 video footage is captured and MS18 target is extended to 2026-06-07 (see PR #89). MS19 doc no longer needs the parenthetical flagging MS18 as Rachel-blocked. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Video publishing (MS18 Phase 3 §3.6–3.8):
- Add dedicated page /learn/how-to-invoice-someone-in-bitcoin/ with
privacy-friendly embed, VideoObject schema (uploadDate 2026-05-30,
duration PT12M35S), related-guide links, and a sitemap entry.
- Video-led layout: content column widens to the native 1080p width.
- Guide list: poster + play-badge card, ordered directly after the
accepting-Bitcoin-payments guide via a learnList/stagingList collection.
- Embed a reusable video-link card into the accepting-Bitcoin-payments
guide ("So is it complicated?" section).
Content promises:
- CONTENT_PROMISES: add Major 4 (rate refreshes on open / no expiry),
Minor 18-19 (public link, email delivery); video source on Major 2.
Milestone close-out:
- Mark MS18 and Phase 3 strategy Complete; x-prefix the milestone doc;
fix parent links; move MS18 to PLAN's Completed table; de-stale the
Rachel/hard-cap framing.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Repo-wide: completed milestone/strategy docs were x-prefixed but
cross-references (PLAN completed table, parent links, Current Focus
links, inline backtick refs) still pointed at the pre-x filenames.
Repoint all to the x-prefixed targets. Verified zero broken links.
- PLAN.md: MS19 status was stale ("pre-flight, awaiting decisions");
now reflects Phase 1 (notification coverage audit & verification)
in progress.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- NOTIFICATIONS.md items 15-16: transient send failures must retry (terminal `failed` only after the retry budget is exhausted; idempotency holds across retries); a failed delivery must be resendable for every notice class, not receipts alone. - 19.1 §5.13: action items for #90 (tries=3 + backoff [60,300]; resend coverage = Finding B); §6.3 now depends on §5.13. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixes #90. - DeliverInvoiceMail: a transient send failure now releases the delivery back to `queued` and re-raises so the worker re-attempts; a new failed() hook records terminal `failed` only after the retry budget is exhausted. Job-level $tries=3, $backoff=[60,300]. Previously the catch flipped status to `failed` before throwing, so the retry short-circuited on the status guard and transient failures became permanent on the first attempt. - Resend for every notice class (not just receipts): resendDelivery controller action + invoices.deliver.resend route + a Resend button on `failed` rows in the delivery log. - tests/Feature/DeliveryRetryTest.php: transient-then-success (one send, no duplicate rows), persistent failure (failed only after retries), idempotency across retries, resend of a failed non-receipt delivery. Full suite 342 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…vers Live check on invoice #102: a transient 401 left the delivery Queued with a truthful error, then the automatic retry recovered it to Sent (delivery #72644) with real delivery. §5.13 complete. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- PARTIAL_PAYMENTS.md: redefine paid_at as the settling (last) confirmation that crosses the expected total, consistent with txid/payment_confirmed_at (was "the first confirmed timestamp"). - 19.1 §5.14: action items for #91. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixes #91. - Invoice::refreshPaymentState derived paid_at from min('confirmed_at') (the first confirmed payment), so multi-payment invoices showed the wrong "Paid at" on the issuer notice and contradicted the latest-confirmed txid. Now uses max('confirmed_at') — the settling confirmation — keeping $reference precedence. - tests/Feature/PaidAtTimestampTest.php: multi-payment with distinct confirm times → paid_at = last; single-payment unchanged. Full suite 344 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixes #92. Both settlement mails formatted timestamps with toDayDateTimeString() (minute precision), so same-minute confirmations were indistinguishable on the accounting record. Render with seconds (D, M j, Y g:i:s A) for the issuer "Paid at" and the receipt per-payment "(confirmed …)". Display-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Verified on invoice #101 (3 same-minute, distinct-second confirmations): full txid/amount evidence present and reconciling, Paid at = the settling confirmation, timestamps distinguishable. Closes the §5.12 and §5.14 user QA. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixes #93. The send and the post-send status='sent' write shared one try/catch, so a post-send DB failure (deadlock, lost connection) reverted the row to queued and re-threw — the retry then re-sent an already-delivered email, and failed() could later mislabel a sent delivery. Scope the retryable catch to send() only; record 'sent' outside it, so a bookkeeping failure leaves the row in 'sending' and a retry no-ops (sees 'sending') instead of re-sending. Also closes the concurrent-revert double-send variant. New test: a delivery already in 'sending' is not re-sent on retry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- PARTIAL_PAYMENTS.md: paid_at = confirmed_at of the confirmation that most recently crosses cumulative confirmed total to >= expected (the latest total re-cross), refining the imprecise "last confirmed payment" wording. - 19.1 §5.15: action items for #94 (replace max with the crossing computation). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixes #94. Replaces max('confirmed_at') (which overstated settlement when a redundant payment confirmed after the total was already crossed) with the crossing confirmation: the confirmed_at of the time-sorted payment that first brings cumulative confirmed USD to >= expected. Keeps $reference precedence. New settlementCrossingTime() helper reuses paymentFiatValue, the same valuation as the status decision, so the crossing aligns with confirmedUsd >= expectedUsd. tests/Feature/PaidAtTimestampTest.php: overpay+extra (the crossing, not the later payment) and $reference precedence. Full suite 348 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- NOTIFICATIONS.md item 17: every delivery reaches a truthful terminal state on its own (outcome-only — no mechanism/cron prescribed in spec). - 19.1 §5.16: A+B mechanism (delivery-id tag, webhook-by-identity, redelivery self-reconcile) + recorded no-cron design decision. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Tag every outbound message with v:delivery_id (Symfony MetadataHeader) before send, so its provider outcome is matchable even with no stored message id. - MailgunWebhookController resolves the delivery by delivery_id from event user-variables (fallback to provider_message_id); accepted/delivered -> sent (backfill message id), failed/bounced -> failed. Resolves stuck `sending` rows that never recorded a provider_message_id. Tests: accepted/failed events resolve a sending row by delivery_id with no message id. Part of #95 (Path A — redelivery self-reconcile — to follow). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixes #95. - New MailgunEventLookup queries the Mailgun events API by v:delivery_id. - DeliverInvoiceMail: a re-run on a leftover `sending` row reconciles via the lookup (provider confirms accepted -> sent) instead of returning; if unconfirmed it throws so the queue retries and failed() records terminal failed once $tries is exhausted. Non-Mailgun mailers keep the no-op (can't verify; a throw->fail->resend could duplicate). Post-send `sent` write retries in-process, then throws to the reconcile path rather than re-sending. - Completes A+B: webhook (push) + queue redelivery (deployment-independent) converge stuck deliveries with no cron, per NOTIFICATIONS.md item 17. Tests: a stuck `sending` delivery reconciles to sent when the provider confirms and retries when it does not; never re-sends. Full suite 351 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- §6 (run 2026-06-06, scheduler paused to isolate): 12-invoice burst drained clean (failed_jobs 0, no dupes); worker restart mid-burst resumed with no duplicates; a forced 401 recovered via the #90 retry with no duplicate row. Untested risks (real-volume >100/day, sub-daily rate limit, bounce/spam, extended soak, exact mid-send kill) deferred to #81. - §7: all exit criteria checked. 19.1 (MS19 Phase 1) is fully verified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- 19.1 → x19.1 (status: Complete as of 2026-06-06); all sections §1–§7 done. - MS19 milestone: Phase 1 marked complete; Current Focus advanced to Phase 2 (auth hardening); Phase 1 link repointed to x19.1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Restructures MS19 in response to two findings raised mid-session:
Also corrects a bogus prerequisite (Phase 3 LLC was incorrectly listed as a Phase 4 prereq) and updates the schedule artifact (
.ics) to match reality.What changed
docs/strategies/19.4_VISUAL_IDENTITY_POLISH.md): favicon set across surfaces (theme-cohesive yet purpose-distinguishable), og:image / social previews, and the "open beta" copy refactor (lifted from former Phase 4 §5). No dependency on Phase 3.MAIL_ALIAS_ENABLED) flipped off in §5.0 — rest of MS19 emits mail under prod-like routing. Tasks tagged[Agent]/[Subagent]/[User]per thex17.5convention..icscascade: M19 actually-started 2026-05-08 (was 05-16), expanded scope pushes M19 end → 2026-07-04, M20 → 2026-07-03 → 2026-07-16, M21 → 2026-07-15 → 2026-07-28.docs/milestones/21_RC_DEPLOYMENT.mdanddocs/strategies/x18.3_SITE_ARCHITECTURE_AND_PUBLISHING.mdupdated to point at the new phase numbers.Test plan
docs/milestones/19_RC_HARDENING_OPS.mdend-to-end; confirm Phase Rollup, Current Focus, Exit Criteria all reflect the 8-phase layoutdocs/strategies/19.1_NOTIFICATION_COVERAGE_AUDIT.md§5–§7; confirm scenario coverage is thorough enough for "ready for open beta" claim, especially §5.6 past-due flowsdocs/strategies/19.4_VISUAL_IDENTITY_POLISH.md; confirm scope split (favicon + og:image + open-beta refactor) matches expectations and "Decisions to confirm" lists the right gating questionsdocs/strategies/19.5_LEGAL_LAYER.md; confirm §5 (open-beta refactor) is gone, renumber clean, decisions reflect the movedocs/milestones.icsdates render correctly in your calendar appdocs/milestones/21_RC_DEPLOYMENT.mdreferences "MS19 Phase 5" (Legal Layer) consistently🤖 Generated with Claude Code