Vetrix Docs

PR-Review Rule: Router-Touching Changes Require a Router-Level Test

Status: Mandatory for any PR that adds or modifies a route registration in internal/api/. Owner: Code Review (CR) sub-agent + human reviewers.

Why this rule exists

Handler-only tests pass while the production router is structurally broken, because handler-only tests bypass route registration entirely. A test that invokes a handler method directly (h.CreateFolder(w, req)) never goes through NewRouter(), so it reports full coverage even when the route is missing from the production route table.

Routes that are conditionally mounted on a Services field (for example, svc.DownloadsStore != nil && svc.DownloadsDir != "") are absent from the route table when that field is unset, and chi's default NotFound returns the literal string 404 page not found. A bare-prefix subrouter mounted via r.Route("/api/v1/repos/{owner}/{repo}", h.Mount) can also shadow sibling sub-paths the parent router does not own exactly. Neither failure is caught by a test that builds a fresh chi.NewRouter() and re-mounts routes with test-local helpers instead of exercising NewRouter(svc). This document is the rule that closes that hole.

1. Trigger - when this rule applies

This rule applies to any PR whose diff adds, modifies, or moves a route registration in any file under internal/api/. Concretely, the diff is in scope if it touches any of these chi calls:

r.Post(    r.Get(    r.Put(    r.Patch(    r.Delete(    r.Head(    r.Options(    r.Mount(    r.Route(    r.Method(    r.Handle(

The CR sub-agent and the proposed CI script (Section 4) MUST flag any PR whose diff matches this regex inside internal/api/:

git diff --unified=0 origin/<base>...HEAD -- 'internal/api/*.go' \
  | grep -E '^\+\s*r\.(Post|Get|Put|Patch|Delete|Head|Options|Mount|Route|Method|Handle)\('

(The ^\+ anchor restricts to added/changed lines. Deletions of routes are also in scope - the same regex with ^\- MUST be checked for "did the PR also delete the test?")

Also in scope:

  • Any new if svc.<X> != nil (or equivalent feature-flag guard) wrapping a route registration block. The conditional itself becomes part of the test surface - see Section 3.
  • Any change to cmd/server/main.go that gates whether a Services field is populated (e.g., envOr("VETRIX_X_DIR", "")). Each such gate MUST have at least one router-level test that wires the field and one that leaves it unset.

Out of scope (this rule does NOT apply):

  • Changes confined to handler bodies that do not alter the route table (router.go is unchanged and no r.<Verb>( lines are added/removed in internal/api/*.go).
  • Pure refactors that preserve the route table verbatim (verify by route-table snapshot diff).

2. Required test shape

Every in-scope PR MUST add or modify at least one *_test.go file in internal/api/ that:

  1. Builds the production router via NewRouter(svc) from internal/api/router.go - or via a documented test helper that delegates to NewRouter(svc). Tests that build a fresh chi.NewRouter() and re-mount routes with test-local helpers do NOT satisfy this rule. They certify "if you mount these routes in this shape, the behaviour is correct" - they do not certify "the production wiring mounts them at all."
  2. Exercises the new/changed route by URL via r.ServeHTTP(w, req) - NOT by direct handler-method invocation (h.CreateFolder(w, req)).
  3. Asserts the response status is the expected non-404 outcome (2xx for happy paths, 401 for auth-gated routes called anonymously, 400/422 for validation failures, etc.). A test that ends in 404 against the production router is a defect, not a passing test.

Skeleton

package api

import (
    "context"
    "net/http"
    "net/http/httptest"
    "strings"
    "testing"
)

func TestFOR_XXXX_NewRoute_MountedInProductionRouter(t *testing.T) {
    // 1. Build a fully-wired Services fixture. Whatever your route depends
    //    on (Store, Dir, MaxUpload, AuthService, etc.) MUST be populated so
    //    the conditional-mount block in router.go evaluates true.
    svc := newTestServicesFullyWired(t)

    // 2. Build the production router. NOT chi.NewRouter() + re-mount.
    r := NewRouter(svc)

    // 3. Issue a real HTTP request through the router. The URL must match
    //    what the FE / API consumers will call.
    body := strings.NewReader(`{"name":"folder1"}`)
    req := httptest.NewRequest(http.MethodPost,
        "/api/v1/repos/alice/foo/downloads/folders", body)
    req.Header.Set("Content-Type", "application/json")
    // Attach an authenticated context if the route is auth-gated:
    req = req.WithContext(withTestAuthCtx(context.Background(), "alice"))

    w := httptest.NewRecorder()
    r.ServeHTTP(w, req)

    // 4. Assert a non-404 outcome. The defect class is chi returning
    //    "404 page not found" because the route was never registered;
    //    asserting != 404 is the floor. Asserting the exact expected
    //    status (201 Created, 401, etc.) is better.
    if w.Code == http.StatusNotFound &&
        strings.Contains(w.Body.String(), "404 page not found") {
        t.Fatalf("route not registered in production router; "+
            "chi default NotFound fired. body=%q", w.Body.String())
    }
    if want := http.StatusCreated; w.Code != want {
        t.Fatalf("status=%d body=%q want %d", w.Code, w.Body.String(), want)
    }
}

Anti-patterns (these do NOT satisfy the rule)

// BAD - Handler-only test, bypasses chi entirely.
h := NewDownloadsHandler(&Services{}, store, "/tmp/dl", 0)
h.CreateFolder(w, req)

// BAD - Test-local router that re-mounts the routes; does not exercise NewRouter().
r := chi.NewRouter()
for1935MountDownloadsMutations(r, h, authSvc)  // local helper, not router.go
r.ServeHTTP(w, req)

// BAD - Asserting on a route table the test built itself rather than NewRouter()'s.
r := chi.NewRouter()
r.Post("/api/v1/repos/{owner}/{repo}/downloads/folders", h.CreateFolder)
// Testing this proves your test setup works, not the production wiring.

3. Conditional-mount cases

If the new or changed route is gated on a Services field - i.e., it sits inside an if svc.<X> != nil && svc.<Y> != "" block in router.go, or its handler is conditionally instantiated based on an env var in cmd/server/main.go - the PR MUST include two router-level test fixtures:

(a) Field WIRED - assert non-404

Build Services with the gating field populated. Issue the request through NewRouter(svc). Assert the status is the expected 2xx/4xx (validation, auth, etc.) - anything except chi's 404 page not found.

func TestFOR_XXXX_DownloadsRoute_WhenStoreWired(t *testing.T) {
    svc := &Services{
        DownloadsStore: newTestStore(t),
        DownloadsDir:   t.TempDir(),
        // ... other required fields ...
    }
    r := NewRouter(svc)
    req := httptest.NewRequest(http.MethodPost, "/api/v1/repos/alice/foo/downloads/folders", strings.NewReader(`{}`))
    w := httptest.NewRecorder()
    r.ServeHTTP(w, req)
    if w.Code == 404 && strings.Contains(w.Body.String(), "404 page not found") {
        t.Fatalf("route not mounted under wired Services - chi 404 fired")
    }
    // assert exact expected status here (e.g., 401 if anon, 400 if bad body, 201 on happy path)
}

(b) Field UNSET - assert fail-stop OR documented disabled-status

When the gating field is unset, the conditional block never registers the route and chi's default NotFound returns 404 page not found. The desired behaviour is one of:

  • Boot fail-stop: the binary refuses to start, so this fixture is exercised at the cmd/server smoke-test level, not the router level. This is the canonical answer for env-gated features.
  • Explicit disabled response: the router returns a 503 Service Unavailable (or 404 with a structured {"error":"feature disabled"} JSON body) so callers can distinguish "feature off" from "route never existed."

The floor is: the test MUST exist and MUST assert the documented disabled-state behaviour, even if that means asserting on the chi 404 string. If the disabled-state behaviour later changes, this test fails loudly — a one-line test diff instead of a silent production regression.

func TestFOR_XXXX_DownloadsRoute_WhenStoreUnset(t *testing.T) {
    svc := &Services{
        // DownloadsStore intentionally nil; DownloadsDir intentionally "".
        // Wire only the bare minimum for NewRouter to construct.
    }
    r := NewRouter(svc)
    req := httptest.NewRequest(http.MethodPost, "/api/v1/repos/alice/foo/downloads/folders", strings.NewReader(`{}`))
    w := httptest.NewRecorder()
    r.ServeHTTP(w, req)
    // chi 404 because the conditional block didn't register the route.
    // If the disabled-state behaviour changes to a boot fail-stop, this
    // test needs updating. The point is that EITHER outcome is asserted,
    // not silently observed.
    if w.Code != http.StatusNotFound {
        t.Fatalf("expected 404 (route gated off); got status=%d body=%q", w.Code, w.Body.String())
    }
}

4. Enforcement option

Enforcement is two-layer: a PR-template checklist line AND a scripts/check-router-test-coverage.sh CI script.

Rationale

A checklist alone is insufficient: a human-only checklist is the same defense that lets a handler-only test pass review while looking like a route test, so it must be backed by mechanical enforcement. A script alone catches the obvious cases but lacks the social signal that tells new contributors the rule exists before they write the PR. The checklist surfaces the rule at PR-open time; the script enforces it at PR-merge time; the two layers reinforce each other. The cost is low - one paragraph in the template, one shell script in scripts/, one entry in the CI manifest.

Checklist line (PR template)

The PR template MUST include the following line:

  • Router test: If this PR adds or modifies any r.Post/Get/Put/Patch/Delete/Mount/Route/... line in internal/api/, I have added or modified a test in internal/api/*_test.go that builds the production router via NewRouter(svc) and exercises the new/changed route by URL via r.ServeHTTP(w, req). If the route is conditionally mounted, I have added BOTH the wired and unset fixtures per router-test-rule.md Section 3.

CI script (scripts/check-router-test-coverage.sh)

A shell script that, given a base branch, computes whether the diff matches the Section 1 trigger regex AND whether any internal/api/*_test.go file in the same diff calls NewRouter(. Fails the CI job with an actionable error message pointing at this document if the trigger fires without a matching test.

Subagent prompt insertion

See Section 5 for the verbatim block to add to the Code Review subagent's operating instructions.

5. Code Review subagent prompt insertion

Add the following block verbatim to the Code Review subagent's operating instructions (the prompt the Manager issues when dispatching CR for any ticket). Insert it under whatever existing "test-coverage gating" section already exists, or as a new top-level section if none exists.

### Router-test gate (see router-test-rule.md)

For any PR whose diff includes a line under `internal/api/` matching the regex
`^\+\s*r\.(Post|Get|Put|Patch|Delete|Head|Options|Mount|Route|Method|Handle)\(`,
you MUST verify all of the following before approving:

1. **A router-level test exists in the same PR.** Locate at least one
   `internal/api/*_test.go` file in the diff that calls `NewRouter(` (the
   production router constructor from `internal/api/router.go`) - NOT
   `chi.NewRouter()` followed by a test-local mount helper. If you cannot
   find such a test, the PR FAILS this gate. Cite the missing test as a
   blocking comment and request changes.

2. **The router-level test exercises the new/changed route by URL.** Read
   the test body and confirm it issues `r.ServeHTTP(w, req)` against the
   path that appears in the new `r.<Verb>(...)` registration. A test that
   only calls `h.CreateFolder(w, req)` (or any other direct handler-method
   invocation) does NOT satisfy this gate even if `NewRouter(` appears
   elsewhere in the file.

3. **The test asserts a non-404 outcome on the wired path.** A test that
   ends with `if w.Code == 404 { /* OK */ }` against `NewRouter(svc)` with
   the gating field WIRED is a defect - it is asserting the bug, not the
   fix. The wired-fixture assertion MUST be 2xx, 4xx (auth/validation),
   or any explicit non-default status. Specifically, the body MUST NOT
   contain the literal string `404 page not found` when the gating field
   is populated.

4. **Conditional-mount routes have BOTH fixtures.** If the new
   registration sits inside an `if svc.<X> != nil` (or equivalent
   feature-flag) block, the PR MUST include two router-level tests: one
   with the field wired (asserting non-404), one with the field unset
   (asserting the documented disabled-state behaviour - chi 404, or a
   boot fail-stop / explicit 503 where the feature is gated that way).
   See `router-test-rule.md` Section 3.

5. **Source-code comments referencing tests are not evidence.** If a
   PR's source-code comment claims "covered by the smoke test in CI",
   you MUST grep the repository for that test file and confirm it
   exists AND runs in the CI manifest. A comment referencing a boot
   smoke check that was never written is exactly the failure this rule
   guards against.

If any of (1)-(4) fail, post a blocking review comment citing
`router-test-rule.md` and the specific gate that
failed. Do not approve until the contributor adds the missing test.

If (5) is the only failure (the comment claims a test that does not
exist), file an incidental bug ticket per the Manager's Section 8.4 process
and request the contributor either (a) write the test the comment
claims, or (b) delete the misleading comment. Do not block the PR on
(5) alone unless it is the PR that introduced the misleading comment.