diff --git a/agent-feedback/bugs.md b/agent-feedback/bugs.md index 84cb4bb7..1a1dde05 100644 --- a/agent-feedback/bugs.md +++ b/agent-feedback/bugs.md @@ -139,3 +139,153 @@ The entry cell prints "Handler" whenever `route.handler` exists, without checkin `packages/run/src/vite/routes/builder.ts` › `RoutableFileRegex` | 2026-08-14 | impact:med | effort:low `nonMarkoFiles` (and `markoFiles`) allow an arbitrary middle segment — `(+middleware|+handler|+meta)\.(?:.*\.)?(.+)` — and nothing ever reads that group (`matchRoutableFile` and `onFile` only use the type and the trailing extension). A colocated `+handler.test.ts` next to `+handler.ts` therefore builds a second `RoutableFile` of type `handler` for the same directory, and `VDir.addFile` takes both. The same holds for `+page.test.marko`. Either drop the optional middle segment, or ignore known test/spec segments there. Re-verify: add `+handler.test.ts` beside `+handler.ts` in a route directory and inspect `buildRoutes` output. + +## Make `Run.href`'s `search` keys optional; the documented single-key call does not compile + +`packages/run/src/runtime/types.ts` › `HrefBaseOptions` | 2026-08-20 | impact:high | effort:low + +`HrefBaseOptions.search` is the homomorphic mapped type `[K in keyof Valid>]: string | number | null | undefined`, which preserves the validator's required modifiers, so every key the route's `search` validator returns becomes mandatory at every call site. On a route whose validator returns `{q, from, to, sort, dir, page}`, the exact shape the README's "Typed URLs" section and `website/docs/marko-run/runtime.md` show — `Run.href("/tx", { search: { q: "figma" } })` — fails `mtc` with `TS2739 Type '{ q: string; }' is missing the following properties … from, to, sort, dir, page`. The modifier buys nothing, because `undefined` is already an accepted value and `href` omits those entries from the output, so the only effect is that every link must spell out five `: undefined` keys or funnel through a hand-written `partial → full` helper, which also forfeits the build-time `Run.href` folding the client build does on direct calls. Making the mapped type optional (`]?:`) accepts the documented call, still rejects an unknown key with `TS2353`, and leaves an app that already spells every key out type-checking unchanged. + +## Type `Run.href`'s `search` as a string record when the route declares no `search` validator; today it is `{}` + +`packages/run/src/runtime/types.ts` › `HrefBaseOptions` | 2026-08-20 | impact:low | effort:low + +`HrefBaseOptions.search` maps over `keyof Valid>`, and `GetRawSearchValidator` resolves to `never` for any route without a `search` option, so the whole option collapses: `HrefOptions<"/">["search"]` probes as exactly `{}`. Everything then type-checks — `Run.href("/", { search: 5 })`, `Run.href("/", { search: { bogusKey: "x" } })` and `Run.href("/", { search: { bogusKey: { nested: true } } })` all pass `mtc` clean, and the last serializes `?bogusKey=%5Bobject%20Object%5D` — while the path argument on the same line is strictly checked against the route union and the same misspelled key _is_ caught (TS2353) once any validator is declared. Since most routes never declare one, "type checking of the path, params, search, and hash" (`packages/run/README.md`, Typed URLs) holds for a minority of call sites, and the failure is silent in the direction that matters: a typo'd or wrongly-typed query key ships a link nothing rejects. Fall back to `Record` instead of the empty mapped type when the route has no validator — that keeps arbitrary keys legal (a route may still read `$global.url.searchParams`, and analytics params are legitimate) while rejecting the non-serializable values, and it mirrors how `params` falls back to `PathParams` rather than to nothing. The same mapped type is wrong in the other direction too, making every key a function validator returns mandatory at every call site; both are one edit to `HrefBaseOptions` and both want a case in `packages/run/src/__tests__/typecheck.test.ts`, which has no `href` coverage today. + +## Accept an `interface` in `next(data)` instead of requiring an index signature + +`packages/run/src/runtime/types.ts` › `NextFunction` | 2026-08-20 | impact:med | effort:low + +`NextFunction`'s data overload is `>(data: Data)`, and an `interface` has no implicit index signature, so a payload declared the way most style guides recommend is rejected: declaring `interface PageData { … }` instead of `type PageData = { … }` in a `+handler.ts` turns `next(page({…}))` into `TS2345 Argument of type 'PageData' is not assignable to parameter of type 'Record'. Index signature for type 'string' is missing in type 'PageData'`. The damage is mostly downstream: `Route["data"]` and `RouteForFileDef["data"]` extract with `infer T extends Record`, which the interface also fails, so `data` falls back to `Record` and the template that reads it fills with `TS18046 'employees' is of type 'unknown'` and `TS2339 Property 'id' does not exist on type 'never'`. On one 4-route app that was 2 errors at the `next()` calls and 12 more in `+page.marko` blaming the markup; flipping the one `interface` back to `type` cleared all 14. Widening the four `Record` constraints in `NextFunction`, `Route["data"]`, `RouteForFileDef["data"]` and `HandlerTypes["data"]` to `object` type-checks that app clean with the `interface` spelling and leaves the `type` spelling resolving identically, so the fallback `Record` only has to stay in the _false_ branches. + +## Count a route's CSS in the build table's SIZE/GZIP column, or say the column measures JS only + +`packages/run/src/vite/utils/log.ts` › `computeChunkSize` | 2026-08-20 | impact:low | effort:low + +`computeRouteSize` looks up the client entry chunk whose `facadeModuleId` is `.html` and `computeChunkSize` sums `chunk.code` plus, recursively, `chunk.imports` — JS only. `chunk.viteMetadata.importedCss` is never consulted, so the number understates any route that ships styles and is simply wrong for a route that ships nothing else: in a two-route app whose `/tx` page emits `tx-*.js` (13,674 B), its own `tx-*.css` (2,939 B) and a shared `_layout-*.css` (539 B), the table prints `13.7/6.3 kB` — exactly the JS chunk — while a page with no client component that still links 4,253 B + 470 B of stylesheet prints `0.0 kB`, because with no JS there is no entry chunk to find and `computeRouteSize` returns `undefined`. Read as printed, `0.0 kB` says "this route ships nothing", and the table is the main at-a-glance view of a route's cost. Add the entry's `viteMetadata.importedCss` (and `importedAssets`) into `computeChunkSize`, recursing through imported chunks the same way it already does for JS, and decide what a CSS-only route should report given it has no chunk to hang the bytes on — or rename the column so it reads as the route's JS entry size. Worth pinning in the same pass: the ~900 B of inline resume runtime every streamed page carries comes from marko's HTML output rather than the bundle, so it is out of this column's reach and should not be conflated with the CSS gap. + +## Answer a wrong-verb request to an existing page path with 405 and an `Allow` header, not the 404 page + +`packages/run/src/vite/codegen/index.ts` › `renderRouter` | 2026-08-20 | impact:med | effort:med + +Against a production build of a route with a `+page.marko` and no `+handler`, `POST /venues/new` answers `404` and renders `+404.marko`; `PUT`, `DELETE` and `OPTIONS` do the same, while `HEAD` correctly 200s off the GET matcher. `renderRouter` emits one `case '':` block per supported verb inside `match_internal` and returns `null` for every other method, so `invoke` falls through to the same generic 404 it uses for a path that does not exist — even though the path is in the build's own route table and only the verb is unsupported. Posting a form to a page that has no POST handler is a routine mistake, and a 404 sends the author hunting for a routing bug that is not there; `405`, `Method Not Allowed` and `Allow` have no occurrences anywhere under `packages/run`. The route trie already knows the full verb set for each path, so `match_internal` can distinguish "no such path" from "path exists, wrong verb" and `invoke` can answer `405` with an `Allow` header derived from the same table. + +## Make a catch-all route match its own parent path, or stop `Run.href` from emitting that path + +`packages/run/src/vite/codegen/index.ts` › `writeRouterVerb` | 2026-08-20 | impact:med | effort:low + +With `src/routes/archive/$$rest/+page.marko` as the only route under `/archive`, `GET /archive/a/b` answers 200 but `GET /archive` and `GET /archive/` both 404 — and `/archive/` does not even get the trailing-slash redirect every matched route gets. The generated matcher makes it structurally unreachable: the catch-all is emitted as `else if (pathname.slice(1, i1 - 1) === "archive") return …`, on the branch taken only when `i1` (the index after the next `/`) is neither 0 nor the end of the string, so a zero-segment remainder never reaches it. The framework contradicts itself here: `href("/archive/$$rest", { params: { rest: [] } })` returns `"/archive/"`, a URL its own router answers 404, while `packages/run/README.md` says a catch-all matches "to the end of the path" and is how you build "404 Not Found routes at any level, including the root". Either match with an empty remainder (`rest === ""`) or reject an empty catch-all param in `href` and say so in the README; add match fixtures covering `/archive`, `/archive/` and `/archive/a/b` beside the existing `dynamic-rest` fixture, which only exercises a populated remainder. + +## Default the client build's `sourcemap` off (or to `hidden`); today every deploy publishes original template source + +`packages/run/src/vite/plugin.ts` › `config` | 2026-08-20 | impact:med | effort:low + +The `config` hook sets `pluginConfig.build.sourcemap ??= config.build?.sourcemap ?? (isBuild && !isSSRBuild)`, so unless an app names `build.sourcemap` itself the client pass is built with full external source maps — Vite's own default is `false`. A stock `marko-run build` of an app with one interactive page writes `dist/public/assets/heavy-DOUhrDvS.js` at 2,577 bytes next to `heavy-DOUhrDvS.js.map` at 73,457 bytes, whose `sources` are `../../../src/routes/heavy/+page.marko` and `marko/dist/dom-*.mjs` and whose `sourcesContent[1]` is the verbatim template text; the bundle ends in `//# sourceMappingURL=heavy-DOUhrDvS.js.map`, so browsers and crawlers fetch it. That is a 28x asset-size multiplier and the app's unminified original source on the public origin, from a build the author never configured, and no `@marko/run` doc mentions source maps at all. Default the client build to `false` (or `"hidden"`, keeping the map for upload without the `sourceMappingURL` comment) and let apps opt in, since the SSR pass — where a readable stack is actually worth something — is already excluded by the `!isSSRBuild` half of the same expression. + +## Stop logging `200` for a dev request that never produced a response + +`packages/run/src/adapter/logger.ts` › `logResponse` | 2026-08-20 | impact:low | effort:low + +`done("close")` reads `res.statusCode` and prints it, but when the socket closes before anything is written that value is still Node's unset default, so a request that produced no response at all is logged as a success status. A handler returning a never-settling promise, hit with `curl --max-time 4` (client sees `000`), logs `━━x GET /hang 200 4002ms -`; the same shape appears for every request against a wedged dev server — `━━x GET /r5 200 20.0s -` for URLs `curl` reported as `000`. The failure arrow `x` is the only signal that anything went wrong, and it is easy to read past next to a green-looking `200` and a `-` byte count, which is why a hung dev server reads as a working one in `dev.log`. Gate the status column on `res.headersSent` (or on the `close`-without-`finish` path `done` already distinguishes) and print a distinct terminal state instead. Same file as the filed cleanup entry on the leaking in-flight `⁺` slot, so both can be covered by one test over the logger middleware. + +## Recover the dev server after a bulk route-file rewrite instead of wedging every later request forever + +`packages/run/src/vite/plugin.ts` › `configureServer` | 2026-08-20 | impact:high | effort:med + +Rewriting the contents of every file under `src/routes` in one fast pass — what `prettier --write` with real changes, a codemod, a `git checkout` or a format-all-on-save does — permanently wedges `marko-run dev`. On the `app` starter grown to 18 route directories (50 files under `src/routes`), `GET /r5` answers 200 in 73 ms; after a single pass appending a newline to every `.marko`/`.ts` under `src/routes`, the next requests return `000` at a 20 s client timeout twice and then 500, and the dev log carries `transport invoke timed out after 60000ms (data: {"type":"custom","event":"vite:invoke","data":{"name":"fetchModule",…,"data":["@marko/run/router",null,{"cached":false,"startOffset":3}]}})` thrown from vite's `reviveInvokeError`. It never recovers: another 35 s of waiting, a single-file `touch` and a further request all return `000`, while the process sits at 2.6% CPU — a promise that never settles, not a spin — so only SIGTERM and a restart get the app back. It reproduced 2 of 2 times with one fast pass and 0 of 1 with a slower per-file pass (one `sh -c` per file), so it is a race in the burst: the watcher's `all` listener calls `invalidateVirtualFiles()` (clearing `renderVirtualFilesResult` mid-flight) while `writeEntryTemplate` re-emits `watcher.emit("change", …)` from inside the pass it just invalidated, and the `load` of `@marko/run/router` that every request awaits via `renderVirtualFiles` never resolves. The existing dx entry on add/remove churn covers reload volume, not a permanent wedge; add a test that rewrites every route file under a live dev server and asserts the following request answers. + +## Turn a multipart limit breach into `issues` instead of a bodiless 413 that also discards the fields parsed before it + +`packages/run/src/runtime/internal.ts` › `readBody` | 2026-08-20 | impact:med | effort:med + +`readBody` catches the parser's limit errors and rethrows them stamped with a status (`clientError("Request body too large", 413, error)` keeps the original error object), so `await ctx.body` rejects and an unhandled rejection becomes `HTTP/1.1 413 Payload Too Large` with `content-length: 0` — a literally blank page for a no-JS form post. Handled, it is barely better: `Run.POST({ form: { maxFiles: 2, maxFileBytes: 1000, validator: (v) => v } }, ...)` posted `-F caption=hello -F receipts=@4k.txt` yields `MaxFileSizeExceededError` with message `File size exceeds maximum allowed size of 1000 bytes` and `Object.keys(err)` of `["name"]`, so per-file messages are impossible, and the `caption` text field that parsed before the offending part is thrown away with the body. Neither error class is reachable from `@marko/run` — they come from the transitive `@remix-run/form-data-parser` and the package `exports` map has no error surface — so `err.constructor.name` is the only discriminator an app has. The README frames `form` options as validation that re-renders the page with `issues` rather than a bare 400; a limit breach is exactly the case that cannot do that today, so surface it as an `issues` entry naming the field and file with the already-parsed fields in the value half of the tuple, and at minimum re-export the four error classes. The "Build the `context.body` thenable whenever a `json`/`form` option is configured" entry defers its 400 handling to a "Return 400/413 instead of 500" entry that is no longer in this backlog; this is where that work now lives. + +## Fail when more than one `@marko/run-adapter-*` is installed instead of silently picking the first + +`packages/run/src/vite/plugin.ts` › `resolveAdapter` | 2026-08-20 | impact:high | effort:med + +`resolveAdapter` concatenates `dependencies` and `devDependencies` and returns the first name matching `@marko/run-adapter*` or `marko-run-adapter`, announcing its choice only through `debug()`. With `@marko/run-adapter-netlify`, `-node` and `-static` all installed and no explicit `adapter` option, `marko-run build` picks netlify on package.json key order and writes a Netlify Functions module (`export { config, fetch as default }`) to `dist/index.mjs`; the build prints its usual route table, exits 0, names no adapter, and `node dist/index.mjs` then exits 0 immediately with no output and no listener — a successful-looking build and start that serves nothing. Having two or three adapters installed is normal while evaluating deploy targets, and nobody expects dependency ordering to select one. Throw when auto-discovery finds more than one adapter package and no `adapter` option was given, naming the packages it found and the `marko({ adapter })` option, and print the resolved adapter in the build output instead of only behind `DEBUG`. + +## Skip the empty placeholder part a browser sends for an untouched file input before calling `onFile` + +`packages/run/src/runtime/internal.ts` › `readBody` | 2026-08-20 | impact:low | effort:low + +Every browser submits an untouched `` as a part with `filename=""` and a zero-byte body, and `readBody` forwards `onFile` straight into `parseFormData`, so the hook fires for it. Feeding `@remix-run/form-data-parser` the exact bytes a browser sends produces `onFile("receipts") name="" size=0` and no form entry, so every app has to hand-write `if (!file.name && file.size === 0) return;` or report "unnamed: empty file" at a user who did nothing wrong. Wrap the callback passed at the `parseFormData` call site so a part with an empty name and no bytes never reaches the app's `onFile`. Note that the parser increments its own `fileCount` against `maxFiles` before invoking the handler, so a form with several optional file inputs spends that budget on placeholders either way; if that matters the limit has to be enforced on run's side of the callback. + +## Narrow `form.onFile`'s return type; an object returned from it lands in the parsed field as `"[object Object]"` + +`packages/run/src/runtime/types.ts` › `FormBodyValidatorOptions` | 2026-08-20 | impact:med | effort:low + +`onFile?(ctx: Ctx, file: Multipart): any` widens a return type that `@remix-run/form-data-parser` declares precisely — its `FileUploadHandler` returns `void | null | string | Blob | Promise` — and `parseFormData` feeds whatever comes back to `formData.append(fieldName, value)`, which USVString-coerces anything that is not a `Blob`. So the natural thing to return after saving an upload, the metadata record `{ id, path }`, silently becomes the string `"[object Object]"`: `Run.POST({ form: { onFile: () => ({ id: 1, path: "/tmp/x" }), validator: (v) => v } }, ...)` posted with two files answers `{"receipts":{"type":"object","value":"[object Object],[object Object]"}}`. Nothing catches it — the option is typed `any`, so `mtc` is silent, and the app has to smuggle metadata through a JSON string or a side channel keyed on `ctx`. Reuse the parser's own return type on the `onFile` signature so the mistake is a compile error, and say in the README's `form` bullet that the returned value is what is stored in the field. + +## Stop letting `new URL` collapse percent-encoded dot segments: `/a/%2e%2e/b` silently serves route `/b` + +`packages/run/src/adapter/middleware.ts` › `createMiddleware` | 2026-08-20 | impact:med | effort:low + +`%2e` is an escaped literal `.`, so `/service/%2e%2e/raw` names a segment called `..`, but `createMiddleware` passes the raw target to `new URL(req.url!, origin)` and the WHATWG parser treats `%2e%2e` as a double-dot path segment and resolves it away. On a default node build the request returns `200` with the `/raw` page and `$global.url` serialized as `new URL("http://localhost:PORT/raw")` — no redirect, no 404, and the browser keeps the disguised URL. This is the classic proxy-vs-origin normalization mismatch: a gateway that authorizes `/service/*` by prefix passes `/service/%2e%2e/admin` through to a route it was meant to protect, and any `+middleware.ts` on the abandoned prefix never runs because matching happens on the collapsed path. Decide it explicitly at the same place the `//` case is handled: either reject a target containing a `%2e`-spelled dot segment, or canonicalize with a 308 before matching so the served path and the requested path agree; a parse/match fixture over `%2e%2e`, `..` and `.` segments would pin whichever is chosen. + +## Reject a request target that starts with `//`; the WHATWG parser reads it as protocol-relative and hands the attacker `ctx.url.origin` + +`packages/run/src/adapter/middleware.ts` › `createMiddleware` | 2026-08-20 | impact:high | effort:low + +`createMiddleware` builds the request URL as `new URL(req.url!, origin || getOrigin(req, trustProxy))`, and a request target beginning with `//` is protocol-relative, so the base is discarded entirely: `new URL("//evil.example.com/raw/", "http://localhost:3000")` is `http://evil.example.com/raw/`. Against a default `marko-run build` served by `@marko/run-adapter-node`, `curl --path-as-is 'http://localhost:PORT//evil.example.com/raw/'` answers `302 Found` with `location: http://evil.example.com/raw` — the framework's own `RedirectWithout` trailing-slash canonicalization (`packages/run/src/vite/codegen/index.ts` › `renderTrailingSlashPolicy`) turned into an open redirect on every app that has not opted out. Without the trailing slash it is worse: the same path returns `200` with the app's page and the serialized `$global.url` reads `new URL("http://evil.example.com/raw")`, so every `ctx.url`-derived absolute link, canonical tag and sitemap entry in the response is rooted at the attacker's host, and `ctx.redirect("/x")` resolves against it too. Normalize before constructing the URL — treat a target whose first two characters are `/` `/` as a path, not an authority (`req.url.replace(/^\/+/, "/")` or an explicit 400) — and pin it with a fixture asserting `ctx.url.origin` equals the `Host`-derived origin for a `//`-prefixed target. + +## Settle out-of-order streaming boundaries before writing a prerendered page, or report them + +`packages/adapters/static/src/crawler.ts` › `createCrawler` | 2026-08-20 | impact:high | effort:med + +`visit` pipes the SSR response body straight into the page's write stream, so a page whose content sits behind a ``/`<@placeholder>` boundary is frozen mid-stream on disk. A route rendering `

REAL CONTENT

<@placeholder>
LOADING SKELETON
` builds to `dist/public/stream.html` with the skeleton in document flow inside `
` and the real content parked after `
` as `` — while the summary reads `Crawled 4, success 4, failed 0, redirect 0, not found 0`. A prerendered file is exactly the artifact that no-JS clients, crawlers and CDNs consume, and nothing is ever going to stream into it, so for them the page shows a loading skeleton permanently; the `success` count says the opposite. Either settle the response before writing (the crawler already has the whole body in hand and there is no client waiting), or scan the emitted HTML for unresolved `