test(server): pin api fallback guards; fix API-only banner (#281)

[dborup]

Relates to #281

Summary

Four small follow-ups from the round-2 re-review of #266 (merged as 81b13b20, review comment on #266):

1. N1 — apiRoutesShadowedByFallback (cmd/server/api_fallback.go:147) flags a late /api route via tmpl == "/api" || strings.HasPrefix(tmpl, "/api/"). The existing TestAPIRoutesShadowedByFallbackReportsLateRoutes only registers late routes under the prefix (/api/late, /api/late/{id}), so dropping the tmpl == "/api" arm left the suite green (mutant D). Added TestAPIRoutesShadowedByFallbackReportsLateBareAPIRoute, which registers a late route at exactly /api and asserts it's reported as shadowed.
2. N2 — getRouteHandler's self-dispatch guard (cmd/server/api_fallback.go:123) returns nil when the only route a GET would reach is the fallback itself. Removing it left the suite green (mutant L) because the recursive call it would otherwise allow carries Method: GET, which lands on the exact same 404/405 computation and produces byte-identical HTTP output — no black-box test can distinguish the two. Added TestGetRouteHandlerSelfDispatchGuardReturnsNilForUnknownPath, a white-box unit test calling getRouteHandler directly and asserting it returns (nil, nil).
3. N3 — TestPostPacketsRemovedFallsThroughToSPAInProductionRouter has asserted 405 + Allow since fix(server): JSON 404/405 for unknown /api paths and wrong methods #266 (the doc comment was already updated then; the name wasn't). Renamed to TestPostPacketsRemovedReturns405NotSPAInProductionRouter.
4. N4 — the API-only banner in cmd/server/main.go (served by newHTTPRouter when the static/public directory doesn't exist) said "API available at /api/", which fix(server): JSON 404/405 for unknown /api paths and wrong methods #266 turned into a JSON 404. Checked what actually resolves: /api/docs (interactive Swagger UI, routes.go:430) and /api/spec (raw OpenAPI JSON, routes.go:429) both exist; picked /api/docs as the more useful human-facing pointer. Added TestAPIOnlyBannerPointsToExistingEndpoint, which builds the production router with a missing public dir and asserts the banner text names /api/docs and that a GET to it actually returns 200.

Not touched

• cmd/server stays read-only; no SQL added.
• No new map[string]interface{} outside tests.
• Fork-guards unchanged: deploy.yml 9, release-fast-path.yml 1 (grepped, matches baseline).
• No frontend files touched.

Tests

#	Requirement	Test	Mutant
N1	bare /api shadow-guard arm pinned	TestAPIRoutesShadowedByFallbackReportsLateBareAPIRoute (new)	D — dropped tmpl == "/api" arm → got [], want [/api] → red. Confirmed, then reverted.
N2	self-dispatch guard pinned	TestGetRouteHandlerSelfDispatchGuardReturnsNilForUnknownPath (new)	L — removed the GetMethods() error-guard → getRouteHandler returned a non-nil handler instead of (nil, nil) → red. Confirmed the rest of the HTTP-level suite (TestAPI*, TestPostPackets*) stays green under mutant L, matching the review's "harmless today, unpinned" finding. Reverted.
N3	stale test name	rename only, no new test	n/a — build fails if any reference to the old name survives; grepped clean.
N4	API-only banner	TestAPIOnlyBannerPointsToExistingEndpoint (new)	Reverted the banner text to /api/ → test failed with "does not mention /api/docs" → red. Reverted the revert.

Evidence legend: [T] automated Go test, [A] live/manual check, [K] known gap.

• N1–N4: [T] all four tests pass on the fix, fail on their respective mutant/revert (verified locally, not committed).
• [A] Built the server + cmd/migrate binaries, migrated a copy of test-fixtures/e2e-fixture.db, ran the server on port 13901 with a missing -public dir:
  • GET / → banner text contains API available at /api/docs.
  • GET /api/docs → 200.
  • GET /api (bare) → 404 application/json.
  • POST /api/packets → 405, Allow: GET, HEAD.
  • HEAD /api/this-path-does-not-exist → 404 application/json (no recursion/hang).
• [A] Grepped all test-*.js E2E files for any reference to the banner text or the renamed test name — none found, so no E2E test is affected by this change (it's a backend-test-only + banner-text change; the CI E2E jobs run with a real public dir, which never hits the API-only banner branch at all).

CI (local, per job — not run through GitHub Actions UI from this session)

• cd cmd/server && go build ./... && go vet ./... — clean.
• gofmt -l on all touched files — no output.
• go test ./... in cmd/server — all passing (41.6s), no regressions.
• sh test-all.sh — 221/221 files passed.
• node test-frontend-helpers.js — 707/707 passed.
• scripts/check-xss-sinks.sh --diff origin/master — clean (no public/**/*.{js,html} changes).

GitHub Actions CI on this PR has not been inspected from this session yet; will need a look once it runs. Known flake: #271 — rerun once if it's the only failure.

Rester (leftovers / out of scope)

None beyond what #266 already recorded (the plain-text http.NotFound handlers inside some /api/* handlers, tracked as out of scope there too).

[dborup]

Rapport — CS-MacBook PR#285 #281 — head f7acaf8

Status: All four follow-ups done, local verification + full CI green, no open blockers.

Requirements

#	Requirement	Test	Mutant / red-before evidence
N1	Lock the bare /api arm of apiRoutesShadowedByFallback (cmd/server/api_fallback.go:147)	TestAPIRoutesShadowedByFallbackReportsLateBareAPIRoute (new, api_fallback_test.go) — registers a late route at exactly /api and asserts it's reported as shadowed. [T]	Mutant D: dropped the tmpl == "/api" arm, leaving only strings.HasPrefix(tmpl, "/api/"). Result: got [], want [/api] — test failed red. Reverted, test green again. [A]
N2	Lock the self-dispatch guard in getRouteHandler (cmd/server/api_fallback.go:123)	TestGetRouteHandlerSelfDispatchGuardReturnsNilForUnknownPath (new, api_fallback_test.go) — calls getRouteHandler directly for an unknown /api path and asserts (nil, nil). [T]	Mutant L: removed the GetMethods() error-guard. Result: the function returned the fallback's own (non-nil) handler instead of (nil, nil) — test failed red. Also confirmed the rest of the HTTP-level suite (TestAPI*, TestPostPackets*) stays fully green under this mutant, matching the review's finding that no black-box test can observe the guard (the recursive call it would otherwise allow carries Method: GET and lands on byte-identical 404/405 output). Reverted. [A]
N3	Rename TestPostPacketsRemovedFallsThroughToSPAInProductionRouter to match the 405 + Allow assertion	Renamed to TestPostPacketsRemovedReturns405NotSPAInProductionRouter (post_packets_removed_223_test.go); doc comment above it already described the current behavior from #266, only the identifier was stale.	n/a (pure rename) — grepped the repo for the old name, no other references survive. [A]
N4	API-only banner (cmd/server/main.go:662) must point at a real endpoint	TestAPIOnlyBannerPointsToExistingEndpoint (new, api_fallback_test.go) — builds the production router (newHTTPRouter) with a missing public dir, asserts the banner text names /api/docs and that GET /api/docs on that router returns 200. [T]	Checked both candidates first: /api/docs (interactive Swagger UI) and /api/spec (raw OpenAPI JSON) both exist (routes.go:429-430); picked /api/docs as the better human-facing pointer. Reverted the banner text back to /api/ — test failed with "does not mention /api/docs" — red. Reverted the revert. [A]

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

Additional live verification [A]

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

GET  /                              -> banner text contains "API available at /api/docs"
GET  /api/docs                      -> 200
GET  /api        (bare)             -> 404 application/json
POST /api/packets                   -> 405, Allow: GET, HEAD
HEAD /api/this-path-does-not-exist  -> 404 application/json (no recursion/hang)

Grepped every test-*.js E2E file for the banner text or the renamed test name — no hits, so no E2E test exercises this change (the CI E2E jobs always run with a real public dir, which never reaches the API-only banner branch at all; the renamed test is Go-only).

Scope / guardrails

• cmd/server stayed read-only; no SQL touched (only .go test files + one text-literal line 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".
• Fork-guards unchanged: grepped github.repository == 'Kpa-clawbot/CoreScope' — deploy.yml 9, release-fast-path.yml 1, matching the required baseline.

CI (per job, GitHub Actions on this PR)

Job	Result	Duration
Go Build & Test	✅ pass	15m4s
Playwright E2E Tests	✅ pass	20m10s (including the #1616 slide-over flake-gate, 3/3 repeats, 27 passed each)
Build & Publish Docker Image	✅ pass	58s
Release Artifacts / Deploy Staging / Publish Badges	skipped (expected — not a push to the deploy branch)

No failures in any job — the known-flaky #271 did not surface, so no rerun was needed.

Local verification (this session, before pushing)

• cd cmd/server && go build ./... && go vet ./... — clean.
• gofmt -l on all touched files — no output.
• go test ./... in cmd/server — all passing (41.6s), no regressions.
• sh test-all.sh — 221/221 files passed.
• node test-frontend-helpers.js — 707/707 passed.

Rester (leftovers / out of scope)

• [K] None new. The plain-text http.NotFound handlers inside some /api/* handlers (noted during fix(server): JSON 404/405 for unknown /api paths and wrong methods #266's review) remain out of scope here, unchanged.

[adminopenclaw8-sketch]

Review — CS-Macmini PR#285 — head f7acaf8

Dom: APPROVE med nits

Independent read-only review. Verified on the merged tree git merge-tree --write-tree origin/master f7acaf8c = 58d9cce16062bbd459c69719bd0f100ffb5962cb (conflict-free; its diff against origin/master 4f1de049 is exactly the three files this PR touches, nothing else). git ls-remote on the branch was f7acaf8c… before and after the review — head did not move.

Findings

#	Severity	Where	Finding
F1	nit	cmd/server/main.go:662	/api/docs is a Swagger UI shell: its CSS and JS come only from an external CDN (openapi.go:1039,1049). API-only mode — the only mode that renders this banner — is by definition a deployment shipped without frontend assets, which may equally have no outbound egress; the page then loads 200 but renders blank. /api/spec, the other candidate #281 N4 named, is self-contained JSON. Consider naming both, e.g. "API available at /api/docs (raw spec: /api/spec)". [A]
F2	nit	cmd/server/api_fallback_test.go:470	TestAPIOnlyBannerPointsToExistingEndpoint asserts strings.Contains(body, "/api/docs") and then GETs the literal /api/docs. It does not assert that the endpoint the banner names is the one probed, so a banner that mentions /api/docs in passing while pointing somewhere else still passes. Extracting the path out of the banner text with a small regex and GETting that would pin the invariant the test name claims. [T]
F3	nit (pre-existing, latent)	cmd/server/api_fallback.go:73	allowedMethodsForPath filters on strings.HasPrefix(path, "/api/"), which excludes exactly /api. N1 has now pinned the bare-/api arm of the shadow check, but the 405/Allow computation still ignores bare /api, so the two disagree. My own test: a real POST-only route at /api makes GET /api answer 404 with no Allow header, while the same shape one level down (GET /api/bare) correctly answers 405 + Allow: POST. Latent today — no real route is registered at bare /api — and it predates this PR, so out of scope here. [T]

Nothing blocking. All three follow-up fixes do what #281 asks, each with a test that is red on the corresponding mutant and green on the fix.

Point-by-point

N1 — bare-/api arm of the shadow guard. Met. TestAPIRoutesShadowedByFallbackReportsLateBareAPIRoute registers a late route at exactly /api and asserts the reported set is exactly [/api]. Red-before confirmed by running the master api_fallback_test.go against my mutant MM1 (bare arm dropped): the whole cmd/server suite stayed green, so the gap the issue describes was real. [T]

N2 — self-dispatch guard in getRouteHandler. Met. TestGetRouteHandlerSelfDispatchGuardReturnsNilForUnknownPath calls getRouteHandler directly and asserts (nil, nil). Red-before confirmed the same way: with the guard deleted (MM2) and master's test file in place, the full suite was green. The author's claim that no black-box test can observe this guard holds up — I reproduced it. The white-box unit test is the right call here. [T]

N3 — stale test name. Met. TestPostPacketsRemovedReturns405NotSPAInProductionRouter matches what the body asserts (405 + Allow: GET, HEAD + unchanged packet tables). Grep over the whole merged tree: zero remaining occurrences of the old identifier, in Go, docs, scripts or workflows. The doc comment above it already described the current behavior, as stated. [A]

N4 — API-only banner. Met in the sense the issue asked for: the banner now names a path that resolves (GET /api/docs → 200, verified both in-process and against a live server). Red-before confirmed: reverting the literal to /api/ fails the new test. See F1 for the one reservation about which of the two candidates is the better pointer. [T][A]

Scope. Clean. The diff is three files: two Go test files and one text literal. No behaviour change outside the banner string. cmd/server stays read-only — no INSERT/UPDATE/DELETE/.Exec( in any added line, so no write was added anywhere, let alone outside cmd/ingestor. No new map[string]interface{} (none anywhere in the diff). No frontend files: public/ is byte-identical to origin/master, and scripts/check-xss-sinks.sh --diff origin/master exits 0 with "no public/**/*.{js,html} changes to scan". No hardcoded colors (the only hits for a color regex on added lines are the #281 issue references in comments). Fork-guards: deploy.yml 9, release-fast-path.yml 1 — in both the head tree and the merged tree. No closing keywords in the commit message or the PR body ("Relates to #281"). Commit author and committer are both dborup <kontakt@meshview.dk>. gofmt -l on the three touched files: no output. The PR is still a draft. [A]

Mutants I ran myself

Mutant	Change	Result
MM1	apiRoutesShadowedByFallback: drop the tmpl == "/api" arm, keep only the /api/ prefix	Killed by the new N1 test (got [], want [/api]). With master's api_fallback_test.go instead, the full suite was green — the gap was real.
MM2	getRouteHandler: delete the GetMethods() error-guard entirely	Killed by the new N2 test (returned the fallback's own handler + a non-nil request). With master's test file, the full suite was green.
MM3	shadow guard: collapse both arms to strings.HasPrefix(tmpl, "/api") (no trailing slash) — the plausible "simplification"	Killed, but by the pre-existing TestAPIRoutesShadowedByFallbackReportsLateRoutes, whose /apiary-late route catches the missing slash. Already covered; no gap.
MM4	getRouteHandler: keep the guard but return match.Route.GetHandler(), nil — a half-guard	Killed by the new N2 test. The test pins both return values, not just the handler.
MM5	revert the banner literal to API available at /api/	Killed by the new N4 test.

Edge case I added and ran

The one in F3: a real POST-only route registered at exactly /api, ahead of the fallback (so it is genuinely not shadowed — asserted as a precondition). Observed, in a throwaway test on the merged tree:

GET /api      -> 404  Allow=""      body={"error":"not found"}
GET /api/bare -> 405  Allow="POST"

So the bare-/api path that N1 now pins on the shadow side is still invisible to the Allow side. Pre-existing from #266, latent while nothing registers at bare /api; recorded here, not fixed, and not a reason to hold this PR.

Tests I ran

On the merged tree (58d9cce1):

• cmd/server: go build ./... and go vet ./... clean. go test ./... run 7 times — 6 green (≈45–57 s each), 1 red on my very first run, whose failing-test identity I lost to my own output truncation. Three further runs on plain origin/master were green as well, and the three new tests are green 50/50 under -count=50, so I cannot attribute it to this PR; recorded as unverified below rather than as a finding. [T]
• cmd/ingestor: go test ./... — green (ok github.com/corescope/ingestor 104.6s), confirming the merged tree is sound outside cmd/server too. [T]
• sh test-all.sh — 222 passed, 0 failed (222 files). [T]
• node test-frontend-helpers.js — 709 passed, 0 failed. [T]
• scripts/check-xss-sinks.sh --diff origin/master — exit 0, nothing to scan. [T]
• Live server from the merged tree, fixture prepared the way CI does (freshen, the two seed inserts, corescope-migrate), started with a missing -public dir to reach the banner branch, stopped afterwards by port lookup:

GET  /                              200 text/html, "...API available at /api/docs"
GET  /api/docs                      200 text/html   (external refs: CDN css + bundle only — see F1)
GET  /api                           404 application/json
POST /api/packets                   405, Allow: GET, HEAD, application/json
HEAD /api/this-path-does-not-exist  404 application/json (no recursion, no hang)
GET  /api/spec                      200 application/json
startup log                         "API-only mode", no shadowed-route warning

Every line matches the author's report. [A]

• E2E: no test-*.js references the banner text, the renamed identifier, or /api/docs, and public/ is byte-identical to master, so nothing in the E2E set is affected by this diff. I still ran test-e2e-playwright.js against a local Go server on the prepared fixture: it fails locally at "Version info lives on Perf dashboard, not in navbar" (#navStats wait times out). The same test fails identically against a plain origin/master build on the same fixture, so this is my local environment, not the PR. The CI Playwright job on this head is green. [A][K]

CI — checked per job myself

Run 37399292352 on f7acaf8c:

Job	Result
Go Build & Test	pass, 15m4s
Playwright E2E Tests	pass, 20m10s
Build & Publish Docker Image	pass, 58s
Release Artifacts / Deploy Staging / Publish Badges	skipped (expected for a non-deploy ref)

Neither of the known flakes (#256 Hash Stats sort, #267 backfill write-hold) surfaced; no rerun was needed. The PR body's "GitHub Actions CI on this PR has not been inspected from this session yet" is now stale — the later report comment has the per-job table and it matches what I see.

Not verified

• [K] The identity of the single red cmd/server run. Output was truncated before I captured the failing test name; 6 subsequent green runs on the merged tree, 3 green on plain master, and 50/50 green on the three new tests leave it unattributed. Worth watching, but it is not evidence against this PR, which adds only deterministic tests.
• [K] The local Playwright suite past its first failure — that failure reproduces on plain master, so I relied on the green CI Playwright job for E2E coverage.
• [K] Whether /api/docs actually renders with egress blocked. I confirmed the CDN-only asset references in the served HTML, not an offline browser load.
• [K] Staging or production behaviour — no staging access was used, and no API key.
• [K] internal/* modules and cmd/migrate/cmd/decrypt test suites — untouched by this diff.

Evidence legend: [T] automated test I ran, [A] manual/live check I ran, [K] known/accepted gap.