The branch review's six findings, and the one that blocked the merge

• Status: all six FIXED and pushed on feat/admin-and-merchant-portals, 2026-08-27. Jobs 1, 2, 3 and 5 verified in a real browser; jobs 4 and 6 verified by tests that fail against the old code.
• Source: a review of the 97-commit session branch returned eleven findings. Six were introduced by that day's work and are the ones recorded here. The other five are being filed separately and were not touched.
• Pull request: #976, the session draft. No new PR was opened.
• Commits, one per finding:
  • 31b8b2bb the confidentiality leak (BLOCKING)
  • 9172fcf3 the Financials Costs tab false error
  • 6a1226a4 the dead /admin/billing route
  • efe676be the unreachable cold-start allowance
  • 3423f6d0 the Errors 24h card understating its cap
  • 540cbe73 two rail counts rendering a cap as a total

What was the blocking finding?

The admin build shipped internal documents as publicly readable static assets.

• The Dashboard reading column pulled its two sources in at BUILD time: docs/business/launches/README.md through ?raw, and every docs/changelogs/dev/*.md through a lazy import.meta.glob.
• Admin deploys to Cloudflare Pages, which serves /assets/* as static files before any JavaScript authentication runs. This repo's own record at docs/engineering/testing/runs/onboarding-ai-prompt/README.md:66 has the operator confirming admin.dev.ourlantern.app returns 200 with no Access redirect.
• So the launch stage table, the December 2026 and Summer 2027 targets, the business-plan and ideation-matrix document links, and every engineering changelog since January were readable by anyone who could guess or crawl an asset path.

It was found against the build, not the source, and that is how it was closed.

• Before: the canonical business-plan Doc URL was present in apps/admin/dist/assets/AdminShell.BSJBOr1m.js, and dist/assets/ held 66 dev-MM.DD.YYYY-v*.js chunks.
• After a clean rebuild (rm -rf apps/admin/dist && npm run build -w apps/admin):

Probe against apps/admin/dist/	Before	After
Business-plan Doc URL (1NiVG3pw...)	1 file	0
Ideation-matrix Doc URL (1pc8nHUU...)	present	0
dev-MM.DD.YYYY-v*.js chunks	66	0
Stage-table prose (Test if an idea works)	present	0
We are in Prototype, Summer 2027, December 2026	present	0
Changelog body text (All notable changes)	present	0
Any raw markdown table (|---|---|)	present	0
Total files in assets/	92	26

• The only two strings that remain are the request PATHS, docs/changelogs/dev and docs/business/launches/README.md, in the AdminShell chunk. That is what a fetch target is, not content.

What shape was chosen, and why?

Runtime fetch through the docs API, which already serves these exact files behind a role gate.

• services/api/docs serves markdown out of the repo with requireRole('viewer') on every read, and the portal already has a client for it in apps/admin/src/shared/lib/docsApi.js, used by the docs editor.
• So the content moves channel rather than going away. The reading column is unchanged, which was the constraint: it is the substance of the redesign, and deleting it was not a fix.
• The alternative shapes were worse for obvious reasons: restating the stage table in code reintroduces the drift the parsing exists to prevent, and a build-time redaction still ships a bundle whose only defence is that nobody looked.

The one piece of invention is a ?dir parameter, and it exists for cost, not for the leak.

• GET /api/documents listed recursively from docs/ with no way to narrow it. The docs tree is 252 directories, and listFiles costs one GitHub getContent call per directory, so an unscoped listing is ~250 requests to display six changelog days.
• dir defaults to docs, so the docs editor is unaffected. The Dashboard asks for docs/changelogs/dev and pays one call.
• The client still filters what comes back. A server that predates the parameter ignores it and answers with the whole tree, which is slow but correct. listReleaseFiles therefore checks the folder as well as the filename, and that check has its own test.

Listing got its own validator rather than reusing the file one.

• validatePath(path, { allowFolder: true }) carries a PROTECTED_FOLDERS guard whose message is "Cannot delete protected folder", and docs/changelogs is in that list. Listing is a read, so refusing it with a deletion error is the wrong answer to the wrong question.
• validateDirectory shares the denied patterns (traversal, secrets, env files, keys) and the folder depth limit, and drops the delete guard. Six tests cover it.

The docs service had no validate script and no entry in the gate, so its path validator was ungated.

• Fixed in the same commit (rule 18): test:run and validate added to its package.json matching its five sibling services, plus an entry in tooling/scripts/validate.js. npm run validate -- --workspace services/api/docs now reports Docs API Validation PASS.

What was found while verifying it?

The reading column takes 28 seconds to fill until this branch deploys, and that is expected.

• Measured on her stack: launch stages render at +1.1s, the release stream at +28.6s.
• The request URL in the run is .../api/documents?dir=docs%2Fchangelogs%2Fdev, and the DEPLOYED docs-api ignores the parameter and walks all 252 directories. That is the graceful-degradation path working exactly as designed.
• It resolves on deploy, when the same call becomes one GitHub request. Nothing else on the page waits for it: the stage block, the rail and the nav are all live in about a second, and the stream shows "Reading the changelog." meanwhile.

What were the other five?

The Financials Costs tab painted a red error on every open.

• InfraCosts branched on healthError alone, but on first paint healthData and healthError are BOTH null, so the loading case fell into BillingMetrics, whose if (!data) returns "Failed to load billing metrics".
• The guard existed and was dropped in the move: SystemHealth had if (loading && !healthData). Financials returns this tab BEFORE its own loading gate, deliberately, so the gate has to live in InfraCosts.
• It tests the payloads and not just the flag, so a refresh with data on screen keeps the data instead of flashing a spinner. The loading flag is released before the OTP call, since the billing cards are ready as soon as health lands.
• BillingMetrics keeps a message for the case it is actually left with, a health payload carrying no billing section, but now says that instead of claiming a fetch failed. That case is permanent, so the wrong text was on screen forever.

The Billing to Financials rename left /admin/billing dead.

• No billing route and no redirect, so the catch-all sent every /admin/billing* URL to the Dashboard with no explanation. App.jsx's old-URL map cannot help: it only rewrites paths that predate the /admin prefix.
• Follows the pattern the same change already used for its sibling, where bigquery/console kept a Navigate with a comment saying saved links point at it.
• Financials keeps its tabs in component state rather than in the URL, so there is no sub-path to preserve and every billing/* URL lands on the same place.

The 12s cold-start allowance was unreachable, and its test could not see that.

• slowMs: 12000 sat behind a hardcoded 10000 abort, so anything between the two was killed and recorded as a timeout. The measured 9.2s assistant-api cold start had 800ms of headroom rather than the 2.8s intended, and the slowMs branch could only fire between 3000 and 10000. The 12000 was dead code.
• The abort was raised, not the bar lowered. Lowering the bar to 10000 leaves the worst observed cold start 800ms of room and reintroduces exactly what the bar was raised to fix: an 8s bar flagged assistant-api on every cold call. Raising the abort to 15000 leaves a real 12s to 15s degraded band.
• It costs nothing in worst-case latency. getSystemHealth already runs getUptimeMetrics in the same Promise.all as getServerErrorsMetrics, whose Cloud Logging call carries a 15 second timeout, so the payload's worst case was already 15s.
• Both numbers are named constants per endpoint class now, so the pair that has to agree is visible in one place.

Two Dashboard rail counts saturated at 100 and rendered as totals.

• Both queues took .length off a list the server had already cut. The hook's own header says a missing number is not zero and it handles null correctly; a capped number is the other half of that idea and was unhandled.
• Rows carry truncated alongside value, and the rail renders a saturated count as 100+ with an at least 100 aria-label, the same marker System Health already uses for its capped error counts.

The Errors 24h card under-reported while the 7d card beside it admitted its cap.

• One pageSize: 200 page backed last24h, last7d and bySource, and a single truncated flag was rendered on the 7 day card only. At 500 errors an hour both cards read 200 and one claimed to be a count.
• Every figure carries its own marker now, and the 24h one is computed rather than copied. The page is ordered newest first, so even a full page holds every error newer than its oldest entry: the 24h count is exact whenever the page reaches back past the boundary, and a floor only when the cap bit inside it.
• bySource gets a flag and a section subtitle, because a capped page can omit whole sources as well as undercount the ones it lists.

What in the review did not hold up?

"The offers route already returns a count; use it" does not work.

• Both handlers return count: array.length, computed AFTER the limit was applied (adminOffers.js and adminMerchantApplications.js). It saturates identically to the array and carries no more information.
• The only signal available without paginating is comparing the length against a limit the CALLER chose, so the hook now passes limit explicitly instead of relying on two different defaults it cannot see. That needed limit forwarding through merchantsApi.listOffersForReview, which only ever sent status.
• All six were real defects. This was a wrong remedy, not a wrong finding.

How was it verified?

One signed-in browser pass on her stack, http://localhost:3001 (HERS, untouched).

• One signin for the whole run. Both portal signin routes share a single 10-per-15-minutes-per-IP bucket, and a limited run lands on the login screen reporting zeros that read as a clean pass, so the run asserts it is signed in before it checks anything.
• Post-login URL every run: http://localhost:3001/admin, signed in as Agent Probe (Administrator).
• storageState cannot cache this session. Firebase Auth persists in IndexedDB, which Playwright does not capture, so a "reused" context silently lands on the login screen. That is the false clean pass the constraint warns about, and it happened once during this work before the script was changed to sign in every run and assert the result.

Route loaded	Post-login URL reached	What it showed
/admin	http://localhost:3001/admin	4 stage rows (Prototype WE ARE HERE, Alpha, Beta, 1.0), 6 release days, 16 changelog entries, no error text
/admin/billing	http://localhost:3001/admin/financials#overview	heading "Financials"
/admin/billing/costs	http://localhost:3001/admin/financials#overview	heading "Financials"
/admin/financials then Costs tab	http://localhost:3001/admin/financials#costs	loading state at 250ms, zero "Failed to load billing metrics" at every sample across 2s and after settle, 12 cost cards
/admin with a 100-offer response	http://localhost:3001/admin	request carried ?status=pending_review&limit=100; rail read Offers to review 100+ with aria-label: at least 100, Merchant applications 7 bare

The saturated queue was driven with an intercepted response, and that is the honest way to read it.

• Live dev has fewer than 100 pending offers, so the cap cannot be reached with real data. Playwright fulfilled the offers request with 100 rows, which exercises the real component in a real browser.
• Her local stack cannot serve that call at all: VITE_MERCHANTS_API_URL=/api makes the path /api/admin/offers, and the Vite proxy has /api/merchants and /api/offers but no /api/admin, so the dev server answers with the SPA shell. That is a pre-existing local-only gap the rail already handles (it renders "Not available"), not something this work introduced.

Jobs 4 and 6 are backend and cannot be seen in her portal at all.

• The admin portal proxies to DEPLOYED Cloud Run, so a local analytics-api edit can never appear on 3001. Both are covered by tests instead, and both tests were run against the OLD code to prove they catch the defect:
  • Job 4: fails with the abort back at 10000, expected 'timeout' to be 'healthy'. The pre-existing cold-start test could not do this because it mocks Date.now and resolves fetch immediately, so no AbortController is ever involved; the new case uses a fetch mock that honours the signal and lands at 11s.
  • Job 6: two cases covering the fork, 200 errors all inside the last hour (24h count is a floor) and 200 whose oldest is five days back (24h count is a fact).
• The /admin/billing redirect tests were also run against the routes removed, and fail.

What is left?

• The 28 second release stream resolves itself on deploy and needs no work, but it is worth a look on the post-deploy pass to confirm it drops to one call.
• The docs service is in the validate gate now but has only the path-validator tests. Its routes have no coverage and there is no supertest dependency in that workspace, so adding route-level tests is its own piece of work.
• /api/admin/offers has no Vite proxy entry, so the offer-review rail count and the offer review queue cannot be exercised on a local portal. Pre-existing, worth an issue.