Introduce runtime-neutral application services and composition roots#353
Conversation
Move server configuration and resource construction into a shared runtime package, and expose runtime-neutral application contexts to the model layer. Recompose the legacy web and GraphQL entry points around those resources, preserve federation queue lifecycles and transactional context binding, and register the package in Deno, pnpm, and container build manifests. Assisted-by: Codex:gpt-5.6-sol
dahlia
added
enhancement
|
@codex review |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a runtime-neutral ChangesRuntime-neutral application architecture
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant EntryPoint
participant RuntimeResources
participant Federation
participant ApplicationContext
participant Server
EntryPoint->>RuntimeResources: load configuration and create resources
RuntimeResources->>Federation: build federation resource
EntryPoint->>ApplicationContext: configure runtime dependencies
EntryPoint->>Server: start with AbortSignal
Server->>Federation: handle requests and run queue
Server->>RuntimeResources: close resources on shutdown
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new runtime package to centralize configuration loading, resource initialization (database, key-value store, storage, email, AI, and federation), and lifecycle management. It refactors the graphql and web entry points to use these shared resources and migrates the codebase from a Fedify-specific RequestContext to a runtime-neutral ApplicationContext. Feedback on the changes highlights several critical issues: a potential process hang in Deno due to unremoved signal listeners, a TTL unit mismatch in the deduplication lock causing premature expiration, and several path resolution vulnerabilities on Windows or with percent-encoded characters. Additionally, improvements were suggested for robustly detecting abort errors and removing a redundant type cast.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/routes/_middleware.ts (1)
1-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMove the new import before
@hackerspub/models/i18nfor alphabetical ordering.The coding guideline requires external imports to be sorted alphabetically within their group.
@hackerspub/federation/contextshould precede@hackerspub/models/i18n("federation" < "models").♻️ Proposed fix
import { + toApplicationContext, +} from "`@hackerspub/federation/context`"; +import { isLocale, type Locale, normalizeLocale, } from "`@hackerspub/models/i18n`"; -import { toApplicationContext } from "`@hackerspub/federation/context`"; import { acceptsLanguages } from "`@std/http/negotiation`";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/routes/_middleware.ts` around lines 1 - 7, Reorder the external imports so `@hackerspub/federation/context` appears before `@hackerspub/models/i18n`, preserving the existing imports and their contents.Source: Coding guidelines
🧹 Nitpick comments (2)
models/article.ts (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore external-before-internal import grouping.
postgresis placed between internal./context.tsimports, and the context types are split across multiple imports. Consolidate the context types and keep all external imports before internal imports.As per coding guidelines, external imports must come first and internal imports second.
Proposed import cleanup
-import type { ApplicationModel } from "./context.ts"; ... -import type { StorageService } from "./context.ts"; import postgres from "postgres"; -import type { ApplicationContext, Models } from "./context.ts"; +import type { + ApplicationContext, + ApplicationModel, + Models, + StorageService, +} from "./context.ts";Also applies to: 15-17
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@models/article.ts` at line 4, Update the imports in models/article.ts so all external package imports precede internal imports, and consolidate the context-related types into a single import from "./context.ts". Preserve the existing imported symbols while restoring the required grouping.Source: Coding guidelines
models/account.ts (1)
14-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport order: external import sandwiched between internal imports.
sharp(external) sits between two./context.ts(internal) type imports, violating the repo's import-ordering rule.🔧 Proposed reordering
+import sharp from "sharp"; + import type { StorageService } from "./context.ts"; -import sharp from "sharp"; import type { ApplicationContext } from "./context.ts";As per coding guidelines,
**/*.{ts,tsx,js,jsx}: "External imports first, internal imports second (alphabetically within groups)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@models/account.ts` around lines 14 - 16, Reorder the imports in the account module so the external sharp import appears before the internal ./context.ts type imports, keeping the internal imports alphabetized.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@graphql/composition.test.ts`:
- Around line 8-15: Add an assertStringIncludes check for instrumentImport in
the test before comparing import positions, mirroring the existing loggingImport
presence assertion; keep the ordering assertion in place to verify
instrumentation precedes logging.
In `@graphql/worker.ts`:
- Around line 214-222: Update the cleanup logic surrounding the federation
worker’s try/finally and resources.close() so errors from closing resources are
caught and logged without replacing an error thrown by federation.startQueue.
Preserve the existing cleanup attempt and normal shutdown behavior, using the
existing logger for close-error reporting.
In `@runtime/resources.ts`:
- Around line 296-305: Guarantee resource cleanup across construction, shutdown,
and server lifecycles. In runtime/resources.ts:296-305, wrap all resources
created after PostgreSQL in the existing cleanup-protected scope; in
runtime/resources.ts:322-330, make cleanup attempt every resource even when one
fails while preserving or aggregating the original error. In
graphql/main.ts:23-38, protect server construction after resources are created,
and in graphql/main.ts:92-96, move resources.close() into finally so it runs
even when server.finished rejects.
---
Outside diff comments:
In `@web/routes/_middleware.ts`:
- Around line 1-7: Reorder the external imports so
`@hackerspub/federation/context` appears before `@hackerspub/models/i18n`,
preserving the existing imports and their contents.
---
Nitpick comments:
In `@models/account.ts`:
- Around line 14-16: Reorder the imports in the account module so the external
sharp import appears before the internal ./context.ts type imports, keeping the
internal imports alphabetized.
In `@models/article.ts`:
- Line 4: Update the imports in models/article.ts so all external package
imports precede internal imports, and consolidate the context-related types into
a single import from "./context.ts". Preserve the existing imported symbols
while restoring the required grouping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64943790-358e-4bb8-bd3c-97a35204732c
⛔ Files ignored due to path filters (2)
deno.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (108)
Dockerfileai/moderation.tsai/services.tsai/summary.tsai/translate.tsdeno.jsonfederation/context.test.tsfederation/context.tsfederation/deno.jsonfederation/inbox/actor.tsfederation/inbox/flag.tsfederation/inbox/following.tsfederation/inbox/quote.tsfederation/inbox/subscribe.tsfederation/objects.test.tsfederation/objects.tsfederation/person.tsfederation/services.tsgraphql/ai.tsgraphql/builder.tsgraphql/composition.test.tsgraphql/db.tsgraphql/drive.tsgraphql/email-helpers.tsgraphql/email.tsgraphql/federation.tsgraphql/hashtag.tsgraphql/invitation-link.tsgraphql/invite.tsgraphql/kv.tsgraphql/login.tsgraphql/main.tsgraphql/moderation-appeal.test.tsgraphql/moderation-email.tsgraphql/moderation-notification.test.tsgraphql/moderation.tsgraphql/poll.test.tsgraphql/post.more.test.tsgraphql/post.tsgraphql/query-tx-plugin.test.tsgraphql/query-tx-plugin.tsgraphql/services.tsgraphql/worker.tsmodels/account.tsmodels/actor.tsmodels/appeal.test.tsmodels/article.background.test.tsmodels/article.lifecycle.test.tsmodels/article.tsmodels/blocking.tsmodels/context.tsmodels/flag.test.tsmodels/flag.tsmodels/following.tsmodels/markup.tsmodels/medium.tsmodels/moderation-action.test.tsmodels/moderation-notification.test.tsmodels/moderation.test.tsmodels/moderation.tsmodels/note.lifecycle.test.tsmodels/note.tsmodels/notification.test.tsmodels/organization.test.tsmodels/organization.tsmodels/package.jsonmodels/pin.tsmodels/poll.tsmodels/post.remote.test.tsmodels/post.sync.test.tsmodels/post.tsmodels/question.lifecycle.test.tsmodels/question.tsmodels/reaction.tsmodels/relay.test.tsmodels/relay.tsmodels/services.tsmodels/tx.test.tsmodels/tx.tspnpm-workspace.yamlruntime/config.test.tsruntime/config.tsruntime/deno.jsonruntime/imports.test.tsruntime/package.jsonruntime/resources.test.tsruntime/resources.tsscripts/addaccount.test.tsscripts/addaccount.tsscripts/repair-link-previews.tstest/database.tstest/package-boundaries.test.tstest/postgres.tstest/services.tsweb/ai.tsweb/composition.test.tsweb/db.tsweb/dev.tsweb/drive.tsweb/email.tsweb/federation.tsweb/kv.tsweb/main.tsweb/routes/@[username]/[idOrYear]/[slug]/index.tsxweb/routes/_middleware.tsweb/routes/search.tsxweb/services.tsweb/utils.ts
💤 Files with no reviewable changes (1)
- graphql/post.more.test.ts
Place the federation context import before the models import to keep the external package group in alphabetical order. hackers-pub#353 (review) Assisted-by: Codex:gpt-5.6-sol
Convert file URLs through platform path helpers, recognize both DOM and Error abort failures, and make construction and shutdown attempt every cleanup without losing the triggering error. Remove signal listeners when GraphQL processes stop. hackers-pub#353 (comment) hackers-pub#353 (comment) hackers-pub#353 (comment) hackers-pub#353 (comment) hackers-pub#353 (comment) hackers-pub#353 (comment) hackers-pub#353 (comment) Assisted-by: Codex:gpt-5.6-sol
Express the reply backfill lock timeout in milliseconds so Keyv keeps the lock for five minutes instead of expiring it almost immediately. hackers-pub#353 (comment) Assisted-by: Codex:gpt-5.6-sol
Use the optional key-pair capability already declared by ApplicationContext instead of widening the same type with a redundant cast. hackers-pub#353 (comment) Assisted-by: Codex:gpt-5.6-sol
Assert that each GraphQL composition root imports the Sentry setup before checking its order relative to LogTape initialization. hackers-pub#353 (comment) Assisted-by: Codex:gpt-5.6-sol
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new runtime package to centralize configuration loading and resource management (database, key-value, drive, email, AI, and federation), refactoring both the GraphQL and legacy web entry points to use it. It also transitions the codebase from a Fedify-specific context to a runtime-neutral ApplicationContext to simplify database rebinding during transactions. Feedback on these changes highlights critical runtime TypeError risks: first, using Object.assign to override read-only properties on the cloned Fedify context will fail in strict mode and requires Object.defineProperty; second, the WarningLogger interface and its logger calls use warning instead of LogTape's standard warn method, which must be corrected along with their corresponding test mocks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@codex review |
![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: 522c6843c4
ℹ️ 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".
Expose the root database and an after-commit task queue while GraphQL queries use their snapshot transaction. Deferred federation work can then rebind its capabilities to the root database after the transaction has closed. Route reply backfills through the same queue so they cannot retain a completed transaction context. hackers-pub#353 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request refactors the application's configuration and resource initialization by introducing a unified runtime package, and transitions the codebase to a runtime-neutral ApplicationContext to simplify database handle swapping during transactions. The reviewer identified several critical issues, including a deadlock risk in runWithFederationQueue, missing system signal listeners and signal propagation in the legacy web server that prevent graceful resource cleanup, a runtime TypeError caused by using Object.assign on read-only prototype getters in getFedifyContext, and a TypeScript compilation error due to a type mismatch in the StorageService interface.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Forward process shutdown signals through the shared lifecycle so the HTTP server and federation queue stop together before their database and Redis resources close. Remove signal listeners after shutdown so a second signal retains its normal termination behavior. Cover externally initiated shutdown when the queue reports an abort error and the server finishes normally. hackers-pub#353 (comment) hackers-pub#353 (comment) hackers-pub#353 (comment) Assisted-by: Codex:gpt-5.6-sol
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the application's configuration and resource management by introducing a new @hackerspub/runtime package, which centralizes configuration loading and resource initialization (database, key-value, drive, email, and federation). It also replaces the direct use of Fedify's RequestContext with a runtime-neutral ApplicationContext across models, federation handlers, and GraphQL/web entry points, allowing database handles to be rebound during transactions. A critical bug was identified in toApplicationContext where the withDatabase method fails to propagate rootDb and afterCommit properties to the rebound context, which would cause deferred after-commit tasks to execute prematurely and lead to race conditions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38d2da4bc4
ℹ️ 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".
Carry the active root database and after-commit queue into cloned Fedify contexts. This keeps nested database rebinding inside the transaction boundary instead of falling back to adapter construction-time state. hackers-pub#353 (comment) Assisted-by: Codex:gpt-5.6-sol
Allow the mock email transport only in CI and explicit non-production modes. Production startup now rejects missing Mailgun credentials instead of accepting messages that will never be delivered. hackers-pub#353 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request centralizes configuration and resource management by introducing a new @hackerspub/runtime package, refactoring the graphql and web entry points to use it, and migrating from Fedify's RequestContext to a runtime-neutral ApplicationContext across the models and federation packages. Feedback on these changes highlights several opportunities to improve robustness: safely checking the type of model in ai/services.ts to prevent a runtime TypeError, using AggregateError in graphql/worker.ts for consistent error propagation, and wrapping deferred afterCommit tasks in try-catch blocks within graphql/query-tx-plugin.ts to prevent unhandled task failures from crashing GraphQL responses.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Report queue and resource cleanup failures together so neither cause is lost when both operations fail during worker shutdown. hackers-pub#353 (comment) Assisted-by: Codex:gpt-5.6-sol
Log each deferred task failure and continue draining the queue after the query transaction commits. A background failure can no longer replace a successful GraphQL result or prevent later tasks from running. hackers-pub#353 (comment) hackers-pub#353 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a major architectural refactoring by extracting a runtime-neutral @hackerspub/runtime package to manage configuration parsing and resource lifecycle management (database, key-value store, file storage, email, AI models, and federation). It replaces direct usage of Fedify's request context with a custom ApplicationContext across the models, federation, graphql, and web directories, allowing database rebinding for transactions and deferred afterCommit background tasks. The review comments identify several critical issues: a type mismatch in the StorageService interface where getBytes is unimplemented by Disk, a runtime TypeError risk in federation/context.ts due to attempting to overwrite read-only Fedify Context properties with Object.assign, a potential crash in web/email.ts if receipt.errorMessages is null/undefined, and a hanging script process in repair-link-previews.ts because the database connection pool is never closed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Closes #338.
Why this boundary exists
Application operations have historically accepted Fedify's request context as their dependency container. That made the model layer depend on an HTTP and federation adapter even when an operation only needed a database, storage, or an external service. It also meant that replacing the database inside a transaction required manufacturing another Fedify context, while importing a resource module could immediately read environment variables and open connections.
This PR introduces a boundary that keeps those concerns on the runtime side. The aim is not to hide the services behind another global abstraction. It is to make each composition root responsible for constructing its resources and to give application code a small, explicit set of capabilities. That creates a usable seam for isolated tests and for the planned Node.js migration without changing the behavior of the current Deno processes.
How the boundary works
The model layer now defines
ApplicationContextin models/context.ts. It describes the capabilities application operations use, including persistence, storage, model handles, object lookup, and activity delivery, without exposing Fedify's context type or AI SDK model types. The AI and federation packages implement those contracts at the edge of the application.The adapter in federation/context.ts translates a Fedify context into this application context and can translate it back when a federation service needs the underlying adapter.
withDatabase()is part of that contract so a transaction does not merely replace the visibledbproperty. It also rebinds lookup, rendering, and delivery capabilities that close over the adapter's database handle. This preserves a single transaction snapshot for direct queries and Fedify-backed work.Configuration parsing and resource construction live in runtime/config.ts and runtime/resources.ts. The loaders accept an environment-shaped value and return typed configuration, which keeps validation deterministic and testable. The factories construct database, KV, storage, email, AI, and federation resources only when a composition root asks for them. Smaller operational commands use narrower loaders, so a database repair does not require storage or federation credentials, and account creation only initializes its origin and KV store.
Mailgun is selected only when its credentials are present as a complete set. An incomplete configuration still fails with a typed error, while an unconfigured development or build environment uses a mock transport and emits a warning. This keeps imports and static builds independent of delivery credentials without making partial production configuration look valid.
Runtime ownership and compatibility
The legacy web server, GraphQL API, and GraphQL worker now assemble their own resources. The GraphQL API serves requests and enqueues federation work, while the worker owns queue consumption. The legacy web process starts the queue beside the HTTP server because it remains a single-process deployment. Their shutdown paths abort and await background work before closing Redis and PostgreSQL, which makes resource ownership match process ownership.
The Fresh development entry point uses the same queue lifecycle as production, but static builds close their resources without starting a consumer. The legacy composition root also retains Cavage-first HTTP signatures for affected peers and resolves filesystem storage from the same base used by the media handler. These choices keep the refactor from changing federation interoperability or the meaning of relative media paths.
The new runtime package is registered in both workspace systems and copied into the dependency-only stages in Dockerfile. This keeps clean pnpm installs, frozen lockfile installs, and container builds aligned with the dependency graph rather than relying on files that happen to exist in a developer checkout.