Replace url-template and uri-template-router with @fedify/uri-template#758
Open
2chanhaeng wants to merge 30 commits intofedify-dev:mainfrom
Open
Replace url-template and uri-template-router with @fedify/uri-template#7582chanhaeng wants to merge 30 commits intofedify-dev:mainfrom
url-template and uri-template-router with @fedify/uri-template#7582chanhaeng wants to merge 30 commits intofedify-dev:mainfrom
Conversation
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
📝 WalkthroughWalkthroughAdds new 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🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
Author
|
To use Codex here, create a Codex account and connect to github. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces the @fedify/uri-template package, a dependency-free RFC 6570 URI Template implementation for expansion and round-trip route matching. It replaces Fedify's internal federation routing and deprecates the existing Router export in packages/fedify/. Feedback was provided to correct and simplify the ucschar range validation logic in packages/uri-template/src/template/encoding.ts to properly include the lower range of Plane 14 code points.
Comment on lines
+182
to
+184
| if (code > 0xefffd) return false; | ||
| const offset = code % 0x10000; | ||
| return offset <= 0xfffd && (code < 0xe0000 || offset >= 0x1000); |
Contributor
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.
Suggested change
| 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; |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces the @fedify/uri-template package, a dependency-free RFC 6570 implementation for URI template expansion and round-trip pattern matching, replacing the previous reliance on third-party libraries. The changes include the full implementation of the new package and the migration of Fedify's internal routing logic to use it. Feedback focuses on improving type flexibility in the Router by using ExpandContext for composite values, ensuring the matcher remains robust by disabling strict mode during internal round-trip checks, and hardening the isPath utility against malformed templates and unsafe property access.
| /** | ||
| * The values extracted from the URI. | ||
| */ | ||
| values: Record<string, string>; |
Contributor
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.
Suggested change
| 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 => |
Contributor
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.
Suggested change
| 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; |
Contributor
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.
Suggested change
| return expand(tokens, context, options) === uri ? context : null; | |
| return expand(tokens, context, { ...options, strict: false }) === uri ? context : null; |
Comment on lines
+21
to
+28
| 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; |
Contributor
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.
Suggested change
| 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
| "tsdown": "catalog:", | ||
| "typescript": "catalog:" | ||
| }, | ||
| "dependencies": {} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding "sideEffects": false for tree-shaking.
Since this is a zero-runtime-dependency utility library with no module-level side effects, declaring "sideEffects": false allows webpack, rollup, and other bundlers to dead-code-eliminate unused exports in consumer bundles.
✨ Proposed addition
"license": "MIT",
+ "sideEffects": false,
"devDependencies": {🤖 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/package.json` at 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.
Comment on lines
+271
to
+276
| const maxLength = parts.length - partIndex; | ||
| const minLength = Math.max( | ||
| 1, | ||
| parts.length - partIndex - remainingVars(vars, varIndex), | ||
| ); | ||
| for (let length = minLength; length <= maxLength; length++) { |
There was a problem hiding this comment.
consumeUnnamed() prunes valid matches here.
minLength is computed as if every remaining variable can consume at most one part. That skips valid decompositions such as {x:1,y} matching a,b,c (x = "a", y = ["b", "c"]). The matcher never even tries that branch.
Suggested fix
- const minLength = Math.max(
- 1,
- parts.length - partIndex - remainingVars(vars, varIndex),
- );
+ const minLength = 1;🤖 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 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.
Comment on lines
+1
to
+17
| 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.
Suggested change
| 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.
Comment on lines
+52
to
+57
| try { | ||
| tokens.push(parseExpression(expression, template, index, options)); | ||
| } catch (error) { | ||
| report(error instanceof Error ? error : new Error(String(error))); | ||
| appendLiteral(template.slice(index, closeIndex + 1)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if parseExpression invokes options.report (or the report param) internally.
fd -e ts expression.ts packages/uri-template/src/template/ --exec \
rg -n 'report\b' {}Repository: fedify-dev/fedify
Length of output: 117
🏁 Script executed:
# Get the full parseExpression function from expression.ts
fd -e ts expression.ts packages/uri-template/src/template/ --exec cat -n {}Repository: fedify-dev/fedify
Length of output: 6218
🏁 Script executed:
# Get more context around the try-catch block in token.ts
sed -n '40,70p' packages/uri-template/src/template/token.ts | cat -nRepository: fedify-dev/fedify
Length of output: 1216
🏁 Script executed:
# Find the report function definition (likely in a types/options file)
fd -e ts -e tsx packages/uri-template/ --exec rg -l 'TemplateOptions|interface.*report' {} \;Repository: fedify-dev/fedify
Length of output: 455
🏁 Script executed:
# Find TemplateOptions type definition
fd -e ts -e tsx --full-path 'packages/uri-template/' | xargs rg -l 'TemplateOptions|interface.*report'Repository: fedify-dev/fedify
Length of output: 358
🏁 Script executed:
# Get the TemplateOptions type and report function definition
fd -e ts -e tsx --full-path 'packages/uri-template/' | xargs rg -A 10 'type TemplateOptions|interface TemplateOptions'Repository: fedify-dev/fedify
Length of output: 953
🏁 Script executed:
# Read the full types.ts file to understand TemplateOptions and report function
cat -n packages/uri-template/src/types.tsRepository: fedify-dev/fedify
Length of output: 3401
🏁 Script executed:
# Also check template.ts to see how the report function is created/wrapped
cat -n packages/uri-template/src/template/template.ts | head -100Repository: fedify-dev/fedify
Length of output: 3499
🏁 Script executed:
# Let me trace through what happens in strict mode
# Check if reportExpressionError can actually throw when report is wrapped
cat -n packages/uri-template/src/template/token.ts | sed -n '30,70p'Repository: fedify-dev/fedify
Length of output: 1596
Double invocation of report callback in strict mode when parseExpression reports-then-throws.
In strict mode, report is wrapped by strictWrapper (template.ts line 96) which calls the user's reporter callback and then throws. When parseExpression encounters certain errors (empty expression, reserved operator, unknown operator), it calls reportExpressionError, which invokes report(error). In strict mode, this invocation triggers the user's reporter and throws. The exception then propagates to the catch block in token.ts, which calls report a second time—invoking the user's reporter callback again for the same error.
This double invocation is specific to errors handled by reportExpressionError. Errors thrown by parseVarList bypass reportExpressionError and are reported only once by the catch block.
🐛 Proposed fix
Since parseExpression already reports errors via reportExpressionError in strict mode, the catch block should avoid re-reporting these errors. One approach is to only report errors from parseVarList in the catch block and handle reportExpressionError paths differently, or refactor to move all error reporting to the tokenizer level to prevent double invocation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/uri-template/src/template/token.ts` around lines 52 - 57,
parseExpression/reportExpressionError currently calls report in strict mode and
then throws, causing token.ts's catch block to call report again; fix this by
introducing a sentinel error type (e.g., ReportedExpressionError) that
reportExpressionError throws after invoking report, and update the catch in
token.ts (around tokens.push(parseExpression(...))) to detect this sentinel and
skip calling report a second time while still appending the literal; keep other
thrown errors (from parseVarList or others) reported in the catch as before.
Comment on lines
+340
to
+349
| 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.
Comment on lines
+21
to
+29
| 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.
isPath throws on invalid templates instead of returning false.
new Template(path) runs the tokenizer in strict mode (the default), which will throw for any invalid template (unclosed braces, bad literals, etc.). The JSDoc contract — "Templates that fail to parse … return false" — is violated; callers following the docs will get an unhandled exception instead.
Wrap the body in a try/catch (or construct Template with strict: false):
🐛 Proposed fix
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;
+ try {
+ const template = new Template(path);
+ const [first] = template.tokens;
+ if (first == null) return false;
+ if (isLiteral(first)) return first.text.startsWith("/");
+ if (first.operator === "/") return true;
+ return false;
+ } catch {
+ return false;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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); | |
| const [first] = template.tokens; | |
| if (first == null) return false; | |
| if (isLiteral(first)) return first.text.startsWith("/"); | |
| if (first.operator === "/") return true; | |
| return false; | |
| } catch { | |
| return false; | |
| } | |
| } |
🤖 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 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.
| const [first] = template.tokens; | ||
| if (first == null) return false; | ||
| if (isLiteral(first)) return first.text.startsWith("/"); | ||
| if (first.operator === "/") return true; |
There was a problem hiding this comment.
isPath type guard is over-permissive for bare path-expansion expressions.
isPath("{/var}") returns true (line 27 branch), but "{/var}" satisfies neither variant of Path:
/${string}— no, doesn't start with/`{/${string}}/${string}`— no trailing/${string}after the closing}
The JSDoc is also explicit: "a path-expansion expression ({/var}) followed by a literal segment that starts with /" — the implementation never checks the second token.
Either fix the predicate to check the next token, or update the Path type and JSDoc to explicitly permit bare {/var} templates (if the router can handle them):
🐛 Option A — enforce the documented constraint
- if (first.operator === "/") return true;
+ if (first.operator === "/") {
+ const second = template.tokens[1];
+ return second != null && isLiteral(second) && second.text.startsWith("/");
+ }♻️ Option B — relax `Path` to match actual behaviour
-export type Path = `/${string}` | `{/${string}}/${string}`;
+export type Path = `/${string}` | `{/${string}}${string}`;(Update JSDoc to match.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (first.operator === "/") return true; | |
| if (first.operator === "/") { | |
| const second = template.tokens[1]; | |
| return second != null && isLiteral(second) && second.text.startsWith("/"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/uri-template/src/utils.ts` at line 27, The isPath type guard is
returning true for bare path-expansion tokens like "{/var}"; change its logic so
that when checking first.operator === "/" you also verify the next token exists
and is a literal segment that starts with "/" (e.g., tokens[1].type ===
"literal" && tokens[1].value.startsWith("/")) before returning true, and update
the JSDoc if you prefer to relax the Path definition instead; target the isPath
function and the tokens array handling around the first/operator check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #418.
Background
Fedify previously combined two third-party libraries for URI Template handling:
The two libraries do not share a parser or an encoding model, so a value expanded by one could not be decoded back by the other. This asymmetry has been the root cause of recurring encoding/decoding bugs (#416 and related), because RFC 6570 only specifies expansion, while
uri-template-routeradds matching as a non-standard extension with its own rules.What this PR does
Adds
@fedify/uri-template(packages/uri-template/)A new workspace package that implements RFC 6570 expansion and round-trip pattern matching from a single strict parser. Highlights:
{var},{+var},{#var},{.var},{/var},{;var},{?var},{&var}).match(uri)accepts a candidate only when the recovered values re-expand to the exact input URI. This eliminates the asymmetric cases where a URI matched but the values could not reproduce it.errors.ts);strict/reportoptions let callers collect every diagnostic in one pass instead of failing on the first issue.Routerclass with a prefix trie keyed by initial literal prefix, deterministic candidate ordering, batch registration viaRouter#register/ constructor /Router.from, andRouter.compile/Router.variablesfor variable extraction without mutating a router.old/that run Fedify's expected behavior againsturl-templateanduri-template-router, recording the gaps the new implementation closes (double-encodedpct-encodedvariable names, prefix modifier on composite values, leading path expansion templates rejected by the old router, optional form-style query templates missing on partial matches, reserved-expansion decoding mismatch, etc.). Run withdeno task test:old.Migrates
@fedify/fedifyto the new packageRouterandRouterErrorinFederationBuilder, middleware, NodeInfo handler, and testing context now come from@fedify/uri-template.packages/fedify/src/federation/router.tsis kept as a thin compatibility shim with@deprecatedJSDoc directing callers to@fedify/uri-template. No public re-exports are removed.Routersplits route registration from variable extraction:router.add(template, name)returnsvoid, while the staticRouter.variables(template)returns the variable names. All call sites are updated.validateSingleIdentifierVariablePathis rewritten on top ofRouter.compileandisExpressionso it inspects the parsedVariableSpecdirectly. This lets it reject explode ({var*}) and prefix ({var:N}) modifiers in addition to the previously-rejected query/fragment operators. Regression tests are added inbuilder.test.ts.Notable behavior changes for downstream users
These follow from the stricter parser and the round-trip matching contract; they are not opt-in:
{?abc%20def}now expands to?abc%20def=…rather than?abc%2520def=…./files/a%2Fbcould be reported asa/b; this no longer happens because such a binding does not round-trip.Router.compile()at registration time. Ordinary slash-prefixed paths and the leading path expansion form used by Fedify routes ({/identifier}/inbox) are both supported.url-template(unmatched braces, raw%outside a pct-encoded triplet, forbidden literal characters, etc.) now raise typed errors. Applications that need a looser parser can opt in viastrict: falseand a customreportfunction.Other
@fedify/uri-templateis listed in the packages table inpackages/fedify/README.mdand added topackages/fedify/package.jsonas a workspace dependency.CHANGES.mdnotes both the new package and the internal migration under issue Implement symmetric RFC 6570 expansion and pattern matching #418.AI disclosure
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.