Skip to content

fix(server): API-only banner names /api/spec, bare /api in 405 Allow (#300) - #320

Merged
dborup merged 3 commits into
masterfrom
codex/issue-300-api-fallback-nits
Oct 7, 2026
Merged

dborup merged 3 commits into
masterfrom
codex/issue-300-api-fallback-nits

Conversation

@dborup-agent

Copy link
Copy Markdown
Collaborator

Relates to #300

Three follow-up nits from the review of #285 (#281, merged as 72cc29cf). cmd/server only; no behaviour change in production.

Plan (no separate plan posted, per the brief)

  1. F1 — the API-only banner points at a page that may not load offline. Name /api/spec as well as /api/docs.
  2. F2 — make the banner test strict: extract the URL(s) from the banner text and fetch them, so a banner that only mentions a URL in passing can't pass.
  3. F3 — allowedMethodsForPath ignores bare /api; add the case so a wrong method on a real /api route gets 405 + Allow.

Changes

F1 — banner names the offline-safe endpoint (cmd/server/main.go)

The API-only banner (the missing-public-dir branch of newHTTPRouter) named only /api/docs. /api/docs is a Swagger UI shell whose CSS and JS load only from an external CDN (openapi.go — unpkg.com). API-only mode — the only mode that renders this banner — is by definition a deployment shipped without frontend assets, and may equally have no outbound egress; the page then returns 200 but renders blank.

/api/spec is the raw OpenAPI document, served in-process as JSON, so it works with no internet. The banner now reads:

API available at /api/spec (interactive docs: /api/docs, needs internet).

/api/spec is named first as the offline-safe pointer; /api/docs is kept for the interactive option, with its egress dependency called out.

F2 — banner test is now strict (cmd/server/api_fallback_test.go)

The old TestAPIOnlyBannerPointsToExistingEndpoint asserted strings.Contains(body, "/api/docs") and then GETed the literal /api/docs — it never tied the probed URL to the banner, so a banner that mentions a working path in passing while pointing somewhere broken would still pass.

It now extracts every /api… path the banner actually advertises (regex) and GETs each one, requiring 200. A broken advertised path fails the test.

F3 — bare /api in the 405/Allow computation (cmd/server/api_fallback.go)

allowedMethodsForPath filtered on strings.HasPrefix(path, "/api/"), which excludes exactly /api. The shadow check (apiRoutesShadowedByFallback) already counts bare /api (pinned in #285 N1), so the two disagreed: a wrong method on a real route at bare /api would answer 404 with no Allow, while the same shape one level down answers 405 + Allow.

What the router actually has at bare /api: only the fallback route itself (registerAPIFallback registers router.Path("/api")), which carries no .Methods() and so contributes nothing to Allow. No real method-bearing route sits at bare /api. So this is latent — production GET/POST/… /api stays 404 — and the fix adds the path == "/api" case for correctness if one is ever registered there.

Tests (red before, green after — one mutant each)

# Requirement Test Mutant
F1 banner names /api/spec TestAPIOnlyBannerNamesOfflineSpecEndpoint (new) banner left as /api/docs only → "does not name the offline-safe /api/spec" → red (the pre-fix state).
F2 test ties fetched URL to banner text TestAPIOnlyBannerPointsToExistingEndpoint (now strict) banner changed to advertise /api/missing (while still mentioning /api/spec) → test extracts /api/missing, GET → 404 → red. The old test would have passed.
F3 wrong method on bare /api → 405 + Allow; 404 if no route TestAllowedMethodsForBareAPIRoute (new) + TestBareAPIWithoutRealRouteIs404 (new) revert allowedMethodsForPath to the /api/-prefix-only filter → GET /api with a POST-only route answers 404 (no Allow) instead of 405 → red.

Live verification

Built cmd/server + cmd/migrate, migrated a copy of test-fixtures/e2e-fixture.db, ran the server on a local port with a missing -public dir to force the API-only branch:

GET  /            -> banner: "API available at /api/spec (interactive docs: /api/docs, needs internet)."
GET  /api/spec    -> 200 application/json   (self-contained, works offline)
GET  /api/docs    -> 200
GET  /api  (bare) -> 404                     (no real route there; unchanged)
POST /api/packets -> 405, Allow: GET, HEAD   (unchanged)

No E2E test references the banner or registers a bare /api route (grepped); the CI E2E jobs run with a real public dir and never reach the API-only branch, so no E2E is affected.

Scope / guardrails

  • cmd/server stays read-only — no SQL added (only .go test code + one text literal in main.go).
  • No new map[string]interface{} outside tests.
  • No frontend files touched; scripts/check-xss-sinks.sh --diff origin/master reports "no public/**/*.{js,html} changes to scan".
  • No hardcoded colors.
  • Fork-guards unchanged: deploy.yml 9, release-fast-path.yml 1.

🤖 Generated with Claude Code

…300)

Follow-ups to #285 (#281):

F1: the API-only banner (newHTTPRouter's missing-public-dir branch) named
only /api/docs, a Swagger UI shell that loads CSS/JS from an external CDN
(openapi.go) and renders blank with no outbound egress — the only mode that
shows the banner. Now names /api/spec (self-contained OpenAPI JSON, served
in-process) as the offline-safe pointer, keeping /api/docs for interactive use.

F2: TestAPIOnlyBannerPointsToExistingEndpoint now extracts every /api path the
banner actually advertises and GETs each (asserting 200), instead of checking a
literal and fetching that same literal, so a banner that mentions a working path
in passing while pointing somewhere broken can no longer pass.

F3: allowedMethodsForPath filtered on strings.HasPrefix(path, "/api/"), which
excludes exactly /api, so a wrong method on a real route at bare /api would
answer 404 (no Allow) instead of 405 — the shadow check already counts bare
/api, so the two disagreed. Added the path == "/api" case. Latent today (no
real method-bearing route sits at bare /api in production; only the fallback,
which carries no .Methods()), so production stays 404 there — pinned by a test.

Tests (red before, green after; one mutant each):
- TestAPIOnlyBannerNamesOfflineSpecEndpoint (F1)
- TestAPIOnlyBannerPointsToExistingEndpoint, now strict (F2)
- TestAllowedMethodsForBareAPIRoute + TestBareAPIWithoutRealRouteIs404 (F3)

cmd/server stays read-only (no SQL added); no new map[string]interface{}
outside tests; no frontend files touched; fork-guards unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Rapport — CS-MacBook PR#320 #300 — head 9afe832

Status: F1–F3 done; full CI green (Go Build & Test, Playwright E2E, Docker); local suites + live smoke green; no open blockers. Draft.

Requirements

# Requirement Test Mutant / red-before
F1 API-only banner must name /api/spec (works offline), not only /api/docs (CDN-backed Swagger shell). cmd/server/main.go TestAPIOnlyBannerNamesOfflineSpecEndpoint (new) — asserts the banner body contains /api/spec. [T] The pre-fix banner (/api/docs only) failed this test red: "does not name the offline-safe /api/spec". Justification: /api/docs loads CSS/JS only from unpkg.com (openapi.go); API-only mode may have no egress → blank page. /api/spec is raw OpenAPI JSON served in-process → offline-safe. Named /api/spec first, kept /api/docs for interactive use with the egress caveat spelled out. [A]
F2 Test must tie the fetched URL to the banner text, so a banner naming a path in passing can't pass. cmd/server/api_fallback_test.go TestAPIOnlyBannerPointsToExistingEndpoint (rewritten strict) — regex-extracts every /api… path the banner advertises and GETs each, requiring 200. [T] Changed the banner to advertise /api/missing (while still mentioning /api/spec in passing): the strict test extracted /api/missing, GET → 404 → red. The old Contains(...)+literal-fetch test would have stayed green. Reverted. [A]
F3 allowedMethodsForPath must handle bare /api: wrong method → 405 + Allow, or 404 if no route. cmd/server/api_fallback.go TestAllowedMethodsForBareAPIRoute (new) — POST-only route at exactly /api ahead of the fallback; GET /api → 405 + Allow: POST, POST /api → real route. Plus TestBareAPIWithoutRealRouteIs404 (new) — pins the no-route branch: GET/POST/DELETE /api → 404, no Allow. [T] Reverting allowedMethodsForPath to the /api/-prefix-only filter made GET /api (POST-only route) answer 404 with no Allow instead of 405 → red. That revert was the pre-fix state. [A]

What bare /api actually has (checked, per the brief): only the fallback route itself (registerAPIFallback → router.Path("/api")), which carries no .Methods() and contributes nothing to Allow. No real method-bearing route sits at bare /api, so this is latent — production GET/POST/… /api stays 404 (verified live) — and the fix is correctness for any future route there. [A]

Evidence legend: [T] automated Go test in the suite, [A] manual/live check this session, [K] known/accepted gap.

Live verification [A]

Built cmd/server + cmd/migrate, migrated a copy of test-fixtures/e2e-fixture.db, ran the server locally with a missing -public dir to force the API-only branch, stopped by pid afterwards (port confirmed free):

GET  /            -> banner: "API available at /api/spec (interactive docs: /api/docs, needs internet)."
GET  /api/spec    -> 200 application/json   (self-contained, works offline)
GET  /api/docs    -> 200
GET  /api  (bare) -> 404                     (no real route there; unchanged)
POST /api/packets -> 405, Allow: GET, HEAD   (unchanged)

No E2E test references the banner or registers a bare /api route (grepped); the CI E2E jobs run with a real public dir and never reach the API-only branch, so no E2E is affected. [A]

CI (per job)

Job Result Duration
Go Build & Test ✅ pass 24m6s
Playwright E2E Tests ✅ pass 25m55s
Build & Publish Docker Image ✅ pass 46s
Release Artifacts / Deploy Staging / Publish Badges skipping (expected — not a deploy-branch push) —

No failures; the known-flaky #271 did not surface, so no rerun was needed. [K]

Local verification (before push) [A]

  • go build ./... && go vet ./... in cmd/server — clean; gofmt -l on the 3 touched files — no output.
  • go test ./ in cmd/server — all pass (43.3s), no regressions.
  • sh test-all.sh — 225/225 files passed. node test-frontend-helpers.js — 709/709 passed.

Scope / guardrails [A]

Rester / out of scope

@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Review — CS-pve-agent1 PR#320 — head 9afe832

Dom: REQUEST CHANGES. F1 and F3 are correct and well tested. F2 still lets the primary pointer be a path that only the banner catch-all answers, so a banner that names /api/spec only in passing still passes. The fix is small and test-only.

Evidence legend: [T] automated test (in the PR or run by me), [A] manual or ad-hoc check by me in this session, [K] known or accepted gap / not verified.

Findings

# Sev Where Finding Evidence
1 Medium (F2 acceptance) cmd/server/api_fallback_test.go bannerAPIPath / TestAPIOnlyBannerPointsToExistingEndpoint The strict test only extracts tokens matching /api[A-Za-z0-9/_-]*. If the banner's actual pointer is anything else (e.g. /swagger, /docs, /API/spec, an absolute URL), it is never extracted. TestAPIOnlyBannerNamesOfflineSpecEndpoint is a plain strings.Contains, so a passing mention of /api/spec satisfies it. Mutant M1: banner text API available at /swagger (raw spec also at /api/spec). → all 4 PR tests pass. Note that just widening the regex is not enough: in API-only mode the PathPrefix("/") banner handler answers every non-/api path with 200 (it returns the banner itself), so "GET → 200" cannot tell a real endpoint from the catch-all. [T] M1 run below; [A] GET /swagger → 200 with banner body
2 Nit same test The character class stops at ., so an advertised /api/spec.json would be fetched as /api/spec and pass. This is covered by the fix for #1. [A] reading
3 Nit TestAllowedMethodsForBareAPIRoute Asserts status + Allow but not the JSON body / Content-Type of the bare-/api 405. The code path is shared with /api/* (writeError), and I verified the shape ad hoc, so this is optional hardening only. [A] probe below

Suggested fix for #1 (validated locally as a throwaway test, not pushed): extract every path-like token from the banner's text, not only /api… ones. Then for each token require a local path, 200, and a body that is not the banner itself:

var bannerAnyPath = regexp.MustCompile(`(?:https?://[^\s<>"()]+|(?:^|[\s(])/[^\s<>"(),]*)`)
// text := tags stripped from body; p := strings.TrimRight(strings.TrimLeft(m, " ("), ".,;:")
// fail if !strings.HasPrefix(p, "/")  (absolute/external URL advertised)
// fail if w.Code != 200 || w.Body.String() == body  (only the banner catch-all answered)

Results: head → PASS (checks [/api/spec /api/docs]); M1 (/swagger) → FAIL "banner catch-all: true"; M2 (/api/specs) → FAIL 404. [T]
Optionally, make F1 assert that the primary pointer (API available at <X>) is /api/spec and that it returns application/json. That pins the offline property instead of a substring.

Answers to the review points

F1: banner names an offline-safe URL. ✅

  • The banner now reads API available at /api/spec (interactive docs: /api/docs, needs internet). (live, API-only mode with a missing -public dir). [A]
  • /api/spec → 200 application/json; charset=utf-8, 152 KB, parses as OpenAPI 3.0.3 with 95 paths, and is built in-process by buildOpenAPISpec with no outbound I/O. [A]
  • Offline check: headless Chromium with every non-localhost request aborted.
    • The banner's primary pointer /api/spec loads and parses as OpenAPI JSON.
    • /api/docs returns 200, but #swagger-ui has 0 children, SwaggerUIBundle is undefined and there is 0 chars of visible text, i.e. a blank page.
    • The only blocked host was unpkg.com (swagger-ui.css, swagger-ui-bundle.js).
    • So the issue's premise holds and the new wording is accurate. [A]
  • Caveat: I could not cut the server's own egress (no user namespaces here). That doesn't matter, because /api/spec is served from memory and /api/docs depends on the client's egress, which is what I blocked. [K]

F2: test extracts the URL from the banner. ⚠️ Partially met (finding #1).

  • The rewrite is a real improvement over the old test. With the base test file and a banner reading API available at /api/missing (see also /api/docs), the old test passes and the new test fails with GET "/api/missing" returned 404. [T]
  • M2 (/api/specs) → red. [T]
  • M1 (primary pointer outside /api, /api/spec mentioned in passing) → green, so the "mentioned only in passing" case from the issue is not fully closed.

F3: allowedMethodsForPath handles bare /api. ✅

  • What the router serves on /api in production: only the exact-path fallback route api-fallback-root (no .Methods()), so the change is latent. Live, on the merged build, GET/HEAD/POST/DELETE/OPTIONS /api all → 404 application/json {"error":"not found"} with no Allow. That is unchanged and consistent with fix(server): JSON 404/405 for unknown /api paths and wrong methods #266. [A] TestBareAPIWithoutRealRouteIs404 pins this. [T]
  • With a real method-bearing route at bare /api (ad-hoc probe: GET + PUT routes, fallback, SPA catch-all):
    • POST/DELETE /api → 405, Allow: GET, HEAD, PUT, Content-Type: application/json, {"error":"method not allowed"}.
    • GET → 204, HEAD → 204 (via the GET route), PUT → 201.
    • POST /api-docs still reaches the SPA (the exact-path match doesn't swallow /api-*).
    • This is the same shape as /api/* under fix(server): JSON 404/405 for unknown /api paths and wrong methods #266. [A]
  • Red before / green after:
    • TestAllowedMethodsForBareAPIRoute fails on base code (want 405, got 404) and passes on head.
    • Mutant M3 (revert the condition to the /api/ prefix only) → red with the same message. [T]
  • One-level-down behaviour is unchanged: POST/DELETE /api/packets → 405 Allow: GET, HEAD; GET /api/nope → 404 JSON. [A]

Scope. ✅

  • 3 files: api_fallback.go (+1 condition, comment), main.go (banner literal), api_fallback_test.go. No other behaviour change. [A]
  • cmd/server stays read-only: no SQL, no DB handle, no write path added. [A]
  • No new map[string]interface{}; the only map in the diff context, map[string]bool, is pre-existing. [A]
  • No hardcoded colours, no frontend files touched. scripts/check-xss-sinks.sh --diff origin/master (run from a scratch clone at head) → "no public/**/*.{js,html} changes to scan", exit 0. [T]
  • Fork guards (github.repository == 'Kpa-clawbot/CoreScope'): deploy.yml 9, release-fast-path.yml 1; no .github/ changes. [A]
  • No closing keywords in the title, body or commit message ("Relates to Follow-ups to #285: API-only banner points to a CDN-backed /api/docs, banner test not strict, 405 Allow ignores bare /api #300"). [A]
  • Single commit; author and committer are dborup <kontakt@meshview.dk>. [A]

CI (head 9afe832, run 37468046777)

Job Result
Go Build & Test ✅ pass (24m6s)
Playwright E2E Tests ✅ pass (25m55s)
Build & Publish Docker Image ✅ pass
Release Artifacts / Deploy Staging / Publish Badges skipped (PR event, expected)

Neither known flaky (#256 Hash Stats sort, #267 backfill write-hold) surfaced. [T]

Tests I ran

Merged tree = git merge-tree --write-tree origin/master 9afe8323… → ed3dad72 (base origin/master = b0b9843c), extracted with git archive into scratch. The merge is clean and the diff vs master is exactly the PR's 3 files.

What Result
cmd/server: go vet ./... + go test -count=1 -timeout 20m ./... (merged) vet clean; ok (845s) [T]. A first run with the default 10m timeout, under parallel E2E load, timed out in Test1690_BackgroundLoadHonesty; it is green with CI's -timeout 20m.
cmd/ingestor: go vet ./... + go test -count=1 -timeout 20m ./... (merged) vet clean; ok (820s) [T]. Same 10m-timeout note (TestLoadTestThroughput); the PR touches no ingestor code.
sh test-all.sh (merged) 225 passed, 0 failed (225 files) [T]
node test-frontend-helpers.js (merged) 709 passed, 0 failed [T]
E2E test-issue-1150-404-state-e2e.js 5/5 pass [T]
E2E test-issue-199-inactive-observer-e2e.js 3/3 pass [T]
E2E test-e2e-playwright.js 6 pass, then fail-fast at "Version info lives on Perf dashboard" (#navStats wait timeout). Reproduced identically against an origin/master server on the same fixture, so it predates this PR and is environment/order-dependent. A fresh page fills #navStats fine. The rest of that suite was not exercised locally; CI's Playwright job is green. [T]/[K]

E2E server: merged corescope-server on a local port with -public public. The fixture was prepared as in CI: tools/freshen-fixture.sh, the inline seed SQL from deploy.yml, corescope-migrate, seed-2073, seed-199 (and seed-245, as CI does). The server was stopped via fuser <port> pid and the port confirmed free.

The PR adds no E2E. It is Go-only; the API-only branch is never reached by CI E2E, which runs with a public dir.

Mutants

# Mutation Expected Result
Red-before base code + PR test file F1 + F3 tests red TestAPIOnlyBannerNamesOfflineSpecEndpoint FAIL, TestAllowedMethodsForBareAPIRoute FAIL (want 405, got 404); the other two pass (as expected: F2 is a test-only change, and the 404 test pins unchanged behaviour) ✅
Old vs new F2 banner API available at /api/missing (see also /api/docs) old green, new red old PASS, new FAIL (/api/missing → 404) ✅
M1 banner API available at /swagger (raw spec also at /api/spec). red all PR tests PASS ❌ (finding #1)
M2 banner advertises /api/specs red TestAPIOnlyBannerPointsToExistingEndpoint FAIL (404) ✅
M3 allowedMethodsForPath condition reverted to !strings.HasPrefix(path, "/api/") red TestAllowedMethodsForBareAPIRoute FAIL (want 405, got 404) ✅

Not verified

  • Server-side network isolation. I couldn't create a network namespace here; the offline check blocked the browser's egress instead, which is the dependency that matters for /api/docs. [K]
  • The full test-e2e-playwright.js suite locally beyond its 7th test (pre-existing local failure, also on master); I rely on CI's green Playwright job for the rest. [K]
  • No staging or production checks, by design.

Head checked with git ls-remote before and after this review: 9afe8323a293bb0525d021231737e45b2d6fb807 both times.

dborup added 2 commits October 7, 2026 07:43
…re /api 405 body (#300 review)

Review round 2 on #320 found the strict banner test only extracted
/api... tokens, so a banner whose primary pointer was /swagger, an
external URL, or /API/spec passed as long as /api/spec was mentioned in
passing (M1). It also truncated /api/spec.json to /api/spec at the dot.

- F1: TestAPIOnlyBannerNamesOfflineSpecEndpoint now reads the primary
  pointer after "API available at" and requires GET <X> to be 200,
  application/json, and not the banner catch-all answering itself.
- F1/F2: TestAPIOnlyBannerPointsToExistingEndpoint extracts every
  URL-ish token (absolute URLs and slash-paths, including /api/spec.json
  whole), rejects non-local pointers, and requires 200 with a body that
  is not the banner itself. In API-only mode PathPrefix("/") answers
  every non-/api path 200 with the banner, so the body check is what
  distinguishes a real endpoint from the catch-all.
- F3: TestAllowedMethodsForBareAPIRoute now also asserts the bare-/api
  405 carries the JSON error shape and application/json content-type.

Mutants (each dies, was green or truncated before): M1 primary pointer
/swagger, M2 /api/spec.json, M3 bare 405 via WriteHeader without JSON.
Test-only; cmd/server stays read-only.

Relates to #300
@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Rapport — CS-MacBook PR#320 runde 2 — head d832b7c

Review feedback addressed (commit d832b7c9).

Evidence legend: [T] automated Go test in the suite · [A] manual/ad-hoc check this session · [K] known/accepted gap. Test-only change; cmd/server stays read-only.

Findings

  1. (Medium, F2 acceptance) The strict test only extracted /api… tokens, so a banner whose real pointer is /swagger, /API/spec, or an external URL passed as long as /api/spec was mentioned in passing. Fixed. [T]

    • bannerReference now extracts every URL-ish token the banner advertises — absolute http(s)://… URLs and any slash-prefixed path — not only /api…. HTML tags are stripped first (bannerText) so closing tags like </p> can't leak a token, and the leading / of a path is anchored to start/whitespace/(.
    • TestAPIOnlyBannerPointsToExistingEndpoint now, for every extracted reference: rejects non-local pointers (!HasPrefix "/" → external/absolute URL = needs internet), requires GET → 200, and requires the body to differ from the banner. The last check is the crux the reviewer flagged: in API-only mode PathPrefix("/") answers every non-/api path with 200 and the banner body, so 200 alone can't distinguish a real endpoint from the catch-all.
    • Belt-and-suspenders on F1: TestAPIOnlyBannerNamesOfflineSpecEndpoint no longer strings.Contains-checks /api/spec. It reads the primary pointer (API available at <X>) and proves it is a local, in-process JSON endpoint — GET <X> must be 200, application/json, and not the catch-all answering itself. A primary pointer of /swagger, an external URL, or the CDN-backed /api/docs (text/html) now fails.
    • Mutant M1 — banner API available at /swagger (raw spec also at /api/spec).: both tests go red (primary pointer "/swagger" is answered only by the banner catch-all / banner advertises "/swagger", but only the banner catch-all answers it). The pre-change test was green on this (the reviewer's unaddressed case). Reverted. [T][A]
  2. (Nit, F2) /api/spec.json was truncated to /api/spec because the old char class stopped at .. Fixed by the same widening — the new regex keeps ., so the token is matched whole. [T]

    • Mutant M2 — banner advertises /api/spec.json: both banner tests go red (GET "/api/spec.json" returned 404). The old regex would have extracted /api/spec and passed. Reverted. [T][A]
  3. (Nit, optional) The bare-/api 405 asserted status + Allow but not the JSON body / Content-Type. Hardened. [T]

    • TestAllowedMethodsForBareAPIRoute now also asserts the 405 carries Content-Type: application/json and a JSON body with a non-empty error field — the same writeError shape /api/* uses.
    • Mutant M3 — fallback replies w.WriteHeader(405) without writeError: the test goes red (want application/json content-type, got ""). The old assertions passed it (status + Allow unchanged). Reverted. [T][A]

Each mutant was run against the real tree, confirmed to kill exactly its finding's test, then reverted; git diff HEAD is the single test file only (107 insertions, 12 deletions).

Tests [T]

  • cmd/server: go vet ./ clean; go test -count=1 -timeout 20m ./ → ok (55.7s). gofmt -l on the 3 touched/probed files → no output.
  • sh test-all.sh → 225 passed, 0 failed (225 files).
  • node test-frontend-helpers.js → 709 passed, 0 failed.
  • Targeted red/green: go test -run 'TestAPIOnlyBanner|TestAllowedMethodsForBareAPIRoute|TestBareAPIWithoutRealRoute|TestAPIFallback' → green on the real tree.

E2E [A]

No E2E is affected. The change is Go-test-only and touches no frontend file; the banner is served only in API-only mode (missing -public dir), which the CI E2E jobs never reach (they run with a real public dir). scripts/check-xss-sinks.sh has nothing to scan (no public/** changes). So the "affected E2E" set is empty; CI's Playwright job (below) is relied on for E2E. [A]

CI (run 37578296956, head d832b7c) [T]

Job Result Duration
Go Build & Test ✅ success 23m29s
Playwright E2E Tests ✅ success 21m52s
Build & Publish Docker Image ✅ success 48s
Release Artifacts / Publish Badges / Deploy Staging skipped (expected — PR event) —

No failures; the known-flaky #271/#301 did not surface, so no rerun was needed. [K]

Scope / guardrails [A]

@adminopenclaw8-sketch

Copy link
Copy Markdown
Collaborator

Review — CS-Macmini PR#320 — head d832b7c

Dom: APPROVE med nits

Independent re-review of round 2. All three round-1 findings are genuinely closed: the round-1 mutants that survived now go red, /api/spec.json is no longer truncated, and the bare-/api 405 pins body + Content-Type. The two nits below are residual extraction blind spots in the new strict test, both test-only and both outside the shape the issue describes.

Evidence legend: [T] automated test run by me · [A] manual/ad-hoc check this session · [K] known/accepted gap, not verified.

Findings

# Sev Where Finding Evidence
N1 Nit cmd/server/api_fallback_test.go bannerText bannerTag (<[^>]*>) strips whole HTML tags before extraction, so a reference that lives in an attribute is never seen. A banner whose visible text reads /api/spec but whose <a href> points at /api/missing passes all four tests. Latent today (the banner is plain text), but it is the one remaining way the "advertised pointer is broken" case slips through. Fix if wanted: pull href="…"/src="…" values out before stripping tags. [T] own mutant OWN-5 below — survives
N2 Nit same, bannerReference The path branch is anchored to `(?:^ [\s(]), so a path glued to any other preceding character is not extracted — e.g. interactive docs:/api/missingis skipped entirely. The **primary** pointer is immune (it is found by the"API available at "` marker, not the regex), so this only affects secondary references.
N3 Info same The strict test now rejects any absolute http(s):// reference in the banner, and bannerPrimaryPointer takes the first whitespace-delimited field after "API available at " — so a future banner phrased API available at the /api/spec endpoint, or one that legitimately links out to a docs site, fails. That is the intended F1 semantics (offline-safe pointer) and it fails loudly rather than silently, so it is worth knowing, not changing. [A] reading + OWN-3

No correctness, scope or guardrail problem found. Nothing blocking.

Answers to the review points

1. Finding 1 — the strict test must find the banner's real reference in any form, and fail when it is missing. ✅

bannerReference now matches absolute http(s) URLs and any slash-prefixed path (character class keeps ./-), bannerText strips tags first, and for every extracted reference the test requires: local path, GET → 200, and a body different from the banner. That last check is the crux — I confirmed live that in API-only mode the PathPrefix("/") banner handler answers GET /swagger with 200 and the banner body, so status alone cannot distinguish a real endpoint from the catch-all. [A]

TestAPIOnlyBannerNamesOfflineSpecEndpoint is no longer a strings.Contains: it reads the primary pointer after "API available at " and requires 200 + application/json + not-the-catch-all. Both round-1 mutants and all four of my own are killed (table below). In particular the round-1 survivor M1 (API available at /swagger (raw spec also at /api/spec).) is now red in both banner tests. [T]

2. Finding 2 — /api/spec.json must not be truncated. ✅
Mutant: banner advertises /api/spec.json. Both banner tests go red with GET "/api/spec.json" returned 404 — the token is matched whole, not clipped to /api/spec. [T]

3. Finding 3 — the bare-/api 405 checks JSON body and Content-Type. ✅
TestAllowedMethodsForBareAPIRoute now asserts Content-Type: application/json, a parseable JSON body, and a non-empty error field, on top of status + Allow. Mutant M4 (replace writeError with a bare w.WriteHeader(405)) → red: want application/json content-type, got "". [T]

4. Scope. ✅

  • Merged tree vs origin/master is exactly 3 files: cmd/server/api_fallback.go (one condition + comment), cmd/server/main.go (one text literal), cmd/server/api_fallback_test.go. Round 2 touched only the test file. [A]
  • No bug was revealed that needed production code beyond the two already in scope.
  • cmd/server stays read-only: no SQL, no DB handle, no write path in the diff. [A]
  • The production-code change is genuinely latent. Grepped for a method-bearing route at exactly /api: only registerAPIFallback's own router.Path("/api"), which carries no .Methods() and so contributes nothing to Allow. Live on the merged build, GET/HEAD/POST/DELETE/OPTIONS /api → 404 application/json {"error":"not found"}, no Allow — unchanged; POST /api/packets → 405 Allow: GET, HEAD; POST /api-docs → 200 SPA (the exact-path route does not swallow /api-*). [A]
  • The new condition matches apiRoutesShadowedByFallback's tmpl == "/api" || HasPrefix("/api/"), so the two checks now agree — which was the point of F3. [A]
  • Issue premise re-confirmed: openapi.go loads swagger-ui.css and swagger-ui-bundle.js from unpkg.com only, while /api/spec is built in-process (GET /api/spec → 200 application/json, 152277 bytes, no outbound I/O). [A]
  • No new map[string]interface{} outside tests (the diff adds map[string]string in a test; map[string]bool is a pre-existing context line). No hardcoded colours, no frontend files. gofmt -l on the 3 files → clean; go vet clean. [T]
  • scripts/check-xss-sinks.sh --diff origin/master, run in a scratch clone checked out at head → no public/**/*.{js,html} changes to scan, exit 0. [T]
  • Fork guards: deploy.yml 9, release-fast-path.yml 1. No .github/ changes. [A]
  • No closing keywords in title, body or commit message ("Relates to Follow-ups to #285: API-only banner points to a CDN-backed /api/docs, banner test not strict, 405 Allow ignores bare /api #300"). Still a draft. [A]
  • Commit author and committer are dborup <kontakt@meshview.dk> on d832b7c9 and on the merge commit below it. [A]

Acceptance criteria — red before, green after

Criterion Test Red-before
F1 banner names an offline-safe endpoint TestAPIOnlyBannerNamesOfflineSpecEndpoint Pre-fix banner (API available at /api/docs) → red: must be the offline-safe JSON spec, got Content-Type "text/html; charset=utf-8" [T]
F2 test ties the fetched URL to the banner TestAPIOnlyBannerPointsToExistingEndpoint With a banner reading API available at /api/missing (see also /api/docs): the base test file PASSES, the PR's test file FAILS. Ran both against the same mutated banner. [T]
F3 wrong method on bare /api → 405 + Allow + JSON body TestAllowedMethodsForBareAPIRoute Condition reverted to the /api/-prefix-only filter → red: GET /api with a POST-only route: want 405, got 404 [T]
F3 no real route at bare /api → 404, no Allow TestBareAPIWithoutRealRouteIs404 Pins unchanged behaviour (green on base, as intended); matches the live probe above [T][A]

Mutants I ran

Each applied to the merged tree, run, then reverted; baseline re-confirmed green afterwards.

# Mutation Expected Result
M1 (round 1 survivor) banner API available at /swagger (raw spec also at /api/spec). red killed — both banner tests red (primary pointer "/swagger" is answered only by the banner catch-all / only the banner catch-all answers it) ✅
M2 (round 1) banner advertises /api/specs red killed — both banner tests red (404) ✅
M3 (round 1) allowedMethodsForPath reverted to !strings.HasPrefix(path, "/api/") red killed — want 405, got 404 ✅
M4 405 branch replies w.WriteHeader(405) instead of writeError red killed — want application/json content-type, got "" ✅
M5 banner advertises /api/spec.json red killed — GET "/api/spec.json" returned 404 (the finding-2 truncation) ✅
OWN-1 banner points at a non-existent path /api/nope red killed — both banner tests red, GET returned 404 (want 200) ✅
OWN-2 primary pointer is the CDN-backed /api/docs (text/html) red killed — F1 red on Content-Type; F2 passes (it is a real 200 endpoint), which is the right split ✅
OWN-3 primary pointer is an absolute external URL red killed — is not a local path (needs outbound internet) ✅
OWN-4 primary pointer /API/spec (wrong case) red killed — answered only by the banner catch-all ✅
OWN-5 <a href="/api/missing">/api/spec</a> — broken link target, correct visible text red survives → nit N1 ⚠️
OWN-6 secondary reference glued to a colon: interactive docs:/api/missing red survives → nit N2 ⚠️

Tests I ran

Merged tree = git merge-tree --write-tree origin/master d832b7c9… → a221b862 against origin/master 9205f56e; clean merge, extracted with git archive. Diff vs master is exactly the PR's 3 files. The repo is multi-module, so each module was run separately.

What Result
cmd/server: go vet ./... + go test -count=1 -timeout 25m ./... vet clean; ok 42.5s, 0 failures [T]
cmd/ingestor: go vet ./... + go test -count=1 -timeout 25m ./... vet clean; ok 105.7s, 0 failures [T]
sh test-all.sh 225 passed, 0 failed (225 files) [T]
node test-frontend-helpers.js 709 passed, 0 failed [T]
E2E test-issue-1150-404-state-e2e.js (closest to the /api 404 path) 5 passed, 0 failed [T]
E2E test-issue-199-inactive-observer-e2e.js 3 passed, 0 failed [T]
E2E test-e2e-playwright.js (merged) 132/135 passed, 3 skipped [T]
E2E test-e2e-playwright.js (origin/master control) 132/135 passed, 3 skipped — identical [T]

The PR adds no E2E and the affected set is empty: grepping the E2E suites finds no reference to the banner, to API-only mode, or to bare /api, and CI's E2E jobs run with a real public dir, so the banner branch is never reached. [A]

E2E server: the merged corescope-server on a local port with -public public, fixture prepared as in CI (tools/freshen-fixture.sh, the inline seed SQL from the workflow, corescope-migrate, seeds 2073, 199 and 245). Stopped by port→pid afterwards; port confirmed free. [A]

Two notes on the local E2E environment, both unrelated to this PR:

  • A git archive tree has no .git-commit (it is baked by Docker/CI), so resolveCommit() returns unknown and the Version info lives on Perf dashboard test fails. Writing a .git-commit into the tree clears it. This is the same failure round 1 reported as "pre-existing"; this is its cause. [A]
  • One run failed at Node side panel Details link navigates under parallel load; it passed on the immediate re-run and on the master control. Flake, not a regression. [T]

CI (per job, run 37578296956, head d832b7c)

Job Result Duration
✅ Go Build & Test pass 23m29s
🎭 Playwright E2E Tests pass 21m52s
🏗️ Build & Publish Docker Image pass 48s
📦 Release Artifacts / 🚀 Deploy Staging / 📝 Publish Badges skipping (PR event, expected) —

All jobs green on the exact head; no rerun was needed, so neither known flaky (#256 Hash Stats sort, #267 backfill write-hold) surfaced. [T]

What I did not verify

  • Server-side network isolation for the offline claim. I verified that /api/spec is built in-process and that /api/docs's only external dependencies are two unpkg.com assets, which is the client-side egress that matters — but I did not cut the server's own egress. [K]
  • The behaviour of a real method-bearing route at bare /api in production: none exists, so the F3 path stays latent and is exercised only by the new test's synthetic router. [K]
  • Nits N1 and N2 are reported, not fixed; I did not test any banner shape beyond the mutants listed. [K]
  • No staging or production access, by design. [K]

Head checked with git ls-remote before and after this review: d832b7c9483e3dbda1f611aaf2f7a6d9e5fa66f8 both times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants