Replace url-template and uri-template-router with @fedify/uri-template#758
Open
2chanhaeng wants to merge 48 commits into
Open
Replace url-template and uri-template-router with @fedify/uri-template#7582chanhaeng wants to merge 48 commits into
url-template and uri-template-router with @fedify/uri-template#7582chanhaeng wants to merge 48 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
Suggested reviewers
✨ 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.
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
2chanhaeng
added a commit
to 2chanhaeng/fedify
that referenced
this pull request
May 11, 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]
2chanhaeng
added a commit
to 2chanhaeng/fedify
that referenced
this pull request
May 11, 2026
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
2chanhaeng
added a commit
to 2chanhaeng/fedify
that referenced
this pull request
May 11, 2026
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
2chanhaeng
added a commit
to 2chanhaeng/fedify
that referenced
this pull request
May 11, 2026
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
2chanhaeng
added a commit
to 2chanhaeng/fedify
that referenced
this pull request
May 11, 2026
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
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
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).
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
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
Running `mise run test` was intermittently failing in the install step with `NotFound: Unable to get CWD` while loading the yaml CJS package as part of `@fedify/vocab` codegen. Reproduction with `(deno lint &); deno task install` showed the race is triggered whenever another Deno process starts concurrently with the codegen process. Failure rates measured across stable releases: - Deno 2.6.10: 2/10 - Deno 2.7.7: 3/10 - Deno 2.7.13: 3–5/10 - Deno 2.7.14: 4/10 So neither upgrading nor downgrading Deno avoids it. The root cause is the well-known race in Deno's `nodeModulesDir: "auto"`. When multiple Deno processes start at once they each try to manage `node_modules/.deno/` and the `node_modules/@fedify/*` workspace symlinks (verified with `strace`: deno lint unlinks and recreates these symlinks at startup, and the `.deno.lock.poll` file is also rewritten). Under that race, the CJS loader for `yaml@2.8.3` can hit a state where `Deno.cwd()` returns NotFound from inside `Module._nodeModulePaths`, killing the codegen subprocess. See the related upstream report: denoland/deno#33311 Add `wait_for = ["install"]` to the Deno-based `check:*` tasks (`check:fmt`, `check:lint`, `check:types`, `check-versions`, `check:fixture-usage`). `wait_for` is mise's optional dependency: when `install` is already in the queue (as it is via `prepare` from `test:deno`/`test:node`/`test:bun`), the check tasks wait for it to complete before starting their own Deno processes; when `install` is not queued (e.g. running `mise run check` on its own), the tasks proceed immediately. Empty Deno runs left alone (`check:md`, `check:manifest:workspace-protocol`) are unaffected. With this change the previously flaky reproducer succeeded 10/10 times under the same parallel load. Assisted-by: Claude Code:claude-opus-4-7
The package advertises zero runtime dependencies (in README.md, CHANGES.md, and the empty `dependencies`/`imports` fields of *package.json* and *deno.json*), but Template imported `@logtape/logtape` to back the default reporter. That contradicted the claim and caused tsdown to bundle the entire logger into *dist/*. Drop the import and let the default reporter be a no-op. The default `strict: true` still throws after the reporter runs, so error visibility is preserved on the common path. Callers that opt into non-strict mode can pass their own `report` callback (for example, one backed by their application's logger) to observe diagnostics. Assisted-by: Claude Code:claude-opus-4-7
`isExplodedPairBoundary` was using RFC 6570 varname rules (`isVarcharAt`
plus the point-continuation logic in `readPairKeyEnd`) to detect the
start of the next exploded associative entry. Associative keys are
emitted through `encodeValue` on the expansion side, so the legal key
character set is the full unreserved/reserved alphabet, not just
varname characters. Round-trip matches against keys like `b-c`, `~x`,
or any key containing `-`, `!`, `~`, etc. silently failed: the
boundary detector skipped past the next separator and the entire tail
of the body was treated as one value, leaving the round-trip check to
reject the only decomposition the matcher had reached.
Switch the boundary check to a value-driven scan: at each separator,
walk forward until the next `=` (boundary) or the next separator
(no boundary). The detector still fires only at separator positions,
so an `=` inside an `allowReserved` value cannot be mistaken for a
key/value split. The now-unused `readPairKeyEnd` helper and the
`isVarcharAt` import are removed.
Adds a regression test covering `{keys*}` against `a=1,b-c=2`, plus a
companion test that pins the parser's rejection of the seven actual
1-byte control characters (NUL/SOH/TAB/LF/CR/US/DEL) via
`String.fromCodePoint`. The CTL test documents the boundary that the
`wrong.json` fixture *names* but does not exercise (since JSON parses
the double-escaped `` sequences in those fixtures to 6-character
backslash strings, not control bytes).
Reported in PR fedify-dev#758
(CodeRabbit, 2026-05-11).
Assisted-by: Claude Opus 4.7 (1M context)
`validateSingleIdentifierVariablePath` was applied to every setter
that takes a single-identifier path, which tightened upstream's
inline `variables.size!==1 || !variables.has("identifier")` check
into a stricter one that also rejected the named operators
(`{+identifier}`, `{?identifier}`, etc.) and any explode/prefix
modifier. That restriction only matched the runtime behaviour the
outbox setters historically required; the other eight setters
preserved the looser upstream rule and the TS signatures already
allowed `Rfc6570Expression<"identifier">`.
Split the helper:
- `assertIdentifierPath` keeps the upstream rule (one variable
named `identifier`, any operator).
- `assertStrictIdentifierPath` layers the operator/explode/prefix
guard on top by calling `assertIdentifierPath` first.
Both helpers carry an `asserts path is Path` predicate so callers
get the `Path` narrowing automatically. Apply
`assertStrictIdentifierPath` to setOutboxDispatcher and
setOutboxListeners (the two setters upstream already routed through
the strict centralized validator) and `assertIdentifierPath`
everywhere else. All existing "wrong variables in path" tests pass.
Addresses the type-vs-runtime mismatch flagged by CodeRabbit in
fedify-dev#758 (2026-05-11).
Assisted-by: Claude Opus 4.7 (1M context)
The package previously shipped the `old/` directory and pointed
package consumers at `deno task test:old`, but those compatibility
tests only make sense inside the Fedify monorepo: they import from
`old/url-template.test.ts` and `old/uri-template-router.test.ts`,
and the `test:old` task lives in the workspace's *deno.json*, not
in the published artifact.
Drop the directory from both publish paths and rewrite the README
pointer so it directs readers to the source repository instead of
to a task they cannot run:
- `package.json` gains a `files` allowlist of `dist`, `package.json`,
and `README.md`, mirroring the `@fedify/postgres` layout.
- `deno.json`'s `publish.exclude` adds `old/` alongside the other
development-only directories.
- *README.md* replaces the `deno task test:old` snippet with a
short note that the comparison tests live under
*packages/uri-template/old/* in the source repository and are
intentionally not part of the published package.
Reported by CodeRabbit in
fedify-dev#758 (2026-05-11).
Assisted-by: Claude Opus 4.7 (1M context)
`Trie` declared its own `TrieEntry` interface with the same four fields (`index`, `initialLiteralPrefix`, `literalLength`, `variableCount`) as `PrioritizedRouteEntry` from *./priority.ts*. The duplicate would drift if the priority schema gained a field, since trie callers and priority helpers are already constrained together. Drop the local interface and tighten the class generic to `PrioritizedRouteEntry` so the trie and the ordering helpers share a single source of truth. Reported by CodeRabbit in fedify-dev#758 (2026-05-11). Assisted-by: Claude Opus 4.7 (1M context)
`TemplateParseError` exposed a `throw(): never` method that was never called anywhere in the codebase, and the sibling base class `TemplateExpansionError` did not expose the same helper. The asymmetry is dead surface area — `throw error` is idiomatic — so remove the helper rather than mirror it onto the expansion side. Reported by CodeRabbit in fedify-dev#758 (2026-05-11). Assisted-by: Claude Opus 4.7 (1M context)
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 679f2c9492
ℹ️ 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
+45
to
47
| constructor(options?: _RouterOptions) { | ||
| this.#router = convertRouterError(() => new _Router(options)); | ||
| } |
There was a problem hiding this comment.
The compatibility shim dropped the public trailingSlashInsensitive property that existed on @fedify/fedify's Router, so code that toggles router.trailingSlashInsensitive after construction now silently stops working. In JavaScript, that assignment creates an unrelated own property on the wrapper instance, while route() still uses the internal #router flag from construction time, causing trailing-slash routes to mis-match unexpectedly for existing consumers relying on the old mutable API.
Useful? React with 👍 / 👎.
Comment on lines
+103
to
+105
| return convertRouterError(() => { | ||
| assertPath(url); | ||
| return this.#router.route(url); |
There was a problem hiding this comment.
route() now calls assertPath(url) before routing, which throws RouterError for strings that don't look like path templates (for example full URLs or other non-path inputs). Previously this method delegated to resolveURI() and returned null on non-matches, so callers could safely probe arbitrary strings; with this change, the same calls now raise exceptions and can break control flow in existing integrations using the deprecated shim.
Useful? React with 👍 / 👎.
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (4)
packages/uri-template/src/tests/json/template/wrong.json (1)
211-243:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winControl-character fixtures still contain double-escaped sequences instead of actual control characters.
This issue was previously identified: the templates use
"\\u0000"etc., which parse as literal 6-character backslash sequences rather than the actual control bytes (NUL, SOH, TAB, LF, CR, US, DEL) that the test names describe. This weakens the negative test coverage for CTL rejection in literals.The fix is to remove one level of escaping so JSON parsing yields actual control characters:
Suggested fix
- "template": "\\u0000", + "template": "\u0000",Apply the same change to lines 216, 221, 226, 231, 236, and 241.
🤖 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, Replace the double-escaped Unicode sequences in the failing fixtures so the JSON contains actual control characters instead of six-character backslash sequences: for each test object whose "name" is "NUL control character (CTL forbidden in literals)", "SOH control character ...", "TAB control character ...", "LF control character ...", "CR control character ...", "US control character ...", and "DEL control character ...", change the "template" value from a double-escaped form like "\\u0000" to a single-escaped form "\u0000" (and similarly "\u0001", "\u0009", "\u000A", "\u000D", "\u001F", "\u007F") so JSON parsing yields the actual control bytes used by the parser to trigger InvalidLiteralError.packages/uri-template/src/types.ts (1)
75-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix JSDoc typo: "reported to by" → "reported by".
Line 75 has an extra "to" in the phrase "errors will be reported to by the
reportfunction".📝 Proposed fix
- * 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🤖 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` at line 75, Fix the JSDoc typo in the comment that mentions the `report` function: change the phrase "errors will be reported to by the `report` function" to "errors will be reported by the `report` function" in the JSDoc block that references `report` in types.ts.packages/uri-template/src/template/match.ts (1)
355-356:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAllow empty exploded associative keys at pair boundaries.
Line 355 currently rejects
separator + "="boundaries (i > keyStart), so inputs likea=1,=2can’t be split into two exploded pairs during matching, breaking round-trip for empty object keys.💡 Minimal fix
- if (body[i] === "=") return i > keyStart; + if (body[i] === "=") return i >= keyStart;🤖 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 355 - 356, The current check "if (body[i] === \"=\") return i > keyStart;" in template/match.ts rejects empty keys before "="; change it to allow an empty key when the "=" is immediately after a separator by returning true if the key is non-empty (i > keyStart) OR if the "=" is at keyStart and the preceding text is the separator (e.g. i === keyStart && i > 0 && body.startsWith(separator, i - separator.length)); replace the single-line return with that combined condition so exploded associative pairs like "a=1,=2" are accepted.packages/fedify/src/federation/middleware.ts (1)
2091-2091:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winType assertion to
Pathreduces type safety.Same issue as Line 1484: the cast
uri.pathname as Pathbypasses type checking. See the earlier comment for details.🤖 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/middleware.ts` at line 2091, The direct cast `uri.pathname as Path` in the call to this.federation.router.route bypasses the type system; instead validate/transform uri.pathname into a proper Path before calling router.route. Update the middleware code around the expression `this.federation.router.route(uri.pathname as Path)` to perform a runtime check/normalization (e.g., ensure it's a string, non-empty, properly prefixed) or use an existing helper that returns a Path, and only pass the validated value to `federation.router.route`; this preserves type safety and avoids unsafe assertions.
🤖 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.
Duplicate comments:
In `@packages/fedify/src/federation/middleware.ts`:
- Line 2091: The direct cast `uri.pathname as Path` in the call to
this.federation.router.route bypasses the type system; instead
validate/transform uri.pathname into a proper Path before calling router.route.
Update the middleware code around the expression
`this.federation.router.route(uri.pathname as Path)` to perform a runtime
check/normalization (e.g., ensure it's a string, non-empty, properly prefixed)
or use an existing helper that returns a Path, and only pass the validated value
to `federation.router.route`; this preserves type safety and avoids unsafe
assertions.
In `@packages/uri-template/src/template/match.ts`:
- Around line 355-356: The current check "if (body[i] === \"=\") return i >
keyStart;" in template/match.ts rejects empty keys before "="; change it to
allow an empty key when the "=" is immediately after a separator by returning
true if the key is non-empty (i > keyStart) OR if the "=" is at keyStart and the
preceding text is the separator (e.g. i === keyStart && i > 0 &&
body.startsWith(separator, i - separator.length)); replace the single-line
return with that combined condition so exploded associative pairs like "a=1,=2"
are accepted.
In `@packages/uri-template/src/tests/json/template/wrong.json`:
- Around line 211-243: Replace the double-escaped Unicode sequences in the
failing fixtures so the JSON contains actual control characters instead of
six-character backslash sequences: for each test object whose "name" is "NUL
control character (CTL forbidden in literals)", "SOH control character ...",
"TAB control character ...", "LF control character ...", "CR control character
...", "US control character ...", and "DEL control character ...", change the
"template" value from a double-escaped form like "\\u0000" to a single-escaped
form "\u0000" (and similarly "\u0001", "\u0009", "\u000A", "\u000D", "\u001F",
"\u007F") so JSON parsing yields the actual control bytes used by the parser to
trigger InvalidLiteralError.
In `@packages/uri-template/src/types.ts`:
- Line 75: Fix the JSDoc typo in the comment that mentions the `report`
function: change the phrase "errors will be reported to by the `report`
function" to "errors will be reported by the `report` function" in the JSDoc
block that references `report` in types.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c5f1ef49-10f7-402a-ad44-6af9dc6d5e3d
⛔ Files ignored due to path filters (2)
deno.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (64)
CHANGES.mdcspell.jsondeno.jsonmise.tomlpackages/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
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