Replace url-template and uri-template-router with @fedify/uri-template#758
Open
2chanhaeng wants to merge 41 commits into
Open
Replace url-template and uri-template-router with @fedify/uri-template#7582chanhaeng wants to merge 41 commits into
url-template and uri-template-router with @fedify/uri-template#7582chanhaeng wants to merge 41 commits into
Conversation
|
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 ChangesURI Template library and Fedify integration
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant Router
participant Template
Client->>Middleware: HTTP request (URL)
Middleware->>Router: route(url.pathname as Path)
Router->>Template: match(tokens, path)
Template-->>Router: bindings (ExpandContext) or null
Router-->>Middleware: { name, template, values } or null
Middleware-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
Author
|
To use Codex here, create a Codex account and connect to github. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces the @fedify/uri-template package, a dependency-free RFC 6570 URI Template implementation for expansion and round-trip route matching. It replaces Fedify's internal federation routing and deprecates the existing Router export in packages/fedify/. Feedback was provided to correct and simplify the ucschar range validation logic in packages/uri-template/src/template/encoding.ts to properly include the lower range of Plane 14 code points.
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces the @fedify/uri-template package, a dependency-free RFC 6570 implementation for URI template expansion and round-trip pattern matching, replacing the previous reliance on third-party libraries. The changes include the full implementation of the new package and the migration of Fedify's internal routing logic to use it. Feedback focuses on improving type flexibility in the Router by using ExpandContext for composite values, ensuring the matcher remains robust by disabling strict mode during internal round-trip checks, and hardening the isPath utility against malformed templates and unsafe property access.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/fedify/src/federation/builder.ts (2)
259-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the actor path before registering it.
Line 260 mutates
this.routerbefore the"identifier"check runs. If validation fails, the builder is left with a bogus"actor"route and later retries hit"Actor dispatcher already set."even though no actor dispatcher was configured.As per coding guidelines "Validate all input from federated sources".Suggested fix
- const variables = Router.variables(path as Path); - this.router.add(path as Path, "actor"); - if ( - variables.size !== 1 || - !variables.has("identifier") - ) { - throw new RouterError( - "Path for actor dispatcher must have one variable: {identifier}", - ); - } + validateSingleIdentifierVariablePath( + path, + "Path for actor dispatcher must have one variable: {identifier}", + ); + this.router.add(path as Path, "actor");🤖 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 `@packages/fedify/src/federation/builder.ts` around lines 259 - 267, The code is adding the "actor" route before validating the path, leaving a bogus route if validation fails; change the order in the actor registration in builder.ts so you first compute and validate Router.variables(path) (ensure variables.size === 1 && variables.has("identifier") and throw the RouterError if invalid) and only after successful validation call this.router.add(path as Path, "actor"); ensure any existing error paths (e.g., the RouterError message) remain the same and that no mutation to this.router happens prior to validation to avoid leaving a partial "actor" route.
722-731:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Router.variables()is too weak for these single-identifier routes.These blocks only check the unique variable set, so runtime callers can still slip through shapes like
{identifier:3},{identifier*},{?identifier}, or repeated{identifier}segments. That can truncate, explode, or duplicate the identifier handed to Fedify callbacks. ReusevalidateSingleIdentifierVariablePath()here as well.As per coding guidelines "Validate all input from federated sources".Representative change
- const variables = Router.variables(path as Path); - if ( - variables.size !== 1 || - !variables.has("identifier") - ) { - throw new RouterError( - "Path for inbox dispatcher must have one variable: {identifier}", - ); - } + validateSingleIdentifierVariablePath( + path, + "Path for inbox dispatcher must have one variable: {identifier}", + );Also applies to: 916-926, 983-993, 1046-1056, 1117-1127, 1188-1198, 1257-1266
🤖 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 `@packages/fedify/src/federation/builder.ts` around lines 722 - 731, The current check using Router.variables(path) only ensures there is one variable named "identifier" but allows malformed segment types (e.g., truncated, exploded, optional, or repeated) which can lead to wrong identifier values; replace the weak check with a call to validateSingleIdentifierVariablePath(path) before calling this.router.add(path as Path, "inbox") and throw the existing RouterError on failure so only well-formed single-segment "{identifier}" paths are accepted. Apply the same replacement in the analogous blocks that currently use Router.variables (the other occurrences noted) so all routes use validateSingleIdentifierVariablePath; keep Router.add and the RouterError behavior unchanged.
🤖 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 `@packages/uri-template/package.json`:
- Line 67: Add a "sideEffects": false field to this package's package.json to
enable tree-shaking by bundlers; update the root JSON object (alongside existing
keys like "dependencies") to include the "sideEffects": false property so
consumers can dead-code-eliminate unused exports from this
zero-runtime-dependency utility library.
In `@packages/uri-template/src/template/match.ts`:
- Around line 271-276: The loop in consumeUnnamed() computes minLength
incorrectly by subtracting remainingVars(vars, varIndex) which prevents the
current unnamed variable from absorbing extra parts; change the minLength
calculation to reserve only one part for each of the remaining variables after
the current one (i.e. use remainingVars(vars, varIndex) - 1) so the current
variable can span multiple parts: set minLength = Math.max(1, parts.length -
partIndex - (remainingVars(vars, varIndex) - 1)) while keeping maxLength
unchanged.
In `@packages/uri-template/src/template/template.bench.ts`:
- Around line 1-17: The benchmark is registering tests instead of exercising
Template.expand because it calls test(...) inside Deno.bench; remove the call to
test and invoke the expansion runner directly by calling the function produced
by createTemplatePairTest (the runPairCases variable) with pairTestSuites inside
the loop (i.e., call runPairCases(pairTestSuites) 10000 times), and remove the
now-unused import of test from "@fedify/fixture"; keep Deno.bench,
createTemplatePairTest, runPairCases, pairTestSuites, and Template as the
relevant symbols to change.
In `@packages/uri-template/src/template/token.ts`:
- Around line 52-57: parseExpression/reportExpressionError currently calls
report in strict mode and then throws, causing token.ts's catch block to call
report again; fix this by introducing a sentinel error type (e.g.,
ReportedExpressionError) that reportExpressionError throws after invoking
report, and update the catch in token.ts (around
tokens.push(parseExpression(...))) to detect this sentinel and skip calling
report a second time while still appending the literal; keep other thrown errors
(from parseVarList or others) reported in the catch as before.
In `@packages/uri-template/src/tests/template.ts`:
- Around line 340-349: The benchmark drops Template.match results which lets the
JIT optimize the calls away; modify createMatchBench so it captures each match
return into a local bench sink and consumes it (like other benches do) — e.g.,
introduce a file-local variable (benchSink) and inside the returned function
assign the result of template.match(uri) into it and call the existing
consumeRouterBenchValue/consumeRouterRoute helper (or a new local consume) to
ensure the value is used; update createMatchBench to reference Template.match
and the sink to mirror the other router bench helpers.
In `@packages/uri-template/src/utils.ts`:
- Around line 21-29: isPath currently constructs a Template using strict parsing
which throws on invalid templates; update isPath (the function named isPath that
calls new Template(path) and inspects template.tokens and isLiteral) to handle
parse failures by either creating the Template with non-strict parsing (e.g.
pass strict: false to the Template constructor) or wrap the Template
construction and subsequent access in a try/catch and return false on any thrown
error, preserving the existing logic that inspects template.tokens, first token,
isLiteral(first), and first.operator.
- Line 27: The isPath type guard is returning true for bare path-expansion
tokens like "{/var}"; change its logic so that when checking first.operator ===
"/" you also verify the next token exists and is a literal segment that starts
with "/" (e.g., tokens[1].type === "literal" && tokens[1].value.startsWith("/"))
before returning true, and update the JSDoc if you prefer to relax the Path
definition instead; target the isPath function and the tokens array handling
around the first/operator check.
---
Outside diff comments:
In `@packages/fedify/src/federation/builder.ts`:
- Around line 259-267: The code is adding the "actor" route before validating
the path, leaving a bogus route if validation fails; change the order in the
actor registration in builder.ts so you first compute and validate
Router.variables(path) (ensure variables.size === 1 &&
variables.has("identifier") and throw the RouterError if invalid) and only after
successful validation call this.router.add(path as Path, "actor"); ensure any
existing error paths (e.g., the RouterError message) remain the same and that no
mutation to this.router happens prior to validation to avoid leaving a partial
"actor" route.
- Around line 722-731: The current check using Router.variables(path) only
ensures there is one variable named "identifier" but allows malformed segment
types (e.g., truncated, exploded, optional, or repeated) which can lead to wrong
identifier values; replace the weak check with a call to
validateSingleIdentifierVariablePath(path) before calling this.router.add(path
as Path, "inbox") and throw the existing RouterError on failure so only
well-formed single-segment "{identifier}" paths are accepted. Apply the same
replacement in the analogous blocks that currently use Router.variables (the
other occurrences noted) so all routes use validateSingleIdentifierVariablePath;
keep Router.add and the RouterError behavior unchanged.
🪄 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: 1d562548-911c-4271-b681-e6738399ded6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (62)
CHANGES.mdcspell.jsondeno.jsonpackages/fedify/README.mdpackages/fedify/package.jsonpackages/fedify/src/federation/builder.test.tspackages/fedify/src/federation/builder.tspackages/fedify/src/federation/middleware.test.tspackages/fedify/src/federation/middleware.tspackages/fedify/src/federation/router.tspackages/fedify/src/nodeinfo/handler.tspackages/fedify/src/testing/context.tspackages/uri-template/README.mdpackages/uri-template/deno.jsonpackages/uri-template/old/uri-template-router.test.tspackages/uri-template/old/url-template.test.tspackages/uri-template/package.jsonpackages/uri-template/src/const.tspackages/uri-template/src/mod.tspackages/uri-template/src/router/errors.tspackages/uri-template/src/router/mod.tspackages/uri-template/src/router/node.tspackages/uri-template/src/router/priority.tspackages/uri-template/src/router/router.bench.tspackages/uri-template/src/router/router.test.tspackages/uri-template/src/router/router.tspackages/uri-template/src/router/trie.tspackages/uri-template/src/template/encoding.tspackages/uri-template/src/template/errors.tspackages/uri-template/src/template/expand.tspackages/uri-template/src/template/expression.tspackages/uri-template/src/template/match.tspackages/uri-template/src/template/mod.tspackages/uri-template/src/template/template.bench.tspackages/uri-template/src/template/template.test.tspackages/uri-template/src/template/template.tspackages/uri-template/src/template/token.tspackages/uri-template/src/tests/assert.tspackages/uri-template/src/tests/json/references/fixed.jsonpackages/uri-template/src/tests/json/references/pairs.jsonpackages/uri-template/src/tests/json/references/vars.jsonpackages/uri-template/src/tests/json/router/build-cases.jsonpackages/uri-template/src/tests/json/router/build-suites.jsonpackages/uri-template/src/tests/json/router/clone-suites.jsonpackages/uri-template/src/tests/json/router/compile-error-cases.jsonpackages/uri-template/src/tests/json/router/hit-paths.jsonpackages/uri-template/src/tests/json/router/miss-paths.jsonpackages/uri-template/src/tests/json/router/route-definitions.jsonpackages/uri-template/src/tests/json/router/route-suites.jsonpackages/uri-template/src/tests/json/router/variables-cases.jsonpackages/uri-template/src/tests/json/template/hard.jsonpackages/uri-template/src/tests/json/template/match.jsonpackages/uri-template/src/tests/json/template/wrong.jsonpackages/uri-template/src/tests/mod.tspackages/uri-template/src/tests/router.tspackages/uri-template/src/tests/template.tspackages/uri-template/src/types.tspackages/uri-template/src/utils.tspackages/uri-template/summary.txtpackages/uri-template/tsdown.config.tspnpm-workspace.yamlscripts/check_fixture_usage.ts
Comment on lines
+52
to
+57
| try { | ||
| tokens.push(parseExpression(expression, template, index, options)); | ||
| } catch (error) { | ||
| report(error instanceof Error ? error : new Error(String(error))); | ||
| appendLiteral(template.slice(index, closeIndex + 1)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if parseExpression invokes options.report (or the report param) internally.
fd -e ts expression.ts packages/uri-template/src/template/ --exec \
rg -n 'report\b' {}Repository: fedify-dev/fedify
Length of output: 117
🏁 Script executed:
# Get the full parseExpression function from expression.ts
fd -e ts expression.ts packages/uri-template/src/template/ --exec cat -n {}Repository: fedify-dev/fedify
Length of output: 6218
🏁 Script executed:
# Get more context around the try-catch block in token.ts
sed -n '40,70p' packages/uri-template/src/template/token.ts | cat -nRepository: fedify-dev/fedify
Length of output: 1216
🏁 Script executed:
# Find the report function definition (likely in a types/options file)
fd -e ts -e tsx packages/uri-template/ --exec rg -l 'TemplateOptions|interface.*report' {} \;Repository: fedify-dev/fedify
Length of output: 455
🏁 Script executed:
# Find TemplateOptions type definition
fd -e ts -e tsx --full-path 'packages/uri-template/' | xargs rg -l 'TemplateOptions|interface.*report'Repository: fedify-dev/fedify
Length of output: 358
🏁 Script executed:
# Get the TemplateOptions type and report function definition
fd -e ts -e tsx --full-path 'packages/uri-template/' | xargs rg -A 10 'type TemplateOptions|interface TemplateOptions'Repository: fedify-dev/fedify
Length of output: 953
🏁 Script executed:
# Read the full types.ts file to understand TemplateOptions and report function
cat -n packages/uri-template/src/types.tsRepository: fedify-dev/fedify
Length of output: 3401
🏁 Script executed:
# Also check template.ts to see how the report function is created/wrapped
cat -n packages/uri-template/src/template/template.ts | head -100Repository: fedify-dev/fedify
Length of output: 3499
🏁 Script executed:
# Let me trace through what happens in strict mode
# Check if reportExpressionError can actually throw when report is wrapped
cat -n packages/uri-template/src/template/token.ts | sed -n '30,70p'Repository: fedify-dev/fedify
Length of output: 1596
Double invocation of report callback in strict mode when parseExpression reports-then-throws.
In strict mode, report is wrapped by strictWrapper (template.ts line 96) which calls the user's reporter callback and then throws. When parseExpression encounters certain errors (empty expression, reserved operator, unknown operator), it calls reportExpressionError, which invokes report(error). In strict mode, this invocation triggers the user's reporter and throws. The exception then propagates to the catch block in token.ts, which calls report a second time—invoking the user's reporter callback again for the same error.
This double invocation is specific to errors handled by reportExpressionError. Errors thrown by parseVarList bypass reportExpressionError and are reported only once by the catch block.
🐛 Proposed fix
Since parseExpression already reports errors via reportExpressionError in strict mode, the catch block should avoid re-reporting these errors. One approach is to only report errors from parseVarList in the catch block and handle reportExpressionError paths differently, or refactor to move all error reporting to the tokenizer level to prevent double invocation.
🤖 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 `@packages/uri-template/src/template/token.ts` around lines 52 - 57,
parseExpression/reportExpressionError currently calls report in strict mode and
then throws, causing token.ts's catch block to call report again; fix this by
introducing a sentinel error type (e.g., ReportedExpressionError) that
reportExpressionError throws after invoking report, and update the catch in
token.ts (around tokens.push(parseExpression(...))) to detect this sentinel and
skip calling report a second time while still appending the literal; keep other
thrown errors (from parseVarList or others) reported in the catch as before.
| const [first] = template.tokens; | ||
| if (first == null) return false; | ||
| if (isLiteral(first)) return first.text.startsWith("/"); | ||
| if (first.operator === "/") return true; |
There was a problem hiding this comment.
isPath type guard is over-permissive for bare path-expansion expressions.
isPath("{/var}") returns true (line 27 branch), but "{/var}" satisfies neither variant of Path:
/${string}— no, doesn't start with/`{/${string}}/${string}`— no trailing/${string}after the closing}
The JSDoc is also explicit: "a path-expansion expression ({/var}) followed by a literal segment that starts with /" — the implementation never checks the second token.
Either fix the predicate to check the next token, or update the Path type and JSDoc to explicitly permit bare {/var} templates (if the router can handle them):
🐛 Option A — enforce the documented constraint
- if (first.operator === "/") return true;
+ if (first.operator === "/") {
+ const second = template.tokens[1];
+ return second != null && isLiteral(second) && second.text.startsWith("/");
+ }♻️ Option B — relax `Path` to match actual behaviour
-export type Path = `/${string}` | `{/${string}}/${string}`;
+export type Path = `/${string}` | `{/${string}}${string}`;(Update JSDoc to match.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (first.operator === "/") return true; | |
| if (first.operator === "/") { | |
| const second = template.tokens[1]; | |
| return second != null && isLiteral(second) && second.text.startsWith("/"); | |
| } |
🤖 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 `@packages/uri-template/src/utils.ts` at line 27, The isPath type guard is
returning true for bare path-expansion tokens like "{/var}"; change its logic so
that when checking first.operator === "/" you also verify the next token exists
and is a literal segment that starts with "/" (e.g., tokens[1].type ===
"literal" && tokens[1].value.startsWith("/")) before returning true, and update
the JSDoc if you prefer to relax the Path definition instead; target the isPath
function and the tokens array handling around the first/operator check.
Contributor
Author
|
@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: 4876f79c64
ℹ️ 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".
Comment on lines
+1
to
+6
| import { | ||
| isExpression, | ||
| type Path, | ||
| Router, | ||
| RouterError, | ||
| } from "@fedify/uri-template"; |
There was a problem hiding this comment.
This change switches FederationBuilder to throw RouterError from @fedify/uri-template, while @fedify/fedify/federation still publicly exports a different RouterError class from src/federation/router.ts. Existing consumers that correctly do import { RouterError } from "@fedify/fedify/federation" and catch with instanceof RouterError will stop catching builder/middleware/nodeinfo routing errors after this commit, which is a breaking behavior change despite keeping the old export. Please unify the thrown class (or re-export the new one as the old symbol) so instanceof remains stable.
Useful? React with 👍 / 👎.
2chanhaeng
added a commit
to 2chanhaeng/fedify
that referenced
this pull request
May 9, 2026
Address review comments from automated reviewers:
- Harden isPath against malformed templates so it returns false
instead of throwing, restoring its JSDoc contract.
- Relax the Path type from `{/${string}}/${string}` to
`{/${string}}${string}` so isPath is consistent with the router's
handling of bare path-expansion templates like `{/var}`.
- Run path validation before mutating the router in
setActorDispatcher, so a failed registration does not leave a
stale `actor` route that confuses subsequent attempts.
- Reuse validateSingleIdentifierVariablePath across inbox, outbox,
following, followers, liked, featured, featuredTags, and inbox
listener registrations instead of the loose Router.variables
check, so explode (`*`) and prefix (`:N`) modifiers cannot smuggle
through `{identifier}` paths at runtime.
- Mark @fedify/uri-template as side-effect free for tree-shaking.
- Add a regression test for the unnamed consumeUnnamed minLength
pruning bug. The matcher fix is intentionally left for a separate
commit so the test stays red until then.
fedify-dev#758
Assisted-by: Claude Code:claude-opus-4-7[1m]
There was a problem hiding this comment.
Actionable comments posted: 9
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/builder.ts (1)
528-549: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
mapActorAliasregisters a path without parse-error handling.
Router.variables(path)andthis.router.add(path, ...)will both throw on malformed templates. UnlikevalidateSingleIdentifierVariablePath, there's no try/catch here, so a bad path surfaces a low-level parse error instead of the user-friendlyRouterErrorthrown elsewhere in this method. Consider routing it through a small validation wrapper for consistency.🤖 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 `@packages/fedify/src/federation/builder.ts` around lines 528 - 549, mapActorAlias currently calls Router.variables(path) and this.router.add(path, ...) without parse-error handling so malformed path templates surface low-level errors; wrap the path validation in a try/catch or reuse the existing validateSingleIdentifierVariablePath-style validator before calling Router.variables/add, catching parsing exceptions and rethrowing a RouterError with a clear message (e.g., "Malformed actor alias path") to keep behavior consistent with other checks in mapActorAlias.
🤖 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 `@packages/fedify/src/federation/builder.ts`:
- Around line 89-96: Replace the current denylist check in the expression
validation with an explicit allowlist: define allowedOperators (e.g. ["", "/"])
and validate that operator is one of those; if not, or if varSpec.explode or
varSpec.prefix != null, throw RouterError(errorMessage). Update the check in the
same scope where expressions are destructured (const { operator, vars: [varSpec]
} = expressions[0]) so that semicolon-style and label-style operators (e.g., ";"
and ".") are rejected rather than allowed.
- Around line 73-97: validateSingleIdentifierVariablePath currently calls
Router.compile(path as Path) directly which can throw TemplateParseError
subclasses; wrap the compile call in a try/catch around Router.compile(path as
Path) and if any exception is thrown rethrow a new RouterError(errorMessage) so
parse errors are normalized to RouterError; keep the rest of the function
unchanged and reference Router.compile, RouterError, and
validateSingleIdentifierVariablePath when applying the change.
In `@packages/fedify/src/federation/router.ts`:
- Around line 53-59: The clone() implementation currently calls the public
Router constructor (which calls convertRouterError) and then overwrites
clone.#router, causing double convertRouterError invocation and loss of any
wrapper-state; change clone() to bypass the public constructor (e.g., create an
empty instance with Object.create(Router.prototype) or a private static factory)
and then assign clone.#router = this.#router.clone() and copy any wrapper
fields/state from this to clone so the wrapper state is preserved and
convertRouterError is only invoked once (when cloning the inner `#router` as
needed). Ensure you reference the Router constructor, Router.clone(), private
field `#router` and convertRouterError in the change.
- Around line 140-153: The deprecation warn in convertRouterError is emitted on
every call (constructor, clone, has, add, route, build) and duplicates when
errors are rewrapped; change it to emit once only by adding a module-level (or
static) boolean guard (e.g., warnedDeprecatedRouter) checked and set inside
convertRouterError so the logger.warn runs a single time per process (or use an
instance-level flag if preferred), and remove the logger.warn from the error
rewrap path (i.e., when catching _RouterError and throwing new
RouterError(error.message)); also apply the same one-shot guard inside
RouterError's constructor so its own warning is emitted only once.
In `@packages/uri-template/src/template/template.test.ts`:
- Around line 1-3: Replace the mixed imports so all assertions come from the
strict API: change importing deepEqual and equal (currently from "node:assert")
to import from "node:assert/strict" alongside ok and throws; update the import
line(s) that reference deepEqual, equal, ok, throws so they all come from
"node:assert/strict" (keeping the existing names used in the tests: deepEqual,
equal, ok, throws) and remove the non-strict "node:assert" import.
- Around line 116-135: The failing regression test for Template#match must be
gated instead of skipped with .skip; update the test invocation for the
"Template#match — unnamed minLength must allow current var to consume one part"
case (the test(...) call that constructs new Template("{x:5,y}") and asserts
m.x) to use the Deno.TestDefinition ignore option (e.g. call test({ name: "...",
ignore: true }, () => { ... })) so the intentionally-failing case is excluded
from CI while preserving the test body for future re-enabling.
In `@packages/uri-template/src/tests/template.ts`:
- Line 351: The helper named mod actually performs integer division
(Math.floor(i / j)), so rename the function from mod to div (or inline its use)
and update all call sites to avoid confusion with the remainder operator (i %
26) used elsewhere; specifically change the function identifier mod to div (or
remove it and replace calls with Math.floor(i / j)) in the test file so
semantics and naming match.
In `@packages/uri-template/src/types.ts`:
- Around line 68-90: TemplateOptions currently requires strict and report
despite docs saying they have defaults; make strict and report optional on
TemplateOptions (i.e., strict?: boolean; report?: Reporter) so callers may omit
them and defaults can apply, and update any public API that accepts options to
use Partial<TemplateOptions> if applicable; change Reporter signature or docs so
they match (either make Reporter return void and remove the `@returns` tag or, if
keeping a return value, update the type) and fix the JSDoc typo in
TemplateOptions.report ("errors will be reported by the `report` function"
instead of "reported to by"); touch the TemplateOptions interface, Reporter
type, and the JSDoc for report and strict to keep types and docs consistent.
In `@packages/uri-template/src/utils.ts`:
- Around line 36-42: The error message in assertPath currently contains bad
grammar and an incorrect example; update the RouterError message thrown in
assertPath (which checks isPath) to a clear, grammatical user-facing sentence
that explains the value is not a valid path and shows the correct form (e.g.,
must start with a single leading "/" instead of suggesting "//"); keep the
message concise and include the invalid path value for context.
---
Outside diff comments:
In `@packages/fedify/src/federation/builder.ts`:
- Around line 528-549: mapActorAlias currently calls Router.variables(path) and
this.router.add(path, ...) without parse-error handling so malformed path
templates surface low-level errors; wrap the path validation in a try/catch or
reuse the existing validateSingleIdentifierVariablePath-style validator before
calling Router.variables/add, catching parsing exceptions and rethrowing a
RouterError with a clear message (e.g., "Malformed actor alias path") to keep
behavior consistent with other checks in mapActorAlias.
🪄 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: 07e7f822-5241-4477-9422-e6fab917a2a9
⛔ Files ignored due to path filters (2)
deno.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
CHANGES.mdpackages/fedify/deno.jsonpackages/fedify/package.jsonpackages/fedify/src/federation/builder.tspackages/fedify/src/federation/router.tspackages/uri-template/package.jsonpackages/uri-template/src/mod.tspackages/uri-template/src/template/template.test.tspackages/uri-template/src/tests/template.tspackages/uri-template/src/types.tspackages/uri-template/src/utils.ts
2chanhaeng
added a commit
to 2chanhaeng/fedify
that referenced
this pull request
May 9, 2026
Address review comments from automated reviewers:
- Harden isPath against malformed templates so it returns false
instead of throwing, restoring its JSDoc contract.
- Relax the Path type from `{/${string}}/${string}` to
`{/${string}}${string}` so isPath is consistent with the router's
handling of bare path-expansion templates like `{/var}`.
- Run path validation before mutating the router in
setActorDispatcher, so a failed registration does not leave a
stale `actor` route that confuses subsequent attempts.
- Reuse validateSingleIdentifierVariablePath across inbox, outbox,
following, followers, liked, featured, featuredTags, and inbox
listener registrations instead of the loose Router.variables
check, so explode (`*`) and prefix (`:N`) modifiers cannot smuggle
through `{identifier}` paths at runtime.
- Mark @fedify/uri-template as side-effect free for tree-shaking.
- Add a regression test for the unnamed consumeUnnamed minLength
pruning bug. The matcher fix is intentionally left for a separate
commit so the test stays red until then.
fedify-dev#758
Assisted-by: Claude Code:claude-opus-4-7[1m]
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (11)
packages/uri-template/src/template/match.ts (1)
271-276:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
minLengthis pruning valid unnamed matches.This lower bound rejects cases where a later variable needs multiple parts. For example,
{x,y}should be able to matcha,b,casx = "a"andy = ["b", "c"], but the current calculation never explores that branch. The recursive step already discards impossible distributions, so the lower bound here should stay permissive.Suggested fix
const maxLength = parts.length - partIndex; - const minLength = Math.max( - 1, - parts.length - partIndex - remainingVars(vars, varIndex), - ); + const minLength = 1; for (let length = minLength; length <= maxLength; length++) {If
remainingVars()becomes unused after this change, remove it as follow-up.🤖 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 `@packages/uri-template/src/template/match.ts` around lines 271 - 276, The minLength calculation in match (using remainingVars(vars, varIndex)) prunes valid unnamed matches; change the lower bound to a permissive value (e.g., const minLength = 1) so the for loop over length explores single-part assignments for the current variable and lets the existing recursive logic discard impossible distributions; update the for loop that iterates length to use this new minLength and remove the now-unused remainingVars(varIndex) usage (and delete remainingVars if it becomes unused).packages/uri-template/src/template/token.ts (1)
52-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid reporting the same parse error twice in strict mode.
parseExpression()already callsreport()for empty/reserved/unknown-operator failures. In strict mode that throw lands in this catch, and Line 55 invokes the reporter again before rethrowing, so user-visible side effects run twice for one parse failure.Possible fix
- } catch (error) { - report(error instanceof Error ? error : new Error(String(error))); + } catch (error) { + if (!(error instanceof AlreadyReportedTemplateError)) { + report(error instanceof Error ? error : new Error(String(error))); + } appendLiteral(template.slice(index, closeIndex + 1)); }Use any equivalent sentinel/refactor that makes reporting happen in exactly one place per error path.
🤖 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 `@packages/uri-template/src/template/token.ts` around lines 52 - 56, parseExpression currently calls report() before throwing certain parse errors, and the catch block in the tokenization loop re-calls report() which causes duplicate reports in strict mode; fix by introducing a sentinel on thrown errors (e.g. set error.reported = true inside parseExpression immediately after calling report) and then change the catch in the token loop (the block that calls report(...) and appendLiteral(...)) to only call report if the caught error does not have that sentinel (i.e. if (!error.reported) report(...)); reference parseExpression, report, appendLiteral, and the catch location that pushes into tokens so the duplication is prevented while preserving existing behavior and rethrowing as before.packages/uri-template/src/template/template.bench.ts (1)
1-17:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove
test()call from benchmark loop — it registers tests instead of measuring expansion.This issue was flagged in a previous review and remains unresolved. The
test()function from@fedify/fixtureregisters test cases rather than executing logic. Calling it 10,000 times insideDeno.bench()measures test registration overhead, not template expansion performance.The fix is to invoke
runPairCases(pairTestSuites)directly in the loop and remove the unusedtestimport.🔧 Proposed fix
-import { test } from "@fedify/fixture"; import { createMatchBench, createMatchBenchTestCases, createTemplatePairTest, pairTestSuites, } from "../tests/mod.ts"; import Template from "./template.ts"; Deno.bench("Template (expand)", (b) => { const runPairCases = createTemplatePairTest(Template); + const run = runPairCases(pairTestSuites); b.start(); for (const _ of Array(10000)) { - test("expand: examples", runPairCases(pairTestSuites)); + run(); } b.end(); });🤖 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 `@packages/uri-template/src/template/template.bench.ts` around lines 1 - 17, The benchmark is calling test("expand: examples", runPairCases(pairTestSuites)) inside the loop which merely registers tests; remove the test import and replace the call in Deno.bench with direct invocation of runPairCases(pairTestSuites) inside the loop so the template expansion is executed and measured (update the Deno.bench callback in template.bench.ts to call runPairCases(pairTestSuites) 10000 times and delete the unused test import from the top).packages/fedify/src/federation/router.ts (2)
140-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeprecation warning fires on every router method call — log once.
convertRouterErroris invoked fromconstructor,clone,has,add,route, andbuild, and unconditionally callslogger.warn(...). A single request that uses the compatibilityRouterwill produce many duplicate "deprecated" lines (andclone()triggers it twice — once for itself, once for the innernew Router()it instantiates). On error paths,throw new RouterError(error.message)also triggersRouterError’s own constructor warn, doubling up again.Gate the warn behind a one-shot module-level flag, and drop the warn from the rewrap path:
♻️ Proposed fix
+let warnedRouter = false; +let warnedRouterError = false; + function convertRouterError<T>(func: () => T): T { + if (!warnedRouter) { + warnedRouter = true; + logger.warn( + "The `Router` class from `@fedify/fedify` is deprecated." + + " Please use `Router` from `@fedify/uri-template` instead.", + ); + } try { - logger.warn( - "The `Router` class from `@fedify/fedify` is deprecated." + - " Please use `Router` from `@fedify/uri-template` instead.", - ); return func(); } catch (error) { if (error instanceof _RouterError) { throw new RouterError(error.message); } throw error; } }…and apply an analogous one-shot guard inside
RouterError's constructor (usingwarnedRouterError).🤖 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 `@packages/fedify/src/federation/router.ts` around lines 140 - 153, The deprecation warn is emitted on every call to convertRouterError (used by constructor, clone, has, add, route, build) and duplicated when RouterError rewraps _RouterError; add a module-level one-shot boolean (e.g., warnedRouterDeprecated) and only call logger.warn(...) in convertRouterError when that flag is false, then set it true; also remove the warn from the rewrap path by gating RouterError's constructor warning with a separate one-shot flag (e.g., warnedRouterError) so wrapping _RouterError into RouterError doesn't re-emit the deprecation message.
53-59: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
clone()double-invokesconvertRouterErrorand discards#routerimmediately.
new Router()here delegates toconvertRouterError(() => new _Router(undefined))and creates an inner router that is overwritten on the next line byclone.#router = this.#router.clone(). That wastes one_Routerallocation and (combined with the warn-on-every-call issue above) emits the deprecation warning twice per clone.Bypass the public constructor when cloning:
♻️ Proposed fix
clone(): Router { return convertRouterError(() => { - const clone = new Router(); + const clone = Object.create(Router.prototype) as Router; clone.#router = this.#router.clone(); return clone; }); }🤖 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 `@packages/fedify/src/federation/router.ts` around lines 53 - 59, The clone() implementation currently calls the public Router constructor (which itself delegates to convertRouterError) then overwrites the inner `#router`, causing double convertRouterError invocation and wasted allocation; fix it by bypassing the public constructor when cloning: inside Router.clone() (the method), call convertRouterError once and within its callback create a new object via Object.create(Router.prototype) (so you don't invoke new Router()), assign that object's private field clone.#router = this.#router.clone(), and return the created object—this eliminates the redundant _Router allocation and the double deprecation warning.packages/fedify/src/federation/builder.ts (2)
73-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap
Router.compile()to convert template parse errors toRouterError.
Router.compile(path as Path)can throwTemplateParseErrorsubclasses (e.g.UnclosedExpressionError,RouteTemplatePathError) when the path is malformed. Because every caller usesas Pathcasts that bypass compile-time validation, those exceptions will surface at registration sites that document aRouterErrorcontract.🐛 Proposed fix
function validateSingleIdentifierVariablePath( path: string, errorMessage: string, ): void { - const pattern = Router.compile(path as Path); + let pattern; + try { + pattern = Router.compile(path as Path); + } catch { + throw new RouterError(errorMessage); + } if (pattern.variables.size !== 1 || !pattern.variables.has("identifier")) { throw new RouterError(errorMessage); }The same risk applies to the bare
Router.variables(path as Path)calls inmapActorAlias,setNodeInfoDispatcher,setObjectDispatcher,setInboxListeners(sharedInboxPath), and#setCustomCollectionDispatcher— consider a small helper that catches and re-throws asRouterError.🤖 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 `@packages/fedify/src/federation/builder.ts` around lines 73 - 97, validateSingleIdentifierVariablePath currently calls Router.compile(path as Path) directly which can throw TemplateParseError subclasses; wrap that call in a try/catch that catches template parse errors and rethrows a RouterError with the provided errorMessage. Add a small helper (e.g., safeCompileOrVariables) that wraps both Router.compile and Router.variables calls and converts TemplateParseError to RouterError, then replace direct calls in validateSingleIdentifierVariablePath and the other sites (mapActorAlias, setNodeInfoDispatcher, setObjectDispatcher, setInboxListeners for sharedInboxPath, and `#setCustomCollectionDispatcher`) to use the helper so malformed templates consistently surface as RouterError.
89-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace operator denylist with explicit allowlist.
The current
["?", "&", "#"].includes(operator)denylist lets RFC 6570 operators that aren’t suitable for routing path-style identifiers slip through silently — notably;(path-style parameters),.(label expansion), and+(reserved). Only""(simple) and/(path segment) are appropriate for an{identifier}route variable.🐛 Proposed fix
const { operator, vars: [varSpec] } = expressions[0]; + const allowedOperators = new Set(["", "/"]); if ( - ["?", "&", "#"].includes(operator) || + !allowedOperators.has(operator) || varSpec.explode || varSpec.prefix != null ) { throw new RouterError(errorMessage); }🤖 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 `@packages/fedify/src/federation/builder.ts` around lines 89 - 96, The code currently uses a denylist (["?", "&", "#"].includes(operator)) which misses disallowed RFC6570 operators; change this to an explicit allowlist: only accept operator === "" (simple) or operator === "/" (path segment). In the block that destructures expressions ([const { operator, vars: [varSpec] } = expressions[0];]) replace the denylist condition with a check that throws RouterError(errorMessage) if !(operator === "" || operator === "/") OR if varSpec.explode is truthy OR varSpec.prefix != null, preserving the existing RouterError and errorMessage usage.packages/uri-template/src/utils.ts (2)
36-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix grammar in
assertPatherror message.
"is not looks like"and"Is this start with slash(\//`?)"are ungrammatical, and the//example contradicts the documented requirement of a single leading/`.📝 Proposed fix
- throw new RouterError( - `"${path}" is not looks like a path. Is this start with slash(\`//\`?)`, - ); + throw new RouterError( + `"${path}" does not look like a path. Does it start with a slash (\`/\`)?`, + );🤖 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 `@packages/uri-template/src/utils.ts` around lines 36 - 42, The RouterError message in assertPath is ungrammatical and contradicts the single-leading-slash requirement; update the error thrown in function assertPath to a clear, grammatical message that references the offending path (e.g. `"${path}" does not look like a valid path. Paths must start with a single leading '/'."`) so it no longer uses "is not looks like" or the `//` example and instead clearly states the requirement.
22-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
isPathover-permissive: bare{/var}template returnstrue.Line 29 returns
truefor any expression starting with/operator, but the JSDoc requires "a path-expansion expression ({/var}) followed by a literal segment that starts with/". Templates like"{/var}"(no trailing literal) satisfy neither variant of thePathtype yet pass the guard.Either tighten the predicate to also require a following literal segment that starts with
/, or relaxPathto match the actual behaviour (and update the JSDoc accordingly).🤖 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 `@packages/uri-template/src/utils.ts` around lines 22 - 34, The isPath predicate is too permissive: when the first token is an expression with operator '/' (e.g., "{/var}") it currently returns true even if there is no following literal segment; update isPath to require, in the branch where first.operator === "/", that template.tokens has a next token and that the next token is a literal whose text startsWith("/") (use template.tokens[1] and isLiteral to check the following token), so only expressions like "{/var}/rest" count as Path (alternatively relax the Path JSDoc if you intentionally want bare "{/var}" to be valid).packages/uri-template/src/template/template.test.ts (1)
1-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMix of strict and non-strict
node:assertimports.
equal/deepEqualcome fromnode:assert(loose) whileok/throwscome fromnode:assert/strict. This is an easy footgun — e.g. looseequal(undefined, null)would pass. Import all four fromnode:assert/strict.📝 Proposed fix
-import { test } from "@fedify/fixture"; -import { deepEqual, equal } from "node:assert"; -import { ok, throws } from "node:assert/strict"; +import { test } from "@fedify/fixture"; +import { deepEqual, equal, ok, throws } from "node:assert/strict";🤖 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 `@packages/uri-template/src/template/template.test.ts` around lines 1 - 3, Tests import a mix of loose and strict assertion APIs (deepEqual/equal from "node:assert" and ok/throws from "node:assert/strict"), which can cause subtle failures; update the import line so deepEqual, equal, ok, and throws are all imported from "node:assert/strict" (adjust the existing import that currently references "node:assert" and the separate "node:assert/strict" import), keeping the same named symbols (deepEqual, equal, ok, throws) used in the file.packages/uri-template/src/tests/template.ts (1)
340-347: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsume
match()results in the benchmark loop to avoid dead-code elimination.At Line 346, the benchmark drops
template.match(uri)results. That can make this bench less reliable versus the router benches that explicitly consume outputs.♻️ Minimal fix
+let matchBenchSink = 0; +const consumeMatchBenchResult = (result: ExpandContext | null): void => { + matchBenchSink = (matchBenchSink + (result == null ? 0 : 1)) | 0; +}; + export function createMatchBench( Template: TemplateConstructor, ): (templateText: string) => (uris: readonly string[]) => void { return (templateText: string): (uris: readonly string[]) => void => { const template = new Template(templateText); return (uris: readonly string[]): void => { - for (const uri of uris) template.match(uri); + for (const uri of uris) consumeMatchBenchResult(template.match(uri)); }; }; }🤖 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 `@packages/uri-template/src/tests/template.ts` around lines 340 - 347, The benchmark loop currently calls template.match(uri) and discards the return value, allowing dead-code elimination; modify createMatchBench so the loop consumes each match result (from Template.match) by updating a local sink/accumulator (e.g., combine truthiness or length) for each uri and ensure that accumulator is observed after the loop (returned or assigned to an outer dummy) so the compiler cannot optimize away the calls; update the function that constructs the benchmark to use that accumulator without changing the public API.
🤖 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 `@packages/uri-template/src/router/router.ts`:
- Around line 296-301: The toggleTrailingSlash function currently returns null
for path === "/" because stripping trailing slashes yields an empty string which
is not a valid path according to isPath, so add a one-line comment above
toggleTrailingSlash explaining this is intentional: root "/" will not be toggled
to an empty path (and therefore routing will not retry to ""), reference the
function name toggleTrailingSlash and the isPath check in the comment so future
readers understand the deliberate behavior.
- Around line 181-211: When adding or registering a route that reuses an
existing name, the old RouteEntry is overwritten in `#routesByName` but left in
the trie (causing stale entries to accumulate); update the add and register
implementations to remove the previous entry from the trie before inserting the
new one — lookup the existing entry via this.#routesByName.get(name), call the
trie removal API (implement Trie#remove if missing) or mark a tombstone on the
old RouteEntry, then set the new entry and call this.#trie.insert /
this.#trie.insertAll as currently done; reference functions/fields: add,
register, createRouteEntry, RouteEntry, this.#routesByName, this.#trie, and
this.#isActiveEntry.
In `@packages/uri-template/src/template/template.ts`:
- Around line 62-80: The expand and match methods forward caller-supplied
options directly to the lower-level helpers, causing partial overrides (e.g. {
strict: false }) to drop defaults like the reporter; fix by normalizing per-call
overrides with the same option-normalization used in the constructor before
forwarding them — i.e. inside the expand and match methods call the existing
fillOptions (the same function used in the constructor) to merge the passed
options with this.#fullOptions and pass the normalized result to
expand(this.#tokens, context, options) and match(this.#tokens, uri, options)
respectively.
In `@packages/uri-template/src/tests/assert.ts`:
- Around line 340-357: The ERROR_NAMES set is built from all exports of
ERROR_CLASSES and ROUTER_ERROR_CLASSES which may include non-error helpers;
update the construction of ERROR_NAMES to only include keys whose values are
actual error constructor functions (e.g. use Object.entries on ERROR_CLASSES and
ROUTER_ERROR_CLASSES, filter entries where typeof v === "function" and
Error.prototype.isPrototypeOf((v as Function).prototype), then map to the keys)
so assertErrorName only accepts real Error subclasses.
In `@packages/uri-template/src/tests/mod.ts`:
- Around line 78-100: fixedTestSuites is exported without runtime validation
while all other fixtures use validate(...), so add a runtime assertion for
parity: create or reuse an assertFixedTemplateTestSuite and wrap
_fixedTestSuites with validate(assertFixedTemplateTestSuite, _fixedTestSuites)
(replacing the current explicit cast) to ensure the JSON is checked at module
load; reference the symbols fixedTestSuites, _fixedTestSuites, validate, and
assertFixedTemplateTestSuite when making the change.
---
Duplicate comments:
In `@packages/fedify/src/federation/builder.ts`:
- Around line 73-97: validateSingleIdentifierVariablePath currently calls
Router.compile(path as Path) directly which can throw TemplateParseError
subclasses; wrap that call in a try/catch that catches template parse errors and
rethrows a RouterError with the provided errorMessage. Add a small helper (e.g.,
safeCompileOrVariables) that wraps both Router.compile and Router.variables
calls and converts TemplateParseError to RouterError, then replace direct calls
in validateSingleIdentifierVariablePath and the other sites (mapActorAlias,
setNodeInfoDispatcher, setObjectDispatcher, setInboxListeners for
sharedInboxPath, and `#setCustomCollectionDispatcher`) to use the helper so
malformed templates consistently surface as RouterError.
- Around line 89-96: The code currently uses a denylist (["?", "&",
"#"].includes(operator)) which misses disallowed RFC6570 operators; change this
to an explicit allowlist: only accept operator === "" (simple) or operator ===
"/" (path segment). In the block that destructures expressions ([const {
operator, vars: [varSpec] } = expressions[0];]) replace the denylist condition
with a check that throws RouterError(errorMessage) if !(operator === "" ||
operator === "/") OR if varSpec.explode is truthy OR varSpec.prefix != null,
preserving the existing RouterError and errorMessage usage.
In `@packages/fedify/src/federation/router.ts`:
- Around line 140-153: The deprecation warn is emitted on every call to
convertRouterError (used by constructor, clone, has, add, route, build) and
duplicated when RouterError rewraps _RouterError; add a module-level one-shot
boolean (e.g., warnedRouterDeprecated) and only call logger.warn(...) in
convertRouterError when that flag is false, then set it true; also remove the
warn from the rewrap path by gating RouterError's constructor warning with a
separate one-shot flag (e.g., warnedRouterError) so wrapping _RouterError into
RouterError doesn't re-emit the deprecation message.
- Around line 53-59: The clone() implementation currently calls the public
Router constructor (which itself delegates to convertRouterError) then
overwrites the inner `#router`, causing double convertRouterError invocation and
wasted allocation; fix it by bypassing the public constructor when cloning:
inside Router.clone() (the method), call convertRouterError once and within its
callback create a new object via Object.create(Router.prototype) (so you don't
invoke new Router()), assign that object's private field clone.#router =
this.#router.clone(), and return the created object—this eliminates the
redundant _Router allocation and the double deprecation warning.
In `@packages/uri-template/src/template/match.ts`:
- Around line 271-276: The minLength calculation in match (using
remainingVars(vars, varIndex)) prunes valid unnamed matches; change the lower
bound to a permissive value (e.g., const minLength = 1) so the for loop over
length explores single-part assignments for the current variable and lets the
existing recursive logic discard impossible distributions; update the for loop
that iterates length to use this new minLength and remove the now-unused
remainingVars(varIndex) usage (and delete remainingVars if it becomes unused).
In `@packages/uri-template/src/template/template.bench.ts`:
- Around line 1-17: The benchmark is calling test("expand: examples",
runPairCases(pairTestSuites)) inside the loop which merely registers tests;
remove the test import and replace the call in Deno.bench with direct invocation
of runPairCases(pairTestSuites) inside the loop so the template expansion is
executed and measured (update the Deno.bench callback in template.bench.ts to
call runPairCases(pairTestSuites) 10000 times and delete the unused test import
from the top).
In `@packages/uri-template/src/template/template.test.ts`:
- Around line 1-3: Tests import a mix of loose and strict assertion APIs
(deepEqual/equal from "node:assert" and ok/throws from "node:assert/strict"),
which can cause subtle failures; update the import line so deepEqual, equal, ok,
and throws are all imported from "node:assert/strict" (adjust the existing
import that currently references "node:assert" and the separate
"node:assert/strict" import), keeping the same named symbols (deepEqual, equal,
ok, throws) used in the file.
In `@packages/uri-template/src/template/token.ts`:
- Around line 52-56: parseExpression currently calls report() before throwing
certain parse errors, and the catch block in the tokenization loop re-calls
report() which causes duplicate reports in strict mode; fix by introducing a
sentinel on thrown errors (e.g. set error.reported = true inside parseExpression
immediately after calling report) and then change the catch in the token loop
(the block that calls report(...) and appendLiteral(...)) to only call report if
the caught error does not have that sentinel (i.e. if (!error.reported)
report(...)); reference parseExpression, report, appendLiteral, and the catch
location that pushes into tokens so the duplication is prevented while
preserving existing behavior and rethrowing as before.
In `@packages/uri-template/src/tests/template.ts`:
- Around line 340-347: The benchmark loop currently calls template.match(uri)
and discards the return value, allowing dead-code elimination; modify
createMatchBench so the loop consumes each match result (from Template.match) by
updating a local sink/accumulator (e.g., combine truthiness or length) for each
uri and ensure that accumulator is observed after the loop (returned or assigned
to an outer dummy) so the compiler cannot optimize away the calls; update the
function that constructs the benchmark to use that accumulator without changing
the public API.
In `@packages/uri-template/src/utils.ts`:
- Around line 36-42: The RouterError message in assertPath is ungrammatical and
contradicts the single-leading-slash requirement; update the error thrown in
function assertPath to a clear, grammatical message that references the
offending path (e.g. `"${path}" does not look like a valid path. Paths must
start with a single leading '/'."`) so it no longer uses "is not looks like" or
the `//` example and instead clearly states the requirement.
- Around line 22-34: The isPath predicate is too permissive: when the first
token is an expression with operator '/' (e.g., "{/var}") it currently returns
true even if there is no following literal segment; update isPath to require, in
the branch where first.operator === "/", that template.tokens has a next token
and that the next token is a literal whose text startsWith("/") (use
template.tokens[1] and isLiteral to check the following token), so only
expressions like "{/var}/rest" count as Path (alternatively relax the Path JSDoc
if you intentionally want bare "{/var}" to be valid).
🪄 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: 8b380f00-6ce3-4fb7-b12a-83625b088a66
⛔ Files ignored due to path filters (2)
deno.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (63)
CHANGES.mdcspell.jsondeno.jsonpackages/fedify/README.mdpackages/fedify/deno.jsonpackages/fedify/package.jsonpackages/fedify/src/federation/builder.test.tspackages/fedify/src/federation/builder.tspackages/fedify/src/federation/middleware.test.tspackages/fedify/src/federation/middleware.tspackages/fedify/src/federation/router.tspackages/fedify/src/nodeinfo/handler.tspackages/fedify/src/testing/context.tspackages/uri-template/README.mdpackages/uri-template/deno.jsonpackages/uri-template/old/uri-template-router.test.tspackages/uri-template/old/url-template.test.tspackages/uri-template/package.jsonpackages/uri-template/src/const.tspackages/uri-template/src/mod.tspackages/uri-template/src/router/errors.tspackages/uri-template/src/router/mod.tspackages/uri-template/src/router/node.tspackages/uri-template/src/router/priority.tspackages/uri-template/src/router/router.bench.tspackages/uri-template/src/router/router.test.tspackages/uri-template/src/router/router.tspackages/uri-template/src/router/trie.tspackages/uri-template/src/template/encoding.tspackages/uri-template/src/template/errors.tspackages/uri-template/src/template/expand.tspackages/uri-template/src/template/expression.tspackages/uri-template/src/template/match.tspackages/uri-template/src/template/mod.tspackages/uri-template/src/template/template.bench.tspackages/uri-template/src/template/template.test.tspackages/uri-template/src/template/template.tspackages/uri-template/src/template/token.tspackages/uri-template/src/tests/assert.tspackages/uri-template/src/tests/json/references/fixed.jsonpackages/uri-template/src/tests/json/references/pairs.jsonpackages/uri-template/src/tests/json/references/vars.jsonpackages/uri-template/src/tests/json/router/build-cases.jsonpackages/uri-template/src/tests/json/router/build-suites.jsonpackages/uri-template/src/tests/json/router/clone-suites.jsonpackages/uri-template/src/tests/json/router/compile-error-cases.jsonpackages/uri-template/src/tests/json/router/hit-paths.jsonpackages/uri-template/src/tests/json/router/miss-paths.jsonpackages/uri-template/src/tests/json/router/route-definitions.jsonpackages/uri-template/src/tests/json/router/route-suites.jsonpackages/uri-template/src/tests/json/router/variables-cases.jsonpackages/uri-template/src/tests/json/template/hard.jsonpackages/uri-template/src/tests/json/template/match.jsonpackages/uri-template/src/tests/json/template/wrong.jsonpackages/uri-template/src/tests/mod.tspackages/uri-template/src/tests/router.tspackages/uri-template/src/tests/template.tspackages/uri-template/src/types.tspackages/uri-template/src/utils.tspackages/uri-template/summary.txtpackages/uri-template/tsdown.config.tspnpm-workspace.yamlscripts/check_fixture_usage.ts
Comment on lines
+296
to
+301
| const toggleTrailingSlash = (path: Path): Path | null => { | ||
| if (!path.endsWith("/")) return `${path}/`; | ||
|
|
||
| const trimmed = path.replace(/\/+$/, ""); | ||
| return isPath(trimmed) ? trimmed : null; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
toggleTrailingSlash quietly drops trailing-slash retry for the root path.
For path === "/", the /+$ strip yields "", isPath("") is false, and the function returns null. So a router with trailingSlashInsensitive: true cannot retry routing / against a registered ""-shaped pattern (which is itself invalid, so this is mostly fine), but conversely a request to "" (if it could be constructed) wouldn’t fall back to / either. Worth a one-line comment that this is intentional, since the behaviour is non-obvious.
🤖 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 `@packages/uri-template/src/router/router.ts` around lines 296 - 301, The
toggleTrailingSlash function currently returns null for path === "/" because
stripping trailing slashes yields an empty string which is not a valid path
according to isPath, so add a one-line comment above toggleTrailingSlash
explaining this is intentional: root "/" will not be toggled to an empty path
(and therefore routing will not retry to ""), reference the function name
toggleTrailingSlash and the isPath check in the comment so future readers
understand the deliberate behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/uri-template/src/template/match.ts`:
- Around line 64-116: matchTokens can be driven into pathological exponential
work by adversarial URIs because expressionEnds/matchExpression (and helpers
like consumeUnnamed, matchUnnamedFrom, matchNamedFrom) enumerate many candidates
and the only final filter is expand(...) === uri; add a bounded-work defense
inside matchTokens: thread a counter (e.g., candidateCount) or recursionDepth
through the recursive calls of matchTokens and increment it before exploring
each expression end / expressionBindings branch (the places that call
matchTokens recursively), and when the counter exceeds a configurable threshold
(e.g., options.maxMatchWork or a hardcoded constant) abort early by returning a
typed error (or null if existing API requires) instead of continuing; ensure the
bound is checked at the top of matchTokens and that the error/limit is surfaced
to callers so Router.register users can handle it.
🪄 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: c0f8cc6d-db90-4a19-867b-73465ae93210
📒 Files selected for processing (1)
packages/uri-template/src/template/match.ts
Add a new workspace package that will host Fedify's own symmetric
RFC 6570 URI Template implementation, replacing the third-party
url-template (expansion) and uri-template-router (parsing) libraries
whose asymmetric behavior has been a recurring source of
encoding/decoding bugs.
This commit only lays down the package skeleton and a public API
mockup so that downstream packages can wire imports against the final
module path before the algorithms land:
- Package metadata (deno.json, package.json, tsdown.config.ts,
README.md) modeled after the existing @fedify/webfinger package.
- src/types.ts: shared types (PrimitiveValue, ExpandContext,
Template, VariableSpec, Route, HierarchyNode, Result).
- src/template.ts: parseTemplate stub returning a Template whose
expand() throws.
- src/router.ts: Router class with the public mutable fields
(nid, fsm, routeSet, templateRouteMap, valueRouteMap, hierarchy)
that the current consumer relies on for structural cloning;
addTemplate/resolveURI throw.
- src/mod.ts: explicit named re-exports of the public surface only.
- Register the package in the root deno.json workspace and in
pnpm-workspace.yaml.
The mocked surface mirrors the names and shapes used by
packages/fedify/src/federation/router.ts so that the consumer can
later switch its imports without further interface changes.
fedify-dev#418
Assisted-by: Claude Code:claude-opus-4-7
Co-authored-by: Copilot <copilot@github.com> Add test for template Add test for template
Replace the stub `parseTemplate` exposed by `@fedify/uri-template` with a full RFC 6570 Level 4 expansion implementation, structured as a `Template` class that owns tokenization and delegates expression expansion to a dedicated module. The single `template.ts` is split into a `template/` directory with focused modules: `token` (tokenizer), `expression` (varspec parser), `expand` (expansion algorithm), `encoding` (pct-encoding helpers), and `mod` (the `Template` class). Operator metadata moves into `const.ts` so tokenizer and expander share a single source of truth for the RFC 6570 first/sep/named/ifemp/allow table. Introduce a typed error hierarchy in `errors.ts` covering both parse and expansion diagnostics, plus a `report`/`strict` option pair on `TemplateOptions` so callers can collect every diagnostic in one pass instead of failing on the first issue. Broaden `ExpandContext` to allow `undefined` and `readonly` arrays, and split it into `PrimitiveValue`, `AssociativeValue`, and `ExpandValue` so the expansion contract matches RFC 6570's primitive /list/associative trichotomy. Re-export the new error classes, `Token`, `VarSpec`, `Operator`, `Reporter`, and `TemplateOptions` from `mod.ts`. Add a `test:bun` script and a package-local `cspell.json` for the new identifiers (`varspec`, `varname`, etc.). `summary.txt` keeps an authoritative excerpt of the RFC 6570 grammar and expansion table next to the implementation for reference. Assisted-by: Claude Code:claude-opus-4-7
Add a runtime-agnostic test harness for the new RFC 6570 `Template`
implementation, organized as JSON fixtures plus typed runner
factories so the same cases can drive `template.test.ts` and the
companion `template.bench.ts`.
The fixtures cover four scenarios:
- *tests/json/references/{pairs,fixed,vars}.json*: the canonical
`uritemplate-test` reference cases (template/expansion pairs,
fixed-template variations, and the standard variable context).
- *tests/json/hard.json*: hard-mode cases targeting subtle RFC 6570
rules — pct-encoded literals, mixed encoding/normalization, and
edge cases for explode/prefix modifiers — each with a `reason`
explaining the expected behavior.
- *tests/json/wrong.json*: negative cases pinned to the exact
error class from `errors.ts` so regressions in diagnostic
precision are caught, not just "something throws".
`tests/assert.ts` validates the JSON shapes at load time, and
`tests/lib.ts` exposes `createTemplatePairTest`,
`createFixedTemplateTest`, `createWrongTemplateTest`, and
`createTemplateHardTest` factories that accept any
`TemplateConstructor`, allowing the same suites to exercise
alternative implementations or the benchmark harness.
Assisted-by: Claude Code:claude-opus-4-7
Thread TemplateOptions through URI template expansion so runtime diagnostics use the same report and strict behavior as parsing. The Template instance now uses its normalized options by default while allowing callers to pass explicit expansion options. Also let Template.parse accept parse options and add coverage for non-strict expansion reporting. Assisted-by: Codex:GPT-5
Explain why Fedify ships its own RFC 6570 implementation instead of wrapping url-template. Document the observed compliance gaps and add a bench test that records url-template behavior against the shared test suites. Assisted-by: Codex:gpt-5
Move expansion over parsed tokens so Template.expand delegates the full token stream at once. Simplify URI character predicates with reusable predicate helpers, and keep URI Template spelling words in the root cspell dictionary. Also make Template expose a string form and route default diagnostics through the package logger. Assisted-by: Codex:GPT-5
Add Template.match() and the matcher implementation for URI Template expressions. The matcher backtracks over expression boundaries and value interpretations, then verifies candidates by re-expanding the template. It handles named and unnamed operators, exploded lists and associative values, prefix bindings, and separator-sensitive named associative pairs. Assisted-by: Codex:gpt-5
Add `match.json` carrying match-only test cases that exercise RFC 6570 match semantics: required literals, operator allow-sets, prefix-modifier consistency, boundary disambiguation, named- operator edge cases, list vs. associative composite disambiguation, decoding failures, and multi-expression URL parsing patterns (ActivityPub URIs, REST resource routes, form-style query continuation, fragment-anchored deep links). Add the `MatchTestSuite` interface and JSON validator alongside runner factories — `createMatchOnlyTest`, `createTemplateMatchTest`, `createFixedTemplateMatchTest`, and `createTemplateMatchHardTest` — and wire them into `template.test.ts` so the existing pair, fixed, and hard suites also round-trip through `match` + `expand`. Assisted-by: Claude Code:claude-opus-4-7
Introduce a new Router class for URI Template routing. It is intended to replace the existing federation router implementation while sharing the strict RFC 6570 parser across build, match, and variables. That shared parser preserves percent-encoded variable names and keeps reserved expansion values intact instead of normalizing them through a separate routing parser. Assisted-by: Codex:gpt-5
Add reusable JSON-backed Router test suites for add, compile errors, variables, clone, route, and build behavior. Include benchmark helpers that exercise bulk routes, deep common prefixes, root-adjacent dynamic routes, and inactive route replacement. Assisted-by: Codex:gpt-5
Add a benchmark test copy of the previously used Router from the federation package so its behavior can be checked against the shared Router suites. Keep inline notes for the known correctness gaps: reserved expansion matches decode pct-encoded values, and url-template double-encodes pct-encoded variable names when building URIs. These tests make the old Router failures reproducible while the new Router implementation replaces that approach. Assisted-by: Codex:gpt-5
Splits each feature module into a thin entry plus dedicated
implementation file:
- src/template/mod.ts now re-exports from template.ts and errors.ts.
Template class implementation moves to src/template/template.ts.
- src/router/mod.ts now re-exports from router.ts and errors.ts.
Router class implementation moves to src/router/router.ts.
Consolidates parse/expansion errors with the template feature:
- src/errors.ts moves to src/template/errors.ts with internal
callers updated.
Reorganizes test fixtures and helpers per feature:
- src/tests/lib.ts moves to src/tests/template.ts.
- src/tests/json/{hard,match,wrong}.json move under
src/tests/json/template/.
All imports across the package are updated to the new locations.
Assisted-by: Claude Code:claude-opus-4-7
Updates package descriptions, the README, and the npm:url-template compatibility test to describe the matching behavior as "round-trip" rather than "symmetric", and corrects stale "@fedify/url-template" references to "@fedify/uri-template". Assisted-by: Claude Code:claude-opus-4-7
Drops the "Scenario" suffix from the router memory pressure factory
names and shortens them so they read alongside the other test
helpers in src/tests/mod.ts:
- createRouterHundredsOfRoutesScenario -> createRoutesPressureTest
- createRouterDeepCommonPrefixScenario -> createDeepPrefixRouterTest
- createRouterRootAdjacentDynamicRoutesScenario
-> createDynamicRoutesTest
- createRouterInactiveEntriesScenario -> createInactiveEntriesTest
Also unexports the RouterMemoryPressureScenario interface, which was
only used internally by the bench, and renames the
ERROR_CLASS_BY_NAME alias in src/tests/router.ts to ERROR_CLASSES so
it matches the convention used by src/tests/assert.ts.
Assisted-by: Codex:GPT-5.5
Assisted-by: Claude Code:claude-opus-4-7
Inlines the RFC 6570 Appendix A operator behavior table next to operatorSpecs so the values for first, sep, named, ifEmpty, and allowReserved are easy to cross-check against the spec. Removes NOT_IMPLEMENTED, which is no longer referenced anywhere now that the matching and router modules are implemented. Assisted-by: Codex:GPT-5.5 Assisted-by: Claude Code:claude-opus-4-7
VariableSpec was carried over from an earlier router design and is no longer constructed or referenced anywhere; remove the interface and its re-export from src/mod.ts. Updates the TemplateOptions.report and Reporter doc comments to reflect that the reporter is also invoked for expansion errors (PrefixModifierNotApplicableError), not just parse errors. Also cleans up the line-broken module-level header comment in src/mod.ts. Assisted-by: Codex:GPT-5.5 Assisted-by: Claude Code:claude-opus-4-7
Extends the header comment with the route-shape differences that
the compatibility run records against the previous router:
- Leading path expansion templates such as {/identifier}/inbox
are rejected when they overlap with slash-prefixed routes.
- Optional form-style query templates such as /search{?q,page}
miss when only one of the query variables is present.
These behaviors matter for Fedify routes, so calling them out in
the bench file makes the gap visible alongside the encoding/decoding
differences already noted.
Assisted-by: Codex:GPT-5.5
Assisted-by: Claude Code:claude-opus-4-7
The Template implementation no longer uses RegExp for matching, so the bench label "Template using RegExp" is misleading. Use plain "Template" instead. Assisted-by: Codex:GPT-5.5 Assisted-by: Claude Code:claude-opus-4-7
The Router previously offered only a single-route `add(path, name)`
entry point. Bulk registration paid the cost of repeated trie inserts,
each running an insertion-sort splice on the destination node, and the
public surface had no ergonomic way to seed routes at construction
time.
Changes:
- Add `Router#register(routes)` that accepts an iterable of
`[pathOrPattern, name]` tuples and routes them through a new batch
insertion path. `Trie#insertAll` groups entries by destination
node so each node performs one sort-and-merge instead of N
insertion-sort splices. `Node#insertAll` reuses the existing
`mergeRouteEntries` linear merge over the already-sorted entries.
- Overload the `Router` constructor to accept `(routes, options?)`,
`(options?)`, or no arguments. The first argument is dispatched by
`Symbol.iterator` presence so plain options objects and arbitrary
iterables (arrays, generators, `Map.entries()`) are both handled.
Add `Router.from(...)` as a static factory mirroring the same
signature.
- Widen `Router#add` and `Router#register` to accept a pre-parsed
`RouterPathPattern` in addition to a `Path` string, so callers
that already hold a `Router.compile(...)` result can skip
re-parsing.
- Rebuild `Router#clone` on top of the constructor's batch path. The
clone now hands its active patterns straight to a new router
instead of replaying `add` for every entry, sharing the parsed
templates with the original.
- Export the new `RouterRoute` type plus the existing
`RouterOptions`, `RouterPathPattern`, and `RouterRouteResult`
types from `router/mod.ts` and re-route the public `mod.ts`
re-exports through that module entry point.
- Cover the new surface with unit tests: bulk registration parity
with repeated `add`, generator inputs, pre-parsed pattern inputs
on every entry point (`add`, `register`, constructor, `from`),
and all four constructor and `from` argument variants.
Assisted-by: Claude Code:claude-opus-4-7[1m]
Move the compatibility tests for url-template and uri-template-router from the benchmark task into the old test area. Gate the old-library comparison tests behind OLD=true and expose them through deno task test:old, so the regular test task only runs the current src tests. Update the README and Deno package metadata to use the new command and location. Assisted-by: Codex:gpt-5
Restructure the test factories in src/tests/template.ts to accept
arrays of test suites instead of flat case lists. Each suite now
becomes an outer t.step containing per-case inner steps, so the
test reporter mirrors the suite/case shape declared in the data
files instead of the call-site for-loop.
In the old/ test files, replace the `if (isTest) { ... }` wrapper
with the `{ ignore: !isOldTest }` test option so every test is
registered unconditionally and is visible to the reporter even
when skipped.
Update deno.json to exclude bench files, the shared src/tests/
helpers, and summary.txt from the published artifact, and drop the
`src/` filter from the default `test` task so all tests under the
package are picked up.
Update template.bench.ts to pass the suite array through the
revised factory signature.
Assisted-by: Claude Code:claude-opus-4-7
Replace the internal `Router`/`RouterError` from
*src/federation/router.ts* with the implementations from the new
`@fedify/uri-template` workspace package across `FederationBuilder`,
middleware, NodeInfo handler, and testing context. The legacy
*router.ts* exports are kept as a thin compatibility shim with
`@deprecated` JSDoc directing callers to the new package.
The new `Router` splits route registration from variable extraction:
`router.add(template, name)` now returns `void`, while the static
`Router.variables(template)` returns the variable names. All call
sites are updated accordingly.
`validateSingleIdentifierVariablePath` is rewritten to use
`Router.compile` and `isExpression` so it can inspect the parsed
`VariableSpec` directly, which lets it reject explode (`{var*}`) and
prefix (`{var:N}`) modifiers in addition to query/fragment operators.
Corresponding regression tests are added in
*builder.test.ts*.
Also list `@fedify/uri-template` in the packages table in
*packages/fedify/README.md* and add the workspace dependency to
*packages/fedify/package.json* / *pnpm-lock.yaml*.
Assisted-by: Codex:GPT-5.5
Assisted-by: Claude Code:claude-opus-4-7
Add changelog entries for the new *@fedify/uri-template* package and for replacing Fedify's internal federation routing with it. Both entries reference fedify-dev#418. Assisted-by: Claude Code:claude-opus-4-7
Move the path-template predicate out of the router and into the
public utilities module so callers can validate `Path` values without
reaching into router internals. The new implementation parses the
input as a `Template` and inspects the first token, which accepts
multi-variable path expansions like `{/a,b}/...` that the previous
regex-based check rejected.
Also adds a sibling `isLiteral` helper alongside the existing
`isExpression` token guard.
Assisted-by: Claude Code:claude-opus-4-7
Two small correctness/performance fixes in the match backtracker:
- `consumeUnnamed`'s `minLength` heuristic called
`remainingVars(vars)` with the full var list, which collapsed to
`vars.length - 1` regardless of progress. Pass `varIndex` through
so the bound actually reflects how many variables remain after the
current one.
- `consumeNamedList` checked `if (values)` against an array, which is
always truthy. The intent was to skip empty matches; replace with
`values.length > 0` so empty lists no longer yield no-progress
bindings that the surrounding backtracker has to discard.
Neither change affects matching correctness — round-trip verification
filters out the spurious branches — but both reduce the search space
the backtracker has to walk through.
Assisted-by: Claude Code:claude-opus-4-7
Drop the no-op `Template (match)` bench that ran the RFC 6570 example
suite as a wrapper around `test()` — pair examples have variables of
1–3 parts with literal anchors on both sides, so the backtracker
collapses to one branch per case and the result was indistinguishable
from a constant-time loop.
Replace it with two cases that actually exercise the matcher:
- A 3-var unnamed expression against a 12-part body, where the
comma-distribution backtracker has C(11, 2) = 55 candidate
splits.
- A `{/paths*}` exploded path expansion against 728 progressively
deeper URIs, exercising the path-segment list reader.
Factor the per-template benchmark loop into `createMatchBench` and
the path-corpus generator into `createMatchBenchTestCases` under
`packages/uri-template/src/tests/template.ts` so other Template
implementations can reuse the same harness.
Assisted-by: Claude Code:claude-opus-4-7
…m @fedify/uri-template
Address review comments from automated reviewers:
- Harden isPath against malformed templates so it returns false
instead of throwing, restoring its JSDoc contract.
- Relax the Path type from `{/${string}}/${string}` to
`{/${string}}${string}` so isPath is consistent with the router's
handling of bare path-expansion templates like `{/var}`.
- Run path validation before mutating the router in
setActorDispatcher, so a failed registration does not leave a
stale `actor` route that confuses subsequent attempts.
- Reuse validateSingleIdentifierVariablePath across inbox, outbox,
following, followers, liked, featured, featuredTags, and inbox
listener registrations instead of the loose Router.variables
check, so explode (`*`) and prefix (`:N`) modifiers cannot smuggle
through `{identifier}` paths at runtime.
- Mark @fedify/uri-template as side-effect free for tree-shaking.
- Add a regression test for the unnamed consumeUnnamed minLength
pruning bug. The matcher fix is intentionally left for a separate
commit so the test stays red until then.
fedify-dev#758
Assisted-by: Claude Code:claude-opus-4-7[1m]
The `minLength` formula reserved one part per remaining unnamed
variable, but later variables may also be skipped entirely (consuming
zero parts) via the fallback in `matchUnnamedFrom`. Reserving a
minimum part for each of them prevented the *current* variable from
absorbing the parts that the skipped tail would otherwise have taken,
discarding valid decompositions before round-trip verification could
see them.
Concretely, matching `{x:5,y}` against `abc,def,ghi` now correctly
yields `x="abc"`, `y={"def":"ghi"}` instead of leaving `m.x`
undefined and binding `y` to the entire list.
Drop the lower bound to a constant `1`; the recursive matcher already
discards distributions that fail to consume all parts, so no further
tightening is necessary. Removes the now-unused `remainingVars`
helper.
Assisted-by: Claude Code:claude-opus-4-7
Run assertPath() ahead of Router.compile() so the `as Path` assertion
that bypassed runtime path validation is gone, and switch operator
filtering from a denylist to an allowlist that only accepts a
literal-prefix path or the path-style `/` operator. `{;identifier}`,
`{.identifier}`, `{?identifier}`, `{#identifier}`, and `{+identifier}`
are now rejected; regression tests cover the semicolon and label forms.
Addresses CodeRabbit review threads on
fedify-dev#758.
Assisted-by: Claude Code:claude-opus-4-7
Add the empty string to the `Path` type and let `isPath()` and the
fixture path validator accept it. With trailing-slash-insensitive
routing, requesting `/` now retries against a route registered as the
empty string instead of bailing out. Also rewrite the `assertPath()`
error message: the old text was ungrammatical and the `//` example was
misleading. The new message names the three accepted shapes — empty,
`/`-prefixed, or a `{/var}` path-expansion expression.
Addresses CodeRabbit review threads on
fedify-dev#758.
Assisted-by: Claude Code:claude-opus-4-7
Rename the helper formerly called `mod` to `div`: it computes `Math.floor(i / j)` (integer division), not a modulo, and the next line that uses `i % 26` for the actual modulo makes the misnomer worse. Also explain in tests/mod.ts that the per-suite `validate(...)` wrapper exists only for JSON fixtures whose inferred type is too broad — `fixedTestSuites` has a concrete enough shape that TypeScript can use it directly without a runtime validator. Addresses CodeRabbit review threads on fedify-dev#758. Assisted-by: Claude Code:claude-opus-4-7
In strict mode `parseExpression()` previously called `report(error)` inside `reportExpressionError()` before returning, but the wrapped reporter threw, so the throw propagated to the tokenizer catch block which reported the same error a second time. Drop the intermediate report from `expression.ts` and let the tokenizer's catch path own reporting; the regression test asserts a single invocation for an empty expression in strict mode. Also revert the brief experiment that let `Template#expand()` and `Template#match()` accept per-call options. Options are normalized once at construction (or via `Template.parse()`); the methods now always use the stored full options, and the only-illustrative test that exercised the per-call form is removed. Finally, drop the mixed strict/non-strict `node:assert` import so that template.test.ts uses `node:assert/strict` exclusively. Addresses CodeRabbit review threads on fedify-dev#758. Assisted-by: Claude Code:claude-opus-4-7
`Router.add` and `Router.register` overwrote `#routesByName` but
left the previous entry inside the trie, so repeated registration
of the same name (HMR, test setup, long-running processes) made
`#trie.candidates` traversal cost grow without bound. The runtime
filter `#isActiveEntry` masked the symptom without bounding the work.
Make re-registration the authoritative cleanup point:
- Add `Trie.remove` / `Node.remove` so a registered entry can be
detached from the trie in O(node entries).
- In `add`, remove the previously committed entry before inserting
the new one.
- In `register`, track entries pending in the current batch in
a `Map`; re-occurrences within the batch are spliced from the
pending array (they were never committed), while earlier entries
from prior calls are removed from the trie.
- Drop the now-redundant `#isActiveEntry` filter from both
`Trie.candidates` and `#activeEntries`, shrinking the hot path.
Behaviour is covered by a new "Router treats re-registration as
replacement" suite that exercises `add`, `register`, intra-batch
duplicates, constructor input, repeated re-registration, mixed
`add`/`register`, sibling preservation, and `clone`.
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:GPT-5.5
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/check_fixture_usage.ts (2)
23-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the rule description to include
.bench.tsfiles.The comment describes the rule as applying to
*.test.tsfiles only, but the implementation now also excludes*.bench.tsfiles. Update the description for accuracy.📝 Proposed documentation fix
/** - * Files exempt from the "@fedify/fixture imports must live in *.test.ts" - * rule. Every entry MUST be accompanied by an inline comment explaining + * Files exempt from the "@fedify/fixture imports must live in test/benchmark + * files (*.test.ts or *.bench.ts)" rule. Every entry MUST be accompanied by + * an inline comment explaining * why the exception is justified, so other contributors can decide whether * necessary or not. */🤖 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 `@scripts/check_fixture_usage.ts` around lines 23 - 25, Update the top comment in scripts/check_fixture_usage.ts so it accurately reflects the current exemption pattern by mentioning both *.test.ts and *.bench.ts files; specifically, change the sentence that currently reads "Files exempt from the \"@fedify/fixture imports must live in *.test.ts\" rule." to include ".bench.ts" (e.g., "must live in *.test.ts or *.bench.ts") and ensure any following explanatory lines still match the updated rule language.
2-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the documentation to reflect
.bench.tsexclusion.The comment still says "non-
*.test.tsfiles" but the script now also excludes.bench.tsfiles. Update the documentation to accurately describe the current behavior.📝 Proposed documentation fix
* This script flags `import`/`export ... from "@fedify/fixture"` statements - * in non-`*.test.ts` files under `packages/<pkg>/src/`. It exists to catch + * in non-test/benchmark files (excluding `*.test.ts` and `*.bench.ts`) under + * `packages/<pkg>/src/`. It exists to catch * accidental leaks of the private `@fedify/fixture` package into shipped * code.🤖 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 `@scripts/check_fixture_usage.ts` around lines 2 - 5, Update the top comment in scripts/check_fixture_usage.ts where it currently states "non-`*.test.ts` files" to reflect the current behavior by mentioning the additional exclusion (e.g. "non-`*.test.ts` and non-`*.bench.ts` files" or "files other than `*.test.ts` and `*.bench.ts`") so the header accurately describes that `.bench.ts` files are also excluded; edit the comment block text near the file description to replace that phrase accordingly.
♻️ Duplicate comments (1)
packages/uri-template/src/types.ts (1)
71-86: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winFix grammar issue in JSDoc comment.
Line 75 contains a grammatical error: "errors will be reported to by the
reportfunction" should be "errors will be reported by thereportfunction" (remove "to").📝 Proposed fix
/** * If `true`, the first parse or expansion error will be automatically * thrown after being reported. `true` is the default value. If `false`, - * errors will be reported to by the `report` function, but none will be + * errors will be reported by the `report` function, but none will be * thrown unless the `report` function itself throws. */ strict: boolean;🤖 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 `@packages/uri-template/src/types.ts` around lines 71 - 86, Update the JSDoc for the TemplateOptions interface to fix the grammar: in the comment for the `strict` property replace the phrase "errors will be reported to by the `report` function" with "errors will be reported by the `report` function" so the sentence reads correctly and still references the `report` Reporter function; ensure the change is made near the `TemplateOptions` interface and mentions `report: Reporter` in the surrounding comment.
🤖 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 `@deno.json`:
- Line 28: Add the missing dependency entry for "@fedify/uri-template" to the
deno.json imports array so Deno can resolve the package; update the imports
block (alongside the existing "./packages/uri-template" entry) to include the
mapping for "@fedify/uri-template" matching the package declared in package.json
so imports that reference "@fedify/uri-template" across the codebase resolve
correctly in Deno.
In `@packages/fedify/src/federation/builder.ts`:
- Around line 91-96: The public single-identifier path signatures are
inconsistent with validateSingleIdentifierVariablePath(): create a type alias
SingleIdentifierPathExpression representing the two allowed forms
("{identifier}" and "{/identifier}") and update the signatures of the ten
setters (setActorDispatcher, setInboxDispatcher, setOutboxDispatcher,
setOutboxListeners, setFollowingDispatcher, setFollowersDispatcher,
setLikedDispatcher, setFeaturedDispatcher, setFeaturedTagsDispatcher,
setInboxListeners) to use a template type like
`${string}${SingleIdentifierPathExpression}${string}` so callers can express
both "{identifier}" and "{/identifier}"; also replace the overly-broad
Rfc6570Expression<"identifier"> in setOutboxListeners with the new alias-based
signature to match validateSingleIdentifierVariablePath().
In `@packages/uri-template/README.md`:
- Around line 117-124: Update the README to mark the compatibility test command
and file references as repository-only: change the `deno task test:old`
invocation and the references to `old/url-template.test.ts` and
`old/uri-template-router.test.ts` to explicitly state they only apply when
working in the Fedify repository (e.g., add a "(repository-only)" label or move
the snippet into a "Repository-only" section and explain it must be run from the
repo root where the `old/` tests exist). Ensure the README text around the
compatibility tests clearly warns package consumers that these tests are not
included/usable in the published package.
In `@packages/uri-template/src/router/trie.ts`:
- Around line 4-9: The local TrieEntry interface duplicates
PrioritizedRouteEntry; remove the TrieEntry declaration and instead import and
reuse the PrioritizedRouteEntry type from ./priority.ts; update any references
in this module (e.g., type annotations and function signatures that currently
name TrieEntry) to use PrioritizedRouteEntry so there is a single source of
truth for the route entry shape.
In `@packages/uri-template/src/template/errors.ts`:
- Around line 58-60: TemplateParseError defines a throw(): never helper but
TemplateExpansionError does not, causing asymmetric API; add the same throw():
never method to the TemplateExpansionError class (matching the implementation in
TemplateParseError) so both error base classes expose a consistent helper for
callers, or remove throw() from TemplateParseError if you prefer the idiomatic
`throw error` approach—update TemplateExpansionError to include throw() if you
choose symmetry.
In `@packages/uri-template/src/template/match.ts`:
- Around line 335-356: The current boundary check in isExplodedPairBoundary
assumes the next token is an RFC6570 varname (via readPairKeyEnd) which rejects
valid associative keys like "b-c" or "~x"; change the logic so it does not rely
on varname parsing: after confirming separator at index, scan forward for the
next '=' before the next occurrence of the separator (or end), treat any
sequence (including empty) between separator and '=' as the key, and then verify
there is at least a syntactically valid value after '=' by consuming until the
next separator or end (allow percent-encoded characters and characters allowed
in values); update the similar logic around the other occurrence (lines 358-373)
so both isExplodedPairBoundary and findExplodedPairBoundary use this
value-aware/backtracking approach instead of readPairKeyEnd.
In `@packages/uri-template/src/tests/json/template/wrong.json`:
- Around line 211-243: The fixtures for the CTL tests are using double-escaped
Unicode sequences (e.g. "template": "\\u0000") so the test strings contain a
literal backslash and not the control bytes; update each offending "template"
value by removing one escape level so they contain the actual control characters
(e.g. use "\u0000" instead of "\\u0000") for the entries named "NUL control
character (CTL forbidden in literals)", "SOH control character (CTL forbidden in
literals)", "TAB control character (CTL forbidden in literals)", "LF control
character (CTL forbidden in literals)", "CR control character (CTL forbidden in
literals)", "US control character (last C0 CTL forbidden in literals)", and "DEL
control character (CTL forbidden in literals)".
In `@packages/uri-template/src/utils.ts`:
- Around line 25-33: The isPath() boolean validator currently constructs a
Template(path) which triggers the default error reporter and noisy logs for
invalid inputs; change the Template construction in isPath to use a
silent/error-suppressing reporter when calling new Template(...) so parsing
remains strict but errors are not emitted (i.e., pass a silent reporter option
into the Template constructor where isPath creates template and then continue
using template.tokens, isLiteral(first) and first.operator checks as before).
---
Outside diff comments:
In `@scripts/check_fixture_usage.ts`:
- Around line 23-25: Update the top comment in scripts/check_fixture_usage.ts so
it accurately reflects the current exemption pattern by mentioning both
*.test.ts and *.bench.ts files; specifically, change the sentence that currently
reads "Files exempt from the \"@fedify/fixture imports must live in *.test.ts\"
rule." to include ".bench.ts" (e.g., "must live in *.test.ts or *.bench.ts") and
ensure any following explanatory lines still match the updated rule language.
- Around line 2-5: Update the top comment in scripts/check_fixture_usage.ts
where it currently states "non-`*.test.ts` files" to reflect the current
behavior by mentioning the additional exclusion (e.g. "non-`*.test.ts` and
non-`*.bench.ts` files" or "files other than `*.test.ts` and `*.bench.ts`") so
the header accurately describes that `.bench.ts` files are also excluded; edit
the comment block text near the file description to replace that phrase
accordingly.
---
Duplicate comments:
In `@packages/uri-template/src/types.ts`:
- Around line 71-86: Update the JSDoc for the TemplateOptions interface to fix
the grammar: in the comment for the `strict` property replace the phrase "errors
will be reported to by the `report` function" with "errors will be reported by
the `report` function" so the sentence reads correctly and still references the
`report` Reporter function; ensure the change is made near the `TemplateOptions`
interface and mentions `report: Reporter` in the surrounding comment.
🪄 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: 77460189-be80-4043-8f2d-02f247439caa
⛔ Files ignored due to path filters (2)
deno.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (63)
CHANGES.mdcspell.jsondeno.jsonpackages/fedify/README.mdpackages/fedify/deno.jsonpackages/fedify/package.jsonpackages/fedify/src/federation/builder.test.tspackages/fedify/src/federation/builder.tspackages/fedify/src/federation/middleware.test.tspackages/fedify/src/federation/middleware.tspackages/fedify/src/federation/router.tspackages/fedify/src/nodeinfo/handler.tspackages/fedify/src/testing/context.tspackages/uri-template/README.mdpackages/uri-template/deno.jsonpackages/uri-template/old/uri-template-router.test.tspackages/uri-template/old/url-template.test.tspackages/uri-template/package.jsonpackages/uri-template/src/const.tspackages/uri-template/src/mod.tspackages/uri-template/src/router/errors.tspackages/uri-template/src/router/mod.tspackages/uri-template/src/router/node.tspackages/uri-template/src/router/priority.tspackages/uri-template/src/router/router.bench.tspackages/uri-template/src/router/router.test.tspackages/uri-template/src/router/router.tspackages/uri-template/src/router/trie.tspackages/uri-template/src/template/encoding.tspackages/uri-template/src/template/errors.tspackages/uri-template/src/template/expand.tspackages/uri-template/src/template/expression.tspackages/uri-template/src/template/match.tspackages/uri-template/src/template/mod.tspackages/uri-template/src/template/template.bench.tspackages/uri-template/src/template/template.test.tspackages/uri-template/src/template/template.tspackages/uri-template/src/template/token.tspackages/uri-template/src/tests/assert.tspackages/uri-template/src/tests/json/references/fixed.jsonpackages/uri-template/src/tests/json/references/pairs.jsonpackages/uri-template/src/tests/json/references/vars.jsonpackages/uri-template/src/tests/json/router/build-cases.jsonpackages/uri-template/src/tests/json/router/build-suites.jsonpackages/uri-template/src/tests/json/router/clone-suites.jsonpackages/uri-template/src/tests/json/router/compile-error-cases.jsonpackages/uri-template/src/tests/json/router/hit-paths.jsonpackages/uri-template/src/tests/json/router/miss-paths.jsonpackages/uri-template/src/tests/json/router/route-definitions.jsonpackages/uri-template/src/tests/json/router/route-suites.jsonpackages/uri-template/src/tests/json/router/variables-cases.jsonpackages/uri-template/src/tests/json/template/hard.jsonpackages/uri-template/src/tests/json/template/match.jsonpackages/uri-template/src/tests/json/template/wrong.jsonpackages/uri-template/src/tests/mod.tspackages/uri-template/src/tests/router.tspackages/uri-template/src/tests/template.tspackages/uri-template/src/types.tspackages/uri-template/src/utils.tspackages/uri-template/summary.txtpackages/uri-template/tsdown.config.tspnpm-workspace.yamlscripts/check_fixture_usage.ts
| "./packages/sqlite", | ||
| "./packages/sveltekit", | ||
| "./packages/testing", | ||
| "./packages/uri-template", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check that packages importing `@fedify/uri-template` have it in both deno.json and package.json
echo "=== Checking for imports of `@fedify/uri-template` ==="
# Find all TypeScript files importing from `@fedify/uri-template`
rg -l 'from ["\x27]@fedify/uri-template' --type=ts -g '!**/node_modules/**' -g '!**/dist/**'
echo -e "\n=== Checking package.json files for `@fedify/uri-template` dependency ==="
# Find package.json files that should have `@fedify/uri-template` as dependency
fd -t f '^package\.json$' packages/ -x sh -c 'if grep -q "@fedify/uri-template" "$1"; then echo "✓ $1 has `@fedify/uri-template`"; else echo "✗ $1 missing `@fedify/uri-template` (check if needed)"; fi' _ {}
echo -e "\n=== Checking deno.json files for `@fedify/uri-template` in imports ==="
# Find deno.json files in packages that might import `@fedify/uri-template`
fd -t f '^deno\.json$' packages/ -x sh -c 'if grep -q "@fedify/uri-template" "$1"; then echo "✓ $1 has `@fedify/uri-template`"; else echo "✗ $1 missing `@fedify/uri-template` (check if needed)"; fi' _ {}Repository: fedify-dev/fedify
Length of output: 5341
🏁 Script executed:
# Check the deno.json mentioned in the review (appears to be root deno.json based on context)
head -40 deno.jsonRepository: fedify-dev/fedify
Length of output: 1080
🏁 Script executed:
# Check packages/fedify/deno.json specifically
cat packages/fedify/deno.jsonRepository: fedify-dev/fedify
Length of output: 3299
🏁 Script executed:
# Check packages/fedify/package.json to see how `@fedify/uri-template` is declared
cat packages/fedify/package.jsonRepository: fedify-dev/fedify
Length of output: 5406
Add @fedify/uri-template to packages/fedify/deno.json imports.
The package imports from @fedify/uri-template (verified in 7 files) and correctly declares it in package.json, but it is missing from deno.json. Per the critical coding guideline: "Dependencies must be added to BOTH deno.json and package.json. Forgetting package.json will cause Node.js and Bun tests to fail with ERR_MODULE_NOT_FOUND." This omission will break Deno support.
🤖 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 `@deno.json` at line 28, Add the missing dependency entry for
"@fedify/uri-template" to the deno.json imports array so Deno can resolve the
package; update the imports block (alongside the existing
"./packages/uri-template" entry) to include the mapping for
"@fedify/uri-template" matching the package declared in package.json so imports
that reference "@fedify/uri-template" across the codebase resolve correctly in
Deno.
Comment on lines
+91
to
+96
| const { operator, vars: [varSpec] } = expressions[0]; | ||
| if ( | ||
| !(operator === "" || operator === "/") || | ||
| varSpec.explode || | ||
| varSpec.prefix != null | ||
| ) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Validator behavior:"
sed -n '74,98p' packages/fedify/src/federation/builder.ts
echo
echo "Single-identifier public signatures:"
rg -n '\b(setActorDispatcher|setInboxDispatcher|setOutboxDispatcher|setOutboxListeners|setFollowingDispatcher|setFollowersDispatcher|setLikedDispatcher|setFeaturedDispatcher|setFeaturedTagsDispatcher|setInboxListeners)\b' \
packages/fedify/src/federation/builder.ts -A2 -B0Repository: fedify-dev/fedify
Length of output: 2132
🏁 Script executed:
# Check what Rfc6570Expression is
rg -n "type Rfc6570Expression" packages/fedify/src/federation/builder.ts -A5
# Check imports at top of file
head -30 packages/fedify/src/federation/builder.ts
# Verify exact signatures and line numbers for all affected methods
sed -n '254,257p' packages/fedify/src/federation/builder.ts
sed -n '696,703p' packages/fedify/src/federation/builder.ts
sed -n '844,846p' packages/fedify/src/federation/builder.tsRepository: fedify-dev/fedify
Length of output: 1288
🏁 Script executed:
# Search for Rfc6570Expression definition - likely in uri-template package or this file
rg "type Rfc6570Expression" -B2 -A8
# Also check if it's imported
rg "Rfc6570Expression" packages/fedify/src/federation/builder.ts -B2 -A2Repository: fedify-dev/fedify
Length of output: 4101
🏁 Script executed:
# Read the full context around each affected method to confirm the exact signatures
echo "=== setActorDispatcher (254-260) ==="
sed -n '254,260p' packages/fedify/src/federation/builder.ts
echo
echo "=== setInboxDispatcher (696-705) ==="
sed -n '696,705p' packages/fedify/src/federation/builder.ts
echo
echo "=== setOutboxDispatcher (770-780) ==="
sed -n '770,780p' packages/fedify/src/federation/builder.ts
echo
echo "=== setOutboxListeners (844-850) ==="
sed -n '844,850p' packages/fedify/src/federation/builder.ts
echo
echo "=== setFollowingDispatcher (892-900) ==="
sed -n '892,900p' packages/fedify/src/federation/builder.ts
echo
echo "=== setFollowersDispatcher (958-966) ==="
sed -n '958,966p' packages/fedify/src/federation/builder.ts
echo
echo "=== setLikedDispatcher (1012-1020) ==="
sed -n '1012,1020p' packages/fedify/src/federation/builder.ts
echo
echo "=== setFeaturedDispatcher (1078-1086) ==="
sed -n '1078,1086p' packages/fedify/src/federation/builder.ts
echo
echo "=== setFeaturedTagsDispatcher (1144-1152) ==="
sed -n '1144,1152p' packages/fedify/src/federation/builder.ts
echo
echo "=== setInboxListeners (1210-1216) ==="
sed -n '1210,1216p' packages/fedify/src/federation/builder.tsRepository: fedify-dev/fedify
Length of output: 2974
Create a type alias for single-identifier paths to align TS signatures with validator.
validateSingleIdentifierVariablePath() permits exactly {identifier} and {/identifier}, but public method signatures create a typing mismatch:
- 9 methods use
${string}{identifier}${string}— too narrow, can't express{/identifier} setOutboxListeners()usesRfc6570Expression<"identifier">— too broad, admits invalid operators like{?identifier}that the validator rejects at runtime
Define a type alias for the two allowed forms and apply it consistently across all single-identifier setters (setActorDispatcher, setInboxDispatcher, setOutboxDispatcher, setOutboxListeners, setFollowingDispatcher, setFollowersDispatcher, setLikedDispatcher, setFeaturedDispatcher, setFeaturedTagsDispatcher, setInboxListeners):
Type alias
type SingleIdentifierPathExpression = "{identifier}" | "{/identifier}";Replace path signatures with ${string}${SingleIdentifierPathExpression}${string} across all 10 affected methods.
This ensures TypeScript prevents users from passing invalid forms while supporting the newly validated slash-prefixed route. As per coding guidelines, maintain strict TypeScript typing throughout.
🤖 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 `@packages/fedify/src/federation/builder.ts` around lines 91 - 96, The public
single-identifier path signatures are inconsistent with
validateSingleIdentifierVariablePath(): create a type alias
SingleIdentifierPathExpression representing the two allowed forms
("{identifier}" and "{/identifier}") and update the signatures of the ten
setters (setActorDispatcher, setInboxDispatcher, setOutboxDispatcher,
setOutboxListeners, setFollowingDispatcher, setFollowersDispatcher,
setLikedDispatcher, setFeaturedDispatcher, setFeaturedTagsDispatcher,
setInboxListeners) to use a template type like
`${string}${SingleIdentifierPathExpression}${string}` so callers can express
both "{identifier}" and "{/identifier}"; also replace the overly-broad
Rfc6570Expression<"identifier"> in setOutboxListeners with the new alias-based
signature to match validateSingleIdentifierVariablePath().
Comment on lines
+117
to
+124
| The old implementation differences can be checked by running the compatibility | ||
| tests in *old/url-template.test.ts* and *old/uri-template-router.test.ts*. | ||
| These tests intentionally run Fedify's expected behavior against the old | ||
| libraries, so the failing cases show the gaps: | ||
|
|
||
| ~~~~ bash | ||
| deno task test:old | ||
| ~~~~ |
There was a problem hiding this comment.
Mark this compatibility-test command as repository-only.
This README is shipped with the package, but deno task test:old and the *old/*.test.ts* references only make sense in the Fedify repository context. As written, published-package readers are pointed at a task they cannot run.
🤖 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 `@packages/uri-template/README.md` around lines 117 - 124, Update the README to
mark the compatibility test command and file references as repository-only:
change the `deno task test:old` invocation and the references to
`old/url-template.test.ts` and `old/uri-template-router.test.ts` to explicitly
state they only apply when working in the Fedify repository (e.g., add a
"(repository-only)" label or move the snippet into a "Repository-only" section
and explain it must be run from the repo root where the `old/` tests exist).
Ensure the README text around the compatibility tests clearly warns package
consumers that these tests are not included/usable in the published package.
Comment on lines
+4
to
+9
| interface TrieEntry { | ||
| readonly index: number; | ||
| readonly initialLiteralPrefix: string; | ||
| readonly literalLength: number; | ||
| readonly variableCount: number; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
TrieEntry duplicates PrioritizedRouteEntry.
The local TrieEntry interface has the exact same shape as PrioritizedRouteEntry from ./priority.ts. Reuse the existing type to keep a single source of truth and avoid drift if the priority schema changes (e.g., when adding/removing a field).
♻️ Proposed reuse of `PrioritizedRouteEntry`
import type { Path } from "../types.ts";
+import type { PrioritizedRouteEntry } from "./priority.ts";
import Node from "./node.ts";
-interface TrieEntry {
- readonly index: number;
- readonly initialLiteralPrefix: string;
- readonly literalLength: number;
- readonly variableCount: number;
-}
-
/**
* Prefix trie for registered route candidates.
*/
-export default class Trie<TEntry extends TrieEntry> {
+export default class Trie<TEntry extends PrioritizedRouteEntry> {
readonly `#root` = new Node<TEntry>();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| interface TrieEntry { | |
| readonly index: number; | |
| readonly initialLiteralPrefix: string; | |
| readonly literalLength: number; | |
| readonly variableCount: number; | |
| } | |
| import type { Path } from "../types.ts"; | |
| import type { PrioritizedRouteEntry } from "./priority.ts"; | |
| import Node from "./node.ts"; | |
| /** | |
| * Prefix trie for registered route candidates. | |
| */ | |
| export default class Trie<TEntry extends PrioritizedRouteEntry> { | |
| readonly `#root` = new Node<TEntry>(); |
🤖 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 `@packages/uri-template/src/router/trie.ts` around lines 4 - 9, The local
TrieEntry interface duplicates PrioritizedRouteEntry; remove the TrieEntry
declaration and instead import and reuse the PrioritizedRouteEntry type from
./priority.ts; update any references in this module (e.g., type annotations and
function signatures that currently name TrieEntry) to use PrioritizedRouteEntry
so there is a single source of truth for the route entry shape.
Comment on lines
+58
to
+60
| throw(): never { | ||
| throw this; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Asymmetric throw() helper between the two error base classes.
TemplateParseError exposes a throw(): never helper, but TemplateExpansionError does not. If callers are intended to use error.throw() (e.g., from a reporter callback), the absence on the expansion-error base will lead to inconsistent usage between parse and expansion paths. Either drop throw() (since throw error is idiomatic) or add the same helper to TemplateExpansionError for symmetry.
🤖 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 `@packages/uri-template/src/template/errors.ts` around lines 58 - 60,
TemplateParseError defines a throw(): never helper but TemplateExpansionError
does not, causing asymmetric API; add the same throw(): never method to the
TemplateExpansionError class (matching the implementation in TemplateParseError)
so both error base classes expose a consistent helper for callers, or remove
throw() from TemplateParseError if you prefer the idiomatic `throw error`
approach—update TemplateExpansionError to include throw() if you choose
symmetry.
Comment on lines
+335
to
+356
| function findExplodedPairBoundary( | ||
| body: string, | ||
| start: number, | ||
| separator: string, | ||
| ): number { | ||
| for (let index = start; index < body.length; index++) { | ||
| if (isExplodedPairBoundary(body, index, separator)) return index; | ||
| } | ||
| return body.length; | ||
| } | ||
|
|
||
| const isExplodedPairBoundary = ( | ||
| body: string, | ||
| index: number, | ||
| separator: string, | ||
| ): boolean => { | ||
| if (!body.startsWith(separator, index)) return false; | ||
|
|
||
| const keyStart = index + separator.length; | ||
| const keyEnd = readPairKeyEnd(body, keyStart); | ||
| return keyEnd > keyStart && body[keyEnd] === "="; | ||
| }; |
There was a problem hiding this comment.
Exploded associative matching rejects valid keys here.
isExplodedPairBoundary() only treats the next entry as a boundary when the following key parses like an RFC 6570 varname. Associative keys are expanded as values, so valid keys such as b-c, ~x, or a later empty key will never split correctly. A concrete failure is {keys*} vs. a=1,b-c=2: expansion can produce that URI, but this matcher won't recover it because b-c is not a valid varname. This boundary parser needs to stop assuming varname syntax for object keys and instead parse pair boundaries using value rules/backtracking.
Also applies to: 358-373
🤖 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 `@packages/uri-template/src/template/match.ts` around lines 335 - 356, The
current boundary check in isExplodedPairBoundary assumes the next token is an
RFC6570 varname (via readPairKeyEnd) which rejects valid associative keys like
"b-c" or "~x"; change the logic so it does not rely on varname parsing: after
confirming separator at index, scan forward for the next '=' before the next
occurrence of the separator (or end), treat any sequence (including empty)
between separator and '=' as the key, and then verify there is at least a
syntactically valid value after '=' by consuming until the next separator or end
(allow percent-encoded characters and characters allowed in values); update the
similar logic around the other occurrence (lines 358-373) so both
isExplodedPairBoundary and findExplodedPairBoundary use this
value-aware/backtracking approach instead of readPairKeyEnd.
Comment on lines
+211
to
+243
| "template": "\\u0000", | ||
| "name": "NUL control character (CTL forbidden in literals)", | ||
| "expected": "InvalidLiteralError" | ||
| }, | ||
| { | ||
| "template": "\\u0001", | ||
| "name": "SOH control character (CTL forbidden in literals)", | ||
| "expected": "InvalidLiteralError" | ||
| }, | ||
| { | ||
| "template": "\\u0009", | ||
| "name": "TAB control character (CTL forbidden in literals)", | ||
| "expected": "InvalidLiteralError" | ||
| }, | ||
| { | ||
| "template": "\\u000A", | ||
| "name": "LF control character (CTL forbidden in literals)", | ||
| "expected": "InvalidLiteralError" | ||
| }, | ||
| { | ||
| "template": "\\u000D", | ||
| "name": "CR control character (CTL forbidden in literals)", | ||
| "expected": "InvalidLiteralError" | ||
| }, | ||
| { | ||
| "template": "\\u001F", | ||
| "name": "US control character (last C0 CTL forbidden in literals)", | ||
| "expected": "InvalidLiteralError" | ||
| }, | ||
| { | ||
| "template": "\\u007F", | ||
| "name": "DEL control character (CTL forbidden in literals)", | ||
| "expected": "InvalidLiteralError" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether control-character fixtures currently parse as literal backslash sequences.
python - <<'PY'
import json, pathlib
p = pathlib.Path("packages/uri-template/src/tests/json/template/wrong.json")
data = json.loads(p.read_text(encoding="utf-8"))
suite = next(s for s in data if s["name"] == "Invalid characters in literals")
for c in suite["cases"][:7]:
t = c["template"]
print(f'{c["name"]}: repr={t!r}, first_codepoint={hex(ord(t[0]))}, length={len(t)}')
PYRepository: fedify-dev/fedify
Length of output: 752
Control-character fixtures contain literal backslash sequences instead of actual control characters
Lines 211–243 define test cases named for control characters (NUL, SOH, TAB, LF, CR, US, DEL), but their "template" values use double-escaped Unicode sequences like "\\u0000". When parsed, these become 6-character literal strings starting with a backslash (codepoint 0x5c), not the actual control bytes they claim to test. This weakens negative-test coverage for CTL rejection.
Fix by removing one escape level:
Suggested fix
- "template": "\\u0000",
+ "template": "\u0000",Apply the same change to \\u0001, \\u0009, \\u000A, \\u000D, \\u001F, and \\u007F.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "template": "\\u0000", | |
| "name": "NUL control character (CTL forbidden in literals)", | |
| "expected": "InvalidLiteralError" | |
| }, | |
| { | |
| "template": "\\u0001", | |
| "name": "SOH control character (CTL forbidden in literals)", | |
| "expected": "InvalidLiteralError" | |
| }, | |
| { | |
| "template": "\\u0009", | |
| "name": "TAB control character (CTL forbidden in literals)", | |
| "expected": "InvalidLiteralError" | |
| }, | |
| { | |
| "template": "\\u000A", | |
| "name": "LF control character (CTL forbidden in literals)", | |
| "expected": "InvalidLiteralError" | |
| }, | |
| { | |
| "template": "\\u000D", | |
| "name": "CR control character (CTL forbidden in literals)", | |
| "expected": "InvalidLiteralError" | |
| }, | |
| { | |
| "template": "\\u001F", | |
| "name": "US control character (last C0 CTL forbidden in literals)", | |
| "expected": "InvalidLiteralError" | |
| }, | |
| { | |
| "template": "\\u007F", | |
| "name": "DEL control character (CTL forbidden in literals)", | |
| "expected": "InvalidLiteralError" | |
| "template": "\u0000", | |
| "name": "NUL control character (CTL forbidden in literals)", | |
| "expected": "InvalidLiteralError" | |
| }, | |
| { | |
| "template": "\u0001", | |
| "name": "SOH control character (CTL forbidden in literals)", | |
| "expected": "InvalidLiteralError" | |
| }, | |
| { | |
| "template": "\u0009", | |
| "name": "TAB control character (CTL forbidden in literals)", | |
| "expected": "InvalidLiteralError" | |
| }, | |
| { | |
| "template": "\u000A", | |
| "name": "LF control character (CTL forbidden in literals)", | |
| "expected": "InvalidLiteralError" | |
| }, | |
| { | |
| "template": "\u000D", | |
| "name": "CR control character (CTL forbidden in literals)", | |
| "expected": "InvalidLiteralError" | |
| }, | |
| { | |
| "template": "\u001F", | |
| "name": "US control character (last C0 CTL forbidden in literals)", | |
| "expected": "InvalidLiteralError" | |
| }, | |
| { | |
| "template": "\u007F", | |
| "name": "DEL control character (CTL forbidden in literals)", | |
| "expected": "InvalidLiteralError" |
🤖 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 `@packages/uri-template/src/tests/json/template/wrong.json` around lines 211 -
243, The fixtures for the CTL tests are using double-escaped Unicode sequences
(e.g. "template": "\\u0000") so the test strings contain a literal backslash and
not the control bytes; update each offending "template" value by removing one
escape level so they contain the actual control characters (e.g. use "\u0000"
instead of "\\u0000") for the entries named "NUL control character (CTL
forbidden in literals)", "SOH control character (CTL forbidden in literals)",
"TAB control character (CTL forbidden in literals)", "LF control character (CTL
forbidden in literals)", "CR control character (CTL forbidden in literals)", "US
control character (last C0 CTL forbidden in literals)", and "DEL control
character (CTL forbidden in literals)".
Comment on lines
+25
to
+33
| try { | ||
| const template = new Template(path); | ||
|
|
||
| const [first] = template.tokens; | ||
| if (first == null) return false; | ||
| if (isLiteral(first)) return first.text.startsWith("/"); | ||
| if (first.operator === "/") return true; | ||
| return false; | ||
| } catch { |
There was a problem hiding this comment.
isPath() still reports invalid inputs as errors.
The try/catch hides the exception, but new Template(path) still uses the default reporter first, so every invalid candidate is logged at error level before false is returned. For a boolean validator, that will create noisy false-positive logs on ordinary bad input. Keep strict parsing here, but pass a silent reporter.
🔇 Minimal fix
- const template = new Template(path);
+ const template = new Template(path, { report: () => {} });🤖 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 `@packages/uri-template/src/utils.ts` around lines 25 - 33, The isPath()
boolean validator currently constructs a Template(path) which triggers the
default error reporter and noisy logs for invalid inputs; change the Template
construction in isPath to use a silent/error-suppressing reporter when calling
new Template(...) so parsing remains strict but errors are not emitted (i.e.,
pass a silent reporter option into the Template constructor where isPath creates
template and then continue using template.tokens, isLiteral(first) and
first.operator checks as before).
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.
Resolves #418.
Background
Fedify previously combined two third-party libraries for URI Template handling:
The two libraries do not share a parser or an encoding model, so a value expanded by one could not be decoded back by the other. This asymmetry has been the root cause of recurring encoding/decoding bugs (#416 and related), because RFC 6570 only specifies expansion, while
uri-template-routeradds matching as a non-standard extension with its own rules.What this PR does
Adds
@fedify/uri-template(packages/uri-template/)A new workspace package that implements RFC 6570 expansion and round-trip pattern matching from a single strict parser. Highlights:
{var},{+var},{#var},{.var},{/var},{;var},{?var},{&var}).match(uri)accepts a candidate only when the recovered values re-expand to the exact input URI. This eliminates the asymmetric cases where a URI matched but the values could not reproduce it.errors.ts);strict/reportoptions let callers collect every diagnostic in one pass instead of failing on the first issue.Routerclass with a prefix trie keyed by initial literal prefix, deterministic candidate ordering, batch registration viaRouter#register/ constructor /Router.from, andRouter.compile/Router.variablesfor variable extraction without mutating a router.old/that run Fedify's expected behavior againsturl-templateanduri-template-router, recording the gaps the new implementation closes (double-encodedpct-encodedvariable names, prefix modifier on composite values, leading path expansion templates rejected by the old router, optional form-style query templates missing on partial matches, reserved-expansion decoding mismatch, etc.). Run withdeno task test:old.Migrates
@fedify/fedifyto the new packageRouterandRouterErrorinFederationBuilder, middleware, NodeInfo handler, and testing context now come from@fedify/uri-template.packages/fedify/src/federation/router.tsis kept as a thin compatibility shim with@deprecatedJSDoc directing callers to@fedify/uri-template. No public re-exports are removed.Routersplits route registration from variable extraction:router.add(template, name)returnsvoid, while the staticRouter.variables(template)returns the variable names. All call sites are updated.validateSingleIdentifierVariablePathis rewritten on top ofRouter.compileandisExpressionso it inspects the parsedVariableSpecdirectly. This lets it reject explode ({var*}) and prefix ({var:N}) modifiers in addition to the previously-rejected query/fragment operators. Regression tests are added inbuilder.test.ts.Notable behavior changes for downstream users
These follow from the stricter parser and the round-trip matching contract; they are not opt-in:
{?abc%20def}now expands to?abc%20def=…rather than?abc%2520def=…./files/a%2Fbcould be reported asa/b; this no longer happens because such a binding does not round-trip.Router.compile()at registration time. Ordinary slash-prefixed paths and the leading path expansion form used by Fedify routes ({/identifier}/inbox) are both supported.url-template(unmatched braces, raw%outside a pct-encoded triplet, forbidden literal characters, etc.) now raise typed errors. Applications that need a looser parser can opt in viastrict: falseand a customreportfunction.Other
@fedify/uri-templateis listed in the packages table inpackages/fedify/README.mdand added topackages/fedify/package.jsonas a workspace dependency.CHANGES.mdnotes both the new package and the internal migration under issue Implement symmetric RFC 6570 expansion and pattern matching #418.AI disclosure
Assisted-by: Codex:GPT-5.5
Assisted-by: Claude Code:claude-opus-4-7