Support fixed-path actor dispatchers#753
Conversation
dahlia
added
type/enhancement
|
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:
📝 WalkthroughWalkthroughAdds a fixed-path actor alias mechanism: ChangesFixed-Path Actor Alias Feature
Sequence DiagramsequenceDiagram
participant Client as Client
participant Router as Router
participant Handler as Handler
participant Context as Context
participant Dispatcher as Dispatcher
Client->>Router: GET /bot
Router->>Router: Match route "actorAlias:bot"
Router->>Handler: Invoke handler for actorAlias route
Handler->>Handler: Extract identifier "bot" from route name
Handler->>Context: Create context with invokedFromActorDispatcher
Context->>Dispatcher: Call actor dispatcher with "bot"
Dispatcher->>Client: Return actor JSON (id: https://example.com/bot)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: Turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/fedify/src/federation/builder.test.ts`:
- Around line 28-39: Add a new test that asserts mapActorAlias rejects duplicate
alias registrations once the duplication guard is implemented: use
createFederationBuilder<T>(), call setActorDispatcher("/users/{identifier}",
actorDispatcher) and then call mapActorAlias twice with different paths but the
same alias identifier to verify an error is thrown via assertThrows expecting
RouterError; reference the same pattern used in the existing test
(createFederationBuilder, setActorDispatcher, mapActorAlias, assertThrows,
RouterError) so the test ensures duplicate alias registration fails.
In `@packages/fedify/src/federation/builder.ts`:
- Around line 525-534: The mapActorAlias setter is missing the
duplicate-registration guard used elsewhere; before calling
this.router.add(path, `actorAlias:${identifier}`) check
this.router.has(`actorAlias:${identifier}`) and if true throw the same
RouterError used by other setters (e.g. in setActorDispatcher) to prevent silent
overwrites; keep the existing variable-check with new Router() and only perform
this.router.add(...) when the has-check passes so behavior is consistent with
other methods in the class.
In `@packages/fedify/src/federation/federation.ts`:
- Around line 1111-1123: Add a JSDoc `@throws` entry to the public method
mapActorAlias to document that it can throw a RouterError when route input is
invalid; update the comment block above mapActorAlias (the method returning
ActorCallbackSetters<TContextData>) to include a line like "@throws
{RouterError} If the provided path or identifier is invalid or fails runtime
validation" so the public API docs accurately describe the runtime validation
failure mode.
In `@packages/fedify/src/federation/middleware.ts`:
- Around line 1950-1953: The code computes identifier from
route.values.identifier which can be undefined for fixed-path routes; tighten
runtime checks in the branch that handles actor/actorAlias routes (the code
using route, route.name, and identifier) by asserting or providing a safe
fallback: when route.name startsWith("actorAlias:") keep substring logic, but
when using route.values.identifier add a runtime guard (e.g., if route.name ===
"actor" then require route.values.identifier exists, otherwise throw a clear
error or assert) so the returned { type: "actor", identifier } cannot contain
undefined; update the logic around the identifier assignment and the subsequent
if (route.name === "actor" || route.name.startsWith("actorAlias:")) branch to
enforce this invariant at runtime.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 262c4b63-e59c-4267-b259-96554cde3f2e
📒 Files selected for processing (8)
CHANGES.mddocs/manual/actor.mdpackages/fedify/src/federation/builder.test.tspackages/fedify/src/federation/builder.tspackages/fedify/src/federation/federation.tspackages/fedify/src/federation/middleware.test.tspackages/fedify/src/federation/middleware.tspr-description.md
![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 the mapActorAlias() method to the ActorCallbackSetters interface, enabling the mapping of fixed paths like /actor or /bot to internal sentinel identifiers for instance-level actors. The implementation includes path validation, router updates, and logic in getActorUri and parseUri to handle these aliases, supported by new documentation and tests. A review comment suggests replacing the repeated magic string "actorAlias:" with a constant to improve maintainability and prevent typos across the codebase.
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
dahlia
force-pushed
the
feature/fixed-path-actor-dispatchers
branch
from
56c1e6c to
ca15ae2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/fedify/src/federation/middleware.test.ts (1)
303-334: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a test asserting that
mapActorAliasrejects paths with route variables.The PR description states "alias paths must be fixed; paths containing route variables are rejected." Every other path-registering method in this file has a corresponding "wrong variables in path" step (inbox listeners: line 1479, inbox dispatcher: line 2840).
mapActorAliasis missing this coverage, so the validation could be accidentally removed with no test failure.✅ Suggested test addition (inside the `Federation.createContext()` "`Context`" step, or as its own step)
assertThrows( () => createFederation<number>({ kv }) .setActorDispatcher("/users/{identifier}", () => new vocab.Person({})) .mapActorAlias("/users/{identifier}" as `/${string}`, "alias"), RouterError, "actor alias path must not contain route variables", );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fedify/src/federation/middleware.test.ts` around lines 303 - 334, Add a test that verifies mapActorAlias rejects alias paths containing route variables: call createFederation(...).setActorDispatcher("/users/{identifier}", ...) and then invoke mapActorAlias("/users/{identifier}" as `/${string}`, "alias") inside an assertThrows expecting RouterError with message like "actor alias path must not contain route variables"; place this assertion in the existing Context step (or as its own step) alongside other path-validation tests so removal of the validation will fail the tests.
♻️ Duplicate comments (1)
packages/fedify/src/federation/federation.ts (1)
1111-1123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
@throwsto themapActorAlias()JSDoc (still unaddressed).The
@throws {RouterError}contract is missing. The method performs runtime path validation and throws, but nothing in the public API docs signals this to callers.📝 Proposed doc update
* `@param` path The fixed path to map to the identifier. * `@param` identifier The sentinel identifier to map the path to. * `@returns` The setters object so that settings can be chained. + * `@throws` {RouterError} Thrown if `path` contains route variables. * `@since` 2.3.0 */As per coding guidelines (
packages/fedify/src/**/*.{ts,tsx}): "Include JSDoc comments for public APIs."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fedify/src/federation/federation.ts` around lines 1111 - 1123, The JSDoc for the public method mapActorAlias is missing a `@throws` annotation for runtime path validation errors; update the mapActorAlias JSDoc block to add a `@throws` {RouterError} entry (mentioning when/why it is thrown — e.g., invalid path format or duplicate mapping) so callers see the contract and align with the RouterError thrown by the method at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/fedify/src/federation/middleware.test.ts`:
- Around line 1217-1222: The test assumes body.aliases exists before calling
.includes; add a null-guard for body.aliases (e.g., assertExists(body.aliases)
or an Array.isArray/body.aliases != null check) before the includes assertion so
a missing aliases field yields a clear assertion failure instead of a TypeError;
update the assertion block around the existing selfLink checks (referencing
body.aliases and selfLink) to validate aliases is present and an array before
calling .includes("https://example.com/bot").
In `@pr-description.md`:
- Line 1: Add a setext H1 title to the start of pr-description.md by inserting a
one-line title (e.g., "PR Description") as the first line and immediately
following it with an underline of = characters matching the title length to
satisfy MD041 and the project's markdown guideline; ensure the original body
text is moved below this new two-line setext heading so the document begins with
the required H1.
---
Outside diff comments:
In `@packages/fedify/src/federation/middleware.test.ts`:
- Around line 303-334: Add a test that verifies mapActorAlias rejects alias
paths containing route variables: call
createFederation(...).setActorDispatcher("/users/{identifier}", ...) and then
invoke mapActorAlias("/users/{identifier}" as `/${string}`, "alias") inside an
assertThrows expecting RouterError with message like "actor alias path must not
contain route variables"; place this assertion in the existing Context step (or
as its own step) alongside other path-validation tests so removal of the
validation will fail the tests.
---
Duplicate comments:
In `@packages/fedify/src/federation/federation.ts`:
- Around line 1111-1123: The JSDoc for the public method mapActorAlias is
missing a `@throws` annotation for runtime path validation errors; update the
mapActorAlias JSDoc block to add a `@throws` {RouterError} entry (mentioning
when/why it is thrown — e.g., invalid path format or duplicate mapping) so
callers see the contract and align with the RouterError thrown by the method at
runtime.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d4122c4-f118-43b9-84f2-7d91a5560be3
📒 Files selected for processing (8)
CHANGES.mddocs/manual/actor.mdpackages/fedify/src/federation/builder.test.tspackages/fedify/src/federation/builder.tspackages/fedify/src/federation/federation.tspackages/fedify/src/federation/middleware.test.tspackages/fedify/src/federation/middleware.tspr-description.md
Harden `mapActorAlias()` by adding a duplicate registration guard and using a constant for the alias prefix. Updated JSDoc to include the `RouterError` contract. Improved test coverage with a duplicate alias registration test and added a null-guard for WebFinger aliases in the middleware test. Also added a title to `pr-description.md`. fedify-dev#753 (comment) fedify-dev#753 (comment) fedify-dev#753 (comment) fedify-dev#753 (comment) fedify-dev#753 (comment) fedify-dev#753 (comment) Assisted-by: Gemini CLI:gemini-3-flash-preview Assisted-by: Codex:gpt-5.5
Added `mapActorAlias()` method to `ActorCallbackSetters` interface to support mapping fixed paths to sentinel identifiers for the actor dispatcher. This enables exposing a single, instance-level actor at a fixed path (like `/actor` for a relay or `/bot` for a bot) without leaking the sentinel identifier into the actor's URI. When an alias is mapped, the router will use it for resolving inbound requests and `Context.getActorUri()` will use it for constructing outbound actor URIs. WebFinger responses for the actor will also use the aliased URI. Fixes fedify-dev#752 Assisted-by: Gemini CLI:gemini-3.1-pro-preview Assisted-by: Gemini CLI:gemini-3-flash-preview Assisted-by: Codex:gpt-5.5
Harden `mapActorAlias()` by adding a duplicate registration guard and using a constant for the alias prefix. Updated JSDoc to include the `RouterError` contract. Improved test coverage with a duplicate alias registration test and added a null-guard for WebFinger aliases in the middleware test. fedify-dev#753 (comment) fedify-dev#753 (comment) fedify-dev#753 (comment) fedify-dev#753 (comment) fedify-dev#753 (comment) fedify-dev#753 (comment) Assisted-by: Gemini CLI:gemini-3-flash-preview Assisted-by: Codex:gpt-5.5
dahlia
force-pushed
the
feature/fixed-path-actor-dispatchers
branch
from
f4126bc to
82658ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/fedify/src/federation/builder.ts`:
- Around line 527-540: mapActorAlias currently only checks identifier collisions
but allows registering an alias path that already resolves to an existing route;
update mapActorAlias to call this.router.route(path) and, if it returns a match
(i.e., path already resolves), throw a RouterError rejecting the alias, instead
of proceeding to add; keep the existing identifier collision and variable
checks, then only call this.router.add(path,
`${ACTOR_ALIAS_PREFIX}${identifier}`) when route(path) is null/undefined, and
add a regression test exercising two different identifiers with the same fixed
path to assert the second call throws.
In `@packages/fedify/src/federation/middleware.test.ts`:
- Around line 1217-1219: The test accesses body.links as an array without
validation, which can throw a TypeError; update the test around the selfLink
assignment to first guard that body.links exists and is an array (e.g., use
Array.isArray(body.links) or an assertion) and only then call .find() to locate
the link; reference the selfLink variable and body.links in your change and if
the guard fails make the test assert a clear failure message instead of letting
.find() throw.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c81b26f3-0ea3-4ddb-834e-30f99ee95349
📒 Files selected for processing (6)
packages/fedify/src/federation/builder.test.tspackages/fedify/src/federation/builder.tspackages/fedify/src/federation/federation.tspackages/fedify/src/federation/middleware.test.tspackages/fedify/src/federation/middleware.tspr-description.md
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/fedify/src/federation/middleware.test.ts (1)
1217-1223:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
body.linksbefore calling.find().
body.linksis still assumed to exist here, so a missing or malformed WebFinger payload will throw aTypeErrorinstead of failing the test cleanly.🛡️ Proposed fix
assertEquals(response.status, 200); const body = await response.json() as Record<string, unknown>; assertEquals(body.subject, "acct:bot@example.com"); - const selfLink = (body.links as Record<string, unknown>[]).find((l) => + assertExists(body.links, "WebFinger response must include a links array"); + assert(Array.isArray(body.links), "WebFinger links must be an array"); + const selfLink = (body.links as Record<string, unknown>[]).find((l) => l.rel === "self" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fedify/src/federation/middleware.test.ts` around lines 1217 - 1223, The test assumes body.links exists and calls .find which can throw; before using .find validate and guard body.links (e.g., assertExists(body.links) and/or assert(Array.isArray(body.links))) so malformed/missing WebFinger payloads fail the test cleanly; update the block that computes selfLink (the selfLink variable and subsequent assertions) to first assert that body.links is present and an array, then call (body.links as Record<string, unknown>[]).find(...), and proceed to assertExists(selfLink) and other expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/manual/actor.md`:
- Around line 464-489: The example uses "bot" as both the public path and the
internal dispatcher identifier, so change the dispatcher to use a distinct
opaque sentinel (e.g., "__instance__") and then call
ActorCallbackSetters.mapActorAlias("/bot", "__instance__") so the example
demonstrates alias indirection; update the setActorDispatcher callback to check
for the sentinel identifier (identifier === "__instance__") and construct the
actor using ctx.getActorUri("__instance__") while leaving the public path as
"/bot".
---
Duplicate comments:
In `@packages/fedify/src/federation/middleware.test.ts`:
- Around line 1217-1223: The test assumes body.links exists and calls .find
which can throw; before using .find validate and guard body.links (e.g.,
assertExists(body.links) and/or assert(Array.isArray(body.links))) so
malformed/missing WebFinger payloads fail the test cleanly; update the block
that computes selfLink (the selfLink variable and subsequent assertions) to
first assert that body.links is present and an array, then call (body.links as
Record<string, unknown>[]).find(...), and proceed to assertExists(selfLink) and
other expectations.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: de436a85-4d0f-42ea-8060-3918020c6c91
📒 Files selected for processing (7)
CHANGES.mddocs/manual/actor.mdpackages/fedify/src/federation/builder.test.tspackages/fedify/src/federation/builder.tspackages/fedify/src/federation/federation.tspackages/fedify/src/federation/middleware.test.tspackages/fedify/src/federation/middleware.ts
|
@codex review |
|
/gemini review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
There was a problem hiding this comment.
Code Review
This pull request introduces the mapActorAlias() method to the ActorCallbackSetters interface, allowing developers to map fixed paths (e.g., /bot or /actor) to specific actor identifiers. This feature is particularly useful for instance-level actors where a sentinel identifier in the URI is undesirable. The implementation includes validation to ensure alias paths do not contain variables and prevents duplicate alias registrations. Corresponding updates were made to the routing logic, URI generation, and WebFinger support to ensure aliases are correctly resolved and advertised. Comprehensive tests and documentation have also been added. I have no feedback to provide as there were no review comments to assess.
Verified that `mapActorAlias()` rejects paths containing route variables in `middleware.test.ts`, matching the behavior tested in `builder.test.ts`. Assisted-by: Gemini CLI:gemini-3-flash-preview Assisted-by: Codex:gpt-5.5
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/fedify/src/federation/middleware.test.ts (1)
1227-1233:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
body.linksbefore calling.find()At Line 1227,
body.linksis assumed to be an array. If the field is missing/malformed, the test throws aTypeErrorinstead of failing with a clear assertion. Also worth assertingaliasesis an array before.includes().Suggested patch
assertEquals(response.status, 200); const body = await response.json() as Record<string, unknown>; assertEquals(body.subject, "acct:bot@example.com"); + assertExists(body.links, "WebFinger response must include a links array"); + assert(Array.isArray(body.links), "WebFinger links must be an array"); const selfLink = (body.links as Record<string, unknown>[]).find((l) => l.rel === "self" ); assertExists(selfLink); assertEquals(selfLink.href, "https://example.com/bot"); assertExists(body.aliases); + assert(Array.isArray(body.aliases), "WebFinger aliases must be an array"); assert((body.aliases as string[]).includes("https://example.com/bot"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fedify/src/federation/middleware.test.ts` around lines 1227 - 1233, Guard against missing/malformed arrays before using .find() and .includes(): assert that body.links is an array (e.g., use Array.isArray(body.links) with assertExists or assert) before calling (body.links as Record<string, unknown>[]).find(...) to avoid TypeError, then proceed to locate selfLink; likewise assert that body.aliases is an array before using (body.aliases as string[]).includes(...). Update the test around the symbols selfLink, body.links, and body.aliases to perform these array checks and keep the existing assertions for href and alias membership.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/fedify/src/federation/middleware.test.ts`:
- Around line 1227-1233: Guard against missing/malformed arrays before using
.find() and .includes(): assert that body.links is an array (e.g., use
Array.isArray(body.links) with assertExists or assert) before calling
(body.links as Record<string, unknown>[]).find(...) to avoid TypeError, then
proceed to locate selfLink; likewise assert that body.aliases is an array before
using (body.aliases as string[]).includes(...). Update the test around the
symbols selfLink, body.links, and body.aliases to perform these array checks and
keep the existing assertions for href and alias membership.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15bad549-ad28-4004-93bd-f225fd8c6133
📒 Files selected for processing (1)
packages/fedify/src/federation/middleware.test.ts
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the mapActorAlias() method to the ActorCallbackSetters interface, allowing developers to map fixed paths to specific actor identifiers. This feature enables instance-level actors, such as relays or bots, to have clean URIs (e.g., /actor) without exposing internal sentinel identifiers. The implementation includes path validation, duplicate alias prevention, and updates to URI parsing and generation logic. Comprehensive documentation and tests for routing and WebFinger integration are also provided. I have no feedback to provide.
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ 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". |
2 participants
This PR adds
mapActorAlias()toActorCallbackSetters. An actor dispatcher can now map a fixed path, such as/actoror/bot, to an internal sentinel identifier.Some applications expose a single instance-level actor at a fixed path. Before this change, they had to use a sentinel identifier such as
__instance__, and that identifier became part of the actor URI.mapActorAlias()keeps the sentinel internal. Incoming actor requests resolve through the alias map.Context.getActorUri()uses the same map when building outbound actor URIs.Implementation details:
actorAlias:{identifier}.Context.getActorUri()tries theactorAlias:{identifier}route first, then falls back to the standardactorroute.Context.parseUri()andfetch()routing now recognizeactorAlias:route names and read the identifier from the route name instead of route values.Resolves #752.