Machine-facing paths: 405 for a wrong verb, 404 only for a missing route (+ PWA denylist parity and its first test) - #2079
Merged
Merged
Conversation
The per-prefix machine-path fallbacks added for #1971 are GET/HEAD-scoped so that routing still reaches its own 405 endpoint for other verbs. Inside that pair the scoping does the opposite: a GET or HEAD on a POST-only route is not method-mismatched against the fallback, so it landed there and answered 404 where the framework would have said 405. Routing cannot be asked which verbs a path allows from inside a fallback -- HttpMethodMatcherPolicy partitions the candidate set by verb in the DFA, so a GET never sees the POST endpoint. MachineRouteMethodResolver recovers the answer from the endpoint graph instead: every non-fallback endpoint under a machine prefix is translated once into a template matcher plus its resolved inline constraints, and the request path is matched against that set. Constraints are evaluated, not skipped, so an unsatisfied {id:guid} keeps its 404 rather than being upgraded to a 405 for a path that does not exist. Two further inconsistencies come from the same shared routing node and are corrected with it: - A verb outside GET/HEAD on an UNKNOWN machine path answered a bodyless 405 with Allow: GET, HEAD -- advertising verbs for a path nothing is routed to. #1971 recorded that as an accepted trade; it now answers the same 404 error contract every other unknown machine path gets. - Routing builds Allow from the union of every method at the node, so the catch-all's GET/HEAD were advertised on routes that have neither. The header now lists the methods the route itself declares. The comment block claiming a wrong-verb request on a real route mismatches every candidate and keeps its 405 was true only outside GET/HEAD; it is rewritten to describe what the pipeline actually guarantees. Refs #1992
The denylist gained /hubs and /health in #2029 but kept two boundaries that disagree with the reverse proxy, which routes exactly the same four prefixes with ^/<prefix>(?:/|$): - /^\/api\// required a trailing slash, so an installed PWA answered a navigation to the bare /api (or /api?probe=1) from its own precache with index.html while nginx sends that path to the API container. - /^\/mcp/ had no boundary at all, so /mcpx and any other SPA route merely starting with those letters was denied the shell. All four now use the same ^/<prefix>(?:[/?]|$) shape -- `[/?]` rather than `/` because workbox tests the denylist against pathname + search, so a query string on a bare prefix has to count as the boundary too. Adds the first test over this config. It reads the regex literals out of vite.config.ts and executes them against a table of machine paths and client-side routes, so a pattern that is spelled plausibly but behaves wrong fails. Checked against the previous denylist: three cases fail there (/api, /api?probe=1, /mcpx) and pass here. The regexes are read via a `?raw` import rather than node:fs because tsconfig.vitest.json type-checks new specs without node types, and adding this file to that project's quarantine list is explicitly disallowed. Refs #1992
#1992 reserves the wrong-verb disposition for the maintainer and says it must not be inferred from the implementation or its tests. The code in this branch implements one of the two named options; this ADR is the artefact that lets the choice actually be made, so it lands as Proposed, not Accepted, exactly as ADR-0057 does. It records the measured before-state for all three inconsistencies, why routing cannot answer the question from inside a fallback (HttpMethodMatcherPolicy is a node-builder policy, so the candidate set is already partitioned by verb), and the three alternatives considered -- accept the 404, drop the method scoping, or register a 405 shim per real route -- with the concrete reason each was not taken. Refs #1992
![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: 5f2f7307d2
ℹ️ 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".
Review HIGH, reproduced at the previous head: HEAD /api/boards answered 405
with "Allow: GET, HEAD, POST" -- advertising the very method the same response
had just rejected. A client that honours Allow retries with HEAD and loops on
the same 405. RFC 9110 requires Allow to name the methods the resource
supports, so this was the defect class the change claims to fix, reintroduced
in the header.
The premise behind it was wrong. GetDeclaredMethods added HEAD whenever a
matched route declared GET, on the belief that routing serves HEAD from a GET
endpoint. It does not here, and that was never measured before being written
down. Measured now, on this app:
GET /api/boards (auth) -> 200 the GET action exists and serves
HEAD /api/boards (auth) -> 405 Allow GET,POST routing does not serve HEAD from it
HEAD /api/boards (anon) -> 405 the AllowAnonymous machine fallback
matched HEAD -- that is where it lands
PUT /api/boards (anon) -> 401 control: a verb the fallback does not
accept reaches routing's 405 endpoint,
which has no AllowAnonymous, so the
global FallbackPolicy answers first
The control matters: it makes the two paths distinguishable, so "HEAD falls
through to the fallback" is measured rather than assumed. Taskdeck declares no
[HttpHead] anywhere.
The inference is dropped -- Allow now reports exactly what the route declares.
HEAD on a GET-declaring route answers 405 with "GET, POST", which is
self-consistent. Real HEAD serving is deliberately NOT added: that is new API
surface, not a correction to the 404/405 contract.
The false mechanism was recorded in three places; all three are corrected: the
resolver comment, ADR-0059's Decision section (which had it as measured fact --
now a measurement table, with the one clause I could not isolate in this app
removed rather than restated), and the pinned Allow assertion on
WrongVerbOnExistingApiRoute_StillReturns405, which had "GET, HEAD, POST" baked
in. Adds HeadOnGetDeclaringApiRoute_Returns405WithoutAdvertisingHead as the
explicit regression: it failed on the previous head with exactly this Allow.
Refs #1992
Owner
Author
Review round 1 — one HIGH confirmed and fixed (
|
| Request | Result | What it establishes |
|---|---|---|
GET /api/boards (auth) |
200 |
The GET action exists and serves — the 405 below is not an artefact of a broken route. |
HEAD /api/boards (auth) |
405, Allow: GET, POST |
Routing does not serve HEAD from that GET action. |
HEAD /api/boards (anon) |
405 |
The AllowAnonymous machine fallback matched HEAD — that is where it lands. |
PUT /api/boards (anon) |
401 |
Control. A verb the fallback does not accept reaches routing's 405 endpoint, which carries no AllowAnonymous, so the global FallbackPolicy answers first. |
The control is the point: it makes the two code paths distinguishable by observation, so "HEAD falls through to the GET/HEAD machine fallback" is now measured rather than asserted. Taskdeck declares no [HttpHead] anywhere.
The fix
Dropped the HEAD inference entirely. Allow now reports exactly what the route declares, so HEAD on a GET-declaring route answers 405 with Allow: GET, POST — self-consistent. Real HEAD serving was deliberately not added: that is new API surface, outside this PR's charter.
The false mechanism was durably recorded in three places, all three corrected:
MachineRouteMethodResolver— the comment asserting the implicit-HEAD premise.- ADR-0059, Decision — "plus
HEADwhere it declaresGET, which routing serves implicitly" replaced with the measurement table above. One clause from the proposed wording ("the framework's own 405 for a bareMapGetadvertises onlyGET") is removed rather than restated: I could not isolate it in this app —PUT /is answered by auth (401) andHEAD /by the static-file middleware (200), both before routing — and re-publishing an unmeasured mechanism is the exact mistake being corrected here. WrongVerbOnExistingApiRoute_StillReturns405— the pinned assertion had["GET", "HEAD", "POST"]baked in; now["GET", "POST"].
New regression test: HeadOnGetDeclaringApiRoute_Returns405WithoutAdvertisingHead — 405 whose Allow does not contain HEAD.
Evidence at a83cac62e
dotnet test backend/tests/Taskdeck.Api.Tests -c Release -m:1— 2451 passed, 0 failed, 4 skipped (6m11s), full project, at this head.--filter SpaFallbackRoutingApiTests— 49 passed, 0 failed.node scripts/check-docs-governance.mjs— passed.- Mutation evidence: the new test fails on
5f2f7307dwithAllow: {GET, HEAD, POST}and passes here.
NOT verified
- No live/browser run; this is in-process integration evidence.
HEADstill is not served by any Taskdeck route — it now answers a truthful 405 instead of a self-contradicting one. IfHEADsupport is wanted, that is a separate change.- Hosted CI on this head not yet observed at the time of writing.
The 404/405 disposition remains maintainer-gated per #1992 — ADR-0059 stays Proposed, and I am not merging.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a83cac62ee
ℹ️ 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".
Chris0Jeky
added a commit
that referenced
this pull request
Aug 24, 2026
Codex review round on #2081, both findings accepted. 3846720003: the blanket "no slice was browser- or Playwright-verified" claim was wrong for #2073. Required E2E Smoke runs reusable-e2e-smoke.yml's Playwright step (playwright test --project=chromium) over testDir ./tests/e2e, so it exercised the seven tests/e2e specs #2073 changed - capture-loop and edge-journeys now assert the read-only review-applied-decision-record is visible and the Apply control is gone - and that lane was SUCCESS at merged head cd96411. The claim is narrowed to what is genuinely outstanding: the packaged-desktop spec (testIgnored by playwright.config.ts, own config), the mobile and cross-browser projects (grepInverted out of chromium), and screen-reader speech. The other five slices remain unverified for their own new surfaces, and #2077 got no E2E run at all. The incorrect "still owes a browser pass" item and the matching cold-start clause are dropped. 3846720016: .codex/memories/00_ACTIVE.md still routed on main 55dbf6e, labelled both former saved heads unshipped with no PR and told the next agent to publish them, and listed closed #1973 among the open gates. Reconciled to main f45a1fb with the six wave merges and their PRs, both heads marked shipped, #2079 recorded as maintainer-gated, #2081 named, the wave residuals #2075/#2078/#2080 added, and #1973 removed from the gate list. Format and conventions preserved. Part of #1947.
17 tasks
The framework's synthetic 405 endpoint carries no metadata, so the global FallbackPolicy answered anonymous wrong-verb requests 401 before the correction middleware saw the 405: an anonymous GET typo said 404 while the same PUT typo said 401. On machine paths the endpoint is replaced pre-authorization with an equivalent one carrying AllowAnonymous; the existing correction middleware then owns the honest 404/405 for every verb. Real routes under their declared verb keep 401-first ordering.
…llback Workbox tests the still-encoded pathname while nginx location-matches the decoded URI, so /mcp%2Fmessages was app-shell to the service worker and machine surface to the proxy. The %2[fF] boundary branch aligns the two; double-encoded %252F stays SPA-side in both layers.
The maintainer ruled in-session on 2026-08-24 (walkthrough q-2 A): merge adopts the 405 contract. Accepting the ADR and correcting STATUS in the same change keeps canonical docs from contradicting runtime behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c15f43c3ee
ℹ️ 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".
…DR measurement row The postprocessor now corrects only 405s from routing's synthetic method-mismatch endpoint (or its AllowAnonymous replacement) and the machine fallbacks. Measured: the MCP transport's own wrong-verb 405 was already surviving — its response has started by then — but that immunity was incidental to how the SDK writes; the scope is now pinned explicitly and held by a regression test. ADR-0059's anonymous-PUT measurement row updated to the shipped outcome, with the before-state kept as a note.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4edfaba1bb
ℹ️ 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".
| - **Failed quick captures keep their draft, shared error feedback is inspectable, and persistent toasts do not replay announcements after a skin remount (`#1938`, PRs `#2023`, `#2033`, and `#2067`; latest merge `e9b1390b6`).** Home and Paper Inbox distinguish a failed create from a post-create refresh failure, so a saved capture does not invite a duplicate retry. Default errors persist until dismissal; Legacy and Paper receipts can expand/copy existing detail text, and every Paper toast has a generic dismiss path. Each toast container now baselines the IDs already present when it mounts and announces only later non-error additions. An exact-main Paper-to-Legacy manual remount preserved the persistent toast while the announcer remained empty; actual screen-reader speech was not verified. Receipt localization and announcement roles then shipped as well (PR `#2037`, merge `a6f0dc998`): the persistent error-receipt controls and detail-region names are localized in English, Italian, and Spanish; each disclosure controller is associated with its details region only while that region is mounted; and error toasts announce as assertive alerts while non-error toasts announce as polite statuses, in both Legacy and Paper. The existing persistent-error lifetime and dismiss/copy behavior is unchanged. That slice passed typecheck, a production build, the full frontend suite (336 files, 4,752 tests), a focused Legacy/Paper/catalog run (3 files, 47 tests), lint, diff hygiene, and hosted Required CI at its exact head; it touched no backend file and ran no Playwright pass, and actual screen-reader speech is still unverified. Pinned-build root-cause reproduction, capture-input association/retention, receipt retention policy/ticker efficiency, stale per-toast state cleanup, clipboard-fallback hardening, and the remaining recorded a11y/state-cleanup gaps stay on open `#1938`; Italian and Spanish speaker review stays the human gate on `#1770`. | ||
| - **The source guard catches wider classes of enabled-looking inert controls (`#1949`, PRs `#2027`, `#2032`, `#2038`, `#2055`, `#2068`, and `#2072`; latest merge `aae515012`).** Native buttons require meaningful action handlers and proven form ownership while retaining narrow style-guide specimens; labelled custom buttons require static focusability plus non-empty canonical lowercase `.enter` and `.space` bindings. Mixed-case Vue modifier spellings remain violations, and Ctrl/Alt/Shift/Meta-modified Enter or Space handlers no longer satisfy the custom-button keyboard contract. The recorded parser-hardening residual is now closed for its own class (PR `#2072`, merge `aae515012`): the guard tokenizes opening-tag attributes before scanning, so only actual Vue event directives (`@...` / `v-on:...`) count as handler evidence and directive-looking text inside an ordinary quoted attribute - `data-note='@click=run'` - no longer satisfies the click or keyboard contract. Two false-negative regression cases hold it, the focused guard spec passed 13/13 at that head, and hosted Required CI passed at the exact head. Runtime route actionability, dynamic/compiler-expanded bindings, the AC3 disabled/validation contract, dead keystrokes (`#1968`), and the other acceptance criteria remain on open `#1949`. | ||
| - **Lowercase machine-prefix routing is corrected in the installed PWA and split-container topology (`#1992`, PRs `#2029`, `#2030`, and `#2065`; latest merge `c7a34d55c`).** PWA navigation excludes lowercase `/hubs` and `/health`; both nginx templates route lowercase bare and descendant `/api`, `/hubs`, `/health`, and `/mcp` paths to the API, preserving URI, forwarded headers, hub WebSockets, and unbuffered MCP SSE. Required container CI runs the checked-in static/rendered reverse-proxy contract before compose validation and image builds. Case-insensitive parity and a live split-container request/SSE proof remain on open `#1992`, and the wrong-verb 404-versus-405 contract is still the maintainer's to rule. **Open PR `#2079` implements one of the two options the issue names and is deliberately merge-gated on that ruling**: it would make a wrong-verb request on a real machine-facing route answer `405` with an exact `Allow`, keep a genuinely missing route at `404`, and align the PWA denylist boundaries with the four nginx prefixes. Its ADR-0059 is filed **`Proposed`, not Accepted**, so merging the PR is the ruling and closing it is the other. Nothing from `#2079` is shipped: `main` still answers `404` for `GET`/`HEAD` on a POST-only machine route. | ||
| - **Lowercase machine-prefix routing is corrected in the installed PWA and split-container topology (`#1992`, PRs `#2029`, `#2030`, and `#2065`; latest merge `c7a34d55c`).** PWA navigation excludes lowercase `/hubs` and `/health`; both nginx templates route lowercase bare and descendant `/api`, `/hubs`, `/health`, and `/mcp` paths to the API, preserving URI, forwarded headers, hub WebSockets, and unbuffered MCP SSE. Required container CI runs the checked-in static/rendered reverse-proxy contract before compose validation and image builds. Case-insensitive parity and a live split-container request/SSE proof remain on open `#1992`. **The wrong-verb 404-versus-405 contract is ruled and shipped: ADR-0059 Accepted (maintainer ruling 2026-08-24, recorded on `#1992`) via PR `#2079`** — a wrong-verb request on a real machine-facing route answers `405` with an exact `Allow` that never advertises a verb the route does not declare (HEAD included), a genuinely missing machine route answers the JSON `404` contract under every verb, and both answers are verb-independent for anonymous callers: the framework's metadata-less synthetic 405 endpoint is replaced on machine paths so the global FallbackPolicy cannot re-hide the contract behind a 401, while a real route under its declared verb still answers its normal 401-first auth ordering. The PWA denylist boundaries align with the four nginx prefixes including percent-encoded descendants (`/mcp%2Fmessages` is machine surface in both layers; double-encoded `%252F` is SPA-side in both). |
There was a problem hiding this comment.
Qualify this contract for /mcp: UnknownMcpPath_StillReturns401_WithoutApiKey proves that ApiKeyMiddleware short-circuits an anonymous request before routing, and the later STATUS entry explicitly says an MCP 404 requires a valid key. Therefore the claim that every missing machine route returns JSON 404 and that both outcomes apply to anonymous callers overstates shipped behavior for one of the four named prefixes; the same exception should also be reflected in ADR-0059's anonymous-access paragraph.
AGENTS.md reference: AGENTS.md:L15-L18
Useful? React with 👍 / 👎.
| - **A disabled `.pbtn` Paper button is disabled everywhere, from the token sheet (`#1953`, PR `#1988`).** `#1944`/PR `#1952` fixed the visible symptom inside capture triage with a scoped `:deep(.pbtn:disabled)` override; the systemic defect was that `.pbtn` in `paper-tokens.css` had no disabled treatment at all, so every *other* disabled Paper button still kept its fill, its `cursor: pointer` and its `:hover` change. The sheet now guards each interactive rule with `:not(:disabled)` on the selector — a disabled button never enters a hover or active state rather than having one undone afterwards — and adds two `:disabled` rules neutralising fill, ink, border, `box-shadow` (killing the ember inset) and `transform`, with `cursor: not-allowed`, built from the existing `--paper-2` / `--mute` / `--line-soft` / `--faint` tokens. The family boundary matters: buttons styled outside `.pbtn` (for example `PaperCaptureComposer`'s `paper-composer__*` attachment controls) still have no disabled treatment — the systemic gap is closed for the `PaperHLBtn`/`.pbtn` family, not for every control that can carry `disabled`. The triage-scoped override is **deleted**. A deliberate delta from the stopgap: a disabled ghost keeps a transparent fill, because giving it the same substrate fill as a live ember sibling made the dead control the louder of the two. **Two scope limits worth stating:** the rules match the native `:disabled` pseudo-class only — there is no `[aria-disabled]` or `.pbtn--disabled` selector, so an `aria-disabled`-only or link-shaped control gets nothing — and every selector is prefixed `.paper` / `.paper-night`, so the treatment is app-wide whenever the Paper skin is on and inert when it is off. Held by 49 cases in `tests/paper-disabled-button.spec.ts`, which parses the token sheet off disk and resolves the cascade itself because happy-dom resolves neither `var(--*)` nor real specificity, and which derives the variant list *from the sheet* so a future `.pbtn-quiet` cannot ship interactive rules the guard never exercises. | ||
| - **Toast stamps name the outcome, and the two Inbox counters stop contradicting each other (`#1970` + `#1974`, PR `#1989`).** The stamp was read from the toast's *tone*, and the success tone is named `applied` — so an inbox save, a pre-apply approval and an actual apply all stamped `APPLIED`, while errors stamped `Overdue` and info stamped `Captured`. A `ToastLabel` union plus an options argument let each call site stamp its own word: capture create is `SAVED`, triage enqueue is `QUEUED`, review approve is `APPROVED`, and execute is `APPLIED`, now the only path allowed to say it; unlabelled toasts fall back to `Done`/`Noted`/`Warning`/`Failed`. The tagstamp colour still comes from the tone, so nothing moved visually. On the counters: the sidebar badge reads the server's `capturesNeedingTriage` (`New + Failed`) while the Inbox header rendered `items.length` — every capture fetched, applied and ignored ones included — and called **both** of them a queue; the badge was also fetched once per session, so no capture mutation refreshed it until a full reload. A new `refreshWorkloadCounts()` writes back only the `workload` slice (never re-applying server preferences over newer local intent, never flashing a skeleton), guarded by a module-scoped request version that `fetchHomeSummary` and `clearHomeSummary` also bump, so a slow badge refresh can neither rewind fresher counts nor resurrect a cleared summary; `captureStore` calls it from every mutation that can move `New + Failed`. The eyebrow becomes `{pending} awaiting triage · {total} captured`, borrowing Home's own phrase so badge, Home line and eyebrow name one thing. **The two numbers are not forced equal** — the Inbox list is limit-fetched and can be board-scoped, so it counts rows on screen while the badge is workspace-wide; what they now share is one definition of *pending* and two distinct labels. All three locales updated, including a plural form chosen on `{total}` after `1 catturati` / `1 capturadas` read wrong at a single capture. **Still partial:** the review badge refreshes only as a side effect of a capture mutation — no review-side mutation calls the refresh itself. | ||
| - **Unknown machine-facing paths now 404 instead of 200 + the app shell — a deliberate HTTP contract change (horizon finding H-09 / `#1971`, PR `#1990`).** The SPA fallback matched every unmatched path, so a typo'd, renamed, or removed route under `/api`, `/hubs`, `/health`, or `/mcp` returned `200 OK` with `text/html`; such a path now returns `404` with the standard `ApiErrorResponse` JSON shape (`errorCode`/`message`, `application/json`), and the four catch-alls are excluded from the OpenAPI document (measured 2026-08-22: 160 documented paths become 156 — a measured claim, not one an automated test covers). A matched real route still reaches its normal authentication/authorization path, but `GET`/`HEAD` on a real POST-only route now reaches the fallback and returns 404 rather than the framework's former 405; accepting or restoring that behavior is an explicit owner decision on open `#1992`. **Accepted cost:** the fallbacks are anonymous, so route existence is now distinguishable without credentials (404 vs 401) — information the published OpenAPI document already carries; `/mcp` remains gated by `ApiKeyMiddleware` ahead of routing, so its 404 is reachable only with a valid key. ADR-0036's anonymous opt-out list is amended accordingly. Held by 41 cases in `SpaFallbackRoutingApiTests`, whose fixture supplies a marked `index.html` — load-bearing, because the plain test host ships no wwwroot and the suite would otherwise pass against the *unfixed* pipeline. **Deployment correction:** PRs `#2029` and `#2030` now bypass the installed-PWA shell for lowercase `/hubs` and `/health` navigations and route lowercase bare and descendant `/api`, `/hubs`, `/health`, and `/mcp` paths through both split-container nginx templates. Required container CI now executes the static/rendered proxy contract. The remaining topology bounds — case-insensitive parity, live request/SSE proof, and the owner-held wrong-verb 404/405 decision — stay on open `#1992`. | ||
| - **Unknown machine-facing paths now 404 instead of 200 + the app shell — a deliberate HTTP contract change (horizon finding H-09 / `#1971`, PR `#1990`).** The SPA fallback matched every unmatched path, so a typo'd, renamed, or removed route under `/api`, `/hubs`, `/health`, or `/mcp` returned `200 OK` with `text/html`; such a path now returns `404` with the standard `ApiErrorResponse` JSON shape (`errorCode`/`message`, `application/json`), and the four catch-alls are excluded from the OpenAPI document (measured 2026-08-22: 160 documented paths become 156 — a measured claim, not one an automated test covers). A matched real route still reaches its normal authentication/authorization path, but `GET`/`HEAD` on a real POST-only route now reaches the fallback and returns 404 rather than the framework's former 405; that residual was ruled on 2026-08-24 — ADR-0059 (Accepted) restores the `405` with an exact `Allow`, see the `#1992` entry above. **Accepted cost:** the fallbacks are anonymous, so route existence is now distinguishable without credentials (404 vs 401) — information the published OpenAPI document already carries; `/mcp` remains gated by `ApiKeyMiddleware` ahead of routing, so its 404 is reachable only with a valid key. ADR-0036's anonymous opt-out list is amended accordingly. Held by 41 cases in `SpaFallbackRoutingApiTests`, whose fixture supplies a marked `index.html` — load-bearing, because the plain test host ships no wwwroot and the suite would otherwise pass against the *unfixed* pipeline. **Deployment correction:** PRs `#2029` and `#2030` now bypass the installed-PWA shell for lowercase `/hubs` and `/health` navigations and route lowercase bare and descendant `/api`, `/hubs`, `/health`, and `/mcp` paths through both split-container nginx templates. Required container CI now executes the static/rendered proxy contract. The remaining topology bounds — case-insensitive parity, live request/SSE proof, and the owner-held wrong-verb 404/405 decision — stay on open `#1992`. |
There was a problem hiding this comment.
Remove the wrong-verb decision from this residual list: the same changed line now says the maintainer ruled on it and ADR-0059 is Accepted, while its final sentence still says that the owner-held 404/405 decision remains open on #1992. This leaves the canonical shipped-reality document giving mutually exclusive states for the decision.
AGENTS.md reference: AGENTS.md:L15-L18
Useful? React with 👍 / 👎.
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.
Part of #1992 — ships the wrong-verb contract half under the maintainer's 2026-08-24 ruling (ADR-0059 Accepted); case-insensitive prefix parity and the live split-container request/SSE proof remain on the open issue.
What was actually left
Two of the three residuals on this issue were already shipped and are green on
mainat6d45871ae— I verified both before touching anything, rather than re-implementing them:main/mcpin the split topologylocation ~ ^/<prefix>(?:/|$)blocks.scripts/deploy/Test-TaskdeckReverseProxyConfig.ps1re-run on this branch: "Reverse-proxy static and rendered template contract passed." (exit 0) — and PR #2065 wired it into required CI. Unchanged by this PR./hubsand/healthGET/HEADon a POST-only route 404s instead of 4051. Wrong-verb machine paths answer 405; unknown ones answer 404
Measured on
mainbefore the change, with a live host:GET /api/import/notes/markdown(route is POST-only)404+ error contract405,Allow: POSTHEADon the same404405PUT /api/import/notes/markdown405,Allow: GET, HEAD, POST405,Allow: POSTPUT /api/totally-unknown405,Allow: GET, HEAD, no body404+ error contract, noAllowPUT /api/boards405,Allow: GET, HEAD, POSTGET /api/abuse/actors/not-a-guid/evaluate404{actorUserId:guid}fails)GET /api/boards401The last three rows are the guard rails: the fix must not upgrade a genuinely missing resource to a 405, and must not reorder the auth outcome.
Why the 404 happened. #1971's per-prefix catch-alls are stamped
[GET, HEAD]so that other verbs still reach routing's 405 endpoint. Inside that pair the scoping does the opposite: aGETon a POST-only route is not method-mismatched against the catch-all, so it lands there instead. Routing cannot be asked to fix this from inside a fallback —HttpMethodMatcherPolicyis applied as a node-builder policy, so the candidate set is partitioned by verb inside the DFA and aGETrequest never sees thePOSTendpoint that shares its path.Mechanism.
MachineRouteMethodResolver(new) recovers the answer from the endpoint graph: every non-fallback endpoint under a machine prefix is translated once, lazily, into aTemplateMatcherplus its resolved inline constraints, and the request path is matched against that set. Constraints are evaluated, not skipped — that is what keeps/api/abuse/actors/not-a-guid/evaluatea 404. The fallback handler uses it to pick 405-vs-404 forGET/HEAD; a small post-endpoint middleware uses it for every other verb, correcting routing'sAllowheader or converting its 405-on-an-unknown-path into the 404 contract.Two adjacent inconsistencies came from the same shared routing node (rows 3 and 4 above) and are corrected with it rather than left to contradict the new contract. #1971 recorded row 4 as an accepted trade; it is the same "the status lies about what is there" shape the parent issue is about.
The comment block was overclaiming. It asserted that a wrong-verb request on a real route "mismatches every candidate and keeps its 405" — true only outside
GET/HEAD. That is the same defect class #1971 was filed to fix (a permanent comment recording a safety property the code lacks), so it is rewritten to describe what the pipeline actually guarantees, and theAllowAnonymousparagraph I accidentally duplicated while editing is removed.2. PWA denylist matches the machine-path boundary, and now has a test
#2029added/hubsand/healthbut left two boundaries disagreeing with the reverse proxy, which routes the identical four prefixes as^/<prefix>(?:/|$):/^\/api\//required a trailing slash, so an installed PWA servedindex.htmlfrom its own precache for the bare/apiand/api?probe=1, while nginx sends those to the API container./^\/mcp/had no boundary at all, so/mcpxand any other route merely starting with those letters was denied the shell.All four are now
^/<prefix>(?:[/?]|$)—[/?]because workbox tests the denylist againstpathname + search.src/tests/config/PwaMachinePathDenylist.spec.tsis the first test over this config. It reads the regex literals out ofvite.config.tsand executes them against a table of machine paths and client-side routes, so a pattern that is spelled plausibly but behaves wrong fails. It uses a?rawimport rather thannode:fsbecausetsconfig.vitest.jsontype-checks new specs without node types and forbids adding files to its quarantine list.Verified
dotnet test backend/tests/Taskdeck.Api.Tests— 2450 passed, 0 failed, 4 skipped (5m43s) at the pre-comment-fix tree;SpaFallbackRoutingApiTestsre-run at final head: 48 passed, 0 failed.dotnet test backend/tests/Taskdeck.Architecture.Tests— 26 passed, 0 failed, 1 skipped (layer purity holds for the newTaskdeck.Api.Routingnamespace).npx vitest --run src/tests/config/PwaMachinePathDenylist.spec.ts— 20 passed./api,/api?probe=1,/mcpx). The backend tests' before-state is the measured table above, taken onmain's behaviour.npm run typecheck— clean (the new spec is type-checked, not quarantined).npm run build— succeeds, and the generateddist/sw.jscarriesdenylist:[/^\/api(?:[/?]…, so the config change reaches the shipped artefact.node scripts/check-docs-governance.mjs— passed.scripts/deploy/Test-TaskdeckReverseProxyConfig.ps1— passed (evidence residual 1 is intact, not evidence this PR changed it).NOT verified
sw.jsis inspected; an installed-PWA offline navigation was not driven end to end.Taskdeck.sln) not run — the change is confined to the API pipeline, andApi.Tests+Architecture.Testsare the projects that exercise it. Hosted CI covers the rest.Residual risk
MachineRouteMethodResolvercaches its translated route set for the process lifetime. Taskdeck registers no dynamic endpoints, but a future surface that mutates an endpoint data source after startup would need that cache invalidated. Stated in the class docs.GET/HEADalready leaked it.The gate — please read before merging
#1992and its 2026-08-24 checkpoint say explicitly: "Maintainer decision remains required for wrong-verb GET/HEAD on a real POST-only route… Do not infer the 404/405 contract from current implementation or tests."I have not inferred it. This PR implements one of the two options the issue names and is offered as the concrete thing to rule on — merging it is the ruling, closing it is the other ruling. ADR-0059 is filed as
Proposed, notAccepted, with the measured before-state, the mechanism, and the three alternatives (accept the 404; drop the method scoping; register a 405 shim per real route) and why each was not taken. If the disposition is "accept and document the 404", close this PR and I will re-cut the documentation-only half — but note that the current surface already answers 405 for every verb outsideGET/HEAD, so "404" is not presently a coherent posture either way.Two acceptance items from the checkpoint are deliberately not touched here: case-insensitive parity for PWA and nginx matching (still an open disposition), and the live split-container/MCP SSE proof.