Skip to content

feat(inbox): extract the inbox into its own deployable (phases A–C: pre-refactors, seam, additive project)#950

Merged
FagnerMartinsBrack merged 4 commits into
mainfrom
inbox-split
Jul 10, 2026
Merged

feat(inbox): extract the inbox into its own deployable (phases A–C: pre-refactors, seam, additive project)#950
FagnerMartinsBrack merged 4 commits into
mainfrom
inbox-split

Conversation

@FagnerMartinsBrack

Copy link
Copy Markdown
Member

Extracts the inbox subdomain into projects/inbox — its own Lambda behind hutch's API Gateway via route precedence (blog-site/web-embed recipe) — to isolate the nx cache graph: an inbox-only change now affects ["inbox"] (19s check) instead of invalidating hutch's 135s chain, and hutch changes no longer re-run inbox's tests. Verified both directions with nx show projects --affected --files=….

This PR is additive and safe to merge/deploy on its own. Hutch is untouched except four pre-refactor package extractions and one new stack output. Merging opens a deliberate double-delivery overlap window (both hutch's and inbox's pipeline consumers process inbound mail; all handlers are replay-idempotent — see architecture snapshot .architecture/2026-07-10-bec433e1/). The stacked cleanup PR closes the window.

Contents (three commits)

  1. Pre-refactors: verification-deadline@packages/domain/user; effective-access + subscription read → @packages/subscription-access; etagMatches/poll budget/feature toggle → @packages/web-shell; initS3ReadContent@packages/article-store; new @packages/inbox-store (6 adapters).
  2. Seam: hutch exports inboxNotificationTopicArn (the only genuinely deploy-time cross-stack value; everything else is config-duplicated per the infra doctrine).
  3. The deployable: /inbox web app with hutch's full gate chain reproduced (auth redirect, verification self-heal, write access, locked page), the three inbox-*-prefixed pipeline consumers (explicit rule names — EventBridge PutRule is an upsert and default names would steal hutch's live rules), Pulumi program + stage configs, post-deploy smoke (303→/login assert). 29 suites / 218 tests / 100% coverage on all four axes.

⚠️ Operator checklist BEFORE merging

  • Prod stack init (staging is done; prod creds are not on the dev machine):
    cd projects/inbox
    PULUMI_CONFIG_PASSPHRASE='' pulumi login s3://hutch-pulumi-state-278728209435-ap-southeast-2
    PULUMI_CONFIG_PASSPHRASE='' pulumi stack init prod
    
    (then commit the encryptionsalt line it adds to Pulumi.prod.yaml)
  • Hutch must deploy once with commit 2's output before/with this merge — the single-PR merge deploys hutch and inbox in parallel; if inbox's job races ahead and fails on requireOutput(inboxNotificationTopicArn), re-run the failed job once hutch's deploy completes (one-click; second attempt reads the persisted output).

After deploy

  • /inbox serves from inbox-web-handler immediately (route precedence; hutch's router remains as shadowed fallback = instant rollback by reverting this PR).
  • Confirm the inbox-*-dlq-topic SNS email subscriptions from the alert inbox (one-time clicks).
  • Staging email round-trip scaffold is in scripts/post-deploy.sh (commented; needs one-time smoke-address seeding).

Open-PR impact (audited at creation)

No open PR touches moved inbox paths. Wiring-file overlap (may need rebase after this merges): #943, #935, #934, #918, #870. The conflict-fixer automation handles textual conflicts post-merge; I'll sweep for semantic breaks after each merge.

Pre-refactors for the inbox deployable split. Four relocations, all
mechanical moves with import sweeps:

verification-deadline joins @packages/domain/user; effective-access
and the subscription read provider join @packages/subscription-access
(every deployable that gates on access composes the decision locally
from the same lookup — the moved provider gains its own fake-client
test since the package enforces 100% coverage); etagMatches, the poll
budget (MAX_POLLS + parsePollParam), and QuerystringFeatureToggle join
@packages/web-shell (the toggle now takes a structural {query} source,
keeping the shell express-free); initS3ReadContent joins
@packages/article-store; and the six inbox DynamoDB/S3 adapters move
to a new @packages/inbox-store consumed by hutch's composition roots,
the pipeline mains, and the delete-account worker.
…ployable

The SES receipt topic is auto-named, so its ARN is the one genuinely
deploy-time value the upcoming inbox stack must read via StackReference
(every other seam value is config-duplicated per the infrastructure
doctrine). Exported ahead of the inbox project so the output is
persisted in both stage checkpoints before its first deploy reads it.
New projects/inbox serving /inbox from its own Lambda via API Gateway
route precedence (blog-site/web-embed recipe), plus inbox-prefixed
copies of the three email-pipeline consumers subscribing to the same
SES topic and platform-bus events as hutch's originals. Hutch is
untouched: this deploy opens a deliberate double-delivery overlap
window — every handler is replay-idempotent, so duplicates converge —
and a follow-up removes hutch's consumers to close it. The explicit
inbox-* rule names matter: EventBridge PutRule is an upsert, so
default names would silently steal hutch's live rules.

The web app reproduces hutch's gate chain (auth redirect, verification
status with session self-heal, write-access, locked page) from the
extracted packages plus small local copies, wired through an env-free
createInboxApp factory; route tests run against an in-project harness
that mints sessions directly instead of driving /signup. 29 suites,
218 tests, 100% coverage on all four axes. Architecture snapshot
2026-07-10-bec433e1 documents the overlap-window state of both event
flows. Staging stack is initialised; prod stack init needs operator
credentials before this merges.
@FagnerMartinsBrack

Copy link
Copy Markdown
Member Author

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error —— View job


I'll analyze this and get back to you.

3 similar comments
@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @FagnerMartinsBrack's task in 8m 28s —— View job


Review complete — PR #950 (inbox deployable extraction)

  • Read review instructions & CLAUDE.md guidelines
  • Gather PR context (diff, key files)
  • Review pre-refactor package extractions (hutch untouched-safety)
  • Review new inbox runtime (handlers, app, middleware, gate chain)
  • Review Pulumi infra (route precedence, rule naming, config drift)
  • Review test coverage & conventions
  • Post review

Outcome: 0 High · 0 Medium · 2 Low. This is an exceptionally well-constructed, additive PR. Key safety claims independently verified:

  • Hutch runtime changes are pure import-path refactors — every touched hutch .ts is a @packages/... re-point (verified via git diff); no behavioural change.
  • Domain handlers are byte-identical to hutch's production versionsreceive-email-handler, extract-email-links-handler, crawl-email-link-preview-handler, store-email-body, email-content-id all diff-clean against hutch. The double-delivery idempotency rests on already-proven code.
  • Route precedence is sound — all POSTs target /inbox/{proxy+} sub-paths (/inbox/create, /inbox/disable); GET /inbox is the exact key. No bare POST /inbox gap.
  • Prod config matches hutch's ownhutch-sessions-d17dbb3 / hutch-users-f90c09e are identical across staging & prod in hutch's own Pulumi.*.yaml too, so the unsuffixed names are correct-by-design, not a copy-paste slip.
  • Legacy-row handling correctdynamodb-user-standing's emailVerified === true maps absent→false via dynamoField's nullish transform (tested).

Structured review is emitted as the run output for the auto-apply workflow.
· inbox-split

@FagnerMartinsBrack

Copy link
Copy Markdown
Member Author

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

1. GET /inbox/ (trailing slash) falls through to hutch's $default

The gateway route keys are ["GET /inbox", "ANY /inbox/{proxy+}"] (projects/inbox/src/infra/index.ts:165). API Gateway's greedy {proxy+} requires at least one path segment, so a GET /inbox/ request matches neither key and falls through to hutch's $default. In practice nothing emits the trailing slash — INBOX_PATH / canonicalUrl are /inbox and every internal link and form targets /inbox, /inbox/create, /inbox/disable, /inbox/:id, or /inbox/addresses — and hutch's shadowed router serves it identically during the overlap window regardless. This is purely defensive: worth being aware of only if a future link ever appends the slash. Fix this →

2. lambda.main.ts local-listen branch overlaps server.main.ts

lambda.main.ts:100-104 starts an Express listener when !lambda, but local dev is served by server.main.ts (the dev/start scripts, with seeded in-memory fixtures). The web-Lambda entry needs the full set of real AWS env vars (DYNAMODB_*, CONTENT_BUCKET_NAME, …) to boot, so its !lambda path isn't a usable local server. It mirrors hutch's own entry-point shape and is coverage-excluded (**/*.main.ts in the base config), so this is a consistency nit rather than a defect — no action needed unless you want a single local entry point.

pulumi stack init prod ran against the prod state backend; both stage
stacks now exist, clearing the first-deploy operator step.
@FagnerMartinsBrack

Copy link
Copy Markdown
Member Author

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @FagnerMartinsBrack's task in 5m 11s —— View job


Re-review complete — PR #950 @ 4472c4ce (inbox deployable extraction)

  • Read review instructions & CLAUDE.md guidelines
  • Gather PR context (full diff, incremental commit 4472c4ce)
  • Review pre-refactor package extractions (hutch untouched-safety)
  • Review new inbox runtime (handlers, app, middleware, gate chain)
  • Review Pulumi infra (route precedence, rule naming, config drift, encryption salt)
  • Review test coverage & conventions
  • Post review

Outcome: 0 High · 0 Medium · 3 Low. This is a re-review at 4472c4ce; the only change since the last full review (63a97566) is the one-line encryptionsalt in Pulumi.prod.yaml — safe Pulumi bookkeeping (no secure: values exist in either stack config, so the salt protects nothing yet) and it clears the "prod stack init" operator step.

Independently re-verified the load-bearing safety claims:

  • Hutch runtime changes are pure import re-pointsgit diff confirms initS3ReadContent@packages/article-store, initDynamoDbSubscriptionRead@packages/subscription-access, initDynamoDbInbox*@packages/inbox-store; etagMatches/feature-toggle deleted after moving to @packages/web-shell. Sole hutch infra change is one additive export.
  • Domain handlers byte-identical to hutch's live copies — all five (receive-email-handler, extract-email-links-handler, crawl-email-link-preview-handler, store-email-body, email-content-id) diff-clean. Double-delivery idempotency rests on already-proven code.
  • Route precedence sound — only POSTs are /inbox/create & /inbox/disable (match ANY /inbox/{proxy+}); bare GET /inbox is the exact key. No POST falls through to $default.
  • Gate chain mirrors hutch — cookies → changelog-dismiss → login resolve → verification self-heal → requireAuth (303→/login) → router (requireNotLocked, requireWriteAccess).
  • Legacy-row emailVerified safe & testeddynamoField's .nullish().transform(v => v ?? undefined) means an absent attribute can't throw a ZodError; covered by an explicit legacy-row test.
  • CLAUDE.md compliance — no direct process.env, requireEnv/getEnv throughout, HutchLogger.from, explicit DI (no default noop/in-memory), 100% coverage on all four axes with only the sanctioned src/infra/** exclusion.

The one new observation is a quantification of the deliberate double-delivery window's fan-out magnitude (Low, informational — not a defect). Structured review is emitted as the run output for the auto-apply workflow.
· inbox-split

@FagnerMartinsBrack FagnerMartinsBrack merged commit 963345c into main Jul 10, 2026
9 checks passed
@FagnerMartinsBrack FagnerMartinsBrack deleted the inbox-split branch July 10, 2026 10:46
@FagnerMartinsBrack

Copy link
Copy Markdown
Member Author

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

1. Quantify the deliberate double-delivery window's fan-out before merging

This is informational, not a defect — the overlap is intentional and correctness-safe — but the magnitude is worth stating so the window is sized deliberately. This PR is purely additive: it does not touch hutch's SNS subscription or EventBridge rules, so hutch's full inbound-mail pipeline stays live while inbox adds parallel consumers. The result is multiplicative fan-out per inbound email during the window:

  • Both hutch's and inbox's receive queues subscribe to the same SES→SNS topic (inbox-receive-email-subscription, projects/inbox/src/infra/index.ts:252) → 2 receive workers → 2 EmailReceivedEvents.
  • Both hutch's and inbox's extract rules match that event → 4 extract runs.
  • extract-email-links-handler.ts:118-133 re-publishes CrawlEmailLinkPreview on every run, even when the conditional putLink is a no-op duplicate (per its own comment) → 4·N crawl events, and both crawl rules match → up to ~8·N crawl runs per email (N = links).

Every write is idempotent (conditional putLink, UpdateItem outcome, deterministic content-id / lead-image key) and the handlers are byte-identical to hutch's proven production code, so the terminal state is correct. The cost is transient: a 200-link newsletter can drive up to ~1,600 preview fetches against third-party origins plus the Lambda/SQS spend, for the window's duration. Nothing to change in this PR — the fix is the stacked cleanup PR. Recommend merging that promptly and watching the inbox-crawl-email-link-preview DLQ/throughput during the overlap.

2. GET /inbox/ (trailing slash) falls through to hutch's $default (carried from prior review)

Route keys are ["GET /inbox", "ANY /inbox/{proxy+}"] (projects/inbox/src/infra/index.ts:165). API Gateway's greedy {proxy+} requires ≥1 path segment, so GET /inbox/ matches neither key. Confirmed nothing emits the trailing slash — every internal link/form targets /inbox, /inbox/create, /inbox/disable, /inbox/:id, /inbox/:id/articles, or /inbox/addresses (inbox.page.ts), and hutch's shadowed router serves /inbox/ identically during the overlap regardless. Purely defensive; relevant only if a future link ever appends the slash after the overlap window closes. Fix this →

3. lambda.main.ts local-listen branch overlaps server.main.ts (carried from prior review)

lambda.main.ts:100-104 starts an Express listener when !lambda, but local dev is served by server.main.ts (the dev/start scripts, seeded in-memory fixtures). The web-Lambda entry needs the full set of real AWS env vars to boot, so its !lambda path isn't a usable local server. It mirrors hutch's own entry-point shape and is coverage-excluded (**/*.main.ts), so this is a consistency nit rather than a defect — no action needed unless you want a single local entry point.

FagnerMartinsBrack added a commit that referenced this pull request Jul 10, 2026
… read

The rebase onto main picked up the inbox extraction (#950, 963345c),
which moved initDynamoDbSubscriptionRead out of hutch into
@packages/subscription-access. Git's rename detection reapplied this PR's
listAllSubscriptionRows addition onto the relocated file, but left two
loose ends the rebase could not see:

- The reconcile CLI still imported initDynamoDbSubscriptionRead from the
  now-deleted hutch-local path. Repoint it to @packages/subscription-access,
  matching app.ts and the composed dynamodb-subscription-providers.
- listAllSubscriptionRows coverage lived only in hutch's composed test, so
  the relocated function body was uncovered by its new package's own suite.
  Add the matching unit tests (single page, ExclusiveStartKey pagination,
  empty table) to subscription-access — restoring 100% coverage there.

No Stripe logic changed; this only re-integrates the PR's additions with
main's module move. Full `pnpm check` green (42 projects / 60 tasks).

Co-authored-by: Fayner Brack <FagnerMartinsBrack@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant