Conversation
The base class built a bag of error options while every subclass built a finished error, both under the name generateError, so the base had to declare a return type of either one and each subclass had to assert its way back to the options it had just been handed. Naming the two jobs separately lets the base return what it actually returns and the subclasses take it as read.
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueNo actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (23)
🧰 Additional context used📓 Path-based instructions (8)Review whether tests prove changed behaviour, meaningful error/edge paths, and externally observable contracts without coupling to implementation details.⚙️ CodeRabbit configuration file Files:
New source files must be TypeScript: flag new JS files as a required change unless exempt (DB migrations, apps/ember-admin/, tool/config files, scripts/, docker/, generated code).⚙️ CodeRabbit configuration file Files:
Review lens: "where does this data become trusted?" Boundary data (HTTP input, external API/SDK responses, env/config, DB/filesystem reads, queue/webhook/event payloads) is `unknown` until validated — Zod by default.⚙️ CodeRabbit configuration file Files:
Review package boundaries and production consumption: minimal explicit exports, declared runtime dependencies, source-condition versus built-output parity, copied runtime assets, ESM/NodeNext compatibility, and consumer-facing release impac...⚙️ CodeRabbit configuration file Files:
Prioritise concrete correctness, security, data-integrity, compatibility, and regression risks.⚙️ CodeRabbit configuration file Files:
Type-safe boundaries: Fail only if the PR: consumes boundary data (HTTP input, external API/SDK responses, env/config, DB/filesystem reads, queue/webhook/event payloads) without validating it first — Zod by default, another format only wher...📄 CodeRabbit inference engine (Custom checks) Files:
New files are TypeScript: Fail if the PR adds a new .js/.jsx/.cjs/.mjs source file, unless it is: a DB migration (ghost/core/core/server/data/migrations/), under apps/ember-admin/, a tool/config file, under scripts/ or docker/, or generated...📄 CodeRabbit inference engine (Custom checks) Files:
keep authored code in `src/**/*.ts` and tests in `test/**/*.ts`;📄 CodeRabbit inference engine (packages/README.md) Files:
🧠 Learnings (1)📚 Learning: 2026-08-03T21:09:05.797ZApplied to files:
🔇 Additional comments (11)
WalkthroughEmail count queries now omit the Priority: ➖ Normal Change: Bug fix Merge Risk: ⚪ Minimal · up to The implemented limit-service fixes are ready to merge; no actionable current-head risk remains. 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Full details: Type-Safe BoundariesExplanation The PR activates an unvalidated configuration path. Admin fetches Resolution Add a runtime schema at the HTTP configuration boundary, or at the package entry point before ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run @tryghost/admin:test:acceptance |
✅ Succeeded | 11m 48s | View ↗ |
nx run ghost:test:ci:integration |
✅ Succeeded | 4m 54s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 49s | View ↗ |
nx run-many -t test:unit -p @tryghost/admin-x-f... |
✅ Succeeded | 29s | View ↗ |
nx run ghost:test:ci:e2e |
✅ Succeeded | 4m 26s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 3m 16s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 3m 9s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 29s | View ↗ |
Additional runs (7) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-09-15 10:23:00 UTC
rob-ghost
marked this pull request as draft
September 14, 2026 13:24
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #30733 +/- ##
==========================================
+ Coverage 67.68% 67.72% +0.03%
==========================================
Files 1676 1677 +1
Lines 60518 60605 +87
Branches 10465 10489 +24
==========================================
+ Hits 40964 41042 +78
- Misses 17238 17241 +3
- Partials 2316 2322 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file is where Ghost's own configuration meets the limit service, so unlike the count queries it cannot move into the package: it reads hostSettings, decides the help link, swallows a misconfiguration at boot rather than refusing to start, and hands out the singleton every model and endpoint asks for. What it can do is be checked. Ghost only typechecks TypeScript, so as JavaScript none of that wiring was seen by the compiler, and neither was the limit name that the route guard takes. Ghost does not permit TypeScript syntax that cannot be erased, so the service cannot keep exporting itself as the module. It now exports itself by default, which is how every other converted service here is consumed, and the callers that want the service rather than its two helpers read it from there.
A limit is stored under one name, and the spelling a host happens to use is reconciled with it once, when the limits are loaded. That reconciling was reading the host's settings under the name it had just normalised rather than the key the host actually wrote, so a limit spelled another way was built with no settings and quietly refused nothing, while still reporting itself as limited. Checking one then reached for the unnormalised name again and raised a type error. Loading now reads the settings under the key they arrived under, and every check takes the name the package knows the limit by. Tolerating spelling is a loading concern, so it stops at the boundary rather than being repeated at each lookup. A host that spells a limit differently to us therefore gets the limit it asked for, where before it got a limit in name only.
The manifest was already the allowlist the loader applies, but only at runtime: a caller could ask about any name at all, and asking about one nobody had declared quietly answered that the site was not limited. That is the same shape as a typo, and nothing told them apart. The names are now taken from the manifest itself, so asking about a limit that has not been declared does not compile. The loader narrows the host's spelling onto that set, which is the one place a name arrives as text and the only place it needs checking at runtime. Ghost's own route guard and Admin's limiter take the same set, and Admin names the limit a private feature depends on rather than describing it as a string.
A service is constructed before it is given anything, and one consumer deliberately never gives it anything: a self-hosted site has no host limits, so Admin builds the service and leaves it empty. Asking it a question has to answer rather than fail, and nothing recorded that until now. It already behaves, since a name that finds no limit returns before the service reaches for the error module it was never given, so these only hold it there.
…list Whether a site is over any of its limits is asked without naming one, so every limit has to be able to answer it. An allowlist limit cannot, because it judges one particular value and the question carries no value, so it raised an incorrect-usage error that escaped the sweep entirely. A site that limits which themes it may use, which is every hosted site, could therefore not answer the question at all. Those limits are now passed over rather than asked. The pin that recorded the old failure now records the answer, and the rethrow it used to reach incidentally is covered by naming the limit directly instead.
The number of whole periods since a subscription started was allowed to come out negative, which happens when the start date has not arrived yet: a subscription beginning later today, or a host clock running a little ahead of ours, is enough. The period was then anchored before the subscription existed, so sending from before the site was ever subscribed counted against the current allowance. Nothing has elapsed in that case, so the current period now begins at the start date itself.
Only a limit that resets has a period to count within, so a host capping emails outright rather than per period leaves the count with no date to narrow by. On the server that made the query builder refuse to build the query, and in Admin it made formatting the absent date throw. Both reached the publisher as a failed send rather than as a refused one, and Admin told them their sending was disabled because of an invalid time value. The whole history is what a plain maximum caps, so the period is now only applied where there is one.
The package was moved into Ghost with its history rather than copied, but git log --follow cannot reach the commits from before the move because it does not traverse the merge that brought them in, so following a file stops at the move into src. The README now says where to pick the history back up and which commit to ask from.
rob-ghost
force-pushed
the
chore/limit-service-design
branch
from
September 15, 2026 10:09
f2e2bce to
ed70aec
Compare
Contributor
Author
|
Closing in favour of the same work split into three stacked pull requests, which are easier to review separately than together:
Every commit is the same commit that was here, in the same order, so nothing has been rewritten or re-verified away. They are split at the seams where the kind of review changes: mechanical breadth, then one decision, then the limit behaviour itself. |
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.
ref https://linear.app/ghost/issue/BER-3952/bring-limit-service-onto-the-package-golden-path
Problem
The limit service was brought into this repository as it was published, and converting it deliberately changed nothing about what it does, so that the move could be reviewed as a move. Typing it made several faults visible. Each was recorded and left alone at the time, and each is still there.
A limit whose name the host spells differently to us applies in name only, and raises a type error the moment anything checks it. A site that restricts which themes it may use, which is every hosted site, cannot answer whether it is over any of its limits at all. A host that caps emails outright rather than per period breaks sending instead of limiting it, on the server and again in Admin. A subscription whose start date has not arrived yet counts sending from before the site existed against its current allowance.
Underneath those, a limit could be asked about by any name whatsoever. Asking about one nobody had ever declared answered that the site was not limited, which is the same answer a typo gets, and nothing distinguished them.
Solution
Fix the faults, one to a commit, each with the tests that hold it.
Names are reconciled once, when the limits are loaded, and the settings are read under the key the host actually wrote. That was the fault: the reconciling looked for the host's settings under the name it had just normalised, found none, and built a limit that refused nothing while reporting itself as limited.
The names themselves are now taken from the manifest that already acts as the allowlist, so asking about a limit nobody declared does not compile. Tolerating spelling becomes what it always should have been, a loading concern that stops at the boundary. Ghost's route guard and Admin's limiter take the same declared set, and Admin now names the limit a private feature depends on rather than describing it as text.
Reaching that far meant converting the service where Ghost's configuration meets the package. Unlike the counting queries it cannot move into the package, because it reads host settings, decides the help link, tolerates a misconfiguration at boot rather than refusing to start, and hands out the instance every model and endpoint asks for. As JavaScript none of that was seen by the compiler, and neither was the limit name its route guard takes.
Alongside those, the sweep that asks whether a site is over any limit now passes over the limits that cannot answer that question, the count queries no longer narrow by a period that does not exist, and the arithmetic for a period that has not begun can no longer reach back before the subscription.
Decisions worth disagreeing with
Checking a limit by a name spelled another way no longer works. Ghost has accepted both spellings since the compatibility was added, with a note at the time that supporting both was easy and would stop anyone tripping on it later. The path was never exercised: every limit name on every billing product in production is spelled the one way, every caller in Ghost and Admin uses that spelling, and the mechanism built to prevent the trip contained the fault. Loading still accepts either, so nothing a host sends is refused.
How to read it
The refactor comes first, because it touches the most and changes the least, then the conversion that makes the rest checkable, then naming, then one fault per commit, then a note in the README about reading the package's history from before it moved. Each commit passes on its own.
Where a test recorded a fault, it now records the answer and says what it used to say.
Not doing
The package still holds a database it does not need, which is where its remaining casts come from, and why Admin builds an emails limit it can never check. Moving the counting out is the larger change and is tracked separately.