Replace url-template and uri-template-router with @fedify/uri-template#758
Replace url-template and uri-template-router with @fedify/uri-template#7582chanhaeng wants to merge 35 commits intofedify-dev:mainfrom
url-template and uri-template-router with @fedify/uri-template#758Conversation
📝 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(compiled tokens, path)
Template-->>Router: bindings | null
Router-->>Middleware: { name, template, values } | null
Middleware-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
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}
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.
| if (code > 0xefffd) return false; | ||
| const offset = code % 0x10000; | ||
| return offset <= 0xfffd && (code < 0xe0000 || offset >= 0x1000); |
There was a problem hiding this comment.
The logic for checking supplementary plane ucschar seems to contain a potential bug. The condition offset >= 0x1000 for code points in Plane 14 (U+E0000-U+EFFFF) incorrectly excludes the range U+E0000 to U+E0FFF, which is part of the valid ucschar range according to RFC 3987.
For a code point in Plane 14, code >= 0xe0000, so the condition (code < 0xe0000 || offset >= 0x1000) simplifies to offset >= 0x1000. This incorrectly filters out the lower part of the plane's valid range.
For all supplementary planes covered by ucschar (1-14), the valid code points end at ...FFFD. The check offset <= 0xfffd combined with the upper bound check code <= 0xefffd is sufficient. The logic can be simplified.
| if (code > 0xefffd) return false; | |
| const offset = code % 0x10000; | |
| return offset <= 0xfffd && (code < 0xe0000 || offset >= 0x1000); | |
| if (code > 0xefffd) return false; | |
| const offset = code % 0x10000; | |
| return offset <= 0xfffd; |
There was a problem hiding this comment.
Wrong. Read packages/uri-template/summary.txt
There was a problem hiding this comment.
Wrong. Read packages/uri-template/summary.txt
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.
| /** | ||
| * The values extracted from the URI. | ||
| */ | ||
| values: Record<string, string>; |
There was a problem hiding this comment.
The values property in RouterRouteResult is restricted to Record<string, string>, which prevents the Router from correctly matching templates that use Level 4 operators (like explode *) which capture lists or associative arrays. Since this package aims to support full RFC 6570 Level 4, the router should be able to return these composite values by using ExpandContext instead of a fixed string record.
| values: Record<string, string>; | |
| values: ExpandContext; |
References
- Avoid using Record<string, string> for types that can vary based on conditions, as it can lead to type errors and require unnecessary type assertions. Consider using object or a more flexible type if properties are dynamic.
| * @param values The values to expand the path with. | ||
| * @returns The URL/path, if the name exists. Otherwise, `null`. | ||
| */ | ||
| build = (name: string, values: Record<string, string>): Path | null => |
There was a problem hiding this comment.
The build method restricts values to Record<string, string>, which prevents using Level 4 expansion features (lists and objects) through the router interface. It should accept ExpandContext to be consistent with the underlying Template.expand method and avoid overly restrictive types.
| build = (name: string, values: Record<string, string>): Path | null => | |
| build = (name: string, values: ExpandContext): Path | null => |
References
- Avoid using Record<string, string> for types that can vary based on conditions, as it can lead to type errors and require unnecessary type assertions. Consider using object or a more flexible type if properties are dynamic.
| if (tokenIndex >= tokens.length) { | ||
| if (uriIndex !== uri.length) return null; | ||
| const context = toExpandContext(bindings); | ||
| return expand(tokens, context, options) === uri ? context : null; |
There was a problem hiding this comment.
When options.strict is enabled, the expand call during the round-trip check might throw a TemplateExpansionError (e.g., if a prefix modifier is applied to a composite value). In the context of matching, such an error should simply indicate that the current candidate interpretation is invalid and the matcher should continue backtracking, rather than crashing the entire match() call. Using strict: false for the internal expansion check ensures the matcher remains robust.
| return expand(tokens, context, options) === uri ? context : null; | |
| return expand(tokens, context, { ...options, strict: false }) === uri ? context : null; |
| export function isPath(path: string): path is Path { | ||
| 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; |
There was a problem hiding this comment.
The isPath function has two issues: 1) it will throw a TemplateParseError if the provided path is a malformed URI template (contradicting the JSDoc which says it should return false), and 2) it performs an unsafe property access on first.operator when first is a literal token. Wrapping the logic in a try-catch block and adding a type guard for the expression token resolves both issues.
| export function isPath(path: string): path is Path { | |
| 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; | |
| export function isPath(path: string): path is Path { | |
| try { | |
| const template = new Template(path, { strict: false }); | |
| const [first] = template.tokens; | |
| if (first == null) return false; | |
| if (isLiteral(first)) return first.text.startsWith("/"); | |
| if (isExpression(first) && first.operator === "/") return true; | |
| return false; | |
| } catch { | |
| return false; | |
| } | |
| } |
![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
| 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); | ||
| b.start(); | ||
| for (const _ of Array(10000)) { | ||
| test("expand: examples", runPairCases(pairTestSuites)); | ||
| } | ||
| b.end(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify: confirm test() is only called inside Deno.bench() in this bench file,
# and check how `@fedify/fixture`'s test() is implemented.
rg -n "test\(" packages/uri-template/src/template/template.bench.ts
# Check `@fedify/fixture`'s test implementation to confirm it wraps Deno.test()
fd -i "mod.ts" packages/fixture/src --exec cat {}Repository: fedify-dev/fedify
Length of output: 210
🏁 Script executed:
#!/bin/bash
# Find and read the test.ts file from `@fedify/fixture`
find packages/fixture/src -name "test.ts" -exec cat {} \;
# Also check if there are other benchmark files for comparison
find packages -name "*.bench.ts" -type fRepository: fedify-dev/fedify
Length of output: 6276
🏁 Script executed:
#!/bin/bash
# Read the full template.bench.ts file
cat -n packages/uri-template/src/template/template.bench.ts
# Also check createTemplatePairTest to understand what it returns
fd -i "tests/mod.ts" packages/uri-template/src --exec head -100 {}Repository: fedify-dev/fedify
Length of output: 1259
Remove test() call from inside Deno.bench() — it registers tests instead of measuring expansion
The test() function from @fedify/fixture is a test registration API (wraps Deno.test(), Bun.jest().test, or node:test). Calling it inside a Deno.bench() callback attempts to register 10,000 new tests during benchmark execution rather than running the expansion logic inline. This means the benchmark measures test registration overhead, not template expansion.
The test import on line 1 is unused after this fix.
🔧 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();
});📝 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.
| 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); | |
| b.start(); | |
| for (const _ of Array(10000)) { | |
| test("expand: examples", runPairCases(pairTestSuites)); | |
| } | |
| b.end(); | |
| }); | |
| 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)) { | |
| 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 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.
| 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.
| 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); | ||
| }; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
createMatchBench discards match results — add a bench sink for reliability and consistency.
Every router bench function (createRouterRouteHitsBench, createRouterRouteMissesBench, etc.) pipes results through consumeRouterRoute / consumeRouterBenchValue to prevent the JIT from treating the call as dead code. createMatchBench is the only bench helper that silently drops its return value, making it inconsistent and theoretically susceptible to optimization-away.
♻️ Proposed fix — add a file-local sink and consume the result
+let matchBenchSink = 0;
+const consumeMatchResult = (result: ExpandContext | null): void => {
+ matchBenchSink = (matchBenchSink + (result != null ? 1 : 0)) | 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) consumeMatchResult(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 - 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.
| const [first] = template.tokens; | ||
| if (first == null) return false; | ||
| if (isLiteral(first)) return first.text.startsWith("/"); | ||
| if (first.operator === "/") return true; |
There was a problem hiding this comment.
isPath type guard is over-permissive for bare path-expansion expressions.
isPath("{/var}") returns true (line 27 branch), but "{/var}" satisfies neither variant of Path:
/${string}— no, doesn't start with/`{/${string}}/${string}`— no trailing/${string}after the closing}
The JSDoc is also explicit: "a path-expansion expression ({/var}) followed by a literal segment that starts with /" — the implementation never checks the second token.
Either fix the predicate to check the next token, or update the Path type and JSDoc to explicitly permit bare {/var} templates (if the router can handle them):
🐛 Option A — enforce the documented constraint
- if (first.operator === "/") return true;
+ if (first.operator === "/") {
+ const second = template.tokens[1];
+ return second != null && isLiteral(second) && second.text.startsWith("/");
+ }♻️ Option B — relax `Path` to match actual behaviour
-export type Path = `/${string}` | `{/${string}}/${string}`;
+export type Path = `/${string}` | `{/${string}}${string}`;(Update JSDoc to match.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (first.operator === "/") return true; | |
| if (first.operator === "/") { | |
| const second = template.tokens[1]; | |
| return second != null && isLiteral(second) && second.text.startsWith("/"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/uri-template/src/utils.ts` at line 27, The isPath type guard is
returning true for bare path-expansion tokens like "{/var}"; change its logic so
that when checking first.operator === "/" you also verify the next token exists
and is a literal segment that starts with "/" (e.g., tokens[1].type ===
"literal" && tokens[1].value.startsWith("/")) before returning true, and update
the JSDoc if you prefer to relax the Path definition instead; target the isPath
function and the tokens array handling around the first/operator check.
|
@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".
| 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 👍 / 👎.
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]
Add a new workspace package that will host Fedify's own symmetric
RFC 6570 URI Template implementation, replacing the third-party
url-template (expansion) and uri-template-router (parsing) libraries
whose asymmetric behavior has been a recurring source of
encoding/decoding bugs.
This commit only lays down the package skeleton and a public API
mockup so that downstream packages can wire imports against the final
module path before the algorithms land:
- Package metadata (deno.json, package.json, tsdown.config.ts,
README.md) modeled after the existing @fedify/webfinger package.
- src/types.ts: shared types (PrimitiveValue, ExpandContext,
Template, VariableSpec, Route, HierarchyNode, Result).
- src/template.ts: parseTemplate stub returning a Template whose
expand() throws.
- src/router.ts: Router class with the public mutable fields
(nid, fsm, routeSet, templateRouteMap, valueRouteMap, hierarchy)
that the current consumer relies on for structural cloning;
addTemplate/resolveURI throw.
- src/mod.ts: explicit named re-exports of the public surface only.
- Register the package in the root deno.json workspace and in
pnpm-workspace.yaml.
The mocked surface mirrors the names and shapes used by
packages/fedify/src/federation/router.ts so that the consumer can
later switch its imports without further interface changes.
fedify-dev#418
Assisted-by: Claude Code:claude-opus-4-7
Co-authored-by: Copilot <copilot@github.com> Add test for template Add test for template
Replace the stub `parseTemplate` exposed by `@fedify/uri-template` with a full RFC 6570 Level 4 expansion implementation, structured as a `Template` class that owns tokenization and delegates expression expansion to a dedicated module. The single `template.ts` is split into a `template/` directory with focused modules: `token` (tokenizer), `expression` (varspec parser), `expand` (expansion algorithm), `encoding` (pct-encoding helpers), and `mod` (the `Template` class). Operator metadata moves into `const.ts` so tokenizer and expander share a single source of truth for the RFC 6570 first/sep/named/ifemp/allow table. Introduce a typed error hierarchy in `errors.ts` covering both parse and expansion diagnostics, plus a `report`/`strict` option pair on `TemplateOptions` so callers can collect every diagnostic in one pass instead of failing on the first issue. Broaden `ExpandContext` to allow `undefined` and `readonly` arrays, and split it into `PrimitiveValue`, `AssociativeValue`, and `ExpandValue` so the expansion contract matches RFC 6570's primitive /list/associative trichotomy. Re-export the new error classes, `Token`, `VarSpec`, `Operator`, `Reporter`, and `TemplateOptions` from `mod.ts`. Add a `test:bun` script and a package-local `cspell.json` for the new identifiers (`varspec`, `varname`, etc.). `summary.txt` keeps an authoritative excerpt of the RFC 6570 grammar and expansion table next to the implementation for reference. Assisted-by: Claude Code:claude-opus-4-7
Add a runtime-agnostic test harness for the new RFC 6570 `Template`
implementation, organized as JSON fixtures plus typed runner
factories so the same cases can drive `template.test.ts` and the
companion `template.bench.ts`.
The fixtures cover four scenarios:
- *tests/json/references/{pairs,fixed,vars}.json*: the canonical
`uritemplate-test` reference cases (template/expansion pairs,
fixed-template variations, and the standard variable context).
- *tests/json/hard.json*: hard-mode cases targeting subtle RFC 6570
rules — pct-encoded literals, mixed encoding/normalization, and
edge cases for explode/prefix modifiers — each with a `reason`
explaining the expected behavior.
- *tests/json/wrong.json*: negative cases pinned to the exact
error class from `errors.ts` so regressions in diagnostic
precision are caught, not just "something throws".
`tests/assert.ts` validates the JSON shapes at load time, and
`tests/lib.ts` exposes `createTemplatePairTest`,
`createFixedTemplateTest`, `createWrongTemplateTest`, and
`createTemplateHardTest` factories that accept any
`TemplateConstructor`, allowing the same suites to exercise
alternative implementations or the benchmark harness.
Assisted-by: Claude Code:claude-opus-4-7
Thread TemplateOptions through URI template expansion so runtime diagnostics use the same report and strict behavior as parsing. The Template instance now uses its normalized options by default while allowing callers to pass explicit expansion options. Also let Template.parse accept parse options and add coverage for non-strict expansion reporting. Assisted-by: Codex:GPT-5
Explain why Fedify ships its own RFC 6570 implementation instead of wrapping url-template. Document the observed compliance gaps and add a bench test that records url-template behavior against the shared test suites. Assisted-by: Codex:gpt-5
Move expansion over parsed tokens so Template.expand delegates the full token stream at once. Simplify URI character predicates with reusable predicate helpers, and keep URI Template spelling words in the root cspell dictionary. Also make Template expose a string form and route default diagnostics through the package logger. Assisted-by: Codex:GPT-5
Add Template.match() and the matcher implementation for URI Template expressions. The matcher backtracks over expression boundaries and value interpretations, then verifies candidates by re-expanding the template. It handles named and unnamed operators, exploded lists and associative values, prefix bindings, and separator-sensitive named associative pairs. Assisted-by: Codex:gpt-5
Add `match.json` carrying match-only test cases that exercise RFC 6570 match semantics: required literals, operator allow-sets, prefix-modifier consistency, boundary disambiguation, named- operator edge cases, list vs. associative composite disambiguation, decoding failures, and multi-expression URL parsing patterns (ActivityPub URIs, REST resource routes, form-style query continuation, fragment-anchored deep links). Add the `MatchTestSuite` interface and JSON validator alongside runner factories — `createMatchOnlyTest`, `createTemplateMatchTest`, `createFixedTemplateMatchTest`, and `createTemplateMatchHardTest` — and wire them into `template.test.ts` so the existing pair, fixed, and hard suites also round-trip through `match` + `expand`. Assisted-by: Claude Code:claude-opus-4-7
Introduce a new Router class for URI Template routing. It is intended to replace the existing federation router implementation while sharing the strict RFC 6570 parser across build, match, and variables. That shared parser preserves percent-encoded variable names and keeps reserved expansion values intact instead of normalizing them through a separate routing parser. Assisted-by: Codex:gpt-5
Add reusable JSON-backed Router test suites for add, compile errors, variables, clone, route, and build behavior. Include benchmark helpers that exercise bulk routes, deep common prefixes, root-adjacent dynamic routes, and inactive route replacement. Assisted-by: Codex:gpt-5
Add a benchmark test copy of the previously used Router from the federation package so its behavior can be checked against the shared Router suites. Keep inline notes for the known correctness gaps: reserved expansion matches decode pct-encoded values, and url-template double-encodes pct-encoded variable names when building URIs. These tests make the old Router failures reproducible while the new Router implementation replaces that approach. Assisted-by: Codex:gpt-5
Splits each feature module into a thin entry plus dedicated
implementation file:
- src/template/mod.ts now re-exports from template.ts and errors.ts.
Template class implementation moves to src/template/template.ts.
- src/router/mod.ts now re-exports from router.ts and errors.ts.
Router class implementation moves to src/router/router.ts.
Consolidates parse/expansion errors with the template feature:
- src/errors.ts moves to src/template/errors.ts with internal
callers updated.
Reorganizes test fixtures and helpers per feature:
- src/tests/lib.ts moves to src/tests/template.ts.
- src/tests/json/{hard,match,wrong}.json move under
src/tests/json/template/.
All imports across the package are updated to the new locations.
Assisted-by: Claude Code:claude-opus-4-7
Updates package descriptions, the README, and the npm:url-template compatibility test to describe the matching behavior as "round-trip" rather than "symmetric", and corrects stale "@fedify/url-template" references to "@fedify/uri-template". Assisted-by: Claude Code:claude-opus-4-7
Drops the "Scenario" suffix from the router memory pressure factory
names and shortens them so they read alongside the other test
helpers in src/tests/mod.ts:
- createRouterHundredsOfRoutesScenario -> createRoutesPressureTest
- createRouterDeepCommonPrefixScenario -> createDeepPrefixRouterTest
- createRouterRootAdjacentDynamicRoutesScenario
-> createDynamicRoutesTest
- createRouterInactiveEntriesScenario -> createInactiveEntriesTest
Also unexports the RouterMemoryPressureScenario interface, which was
only used internally by the bench, and renames the
ERROR_CLASS_BY_NAME alias in src/tests/router.ts to ERROR_CLASSES so
it matches the convention used by src/tests/assert.ts.
Assisted-by: Codex:GPT-5.5
Assisted-by: Claude Code:claude-opus-4-7
Inlines the RFC 6570 Appendix A operator behavior table next to operatorSpecs so the values for first, sep, named, ifEmpty, and allowReserved are easy to cross-check against the spec. Removes NOT_IMPLEMENTED, which is no longer referenced anywhere now that the matching and router modules are implemented. Assisted-by: Codex:GPT-5.5 Assisted-by: Claude Code:claude-opus-4-7
VariableSpec was carried over from an earlier router design and is no longer constructed or referenced anywhere; remove the interface and its re-export from src/mod.ts. Updates the TemplateOptions.report and Reporter doc comments to reflect that the reporter is also invoked for expansion errors (PrefixModifierNotApplicableError), not just parse errors. Also cleans up the line-broken module-level header comment in src/mod.ts. Assisted-by: Codex:GPT-5.5 Assisted-by: Claude Code:claude-opus-4-7
Extends the header comment with the route-shape differences that
the compatibility run records against the previous router:
- Leading path expansion templates such as {/identifier}/inbox
are rejected when they overlap with slash-prefixed routes.
- Optional form-style query templates such as /search{?q,page}
miss when only one of the query variables is present.
These behaviors matter for Fedify routes, so calling them out in
the bench file makes the gap visible alongside the encoding/decoding
differences already noted.
Assisted-by: Codex:GPT-5.5
Assisted-by: Claude Code:claude-opus-4-7
The Template implementation no longer uses RegExp for matching, so the bench label "Template using RegExp" is misleading. Use plain "Template" instead. Assisted-by: Codex:GPT-5.5 Assisted-by: Claude Code:claude-opus-4-7
The Router previously offered only a single-route `add(path, name)`
entry point. Bulk registration paid the cost of repeated trie inserts,
each running an insertion-sort splice on the destination node, and the
public surface had no ergonomic way to seed routes at construction
time.
Changes:
- Add `Router#register(routes)` that accepts an iterable of
`[pathOrPattern, name]` tuples and routes them through a new batch
insertion path. `Trie#insertAll` groups entries by destination
node so each node performs one sort-and-merge instead of N
insertion-sort splices. `Node#insertAll` reuses the existing
`mergeRouteEntries` linear merge over the already-sorted entries.
- Overload the `Router` constructor to accept `(routes, options?)`,
`(options?)`, or no arguments. The first argument is dispatched by
`Symbol.iterator` presence so plain options objects and arbitrary
iterables (arrays, generators, `Map.entries()`) are both handled.
Add `Router.from(...)` as a static factory mirroring the same
signature.
- Widen `Router#add` and `Router#register` to accept a pre-parsed
`RouterPathPattern` in addition to a `Path` string, so callers
that already hold a `Router.compile(...)` result can skip
re-parsing.
- Rebuild `Router#clone` on top of the constructor's batch path. The
clone now hands its active patterns straight to a new router
instead of replaying `add` for every entry, sharing the parsed
templates with the original.
- Export the new `RouterRoute` type plus the existing
`RouterOptions`, `RouterPathPattern`, and `RouterRouteResult`
types from `router/mod.ts` and re-route the public `mod.ts`
re-exports through that module entry point.
- Cover the new surface with unit tests: bulk registration parity
with repeated `add`, generator inputs, pre-parsed pattern inputs
on every entry point (`add`, `register`, constructor, `from`),
and all four constructor and `from` argument variants.
Assisted-by: Claude Code:claude-opus-4-7[1m]
Move the compatibility tests for url-template and uri-template-router from the benchmark task into the old test area. Gate the old-library comparison tests behind OLD=true and expose them through deno task test:old, so the regular test task only runs the current src tests. Update the README and Deno package metadata to use the new command and location. Assisted-by: Codex:gpt-5
Restructure the test factories in src/tests/template.ts to accept
arrays of test suites instead of flat case lists. Each suite now
becomes an outer t.step containing per-case inner steps, so the
test reporter mirrors the suite/case shape declared in the data
files instead of the call-site for-loop.
In the old/ test files, replace the `if (isTest) { ... }` wrapper
with the `{ ignore: !isOldTest }` test option so every test is
registered unconditionally and is visible to the reporter even
when skipped.
Update deno.json to exclude bench files, the shared src/tests/
helpers, and summary.txt from the published artifact, and drop the
`src/` filter from the default `test` task so all tests under the
package are picked up.
Update template.bench.ts to pass the suite array through the
revised factory signature.
Assisted-by: Claude Code:claude-opus-4-7
Replace the internal `Router`/`RouterError` from
*src/federation/router.ts* with the implementations from the new
`@fedify/uri-template` workspace package across `FederationBuilder`,
middleware, NodeInfo handler, and testing context. The legacy
*router.ts* exports are kept as a thin compatibility shim with
`@deprecated` JSDoc directing callers to the new package.
The new `Router` splits route registration from variable extraction:
`router.add(template, name)` now returns `void`, while the static
`Router.variables(template)` returns the variable names. All call
sites are updated accordingly.
`validateSingleIdentifierVariablePath` is rewritten to use
`Router.compile` and `isExpression` so it can inspect the parsed
`VariableSpec` directly, which lets it reject explode (`{var*}`) and
prefix (`{var:N}`) modifiers in addition to query/fragment operators.
Corresponding regression tests are added in
*builder.test.ts*.
Also list `@fedify/uri-template` in the packages table in
*packages/fedify/README.md* and add the workspace dependency to
*packages/fedify/package.json* / *pnpm-lock.yaml*.
Assisted-by: Codex:GPT-5.5
Assisted-by: Claude Code:claude-opus-4-7
Add changelog entries for the new *@fedify/uri-template* package and for replacing Fedify's internal federation routing with it. Both entries reference fedify-dev#418. Assisted-by: Claude Code:claude-opus-4-7
Move the path-template predicate out of the router and into the
public utilities module so callers can validate `Path` values without
reaching into router internals. The new implementation parses the
input as a `Template` and inspects the first token, which accepts
multi-variable path expansions like `{/a,b}/...` that the previous
regex-based check rejected.
Also adds a sibling `isLiteral` helper alongside the existing
`isExpression` token guard.
Assisted-by: Claude Code:claude-opus-4-7
Two small correctness/performance fixes in the match backtracker:
- `consumeUnnamed`'s `minLength` heuristic called
`remainingVars(vars)` with the full var list, which collapsed to
`vars.length - 1` regardless of progress. Pass `varIndex` through
so the bound actually reflects how many variables remain after the
current one.
- `consumeNamedList` checked `if (values)` against an array, which is
always truthy. The intent was to skip empty matches; replace with
`values.length > 0` so empty lists no longer yield no-progress
bindings that the surrounding backtracker has to discard.
Neither change affects matching correctness — round-trip verification
filters out the spurious branches — but both reduce the search space
the backtracker has to walk through.
Assisted-by: Claude Code:claude-opus-4-7
Drop the no-op `Template (match)` bench that ran the RFC 6570 example
suite as a wrapper around `test()` — pair examples have variables of
1–3 parts with literal anchors on both sides, so the backtracker
collapses to one branch per case and the result was indistinguishable
from a constant-time loop.
Replace it with two cases that actually exercise the matcher:
- A 3-var unnamed expression against a 12-part body, where the
comma-distribution backtracker has C(11, 2) = 55 candidate
splits.
- A `{/paths*}` exploded path expansion against 728 progressively
deeper URIs, exercising the path-segment list reader.
Factor the per-template benchmark loop into `createMatchBench` and
the path-corpus generator into `createMatchBenchTestCases` under
`packages/uri-template/src/tests/template.ts` so other Template
implementations can reuse the same harness.
Assisted-by: Claude Code:claude-opus-4-7
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
| function validateSingleIdentifierVariablePath( | ||
| path: string, | ||
| errorMessage: string, | ||
| ): void { | ||
| const operatorMatches = globalThis.Array.from( | ||
| path.matchAll(/{([+#./;?&]?)([A-Za-z_][A-Za-z0-9_]*)}/g), | ||
| ); | ||
| if ( | ||
| operatorMatches.length !== 1 || | ||
| operatorMatches[0]?.[2] !== "identifier" | ||
| ) { | ||
| const pattern = Router.compile(path as Path); | ||
| if (pattern.variables.size !== 1 || !pattern.variables.has("identifier")) { | ||
| throw new RouterError(errorMessage); | ||
| } | ||
| if ( | ||
| operatorMatches.some((match) => | ||
| ["?", "&", "#"].includes(match[1]) && match[2] === "identifier" | ||
| ) | ||
| ) { | ||
| const expressions = pattern.template.tokens | ||
| .filter(isExpression) | ||
| .filter((token) => token.vars.some(({ name }) => name === "identifier")); | ||
|
|
||
| if (expressions.length !== 1 || expressions[0].vars.length !== 1) { | ||
| throw new RouterError(errorMessage); | ||
| } | ||
| const variables = new Router().add(path, "outbox"); | ||
| if (variables.size !== 1 || !variables.has("identifier")) { | ||
|
|
||
| const { operator, vars: [varSpec] } = expressions[0]; | ||
| if ( | ||
| ["?", "&", "#"].includes(operator) || | ||
| varSpec.explode || | ||
| varSpec.prefix != null | ||
| ) { | ||
| throw new RouterError(errorMessage); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm Router.compile's failure mode and the error type it throws.
fd -t f 'router.ts' packages/uri-template/src/router
ast-grep --pattern 'static compile($_) { $$$ }'
rg -nP --type=ts -C3 '\bcompile\s*\(' packages/uri-template/src/routerRepository: fedify-dev/fedify
Length of output: 2859
🏁 Script executed:
# Get full Router.compile implementation
sed -n '146,160p' packages/uri-template/src/router/router.ts
# Check what RouteTemplatePathError is
rg -n 'class RouteTemplatePathError' packages/uri-template/src/
# Check for TemplateParseError references
rg -n 'TemplateParseError\|UnclosedExpressionError' packages/uri-template/src/
# Look at isPath function definition
rg -n 'const isPath|function isPath' packages/uri-template/src/Repository: fedify-dev/fedify
Length of output: 613
🏁 Script executed:
# Find Template class and parse method
fd -t f 'template.ts' packages/uri-template/src/
rg -n 'class Template' packages/uri-template/src/
# Get Template.parse implementation
rg -n 'static parse\|parse.*\(' packages/uri-template/src/ -A 5 | head -50Repository: fedify-dev/fedify
Length of output: 431
🏁 Script executed:
# Get Template.parse implementation
sed -n '1,100p' packages/uri-template/src/template/template.ts | grep -A 30 'static parse\|parse('
# Or simpler: show parse method
rg -n 'parse\s*\(' packages/uri-template/src/template/template.ts -A 10 | head -50Repository: fedify-dev/fedify
Length of output: 1107
🏁 Script executed:
# Get Template constructor
sed -n '1,200p' packages/uri-template/src/template/template.ts | grep -B 5 -A 30 'constructor\|#tokens\|parseTokens'Repository: fedify-dev/fedify
Length of output: 2542
🏁 Script executed:
# Check Router.compile call to Template.parse
sed -n '146,160p' packages/uri-template/src/router/router.ts
# Get tokenize implementation
rg -n 'function tokenize|const tokenize' packages/uri-template/src/ -A 5Repository: fedify-dev/fedify
Length of output: 855
🏁 Script executed:
# Get full tokenize implementation to see what errors it creates/throws
sed -n '17,150p' packages/uri-template/src/template/token.tsRepository: fedify-dev/fedify
Length of output: 2018
🏁 Script executed:
# Get isPath implementation
sed -n '22,40p' packages/uri-template/src/utils.ts
# Check error class hierarchy
rg -n 'class UnclosedExpressionError|class NestedOpeningBraceError|export class TemplateParseError' packages/uri-template/src/ -A 2Repository: fedify-dev/fedify
Length of output: 1430
🏁 Script executed:
# Check if there are test cases for operator validation
rg -n 'operator|{;|{.|{+' packages/fedify/src/federation/builder.test.ts -B 2 -A 2
# Check what the actual operator field contains
rg -n 'operator' packages/uri-template/src/template/ -B 1 -A 1 | head -40Repository: fedify-dev/fedify
Length of output: 2787
🏁 Script executed:
# Check builder test for operator cases
rg -n 'assertThrows|identifier' packages/fedify/src/federation/builder.test.ts -B 2 -A 2 | head -80
# Check operatorSpecs to see all valid operators
rg -n 'operatorSpecs' packages/uri-template/src/ -A 15 | head -50Repository: fedify-dev/fedify
Length of output: 6410
🏁 Script executed:
# Get operatorSpecs full definition
sed -n '1,100p' packages/uri-template/src/const.tsRepository: fedify-dev/fedify
Length of output: 2630
🏁 Script executed:
# Verify which operators are actually rejected in current code
sed -n '73,97p' packages/fedify/src/federation/builder.ts
# Check if there are test cases for operators like {.identifier}, {;identifier}, {+identifier}
rg -n '\{.*identifier\}' packages/fedify/src/federation/builder.test.tsRepository: fedify-dev/fedify
Length of output: 2370
🏁 Script executed:
# Check what operators are actually tested to work
rg -n '{/identifier}' packages/fedify/src/federation/builder.test.ts -B 2 -A 2
# Verify the intent: is "/" operator intentionally supported?
rg -n 'users.*identifier' packages/fedify/src/federation/builder.test.ts | head -20Repository: fedify-dev/fedify
Length of output: 1554
🏁 Script executed:
# Check line 238 context - is it expected to succeed or fail?
sed -n '235,245p' packages/fedify/src/federation/builder.test.ts
# Check if there's a pattern: what's the intent of {/identifier}?
sed -n '230,260p' packages/fedify/src/federation/builder.test.tsRepository: fedify-dev/fedify
Length of output: 1317
🏁 Script executed:
# Check where validateSingleIdentifierVariablePath is called
rg -n 'validateSingleIdentifierVariablePath' packages/fedify/src/federation/builder.ts
# Check the callers and context
sed -n '100,150p' packages/fedify/src/federation/builder.tsRepository: fedify-dev/fedify
Length of output: 1854
Wrap Router.compile() to catch and convert template parse errors to RouterError.
Router.compile() can throw TemplateParseError subclasses (e.g., UnclosedExpressionError) when the template is malformed. These exceptions escape unwrapped, bypassing the documented errorMessage contract. Since call sites use as Path casts that bypass compile-time validation, wrap the compile call to convert any thrown error to RouterError with the provided message.
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);
}🤖 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 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.
| const { operator, vars: [varSpec] } = expressions[0]; | ||
| if ( | ||
| ["?", "&", "#"].includes(operator) || | ||
| varSpec.explode || | ||
| varSpec.prefix != null | ||
| ) { | ||
| throw new RouterError(errorMessage); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
RFC 6570 URI Template path segment which operators are appropriate for routing path variables
💡 Result:
In RFC 6570, the operators appropriate for routing path variables are the ones that define how variables are expanded in the path component: 1) {/var} (slash-prefixed path segment expansion) Use operator “/” (i.e., templates like {/var}) when a variable should correspond to one or more path segments in a URI path hierarchy. RFC 6570 describes “Path segment expansion: {/var}” as being useful for describing URI path hierarchies, and it expands by prefixing each variable value with “/” (and uses the unreserved character set for allowed characters during that expansion). [1][2] 2) {;var} (path-style parameter expansion) when routing “;param” style path parameters Use operator “;” (templates like {;var} or {/path{;var}} forms) for semicolon-delimited path parameters that appear inside the path (e.g., “path;property” or “path;name=value”). RFC 6570 explicitly defines this as “Path-Style Parameter Expansion: {;var}”. These are still part of the path syntax and are relevant when your routing system treats “;…” as part of the matched path. [1] 3) {+var} is also path-relevant in practice, but it is “reserved expansion” The “+” operator allows reserved characters to appear unencoded (whereas most other operators pct-encode reserved characters found in values). RFC 6570 defines “+” as “Reserved character strings” in the operator list, and documents that reserved-vs-percent-encoding behavior differs by operator. This can matter if your routing expects slashes or other reserved delimiters to be preserved from the variable value rather than percent-encoded. [1] Important distinction: operators are different from “matching” semantics RFC 6570 is primarily a URI Template expansion standard. It can be used to match/extract variables in some implementations, but the core RFC defines how template expressions are expanded into URIs, and how the allowed/pct-encoding rules depend on the operator. Variable matching only works well when template expressions are delimited in the URI by characters that cannot be part of expansion (RFC notes regex languages are generally better for full matching). [3] Therefore, for routing path variables (as distinct from query variables), the standard, path-specific operators are: - “/” operator via {/var} for path segments (hierarchy routing) [1][2] - “;” operator via {;var} for path-style parameters embedded in the path [1] And only use “+” if you explicitly want reserved characters from the variable value to be allowed rather than percent-encoded (since routing patterns may then interpret embedded reserved delimiters differently). [1]
Citations:
- 1: https://datatracker.ietf.org/doc/html/rfc6570
- 2: https://www.irt.org/rfc/rfc6570.htm
- 3: https://www.potaroo.net/ietf/rfc/rfc6570.txt
🏁 Script executed:
# Locate and examine the builder.ts file
fd -t f "builder.ts" packages/fedify/src/federation/Repository: fedify-dev/fedify
Length of output: 103
🏁 Script executed:
# Read the file to understand the context around lines 89-96
cat -n packages/fedify/src/federation/builder.ts | sed -n '80,110p'Repository: fedify-dev/fedify
Length of output: 1284
🏁 Script executed:
# Read more context to understand the function name and purpose
cat -n packages/fedify/src/federation/builder.ts | sed -n '60,97p'Repository: fedify-dev/fedify
Length of output: 1354
🏁 Script executed:
# Find where validateSingleIdentifierVariablePath is called
rg "validateSingleIdentifierVariablePath" packages/fedify/src/federation/Repository: fedify-dev/fedify
Length of output: 998
🏁 Script executed:
# Find calls to validateSingleIdentifierVariablePath with context
rg -B 2 -A 2 "validateSingleIdentifierVariablePath\(" packages/fedify/src/federation/builder.tsRepository: fedify-dev/fedify
Length of output: 2078
🏁 Script executed:
# Look for tests related to this validator
fd -t f "test|spec" packages/fedify/src/federation/ | head -20Repository: fedify-dev/fedify
Length of output: 718
🏁 Script executed:
# Search for tests related to path validation in builder.test.ts
rg -A 5 -B 5 "validateSingleIdentifierVariablePath|actor.*path|inbox.*path" packages/fedify/src/federation/builder.test.ts | head -100Repository: fedify-dev/fedify
Length of output: 43
🏁 Script executed:
# Check the size of builder.test.ts and read it
wc -l packages/fedify/src/federation/builder.test.tsRepository: fedify-dev/fedify
Length of output: 112
🏁 Script executed:
# Search for test cases that might include path patterns with different operators
rg "path.*=|setActor|setInbox|setOutbox|{.*}" packages/fedify/src/federation/builder.test.ts | head -40Repository: fedify-dev/fedify
Length of output: 213
🏁 Script executed:
# Search for test cases with path patterns
rg "setActor|setInbox|setOutbox" packages/fedify/src/federation/builder.test.ts -A 3 | head -60Repository: fedify-dev/fedify
Length of output: 1894
🏁 Script executed:
# Look for any documentation or comments about RFC 6570 in the codebase
rg -i "rfc.6570|uri.template|operator" packages/fedify/src/federation/builder.tsRepository: fedify-dev/fedify
Length of output: 190
🏁 Script executed:
# Check the RFC 6570 implementation to understand what operators are available
rg -i "operator" packages/fedify/src/ -A 2 -B 2 | grep -i "operator\|enum\|const" | head -30Repository: fedify-dev/fedify
Length of output: 362
🏁 Script executed:
# Check what "@fedify/uri-template" provides and its operator definitions
fd -type f -path "*uri-template*" . | head -20Repository: fedify-dev/fedify
Length of output: 232
🏁 Script executed:
# Check what "@fedify/uri-template" provides and its operator definitions
fd -t f "*uri-template*"Repository: fedify-dev/fedify
Length of output: 355
🏁 Script executed:
# Search for uri-template imports to understand the package
rg "@fedify/uri-template" packages/fedify/src/ -B 1 -A 1 | head -20Repository: fedify-dev/fedify
Length of output: 1350
🏁 Script executed:
# Search for test cases that verify path validation errors
rg "RouterError|must have one variable" packages/fedify/src/federation/builder.test.ts -B 5 | head -60Repository: fedify-dev/fedify
Length of output: 1753
Use an allowlist for URI template operators to reject unsupported path-style and label-style expansions.
The operator validation uses a denylist rejecting only ?, &, and #. However, RFC 6570 defines other operators unsuitable for these federation routing paths: {;identifier} (semicolon-delimited parameters) and {.identifier} (label expansion) silently pass through. An allowlist makes intent explicit and prevents future drift:
Proposed fix
const { operator, vars: [varSpec] } = expressions[0];
+ const allowedOperators = ["", "/", "+"]; // simple, path-segment, reserved
if (
- ["?", "&", "#"].includes(operator) ||
+ !allowedOperators.includes(operator) ||
varSpec.explode ||
varSpec.prefix != null
) {
throw new RouterError(errorMessage);
}📝 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.
| const { operator, vars: [varSpec] } = expressions[0]; | |
| if ( | |
| ["?", "&", "#"].includes(operator) || | |
| varSpec.explode || | |
| varSpec.prefix != null | |
| ) { | |
| throw new RouterError(errorMessage); | |
| } | |
| const { operator, vars: [varSpec] } = expressions[0]; | |
| const allowedOperators = ["", "/", "+"]; // simple, path-segment, reserved | |
| if ( | |
| !allowedOperators.includes(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, 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.
| clone(): Router { | ||
| const clone = new Router({ | ||
| trailingSlashInsensitive: this.trailingSlashInsensitive, | ||
| return convertRouterError(() => { | ||
| const clone = new Router(); | ||
| clone.#router = this.#router.clone(); | ||
| return clone; | ||
| }); | ||
| clone.#router = cloneInnerRouter(this.#router); | ||
| clone.#templates = { ...this.#templates }; | ||
| clone.#templateStrings = { ...this.#templateStrings }; | ||
| return clone; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
clone() no longer copies router state.
new Router() in the new constructor delegates to new _Router(undefined), ignoring whatever options/state the original instance was created with, and then clone.#router = this.#router.clone() overwrites it. That works for #router, but any state held on the wrapper itself (none today) would be lost; more importantly convertRouterError is invoked twice (once for the outer new Router() and once when its constructor calls convertRouterError(...) again), compounding the deprecation-log noise mentioned above. Consider bypassing the public constructor:
♻️ Proposed fix
clone(): Router {
return convertRouterError(() => {
- const clone = new Router();
- clone.#router = this.#router.clone();
+ const clone = Object.create(Router.prototype) as Router;
+ // `@ts-ignore`: assigning private field via the same class context
+ clone.#router = this.#router.clone();
return clone;
});
}📝 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.
| clone(): Router { | |
| const clone = new Router({ | |
| trailingSlashInsensitive: this.trailingSlashInsensitive, | |
| return convertRouterError(() => { | |
| const clone = new Router(); | |
| clone.#router = this.#router.clone(); | |
| return clone; | |
| }); | |
| clone.#router = cloneInnerRouter(this.#router); | |
| clone.#templates = { ...this.#templates }; | |
| clone.#templateStrings = { ...this.#templateStrings }; | |
| return clone; | |
| } | |
| clone(): Router { | |
| return convertRouterError(() => { | |
| const clone = Object.create(Router.prototype) as Router; | |
| // `@ts-ignore`: assigning private field via the same class context | |
| 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 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.
| function convertRouterError<T>(func: () => T): T { | ||
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
Deprecation warning fires on every router call — log once instead.
convertRouterError is invoked from the constructor, clone, has, add, route, and build. Each call unconditionally emits a logger.warn(...), so a single request that uses the compat Router will produce many duplicate "deprecated" lines (and clone() triggers the warn twice — once for itself, once for the inner new Router()). On error paths, the rewrapping throw new RouterError(error.message) (line 149) additionally triggers RouterError's own constructor warn, doubling up again.
Consider warning once per process (or once per instance) and dropping the warn from the rewrap path:
♻️ Proposed fix
+let warnedRouter = false;
+let warnedRouterError = false;
+
function convertRouterError<T>(func: () => T): T {
try {
- logger.warn(
- "The `Router` class from `@fedify/fedify` is deprecated." +
- " Please use `Router` from `@fedify/uri-template` instead.",
- );
+ if (!warnedRouter) {
+ warnedRouter = true;
+ 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 the same one-shot guard inside RouterError's constructor.
📝 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.
| function convertRouterError<T>(func: () => T): T { | |
| 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; | |
| } | |
| } | |
| let warnedRouter = false; | |
| let warnedRouterError = false; | |
| function convertRouterError<T>(func: () => T): T { | |
| try { | |
| if (!warnedRouter) { | |
| warnedRouter = true; | |
| 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; | |
| } | |
| } |
🤖 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 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.
| import { test } from "@fedify/fixture"; | ||
| import { deepEqual, equal } from "node:assert"; | ||
| import { ok, throws } from "node:assert/strict"; |
There was a problem hiding this comment.
Mix of strict and non-strict node:assert imports.
equal and deepEqual are imported from node:assert (non-strict, uses == / Object.is-loose), while ok/throws come from node:assert/strict. This is an easy footgun: e.g., equal(undefined, null) would pass with the non-strict version. Import everything from node:assert/strict for consistency.
📝 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";📝 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.
| 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,
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.
| // Regression for the `consumeUnnamed` minLength bug: when an unnamed expression | ||
| // has more separated parts than variables, the matcher must let the *current* | ||
| // variable absorb fewer parts than the naive `parts - remainingVars` formula | ||
| // allows. With `{x:5,y}` against `abc,def,ghi` the only round-trippable | ||
| // binding has x consume one part (so prefix:5 truncation does not corrupt the | ||
| // joined string); under the buggy minLength formula the matcher only reaches | ||
| // the fallback `x undefined, y absorbs everything` decomposition, leaving | ||
| // `m.x` undefined. | ||
| test("Template#match — unnamed minLength must allow current var to consume one part", () => { | ||
| const template = new Template("{x:5,y}"); | ||
| const m = template.match("abc,def,ghi"); | ||
|
|
||
| ok(m != null, "matcher returned null for a round-trippable URI"); | ||
| equal(template.expand(m), "abc,def,ghi"); | ||
| equal( | ||
| m.x, | ||
| "abc", | ||
| "matcher should reach the binding with x consuming one part", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether `@fedify/fixture`'s `test` exports a `.skip`/`.todo` variant, and
# confirm whether the matcher fix is included in this PR (i.e. whether this test passes).
ast-grep --pattern 'export $$$ test $$$'
rg -nP --type=ts -C3 '\b(skip|todo)\b' packages/fixture/src
fd -t f 'consumeUnnamed' packages/uri-template/src
rg -nP --type=ts -C5 '\bconsumeUnnamed\b|minLength' packages/uri-template/srcRepository: fedify-dev/fedify
Length of output: 7500
🏁 Script executed:
#!/bin/bash
# Check the full export signature and API of the test function from `@fedify/fixture`
cat -n packages/fixture/src/test.ts | head -150Repository: fedify-dev/fedify
Length of output: 5434
🏁 Script executed:
#!/bin/bash
# Look for the actual test function definition and what it exports
ast-grep --pattern 'export $$$ function test'Repository: fedify-dev/fedify
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if the test has been marked with skip, todo, or similar in the actual test file
cat -n packages/uri-template/src/template/template.test.ts | sed -n '110,135p'Repository: fedify-dev/fedify
Length of output: 1294
🏁 Script executed:
#!/bin/bash
# Check if there have been any recent changes to the minLength calculation or consumeUnnamed
rg -B5 -A10 'Math.max.*minLength|minLength.*Math.max' packages/uri-template/srcRepository: fedify-dev/fedify
Length of output: 1283
🏁 Script executed:
#!/bin/bash
# Search for examples of how tests are skipped in the codebase
rg -n 'ignore\s*:\s*true|test\.skip|test\.todo' packages --type ts | head -20Repository: fedify-dev/fedify
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check the Deno.TestDefinition type to see available skip options
rg -B2 -A8 'interface.*TestDefinition|type.*TestDefinition' packages/fixture/src/test.tsRepository: fedify-dev/fedify
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look for any recent matcher fixes or improvements to consumeUnnamed
git log --oneline -20 -- packages/uri-template/src/template/match.ts 2>/dev/null || echo "Git log not available"Repository: fedify-dev/fedify
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Search for how tests use the ignore property to skip
rg -n 'ignore\s*:' packages --type ts --type tsx | head -15Repository: fedify-dev/fedify
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Check if there's a way to conditionally set the test definition
grep -r "ignore.*true\|todo\|skip" packages/uri-template/src/template/template.test.ts || echo "No skip patterns found in regression test"Repository: fedify-dev/fedify
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Look at the git commit that added review feedback to understand what was changed
git show d313e635 --stat 2>/dev/null | head -30Repository: fedify-dev/fedify
Length of output: 1445
🏁 Script executed:
#!/bin/bash
# Check if the test file has any conditional test skipping or todo markers at all
rg -B5 -A5 'ignore|skip|todo' packages/uri-template/src/template/template.test.ts || echo "No skip/ignore in test file"Repository: fedify-dev/fedify
Length of output: 89
Use the correct @fedify/fixture API to gate the intentionally-failing regression test.
The test is intentionally failing per the commit, and will block CI. However, @fedify/fixture's test function doesn't export a .skip() method. Instead, gate the test using the ignore option in Deno.TestDefinition:
🛠️ Suggestion
-test("Template#match — unnamed minLength must allow current var to consume one part", () => {
+test(
+ "Template#match — unnamed minLength must allow current var to consume one part",
+ { ignore: true },
+ () => {Or equivalently:
-test("Template#match — unnamed minLength must allow current var to consume one part", () => {
+test({
+ name: "Template#match — unnamed minLength must allow current var to consume one part",
+ ignore: true,
+ fn: () => {📝 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.
| // Regression for the `consumeUnnamed` minLength bug: when an unnamed expression | |
| // has more separated parts than variables, the matcher must let the *current* | |
| // variable absorb fewer parts than the naive `parts - remainingVars` formula | |
| // allows. With `{x:5,y}` against `abc,def,ghi` the only round-trippable | |
| // binding has x consume one part (so prefix:5 truncation does not corrupt the | |
| // joined string); under the buggy minLength formula the matcher only reaches | |
| // the fallback `x undefined, y absorbs everything` decomposition, leaving | |
| // `m.x` undefined. | |
| test("Template#match — unnamed minLength must allow current var to consume one part", () => { | |
| const template = new Template("{x:5,y}"); | |
| const m = template.match("abc,def,ghi"); | |
| ok(m != null, "matcher returned null for a round-trippable URI"); | |
| equal(template.expand(m), "abc,def,ghi"); | |
| equal( | |
| m.x, | |
| "abc", | |
| "matcher should reach the binding with x consuming one part", | |
| ); | |
| }); | |
| // Regression for the `consumeUnnamed` minLength bug: when an unnamed expression | |
| // has more separated parts than variables, the matcher must let the *current* | |
| // variable absorb fewer parts than the naive `parts - remainingVars` formula | |
| // allows. With `{x:5,y}` against `abc,def,ghi` the only round-trippable | |
| // binding has x consume one part (so prefix:5 truncation does not corrupt the | |
| // joined string); under the buggy minLength formula the matcher only reaches | |
| // the fallback `x undefined, y absorbs everything` decomposition, leaving | |
| // `m.x` undefined. | |
| test( | |
| "Template#match — unnamed minLength must allow current var to consume one part", | |
| { ignore: true }, | |
| () => { | |
| const template = new Template("{x:5,y}"); | |
| const m = template.match("abc,def,ghi"); | |
| ok(m != null, "matcher returned null for a round-trippable URI"); | |
| equal(template.expand(m), "abc,def,ghi"); | |
| equal( | |
| m.x, | |
| "abc", | |
| "matcher should reach the binding with x consuming one part", | |
| ); | |
| }, | |
| ); |
🤖 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 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.
| }; | ||
| } | ||
|
|
||
| const mod = (i: number, j: number) => Math.floor(i / j); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
mod is actually integer division, not modulo.
Math.floor(i / j) is Math.idiv / quotient, not the remainder. The name will mislead future readers (especially given the next line at 368 uses i % 26 for the actual modulo). Rename to div (or inline) for clarity.
📝 Proposed fix
-const mod = (i: number, j: number) => Math.floor(i / j);
+const div = (i: number, j: number) => Math.floor(i / j);
@@
- String.fromCharCode(0x61 + mod(i, 26)) +
+ String.fromCharCode(0x61 + div(i, 26)) +
String.fromCharCode(0x61 + i % 26);🤖 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` at 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.
| export interface TemplateOptions { | ||
| /** | ||
| * 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 | ||
| * thrown unless the `report` function itself throws. | ||
| */ | ||
| strict: boolean; | ||
| /** | ||
| * A function that will be called with any errors encountered while parsing | ||
| * or expanding. By default, errors are ignored. In strict mode, they are | ||
| * still thrown after this reporter runs. | ||
| * @param error The error that was encountered while parsing or expanding the | ||
| * template. | ||
| * @returns The result of the report function. | ||
| */ | ||
| report: Reporter; | ||
| } | ||
|
|
||
| /** | ||
| * Callback used to report recoverable parse and expansion diagnostics. | ||
| */ | ||
| export type Reporter = (error: Error) => void; |
There was a problem hiding this comment.
TemplateOptions.strict and report are documented as defaulted but typed as required.
Both fields are non-optional, yet the JSDoc states "true is the default value" for strict and "By default, errors are ignored" for report. Callers passing new Template("...", { strict: false }) would get a type error today, contradicting the documented contract. Either mark these as optional in the interface, or expose options via Partial<TemplateOptions> at the API boundary so the documented defaults can actually apply.
Also, the @returns tag on report doesn't match the void return of Reporter, and the sentence "errors will be reported to by the report function" has a stray "to".
🛠️ Proposed fix
export interface TemplateOptions {
/**
* 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 to the `report` function, but none will be
* thrown unless the `report` function itself throws.
*/
- strict: boolean;
+ strict?: boolean;
/**
* A function that will be called with any errors encountered while parsing
* or expanding. By default, errors are ignored. In strict mode, they are
* still thrown after this reporter runs.
* `@param` error The error that was encountered while parsing or expanding the
* template.
- * `@returns` The result of the report function.
*/
- report: Reporter;
+ report?: Reporter;
}📝 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.
| export interface TemplateOptions { | |
| /** | |
| * 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 | |
| * thrown unless the `report` function itself throws. | |
| */ | |
| strict: boolean; | |
| /** | |
| * A function that will be called with any errors encountered while parsing | |
| * or expanding. By default, errors are ignored. In strict mode, they are | |
| * still thrown after this reporter runs. | |
| * @param error The error that was encountered while parsing or expanding the | |
| * template. | |
| * @returns The result of the report function. | |
| */ | |
| report: Reporter; | |
| } | |
| /** | |
| * Callback used to report recoverable parse and expansion diagnostics. | |
| */ | |
| export type Reporter = (error: Error) => void; | |
| export interface TemplateOptions { | |
| /** | |
| * 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 the `report` function, but none will be | |
| * thrown unless the `report` function itself throws. | |
| */ | |
| strict?: boolean; | |
| /** | |
| * A function that will be called with any errors encountered while parsing | |
| * or expanding. By default, errors are ignored. In strict mode, they are | |
| * still thrown after this reporter runs. | |
| * `@param` error The error that was encountered while parsing or expanding the | |
| * template. | |
| */ | |
| report?: Reporter; | |
| } | |
| /** | |
| * Callback used to report recoverable parse and expansion diagnostics. | |
| */ | |
| export type Reporter = (error: Error) => void; |
🤖 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 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.
| export function assertPath(path: string): asserts path is Path { | ||
| if (!isPath(path)) { | ||
| throw new RouterError( | ||
| `"${path}" is not looks like a path. Is this start with slash(\`//\`?)`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix grammar in assertPath error message.
The user-facing message has multiple grammar issues: "is not looks like" and "Is this start with slash(//?)" read as unfinished/garbled English. The // example also doesn't actually illustrate a valid path-template (a single leading / is what the type/JSDoc require).
📝 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 (\`/\`)?`,
+ );📝 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.
| export function assertPath(path: string): asserts path is Path { | |
| if (!isPath(path)) { | |
| throw new RouterError( | |
| `"${path}" is not looks like a path. Is this start with slash(\`//\`?)`, | |
| ); | |
| } | |
| } | |
| export function assertPath(path: string): asserts path is Path { | |
| if (!isPath(path)) { | |
| 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 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.
…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]
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
| add = (pathOrPattern: Path | RouterPathPattern, name: string): void => { | ||
| const pattern = resolvePathPattern(pathOrPattern); | ||
| const entry = createRouteEntry({ index: this.#nextIndex++, name, pattern }); | ||
|
|
||
| this.#routesByName.set(name, entry); | ||
| this.#trie.insert(entry); | ||
| }; | ||
|
|
||
| /** | ||
| * Registers multiple path rules at once. Compared to calling {@link add} | ||
| * in a loop, this batches trie insertions into one sorted merge per | ||
| * affected node, which lowers the asymptotic cost of bulk registration. | ||
| * @param routes Iterable of `[pathOrPattern, name]` pairs to register. | ||
| */ | ||
| register = (routes: Iterable<RouterRoute>): void => { | ||
| const entries: RouteEntry[] = []; | ||
|
|
||
| for (const [pathOrPattern, name] of routes) { | ||
| const pattern = resolvePathPattern(pathOrPattern); | ||
| const entry = createRouteEntry({ | ||
| index: this.#nextIndex++, | ||
| name, | ||
| pattern, | ||
| }); | ||
|
|
||
| this.#routesByName.set(name, entry); | ||
| entries.push(entry); | ||
| } | ||
|
|
||
| this.#trie.insertAll(entries); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Re-registering the same route name leaks stale entries into the trie.
When add(...) (or register([...])) is called with a name that already exists, #routesByName.set(name, entry) overwrites the current entry, but the previous one stays in the trie indefinitely — only filtered out on each route() call by #isActiveEntry. In long-running routers that re-register routes (e.g., HMR or test setups), trie traversal cost grows monotonically.
Consider removing the previous RouteEntry from the trie before inserting the new one (e.g., a Trie#remove or a tombstone), or document that re-registration is expected to be append-only.
🤖 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 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.
| 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.
| expand: ( | ||
| context: ExpandContext, | ||
| options?: TemplateOptions, | ||
| ) => string = ( | ||
| context: ExpandContext, | ||
| options: TemplateOptions = this.#fullOptions, | ||
| ): string => expand(this.#tokens, context, options); | ||
|
|
||
| /** | ||
| * Matches a URI against this template, returning the variable context if the | ||
| * URI matches or `null` if it does not. | ||
| */ | ||
| match: ( | ||
| uri: string, | ||
| options?: TemplateOptions, | ||
| ) => ExpandContext | null = ( | ||
| uri: string, | ||
| options: TemplateOptions = this.#fullOptions, | ||
| ): ExpandContext | null => match(this.#tokens, uri, options); |
There was a problem hiding this comment.
Normalize per-call overrides before forwarding them.
The constructor goes through fillOptions(), but expand() and match() pass override objects straight to the lower layers. That means JS callers using the same partial shape as the constructor, e.g. { strict: false }, lose the default reporter and can fail with TypeError: report is not a function on the first recoverable error path.
Suggested fix
- expand: (
- context: ExpandContext,
- options?: TemplateOptions,
- ) => string = (
- context: ExpandContext,
- options: TemplateOptions = this.#fullOptions,
- ): string => expand(this.#tokens, context, options);
+ expand: (
+ context: ExpandContext,
+ options?: Partial<TemplateOptions>,
+ ) => string = (
+ context: ExpandContext,
+ options: Partial<TemplateOptions> = {},
+ ): string =>
+ expand(
+ this.#tokens,
+ context,
+ fillOptions({ ...this.options, ...options }),
+ );
@@
- match: (
- uri: string,
- options?: TemplateOptions,
- ) => ExpandContext | null = (
- uri: string,
- options: TemplateOptions = this.#fullOptions,
- ): ExpandContext | null => match(this.#tokens, uri, options);
+ match: (
+ uri: string,
+ options?: Partial<TemplateOptions>,
+ ) => ExpandContext | null = (
+ uri: string,
+ options: Partial<TemplateOptions> = {},
+ ): ExpandContext | null =>
+ match(
+ this.#tokens,
+ uri,
+ fillOptions({ ...this.options, ...options }),
+ );🤖 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.ts` around lines 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.
| const ERROR_NAMES: ReadonlySet<string> = new Set([ | ||
| ...Object.keys(ERROR_CLASSES), | ||
| ...Object.keys(ROUTER_ERROR_CLASSES), | ||
| ]); | ||
|
|
||
| function assertErrorName( | ||
| value: unknown, | ||
| label: string, | ||
| ): asserts value is string { | ||
| assertString(value, label); | ||
| if (!ERROR_NAMES.has(value)) { | ||
| throw new TypeError( | ||
| `${label} must be one of [${[...ERROR_NAMES].join(", ")}], got ${ | ||
| JSON.stringify(value) | ||
| }`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
ERROR_NAMES allowlist is built from all module exports, not only error classes.
Object.keys(ERROR_CLASSES) includes every value export from ../template/errors.ts (and similarly for ../router/errors.ts). If those modules ever export a helper or constant alongside the error classes, its name becomes an accepted "error name" in the JSON fixtures, weakening this validator silently.
A small hardening: filter to entries whose value is a Function extending Error, e.g.
const ERROR_NAMES: ReadonlySet<string> = new Set(
[...Object.entries(ERROR_CLASSES), ...Object.entries(ROUTER_ERROR_CLASSES)]
.filter(([, v]) => typeof v === "function" &&
(v === Error || Error.prototype.isPrototypeOf((v as Function).prototype)))
.map(([k]) => k),
);🤖 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/assert.ts` around lines 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.
| export const pairTestSuites: readonly PairTestSuite[] = validate( | ||
| assertPairTestSuite, | ||
| _pairTestSuites, | ||
| ); | ||
| export const fixedTestSuites: readonly FixedTemplateTestSuite[] = | ||
| _fixedTestSuites; | ||
| export const wrongTestSuites: readonly WrongTestSuite[] = validate( | ||
| assertWrongTestSuite, | ||
| _wrongTestSuites, | ||
| ); | ||
| export const hardTestSuites: readonly HardTestSuite[] = validate( | ||
| assertHardTestSuite, | ||
| _hardTestSuites, | ||
| ); | ||
| export const matchTestSuites: readonly MatchTestSuite[] = validate( | ||
| assertMatchTestSuite, | ||
| _matchTestSuites, | ||
| ); | ||
| export const routerRouteDefinitions: readonly RouterRouteDefinition[] = | ||
| validate( | ||
| assertRouterRouteDefinitions, | ||
| _routerRouteDefinitions, | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Inconsistency: fixedTestSuites is the only exported fixture not validated at module load.
Every other suite/case fixture passes through validate(...) so the cast to its TS type is runtime-safe; fixedTestSuites is asserted as readonly FixedTemplateTestSuite[] purely structurally. If the JSON drifts, downstream tests will fail in obscure ways instead of with a clear TypeError from assert.ts. Consider adding an assertFixedTemplateTestSuite helper for parity (or document why this fixture is exempt).
🤖 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/mod.ts` around lines 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.
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
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
| function matchTokens( | ||
| tokens: readonly Token[], | ||
| uri: string, | ||
| options: TemplateOptions, | ||
| tokenIndex: number, | ||
| uriIndex: number, | ||
| bindings: Bindings, | ||
| ): ExpandContext | null { | ||
| if (tokenIndex >= tokens.length) { | ||
| if (uriIndex !== uri.length) return null; | ||
| const context = toExpandContext(bindings); | ||
| return expand(tokens, context, options) === uri ? context : null; | ||
| } | ||
|
|
||
| const token = tokens[tokenIndex]; | ||
| if (token.kind === "literal") { | ||
| return uri.startsWith(token.text, uriIndex) | ||
| ? matchTokens( | ||
| tokens, | ||
| uri, | ||
| options, | ||
| tokenIndex + 1, | ||
| uriIndex + token.text.length, | ||
| bindings, | ||
| ) | ||
| : null; | ||
| } | ||
|
|
||
| for (const end of expressionEnds(tokens, uri, tokenIndex, uriIndex)) { | ||
| const expression = uri.slice(uriIndex, end); | ||
| for ( | ||
| const expressionBindings of matchExpression( | ||
| token.vars, | ||
| token.operator, | ||
| expression, | ||
| ) | ||
| ) { | ||
| const merged = mergeBindings(bindings, expressionBindings); | ||
| if (merged == null) continue; | ||
| const result = matchTokens( | ||
| tokens, | ||
| uri, | ||
| options, | ||
| tokenIndex + 1, | ||
| end, | ||
| merged, | ||
| ); | ||
| if (result != null) return result; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Backtracking + round-trip is sound; flagging adversarial-input cost as an operational concern.
The matcher relies on permissive enumeration (every expression end position, every scalar/list/associative reading per variable, every contiguous part slice in consumeUnnamed, plus the skip branch in matchUnnamedFrom/matchNamedFrom) and uses the strict expand(...) === uri check at lines 74–75 as the sole correctness filter. This is correct, but for templates with several unnamed variables and a URI that the anchor-literal pruning in expressionEnds cannot constrain (e.g. multi-variable expressions with no following non-empty literal), the candidate space grows multiplicatively and can become pathological on attacker-controlled URIs that flow into the router.
Worth confirming on the Fedify integration side that templates registered with Router#register are constrained enough (literal separators between expressions, bounded variables per expression) that the matcher cannot be driven into super-linear time on adversarial paths, and/or that callers apply a length cap to incoming URIs before matching. A bounded-work guard (max recursion depth or candidate count, with a typed error) inside matchTokens would also be a defense-in-depth option.
🤖 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 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.
1 participant
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
Per
AI_POLICY.md, parts of this work were AI-assisted. Tools used during implementation:gpt-5,GPT-5.5)claude-opus-4-7,claude-opus-4-7[1m])Per-commit attribution is recorded via
Assisted-bytrailers. All AI-assisted code was reviewed and tested by a human before submission.