.architecture-plan.md

Architecture round 3: Extract crescent/middleware sub-package

Status: Draft plan, needs review before implementation. Prior work: 95b76f3 extracted router/, 31bf3c8 extracted cookie/. Context: Round 3 extracts the three middleware implementations (rate_limit, request_id, security_headers) out of the root package into a new bobzhang/crescent/middleware sub-package. The Middleware/MiddlewareNext type aliases, the App::use_middleware method, and the execute_middlewares runtime all stay at root (they are intrinsic to App, and execute_middlewares is priv today).

Goal

Give middleware implementations a discoverable home (@middleware.rate_limit(...) etc.), remove 3 .mbt files and 3 test files from the root package, and set the precedent for cors/ to potentially relocate into the same namespace in a future round.

What moves

Into new middleware/ directory

Source files (move, then re-qualify root types):

• rate_limit.mbt → middleware/rate_limit.mbt. Body references Middleware, HttpResponse, StatusCode (constructing TooManyRequests), and @async.now(). After the move these become @crescent.Middleware, @crescent.HttpResponse, and @async.now() respectively. The TooManyRequests enum variant should resolve via type-directed inference from the @crescent.HttpResponse(status_code=...) parameter type (the same mechanism that handled same_site=Lax across packages in round 2 — needs codex to double-check this holds for StatusCode enum variants too).
• security.mbt → middleware/security.mbt. Body references Middleware, Event, @httputil.set_missing_header_case_insensitive. After the move: @crescent.Middleware, @crescent.Event, and @httputil.* stays unchanged (but the middleware/ package needs its own bobzhang/crescent/httputil @httputil import).
• request_id.mbt → split. Middleware function and its private state (request_id_counter, generate_request_id) move to middleware/request_id.mbt; the accessor method Event::request_id(self) has to stay at root because MoonBit's coherence rules disallow defining methods on external types (confirmed in round 2 plan review and by cors/'s free-function pattern). The accessor moves into root event.mbt where Event is defined.

Test files:

• rate_limit_test.mbt → middleware/rate_limit_test.mbt. Currently blackbox-style (uses @crescent.App, @test_client.TestClient, etc.). After the move, the local @crescent.rate_limit reference drops to unqualified rate_limit (same package); @crescent.App/TestClient stay qualified.
• request_id_test.mbt → middleware/request_id_test.mbt. Same mechanical edit for @crescent.request_id → request_id. Note: event.request_id() inside the handler stays as a method call — method dispatch works across package boundaries.
• security_wbtest.mbt → rewritten and renamed to middleware/security_test.mbt (blackbox). The current wbtest pokes into priv fn execute_middlewares directly, which won't be accessible from a sub-package. Rewrite each test to use App + TestClient so we can assert on the response headers after a real dispatch. This also aligns with the user's _test.mbt-over-_wbtest.mbt preference recorded in memory. Four tests to port:
  1. security_headers sets base headers by default
  2. security_headers with opt-in policy headers
  3. security_headers preserves handler-set headers
  4. security_headers preserves handler response Each becomes a App() with a single route and use_middleware(security_headers(...)), then a TestClient.get("/test") call and assertions on res.headers.* or res.body_text().

Stays at root

• middleware.mbt (the root file) keeps MiddlewareNext, Middleware type aliases, App::use_middleware, execute_middlewares, execute_middleware_chain, and normalize_middleware_base_path. These are infrastructure the sub-package builds on top of.
• event.mbt absorbs the Event::request_id(self) -> String? accessor (3 lines).
• test_client_wbtest.mbt — its one test that uses security_headers() gets rewritten to use a custom inline middleware (the test is really verifying "TestClient runs middleware", not "security_headers sets X-Frame-Options"). This avoids wbtest → middleware → crescent cycle concerns. Alternatively we could leave this test using the root-level binding if we re-export via a typealias, but a small rewrite is cleaner.

New middleware/moon.pkg

import {
  "bobzhang/crescent",
  "bobzhang/crescent/httputil" @httputil,
  "moonbitlang/async",
}

warnings = "+missing_doc+unnecessary_annotation+74"

• bobzhang/crescent for Middleware, Event, HttpResponse, StatusCode variants (default alias @crescent).
• bobzhang/crescent/httputil for @httputil.set_missing_header_case_insensitive used by security.mbt.
• moonbitlang/async for @async.now() used by rate_limit.mbt and request_id.mbt.

Codex: please verify these are sufficient. Any issues with the +74 warning suppression?

Root moon.pkg change

The root package's library code does NOT import middleware — that would cycle (middleware → crescent → middleware). Only the blackbox test layer of root needs middleware in scope (so fullstack_test.mbt can call @middleware.security_headers()). This requires adding it to the existing for "test" block, NOT the main import block:

import {
  "bobzhang/crescent/middleware" @middleware,
  "moonbitlang/async/os_error",
} for "test"

Rationale: blackbox tests are compiled as a separate package (bobzhang/crescent_blackbox_test) that is allowed to import both the root and any package that depends on root, because they're a downstream of root rather than part of root's own compilation. Codex — please sanity-check whether MoonBit's for "test" import scoping actually supports this pattern, or whether I need a different approach (e.g. inlining a custom security middleware in fullstack_test.mbt).

Codex review feedback incorporated

[P1] README.mbt.md has checked doctest blocks that call the moved functions. Original plan missed these. Fix: update each of the three locations below, and rely on the for "test" root middleware import so the blackbox-test-compiled README snippets can resolve @middleware.*:

• README.mbt.md:143-177 — moonbit check block defines _build_app() calling @crescent.security_headers() and @crescent.request_id(). Update to @middleware.security_headers() / @middleware.request_id().
• README.mbt.md:179-200 — mbt check blackbox test block calls unqualified security_headers() and request_id(). Update to @middleware.security_headers() / @middleware.request_id().
• README.mbt.md:688-694 — documentation table listing the built-in middleware. Update the display names to reflect the @middleware.* location (not compiled; purely docs).
• README.mbt.md:698-710 — moonbit nocheck example for a custom rate limiter. Not compiled, but update the narrative around "writing custom middleware" if it misstates where rate_limit/etc. live.

[P2] Alternative D rationale correction. MoonBit does support re-export mechanisms (pub using, #alias) per codex; the plan was wrong to say there is none. The real reason we can't re-export from root is the import cycle: root cannot import bobzhang/crescent/middleware for its main library compilation because middleware already imports root. Re-exports are therefore impossible in the main package context, which is why we must migrate call sites rather than alias.

Root file edits

• event.mbt — append:

  ///|
  /// Returns the request ID for this event, or `None` if the middleware is not active.
  pub fn Event::request_id(self : Event) -> String? {
    self.req.get_header("X-Request-Id")
  }

• fullstack_test.mbt:114 — @crescent.security_headers() → @middleware.security_headers(). (This is a _test.mbt blackbox test file, so the root's for "test" import is in scope.)

• test_client_wbtest.mbt:69-78 — rewrite the test client runs middleware test:

  async test "test client runs middleware" {
    let ran = Ref::new(false)
    let app = App()
    app.use_middleware((_event, next) => {
      ran.val = true
      next()
    })
    app.get("/test", fn(_) noraise { "ok" })
    let client = @test_client.TestClient(app)
    let res = client.get("/test")
    assert_eq(res.status, OK)
    assert_eq(ran.val, true)
  }

  This test still verifies that TestClient dispatches through the middleware chain, but without depending on security_headers. Coverage of security_headers itself moves to middleware/security_test.mbt.

• examples/route/moon.pkg — add "bobzhang/crescent/middleware" @middleware to the import block.

• examples/route/main.mbt:9 — @crescent.security_headers() → @middleware.security_headers().

What the end state looks like

crescent/
├── middleware.mbt               (unchanged — keeps type aliases + use_middleware + runtime)
├── event.mbt                    (+3 lines for Event::request_id accessor)
├── rate_limit.mbt               (DELETED)
├── request_id.mbt               (DELETED)
├── security.mbt                 (DELETED)
├── rate_limit_test.mbt          (DELETED)
├── request_id_test.mbt          (DELETED)
├── security_wbtest.mbt          (DELETED)
├── test_client_wbtest.mbt       (one test rewritten to use custom inline middleware)
├── fullstack_test.mbt           (@crescent.security_headers → @middleware.security_headers)
├── moon.pkg                     (+1 line in `for "test"` block)
├── pkg.generated.mbti           (auto: removes rate_limit/request_id/security_headers)
├── middleware/                  (NEW)
│   ├── moon.pkg
│   ├── rate_limit.mbt
│   ├── request_id.mbt
│   ├── security.mbt
│   ├── rate_limit_test.mbt      (blackbox)
│   ├── request_id_test.mbt      (blackbox)
│   ├── security_test.mbt        (NEW blackbox — rewritten from wbtest)
│   └── pkg.generated.mbti
├── examples/route/
│   ├── moon.pkg                 (+1 middleware import)
│   └── main.mbt                 (1 call site updated)
└── ... (cookie/, router/, etc. unchanged)

Breaking API changes

Moved out of root:

• @crescent.rate_limit(...) → @crescent/middleware.rate_limit(...)
• @crescent.request_id() → @crescent/middleware.request_id()
• @crescent.security_headers(...) → @crescent/middleware.security_headers(...)

Unchanged:

• @crescent.Middleware, @crescent.MiddlewareNext type aliases
• App::use_middleware(self, mw, base_path?) method
• Event::request_id() method (accessor moves within root to event.mbt but remains on the same type)

Migration for downstream users:

// Before
app.use_middleware(@crescent.rate_limit(requests_per_window=100, window_ms=60000))

// After
app.use_middleware(@middleware.rate_limit(requests_per_window=100, window_ms=60000))

Plus adding "bobzhang/crescent/middleware" @middleware to their own moon.pkg import block.

Verification plan

1. moon check — catches missing qualifiers and the for "test" import scoping question.
2. moon test --verbose — must still report 504/504 passing, and the specific names rate_limit_test.mbt, request_id_test.mbt, security_test.mbt should appear under the middleware/ path in the verbose output.
3. moon info && moon fmt — regenerate mbti. Root mbti loses rate_limit, request_id, security_headers from the // Values section. New middleware/pkg.generated.mbti gains them. No other root mbti changes expected (Middleware type alias, use_middleware, Event::request_id all stay).
4. Spot-check the example builds: moon check should cover examples/route/ as part of the full-workspace check.

Alternatives considered (and rejected)

A. Keep the Round-3 scope even smaller — only extract rate_limit and security, leave request_id at root

Rejected because the Event::request_id accessor is only 3 lines and splitting request_id.mbt is trivial. Leaving the middleware function at root just to avoid a split makes the sub-package feel arbitrary ("why these two but not that one?"). Codex — push back if you think the split adds more complexity than it's worth.

B. Also relocate cors/ → middleware/cors/ in the same commit

Rejected for this round because it adds a rename of an already-extracted package and touches every @cors.* call site in the codebase (examples/route/main.mbt:8, possibly others). Keeping the commit scoped to "extract from root" rather than "reorganize sub-packages" preserves bisect-ability. Worth considering as a standalone round-4 rename.

C. Rewrite security_wbtest.mbt in place (before the move) so the rename is pure

Rejected because a pure rename + content update is harder to review than a single commit that both moves and rewrites. Doing the rewrite inside the move commit is OK as long as the commit message explicitly calls it out (which the plan does).

D. Add pub type rate_limit = @middleware.rate_limit aliases at root for source-level backward compatibility

Rejected because pub type X = Y in MoonBit defines a new type (sometimes transparent), not a function alias. There is no fnalias / pub fn x = @middleware.x syntax I'm aware of. Users must migrate. Same call as round 1 and round 2. Codex — confirm this is the case, or point to a re-export feature I'm missing.

E. Move the Middleware/MiddlewareNext type aliases out of root too

Rejected because they are consumed by App::use_middleware (which stays at root due to the method-orphan rule), and moving just the aliases without the method creates a weird cross-package dependency. They belong with use_middleware, which belongs with App.

Open questions for codex review

1. Enum variant resolution across packages. Does @crescent.HttpResponse(status_code=TooManyRequests) compile inside middleware/rate_limit.mbt, or does the TooManyRequests variant need to be qualified as @crescent.TooManyRequests? Round 2 showed type inference handles enum variants at call sites when the parameter type is explicit. I believe the same holds here because status_code has type @crescent.StatusCode, but please verify by reading rate_limit.mbt:23 context and the round-2 README cookie doctest precedent.

2. for "test" import scoping. Can root's blackbox tests import a package that itself imports root? Conceptually this is the dependency DAG middleware → root, with root_blackbox_test → {root, middleware} as a downstream. Is this actually what MoonBit supports, or will I hit a cycle error at moon check time? If it's not supported, propose an alternative — e.g., drop the @middleware.security_headers() call in fullstack_test.mbt and use an inline custom middleware instead, mirroring what I'm doing for test_client_wbtest.mbt.

3. Does security_wbtest.mbt genuinely use any priv-only access besides execute_middlewares? If not, a blackbox rewrite via TestClient should cover everything. Confirm by reading the file.

4. Method orphan rule for request_id split. Am I right that Event::request_id MUST stay at root, and that moving it to middleware/ would fail with a coherence error? Round 2 plan review said yes based on cors/ precedent — I'm not re-litigating unless you have new evidence.

5. Is the plan's scope right for a single round? Too big? Too small? If you think round 3 should also rename cors/ → middleware/cors/, say so and I'll fold that in. Otherwise I'll keep it as proposed and defer cors relocation to round 4.

6. Test file naming. The existing rate_limit_test.mbt, request_id_test.mbt are blackbox (_test.mbt) — good, they move verbatim. security_wbtest.mbt needs to become security_test.mbt after the rewrite. Any MoonBit convention I should know about for test file naming in a sub-package that imports its parent?

Keep responses concise and actionable — I want to implement, not re-plan. Tag each finding [P1 blocker / P2 should fix / P3 nit].