Code review: origin/dev..feat/admin-and-merchant-portals

• Scope: 182 commits, 568 files, 37,969 insertions, across three working days. Reviewed at high.
• Reviewer: the merchant lane. None of this is my own work. My branch did not daisy-chain, so nothing of mine is in this diff.
• Date: 2026-08-28. Branch tip moved during the review (5fb2bd11 to ada357b4); the new commit is docs-only and changes no finding.
• Not fixed, by instruction. Every finding is routed to the lane that owns it.

What should be looked at first?

1. The PM-lane guard cannot execute at all. .claude/hooks/guard-pm-lane.sh

• The file is committed mode 100644. All seven sibling hooks are 100755, and .claude/settings.json:101 invokes it by path exactly as it invokes them.
• Verified at the target ref:

100755  block-pr-to-main.sh      100755  guard-em-dash.sh
100755  enforce-pr-draft.sh      100755  guard-git-branch.sh
100755  guard-backtick-body.sh   100755  guard-inline-secrets.sh
100644  guard-pm-lane.sh   <-- the only one            100755  guard-pr-ready.sh

• On any fresh clone the new PM lane guard never runs, and the guard the operator specified as always on is silently absent. The guard's own logic is otherwise sound.
• This outranks finding 2. A guard with a hole still runs; this one does not run.

2. The PM-lane guard fails OPEN on brace expansion. tooling/scripts/guard-pm-lane.mjs:245

• The brace check is /\s|$/.test(...), unanchored. $ matches at the end of every string, so the test is true for any input, and a { or } at a word start unconditionally ends the segment and discards the operand after it.
• Verified by running the module against real hook payloads, with controls:

Command	Result
rm -rf tooling/foo	BLOCKED, correctly
rm -rf {tooling,apps}/foo	ALLOWED

• The intent was /^(\s|$)/. This is a guard that stops the plain form of a destructive command and passes the braced form of the same command.
• It is not in the documented gap table in docs/projects/pm-lane-guard/implementation.md, which lists interpreters, wrappers, archives and quoted command substitution but not brace expansion, and no test covers it.

3. A capped Firestore page is rendered as the venue total. apps/admin/src/admin/venues/VenueMetricStrip.jsx:49

• VenuesPage.jsx:73 streams query(collection(db,'venues'), orderBy('nameLower'), limit(200)). venueMetrics.js:88 sets const total = venues.length, and liveShare divides by it.
• With 640 venues, the strip reads "Venues 200", the live share is a percentage of the wrong denominator, and "Needs attention" only counts defects inside the first 200 names alphabetically. An admin who works that queue to zero is told the platform is clean while 440 venues were never examined.
• The old page had the same limit(200) and only rendered rows. The new strip is what turns the cap into a stated total.
• This is the third instance of the same class in two days, after the two Dashboard rail counts and the merchant Overview counter. Worth treating as a pattern rather than three bugs.

4. Clearing an address marks it as human-confirmed. apps/admin/src/admin/venues/VenueForm.jsx:236

• The field's onChange sets addressConfirmed to true on ANY change, with the comment "Typing an address IS confirming it. Nobody edits an address to a value they do not believe." That is true for typing and false for deleting.
• On save, if (form.address.trim()) payload.address = ... drops an empty address from the payload, while if (isEdit) payload.addressConfirmed = addressConfirmed still sends true.
• updateManualVenue does not reverse-geocode on update, so the venue keeps its original unverified geocoder guess and is now stamped confirmed. The "Approximate" badge disappears and it leaves the Needs-attention count.
• The field's own hint invites the action: "Leave blank to auto-fill from the coordinates."
• There is a server-side half of the same root cause. services/api/venues/src/services/venue.service.js:319 invalidates addressConfirmed when the pin moves and no address is sent, but the form ALWAYS sends an explicit addressConfirmed on every edit, which a later branch applies unconditionally. So correcting only the coordinates of a Confirmed venue keeps it confirmed for a location nobody vouched for, which is exactly what that comment says must not happen. The two halves want fixing together.

5. A saved query loaded on one console tab is overwritten by a save on the other. apps/admin/src/admin/analytics/console/ConsolePanel.jsx:187

• ConsolePanel stays mounted across the BigQuery and Firestore tab switch, and loadedQuery is neither cleared nor checked for source.
• Load a SQL query, switch to the Firestore tab, build a query, save: the dialog sees loadedQuery truthy, treats it as an update, and rewrites the SQL query's document with source: 'firestore' and a stale sql field. The original saved query is gone.
• The reverse direction is the same bug. Related: ConsolePanel.jsx:283 passes saved.queries to the BigQuery rail unfiltered, so a Firestore inquiry appears there and loading it blanks the SQL editor.
• This one is a data-loss path, which is why it is in this tier despite being the least likely to be hit by accident.

What else is real?

#	Where	What
5	analytics/systemHealth.service.js:1110	The uptime rollup sets status only from summary.down, and a timeout increments summary.degraded, so all nine endpoints timing out still reports operational
6	analytics/systemHealth.service.js:908	nextPageToken is discarded and truncation is inferred from entries.length >= 200, so the Errors tab states a capped number as an exact count
7	shared/lib/analyticsApi.js:50	A non-JSON 200 is returned as the payload, so an HTML shell becomes {detail, raw:true} and the retention panel reports "BigQuery returned no base tables" as a finding. merchantsApi.js was hardened for exactly this in the same PR; this was not
8	analytics/config/retentionModel.js:61	Downgraded, see the correction below. The ?? mechanism is real but its trigger is unevidenced, and the SERVER has the identical ?? at bqSchema.service.js:130, so the flaw is duplicated rather than normalized away
9	console/useFirestoreConsoleState.js:245	parseScalar coerces any digit-only filter text to a Number, so merchantId == 1234567890 queries a number against a string field and renders a confident empty grid
10	guard-pm-lane.mjs:284 and :314	The same guard cries wolf in two ways: a trailing # comment blocks on ["#","scaffold","the","project"], and git commit -m"..." blocks on a path that does not exist. Both verified by running it. A gate that cries wolf gets switched off

What is lower priority but true?

• tooling/scripts/lint.docs-format.js:451: --update-baseline writes the baseline from only the skills processed in this run, so combining it with --skill deletes every other skill's grandfathered counts. The CI-skip path at line 410 already merges correctly; this path does not.
• admin/dashboard/useDashboardRail.js:42: load() never sets loading: true, so the reload guard is dead and a slow first request can overwrite a newer snapshot.
• tooling/scripts/lantern.mjs:208: one catch spans statSync and readFileSync, so an existing-but-unreadable credential file is reported as missing, with a fix suggestion that cannot fix a permissions problem.
• tooling/scripts/lint.firestore-allowlist.js:372: the "Verified" lines and the allowlist count print unconditionally, including where a source could not be parsed. The exit code is right; the report asserts checks it did not run.
• console/useFirestoreConsoleState.js:69: the schema effect unconditionally resets the limit, so a saved inquiry loaded before the schema settles has its limit silently replaced.
• m2.e2e.mjs sits at the repo root: a one-off Playwright harness that belongs under its project's harness/. Moved to harness/m2.e2e.mjs on 2026-08-30.

What was checked and found clean?

• firestoreQuery.service.js end to end: allowlist, zod route schema, operator and array validation, error classification. No firestore.rules changes anywhere in the diff.
• bqSchema.service.js, assistant tools.js / anthropic.js, docs pathValidator (traversal and secrets still denied), auth adminUsers.js profile-completeness switch, venues addressConfirmed write paths on the server.
• The billing to financials move: same directory depth so relative imports resolve, no stale importer, and both baselines were re-keyed to the new paths, which is the rename trap AGENTS.md warns about, handled.
• AdminShell: every removed icon import has zero remaining references, all four old-URL redirect families resolve ahead of the catch-all, and embedded is a real prop on both dashboards. No route went missing, which was the named risk.
• releaseFeed.js parsers traced against the real changelog corpus. reportModel.js, schemaModel.js, venueMetrics.js arithmetic, savedQueries.service.js, Scaffold, EmptyValue, PageHeader.
• lint.firestore-allowlist.js run and passing, the three workflow quoting fixes, the lint.no-em-dash exclusion anchor, .storybook-admin mock export parity with the real firebase.js.
• Analytics, docs and assistant test suites pass.

What did this review NOT cover?

Said plainly, because a review that does not name its gaps is worth less than one that does.

• guard-pm-lane.mjs (865 new lines) and lint.docs-format.js (479) got a targeted pass, not a line-by-line read. The findings above came from probing specific behaviours, so treat the rest of those files as unreviewed.
• The four large rewrites got a diff read rather than a line-by-line pass: AdminShell.jsx, VenuesPage.jsx, AdDeliveryDashboard.jsx, VenueActivityDashboard.jsx.
• render-agenda.mjs and bootstrap-env.mjs were not reached.
• Almost everything here is a code read. I drove a browser for my own lane's work all day and did not for this diff. Findings 2, 3 and 4 are the ones a running app would confirm or kill fastest, and I would want that before anyone acts on 3.

Two corrections to my own findings

Finding 8 was overstated and a fourth reviewer caught it.

• I confirmed the MECHANISM by reading: retentionModel.js:61 uses ??, which only falls through on null, while the function's own comment says it must read both places.
• I did not check whether the triggering shape ever OCCURS. A fourth reviewer did, against the branch's own real-warehouse fixture: BigQuery echoes the top-level flag and it agrees with the nested one in all eight tables captured. So the scenario has no evidence behind it.
• Their conclusion needed one correction of its own. They called it moot because bqSchema.service.js:130 normalizes the field first. It does not normalize it: that line uses the same ??, so the server would get the same answer wrong in the same way. The honest statement is that the flaw exists in two layers and its trigger is unevidenced, not that one layer protects the other.
• Confirming a mechanism is not confirming a defect, and I reported one as the other.

A correction to my own first report

My initial pass reported that three delegated sub-reviews "returned nothing" and that everything in it was mine alone. That was wrong: all three returned in full afterwards, and findings 2, 3, 4, 7, 8 and the lower-priority items came from them.

I re-verified the ones I have put in the top tier myself rather than relaying them: the guard by running it with controls, the venue total, the address flag and the non-JSON 200 by reading the code at the target ref. Findings 5, 6 and 9 I found independently, and 9 was found twice, which is the only corroboration in here worth anything.

The first attempt at verifying the guard passed a bare string to decide(), which takes a hook payload, and every case came back ALLOWED including the control that should have blocked. A harness that cannot reproduce its own control has not tested anything, and that result is excluded rather than reported.