Repository navigation
Account-scoped verbose diagnostics for App Review (cmuxVerboseDiagnostics) - #11367
Open
azooz2003-bit wants to merge 14 commits into
Open
azooz2003-bit wants to merge 14 commits into
azooz2003-bit wants to merge 14 commits into
Conversation
…he session user The App Review demo account is flagged server-side with clientReadOnlyMetadata.cmuxReviewDemoContent = true (server-writable only). CMUXAuthUser now carries demonstrationContentEnabled, parsed fail-closed (only a JSON boolean true counts) inside the Stack user actor so the non-Sendable metadata dictionary never crosses isolation. The flag rides the session payload the app already fetches at sign-in and persists with the cached identity card; identity cards from older builds decode as not flagged. MobileIdentityProviding exposes it to the shell with a false default, and the app's AuthCoordinatorIdentityProvider reads it off the live session. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MobileDemoContentCatalog is the canned computer behind demonstration mode: one Demo Mac (localized name, en+ja) with three realistic developer workspaces (agent task rows with varied todo statuses, unread state, ANSI session transcripts) and five sample notification-feed items, all built as the ordinary MobileWorkspacePreview / MobileNotificationFeedItem shapes so they flow through production stores unchanged. Identifiers are cmux-demo- prefixed so they can never collide with a real Mac, and a test pins the sample content against internal build-lane vocabulary (Guideline 2.2). MobileDemoTerminalEngine answers demo terminal input deterministically: canned history replays on mount, typed characters echo, backspace erases, Ctrl-C aborts, escape sequences are swallowed, and Enter runs a small command table (ls, pwd, git status, echo, cat, clear, date, ...) ending in a fresh prompt, so the reviewer gets an interactive terminal with no network. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
While the signed-in account carries the server demonstration flag, the shell seeds a Demo Mac through exactly the paths a live Mac uses, so every view renders it unchanged and real Macs keep connecting alongside it: - DemoContentPairedMacStore overlays one MobilePairedMac row outside the build-scope/team/backup stack (never persisted, never backed up, mutations addressed to it swallowed); the Computers list, reconnect routing, and the known-paired-Mac hint all see it through the ordinary loadAll path. - workspacesByMac gains one connected MacWorkspaceState entry, so the multi-Mac aggregation derives its rows and the list chrome reads connected; the notification feed gains one per-Mac snapshot ingested and read-state-mutated through the same revisioned paths as a live feed. - Terminal replay funnels (cold attach and the requestTerminalReplay choke point every resync/reset path shares) answer demo surfaces from the local engine and release any replay barrier; input funnels (raw key bytes, composer submit, explicit sends) route demo surfaces to the engine, whose echo rides the same per-surface output stream. Terminal lanes skip demo surfaces so a real foreground Mac is never asked about them. - switchToMac treats the demo pairing as instantly available without touching the live foreground connection; opening a demo row applies the read receipt in memory. Sign-out and flag-off tear every seed down. Activation is re-evaluated on every shell auth sync, so it follows sign-in, session restore, and account switches with no launch flag, build lane, or hardcoded account identifier in the client. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…w docs
services/account/reviewDemoContent.ts owns the server-side write following
the cmuxPlan/cmuxVmPlan metadata pattern: set cmuxReviewDemoContent: true on
the demo account's clientReadOnlyMetadata (remove the key to disable),
preserving every other key and skipping no-op updates. Operate it with
bun scripts/set-review-demo-content.ts <email> on|off
using production Stack server credentials. The reviewer-setup and
review-notes docs describe the mode, the pre-submission check, and that the
prepared review Mac stays online: demonstration content augments live
pairing, it does not replace it. Nothing here touches the live demo account;
flagging it is a deliberate post-merge operator step.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e auth sync (red) Live dogfood repro (PR #11289): a launch that mounts already authenticated syncs the shell against the CACHED identity card, which predates the flag and decodes as not-flagged; session revalidation later publishes the fresh flagged user WITHOUT an isAuthenticated edge, so activation is never re-evaluated. The paired-Mac decorator reads the flag lazily on every load, so the Demo Mac row rendered ('Not connected · 0 workspaces') while the workspace and notification seeds never landed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…arrives late Root cause of the dogfood defect (Demo Mac row visible with 'Not connected · 0 workspaces', zero demo workspaces/notifications): the shell evaluated demonstration activation ONLY inside signIn() auth syncs. A launch that mounts already authenticated syncs against the cached identity card, which predates the flag and decodes as not-flagged; validateCachedSession later publishes the fresh flagged user via applySignedInUser(.revalidation), which updates currentUser WITHOUT an isAuthenticated edge, so no root onChange re-fires the sync and the seeds never land. Meanwhile DemoContentPairedMacStore reads the flag lazily on every loadAll, so later list loads revealed the row — diverging from the missing seeded state. Two-part fix: - CMUXMobileRootView observes the published user's demonstration flag and re-runs the ordinary shell auth sync on its edge, so revalidation activating the flag seeds immediately (deterministic trigger). - loadPairedMacs() re-evaluates activation (without re-entrant reload) before reading the store, so ANY load that can reveal the demo row also seeds with it — row visibility and seeded state can no longer diverge, and reconnect churn self-heals the seeds. The new seedsSurviveReconnectAndAggregationChurn test also pins that a full secondary reconcile pass and repeated list loads retain the seeds idempotently (one row, three workspaces, unchanged feed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…(red) Dogfood round 2 (PR #11289): with the real Mac wedged, every failed stored-Mac dial funnels into clearRemoteConnectionContext(), which drops all non-foreground workspacesByMac entries — including the demo Mac's. Real secondaries are re-established by the next aggregation pass; the demo Mac has no subscription, so each ~85s reconnect cycle erased the demo workspaces (five terminalClosed events per cycle in cmux-app.log), and a tap racing the wipe bounced back to an empty 'Not Connected' list. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…demo Mac Mechanism of the round-2 defect (from cmux-app.log on the live sim): with the real Mac wedged, each storedMacReconnect attempt fails and funnels into clearRemoteConnectionContext(), whose teardown keeps only the offline FOREGROUND entry of workspacesByMac — dropping the demonstration Mac's seeded entry every ~85s cycle (surfaceListUpdated 0 + five terminalClosed per cycle). A tap racing that wipe resolved no row after switchToMac and bounced back to an empty Not Connected list; the loadPairedMacs self-heal restored the seeds only when a list refresh happened to run. Demo state is now invariant to ALL real-connection churn at the shared choke points, instead of per-caller patches: - clearRemoteConnectionContext retains the demo entry (it has no subscription or transport; nothing else re-establishes it) and never downgrades its connected presentation. - The stored-Mac reconnect loop never admits the demo row as a dial candidate (nothing to dial; registry refresh and dial-failure cleanup must not touch it). Zero-touch and secondary-aggregation candidate selection exclude it explicitly instead of relying on presence contents. - markSecondaryMacUnavailable and refreshRoutesFromRegistry no-op for the demo pairing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ingle-file command catalog Owner direction for the reviewer scenario: every demo workspace opens onto a simulated terminal we control — canned transcript plus a live prompt that fakes ls, cd, echo, and whatever we want to showcase. - cd joins the engine with a coherent fake filesystem per terminal (MobileDemoDirectory trees: src/, tests/, README.md, nested dirs), so cd/ls/pwd/cat compose: 'cd src && ls' lists plausible files, 'cd ..' and 'cd ~' return, unknown dirs answer 'no such file or directory', and the prompt tracks the working directory across replays. - MobileDemoCommandCatalog is THE single extension point for showcase commands (echo, git, date, whoami, hostname, uname, help today): add one entry there and every demo terminal serves it, no engine changes. The engine keeps only line discipline and the session-stateful filesystem commands (cd/ls/pwd/cat/clear). - All three demo workspaces are proven interactive end to end through the production open/mount/input paths (everyDemoWorkspaceOpensAnInteractive- SimulatedTerminal exercises each workspace's every terminal: replay, echo, pwd answer, fresh prompt). Engine tests cover cd/ls/pwd/cat composition, nested paths, .., ~, error copy, and the catalog-as-extension-point contract; content stays realistic English developer material with the vocabulary pin extended over the trees. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r send entries (red) Dogfood round 3 on the rebuilt sim: the workspace opens but the canvas stays blank forever (render tick sinceOutput=-1 — not one byte reached the mounted surface), and the composer shows 'Couldn't send. Check the connection and try again.' Mechanism 1: GhosttySurface's output consumer is gated on the first geometry callback producing a viewport preparation (prepareTerminalViewport), which resolves the workspace through the foreground-scoped workspaceID(forTerminalID:). Demo rows are stamped with the demo mac id and the demo Mac is never foreground, so the resolver returns nil, the gate never opens, the sink never registers, and the mount-time replay never runs. Earlier tests attached the stream directly and bypassed the gate; the new test drives the real order (geometry preparation, viewport answer, THEN sink attach, THEN replay). Mechanism 2: the composer band submits through submitComposerInput / submitComposer -> terminal.paste on the foreground remoteClient, which is nil while only the demo Mac serves content, so the send fails its connection gate before reaching the demo input fence and the red banner shows. The new test drives those exact entries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rminals Blank canvas (sinceOutput=-1): the mounted view's output consumer waits on the first geometry callback producing a viewport preparation, and prepareTerminalViewport resolves the workspace through the foreground-scoped workspaceID(forTerminalID:), which returned nil for demo surfaces — gate never opened, sink never registered, replay never ran. workspaceID(forTerminalID:) now resolves demo surfaces to their demo-owned row regardless of the foreground pairing (demo ids are cmux-demo- prefixed, so the sibling-build ambiguity that scoping defends against cannot occur), and updatePreparedTerminalViewport answers demo reports locally with the phone's natural grid — before the replay-barrier prearm, so no barrier is ever armed against a demo surface, and without an RPC, so the view's bounded retryViewportReport loop settles instead of spinning on nil. 'Couldn't send' banner: every composer route funnels into sendRemoteTerminalPaste, which required the foreground remoteClient. Demo surfaces now branch there to the local engine (text pasted, interior newlines execute per line, the return submit key runs the final line), and submitComposerInput's connection gate admits demo-owned terminals, so the send status reports success and the banner never shows. Demo replays are additionally prefixed with a screen+scrollback erase so a re-delivered replay (view reset, viewport churn, remount) repaints from blank instead of appending a second transcript copy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…oseDiagnostics When an authenticated request's Stack user carries the server-written clientReadOnlyMetadata flag cmuxVerboseDiagnostics: true, verifyRequest emits a structured [cmux-verbose-diag] request line (method, path, query keys, user id, user agent) into the Vercel runtime logs, annotates the active OTel span for Axiom trace filtering, and withApiRouteSpan emits a completion line with route, status, duration, and error details. Routes without the span wrapper get a fallback end line via next/server after. The flag rides the AuthedUser projection so the native auth cache keeps it, and only the literal boolean true counts (fail closed). /api/diagnostics/ingest accepts batched client diagnostic events from flagged accounts: auth is required and the server re-checks the account flag (double gate), the body and every field are bounded, control characters are stripped so one event is one log line, and each event becomes a [cmux-verbose-diag] client_event line. bun scripts/set-verbose-diagnostics.ts <email> on|off owns the metadata write, preserving every other key, mirroring set-review-demo-content. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d accounts CMUXAuthUser gains verboseDiagnosticsEnabled, parsed fail-closed from the server-written cmuxVerboseDiagnostics clientReadOnlyMetadata key exactly like the demonstration-content flag, persisted with the cached identity card, resolved inside the Stack CurrentUser actor, and exposed through MobileIdentityProviding (default false). VerboseDiagnosticsReporter (CmuxMobileAnalytics) fans out of the DiagnosticLog event tap alongside AppLog and TransportSentryReporter. ingest is nonisolated and returns after one synchronized boolean read while the account is unflagged, so ordinary users pay nothing and buffer nothing. Accepted events batch on a single consumer task (AsyncStream channel, barrier flush, injected clock cadence, hard-capped drop-oldest backlog) and POST to /api/diagnostics/ingest with the Stack bearer and refresh tokens. Batches are removed before upload and never retried; any failure (429, outage, server-gate rejection) opens an outage gate that limits further attempts to the cadence, so the reporter can never block UI or connection paths or queue unboundedly. The composition root mirrors the signed-in account's flag into the reporter with withObservationTracking over AuthCoordinator.currentUser, covering sign-in, session restore, late revalidation arrival, and sign-out, and flushes pending events on background alongside analytics. The server re-checks the account flag on every upload, so the client mirror is never the authority. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
15 issues found across 18 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="web/services/account/verboseDiagnostics.ts">
<violation number="1" location="web/services/account/verboseDiagnostics.ts:50">
P3: This module duplicates `reviewDemoContent.ts`'s metadata mutation logic nearly line-for-line, including the full-object write. Extract a shared flag metadata helper so future changes cannot make the two implementations drift.</violation>
<violation number="2" location="web/services/account/verboseDiagnostics.ts:75">
P2: When the stored flag has a non-`true` value, requesting `off` treats it as already disabled and skips the write, so the stale `cmuxVerboseDiagnostics` key is never removed. Detect the key's presence separately and persist the normalized object when disabling.</violation>
<violation number="3" location="web/services/account/verboseDiagnostics.ts:85">
P1: When a billing, TestFlight, or account-deletion metadata update overlaps this call, `next` comes from a stale snapshot and replaces the whole metadata object, losing the other update. Reload the user under the existing account metadata mutation lock before applying this flag.</violation>
</file>
<file name="web/services/observability/verboseDiagnostics.ts">
<violation number="1" location="web/services/observability/verboseDiagnostics.ts:95">
P2: The flagged request's User-Agent is untrusted client input, but this line sends arbitrary header contents into runtime logs without bounds or scrubbing. Bound and sanitize the value before logging it, or omit it.</violation>
<violation number="2" location="web/services/observability/verboseDiagnostics.ts:108">
P3: recordVerboseDiagnosticsRequest schedules an `after` fallback on every flagged request even when the route is wrapped in `withApiRouteSpan`, which always calls completeVerboseDiagnosticsRequest (success or error) and sets entry.finished. For all current flagged traffic (every VM route and the diagnostics ingest route use withApiRouteSpan), the after() closure is a guaranteed no-op, so each flagged request pays for scheduling a `next/server` after-task that never does anything. Since recordVerboseDiagnosticsRequest runs before the route can signal whether it is span-wrapped, this is inherent to the sync design, but worth noting given flagged requests already burst several /api/vm calls per user action.</violation>
<violation number="3" location="web/services/observability/verboseDiagnostics.ts:150">
P1: When a flagged request fails with an exception containing a signed URL, bearer token, or request detail, this line writes the raw message to Vercel logs. Scrub error text before emitting it, or log only `errorName`.</violation>
</file>
<file name="web/app/api/diagnostics/ingest/route.ts">
<violation number="1" location="web/app/api/diagnostics/ingest/route.ts:108">
P2: A flagged or compromised client can flood Vercel logs because this endpoint has no server-side rate limit and emits one line per accepted event. Add a per-user rate limit or quota before processing the batch.</violation>
<violation number="2" location="web/app/api/diagnostics/ingest/route.ts:185">
P3: boundedString counts code points (`[...value]`, `cleaned.length`) but truncates with `cleaned.slice(0, maxChars)`, which slices by UTF-16 code units. When an astral character (a surrogate pair) straddles the cutoff, the slice returns an unpaired (lone) surrogate. That dangling surrogate is later serialized by emitVerboseDiagnosticsLog's JSON.stringify and becomes a U+FFFD replacement character, silently corrupting the end of long summary/name strings and meaning `maxChars` is not a true code-point cap. Slice on the code-point array instead (e.g. build from `[...cleaned].slice(0, maxChars)`), or guard by trimming the truncated string so it never ends on a lone low surrogate.</violation>
</file>
<file name="Packages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/VerboseDiagnosticsReporter.swift">
<violation number="1" location="Packages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/VerboseDiagnosticsReporter.swift:189">
P2: While `drain()` awaits `uploader.upload`, the consumer cannot trim `pending`, but `ingest` continues yielding into this unbounded stream. A stalled transport therefore bypasses `maxPendingEvents` and can grow memory with every diagnostic event; use a bounded event buffer separate from non-droppable flush/control barriers.</violation>
<violation number="2" location="Packages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/VerboseDiagnosticsReporter.swift:222">
P1: When `setAuthorization(false)` runs with queued events, `.authorization(false)` is placed after those `.event` items. `consume` can reach `drain()` and POST them while `isAuthorized` remains true, so sign-out or flag removal does not stop the upload. Carry an authorization generation into events and re-check or cancel before sending.</violation>
<violation number="3" location="Packages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/VerboseDiagnosticsReporter.swift:261">
P3: After any authorized event starts `cadenceTask`, disabling authorization leaves that task alive and waking the actor every `flushInterval` for an unflagged account. Cancel and clear `cadenceTask` when authorization is disabled, then recreate it when authorization is enabled again.</violation>
<violation number="4" location="Packages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/VerboseDiagnosticsReporter.swift:295">
P2: The reporter ships the entire buffered `pending` array as a single batch (up to the 512-event `maxPendingEvents`), but the ingest route rejects any batch over 256 events with a 400 `batch_too_large`, and 4xx (other than 408/429) maps to `.drop`. Once more than 256 events accumulate between flushes the entire batch is discarded and the outage gate opens. Cap each drain at at most 256 events and chunk any remainder, instead of sending everything at once.</violation>
</file>
<file name="ios/cmuxPackage/Sources/cmuxFeature/MobileAnalyticsComposition.swift">
<violation number="1" location="ios/cmuxPackage/Sources/cmuxFeature/MobileAnalyticsComposition.swift:100">
P3: The verbose diagnostics uploader constructs a new `URLSession` (`session ?? Self.analyticsSession()`) instead of reusing the `uploadSession` already created in this same init for the analytics uploader. This makes three ephemeral sessions instead of two and is inconsistent with the "same short-timeout session policy as the analytics uploader" comment. Reuse `uploadSession` (the shared injected/test session) for the diagnostics uploader.</violation>
<violation number="2" location="ios/cmuxPackage/Sources/cmuxFeature/MobileAnalyticsComposition.swift:103">
P2: The production reporter uses 40-event batches instead of the specified 256-event batches, increasing request overhead and reducing diagnostic coverage per failed upload. Pass `flushBatchSize: 256` when constructing the reporter.</violation>
</file>
<file name="web/scripts/set-verbose-diagnostics.ts">
<violation number="1" location="web/scripts/set-verbose-diagnostics.ts:28">
P2: When the email argument is only whitespace, the script can search with an empty email and toggle an unrelated user whose primary email is empty. Reject the normalized email before calling `listUsers`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| user.clientReadOnlyMetadata, | ||
| enabled, | ||
| ); | ||
| await user.update({ |
There was a problem hiding this comment.
P1: When a billing, TestFlight, or account-deletion metadata update overlaps this call, next comes from a stale snapshot and replaces the whole metadata object, losing the other update. Reload the user under the existing account metadata mutation lock before applying this flag.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/services/account/verboseDiagnostics.ts, line 85:
<comment>When a billing, TestFlight, or account-deletion metadata update overlaps this call, `next` comes from a stale snapshot and replaces the whole metadata object, losing the other update. Reload the user under the existing account metadata mutation lock before applying this flag.</comment>
<file context>
@@ -0,0 +1,89 @@
+ user.clientReadOnlyMetadata,
+ enabled,
+ );
+ await user.update({
+ clientReadOnlyMetadata: next as VerboseDiagnosticsJson,
+ });
</file context>
| userId: entry.userId, | ||
| durationMs: Date.now() - entry.startedAt, | ||
| errorName: outcome.errorName, | ||
| errorMessage: outcome.errorMessage, |
There was a problem hiding this comment.
P1: When a flagged request fails with an exception containing a signed URL, bearer token, or request detail, this line writes the raw message to Vercel logs. Scrub error text before emitting it, or log only errorName.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/services/observability/verboseDiagnostics.ts, line 150:
<comment>When a flagged request fails with an exception containing a signed URL, bearer token, or request detail, this line writes the raw message to Vercel logs. Scrub error text before emitting it, or log only `errorName`.</comment>
<file context>
@@ -0,0 +1,160 @@
+ userId: entry.userId,
+ durationMs: Date.now() - entry.startedAt,
+ errorName: outcome.errorName,
+ errorMessage: outcome.errorMessage,
+ });
+}
</file context>
| return true | ||
| } | ||
| guard changed else { return } | ||
| continuation.yield(.authorization(enabled)) |
There was a problem hiding this comment.
P1: When setAuthorization(false) runs with queued events, .authorization(false) is placed after those .event items. consume can reach drain() and POST them while isAuthorized remains true, so sign-out or flag removal does not stop the upload. Carry an authorization generation into events and re-check or cancel before sending.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/VerboseDiagnosticsReporter.swift, line 222:
<comment>When `setAuthorization(false)` runs with queued events, `.authorization(false)` is placed after those `.event` items. `consume` can reach `drain()` and POST them while `isAuthorized` remains true, so sign-out or flag removal does not stop the upload. Carry an authorization generation into events and re-check or cancel before sending.</comment>
<file context>
@@ -0,0 +1,345 @@
+ return true
+ }
+ guard changed else { return }
+ continuation.yield(.authorization(enabled))
+ }
+
</file context>
| user: VerboseDiagnosticsUser, | ||
| enabled: boolean, | ||
| ): Promise<Record<string, unknown>> { | ||
| if (verboseDiagnosticsEnabled(user.clientReadOnlyMetadata) === enabled) { |
There was a problem hiding this comment.
P2: When the stored flag has a non-true value, requesting off treats it as already disabled and skips the write, so the stale cmuxVerboseDiagnostics key is never removed. Detect the key's presence separately and persist the normalized object when disabling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/services/account/verboseDiagnostics.ts, line 75:
<comment>When the stored flag has a non-`true` value, requesting `off` treats it as already disabled and skips the write, so the stale `cmuxVerboseDiagnostics` key is never removed. Detect the key's presence separately and persist the normalized object when disabling.</comment>
<file context>
@@ -0,0 +1,89 @@
+ user: VerboseDiagnosticsUser,
+ enabled: boolean,
+): Promise<Record<string, unknown>> {
+ if (verboseDiagnosticsEnabled(user.clientReadOnlyMetadata) === enabled) {
+ return metadataApplyingVerboseDiagnostics(
+ user.clientReadOnlyMetadata,
</file context>
| // that must not be copied into a log sink verbatim. | ||
| queryKeys: url ? [...url.searchParams.keys()].sort().join(",") || undefined : undefined, | ||
| userId: user.id, | ||
| userAgent: request.headers.get("user-agent") ?? undefined, |
There was a problem hiding this comment.
P2: The flagged request's User-Agent is untrusted client input, but this line sends arbitrary header contents into runtime logs without bounds or scrubbing. Bound and sanitize the value before logging it, or omit it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/services/observability/verboseDiagnostics.ts, line 95:
<comment>The flagged request's User-Agent is untrusted client input, but this line sends arbitrary header contents into runtime logs without bounds or scrubbing. Bound and sanitize the value before logging it, or omit it.</comment>
<file context>
@@ -0,0 +1,160 @@
+ // that must not be copied into a log sink verbatim.
+ queryKeys: url ? [...url.searchParams.keys()].sort().join(",") || undefined : undefined,
+ userId: user.id,
+ userAgent: request.headers.get("user-agent") ?? undefined,
+ });
+
</file context>
| * non-`true` shape reads as off, so no tombstone value is needed). Every other | ||
| * key is preserved verbatim. | ||
| */ | ||
| export function metadataApplyingVerboseDiagnostics( |
There was a problem hiding this comment.
P3: This module duplicates reviewDemoContent.ts's metadata mutation logic nearly line-for-line, including the full-object write. Extract a shared flag metadata helper so future changes cannot make the two implementations drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/services/account/verboseDiagnostics.ts, line 50:
<comment>This module duplicates `reviewDemoContent.ts`'s metadata mutation logic nearly line-for-line, including the full-object write. Extract a shared flag metadata helper so future changes cannot make the two implementations drift.</comment>
<file context>
@@ -0,0 +1,89 @@
+ * non-`true` shape reads as off, so no tombstone value is needed). Every other
+ * key is preserved verbatim.
+ */
+export function metadataApplyingVerboseDiagnostics(
+ raw: unknown,
+ enabled: boolean,
</file context>
Comment on lines
+261
to
+264
| if !enabled { | ||
| pending.removeAll() | ||
| uploadOutageOpen = false | ||
| } |
There was a problem hiding this comment.
P3: After any authorized event starts cadenceTask, disabling authorization leaves that task alive and waking the actor every flushInterval for an unflagged account. Cancel and clear cadenceTask when authorization is disabled, then recreate it when authorization is enabled again.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/VerboseDiagnosticsReporter.swift, line 261:
<comment>After any authorized event starts `cadenceTask`, disabling authorization leaves that task alive and waking the actor every `flushInterval` for an unflagged account. Cancel and clear `cadenceTask` when authorization is disabled, then recreate it when authorization is enabled again.</comment>
<file context>
@@ -0,0 +1,345 @@
+ }
+ case let .authorization(enabled):
+ isAuthorized = enabled
+ if !enabled {
+ pending.removeAll()
+ uploadOutageOpen = false
</file context>
Suggested change
| if !enabled { | |
| pending.removeAll() | |
| uploadOutageOpen = false | |
| } | |
| if !enabled { | |
| cadenceTask?.cancel() | |
| cadenceTask = nil | |
| pending.removeAll() | |
| uploadOutageOpen = false | |
| } |
| }) | ||
| .join(""); | ||
| if (!cleaned) return undefined; | ||
| return cleaned.length > maxChars ? `${cleaned.slice(0, maxChars)}…` : cleaned; |
There was a problem hiding this comment.
P3: boundedString counts code points ([...value], cleaned.length) but truncates with cleaned.slice(0, maxChars), which slices by UTF-16 code units. When an astral character (a surrogate pair) straddles the cutoff, the slice returns an unpaired (lone) surrogate. That dangling surrogate is later serialized by emitVerboseDiagnosticsLog's JSON.stringify and becomes a U+FFFD replacement character, silently corrupting the end of long summary/name strings and meaning maxChars is not a true code-point cap. Slice on the code-point array instead (e.g. build from [...cleaned].slice(0, maxChars)), or guard by trimming the truncated string so it never ends on a lone low surrogate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/app/api/diagnostics/ingest/route.ts, line 185:
<comment>boundedString counts code points (`[...value]`, `cleaned.length`) but truncates with `cleaned.slice(0, maxChars)`, which slices by UTF-16 code units. When an astral character (a surrogate pair) straddles the cutoff, the slice returns an unpaired (lone) surrogate. That dangling surrogate is later serialized by emitVerboseDiagnosticsLog's JSON.stringify and becomes a U+FFFD replacement character, silently corrupting the end of long summary/name strings and meaning `maxChars` is not a true code-point cap. Slice on the code-point array instead (e.g. build from `[...cleaned].slice(0, maxChars)`), or guard by trimming the truncated string so it never ends on a lone low surrogate.</comment>
<file context>
@@ -0,0 +1,186 @@
+ })
+ .join("");
+ if (!cleaned) return undefined;
+ return cleaned.length > maxChars ? `${cleaned.slice(0, maxChars)}…` : cleaned;
+}
</file context>
Suggested change
| return cleaned.length > maxChars ? `${cleaned.slice(0, maxChars)}…` : cleaned; | |
| return cleaned.length > maxChars | |
| ? `${[...cleaned].slice(0, maxChars).join("")}…` | |
| : cleaned; |
| uploader: HTTPVerboseDiagnosticsUploader( | ||
| apiBaseURL: apiBaseURL, | ||
| tokenProvider: AnalyticsTokenProviderBridge(tokenProvider: tokenProvider), | ||
| session: session ?? Self.analyticsSession() |
There was a problem hiding this comment.
P3: The verbose diagnostics uploader constructs a new URLSession (session ?? Self.analyticsSession()) instead of reusing the uploadSession already created in this same init for the analytics uploader. This makes three ephemeral sessions instead of two and is inconsistent with the "same short-timeout session policy as the analytics uploader" comment. Reuse uploadSession (the shared injected/test session) for the diagnostics uploader.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ios/cmuxPackage/Sources/cmuxFeature/MobileAnalyticsComposition.swift, line 100:
<comment>The verbose diagnostics uploader constructs a new `URLSession` (`session ?? Self.analyticsSession()`) instead of reusing the `uploadSession` already created in this same init for the analytics uploader. This makes three ephemeral sessions instead of two and is inconsistent with the "same short-timeout session policy as the analytics uploader" comment. Reuse `uploadSession` (the shared injected/test session) for the diagnostics uploader.</comment>
<file context>
@@ -83,6 +89,19 @@ public struct MobileAnalyticsComposition {
+ uploader: HTTPVerboseDiagnosticsUploader(
+ apiBaseURL: apiBaseURL,
+ tokenProvider: AnalyticsTokenProviderBridge(tokenProvider: tokenProvider),
+ session: session ?? Self.analyticsSession()
+ ),
+ buildStamp: DiagnosticBuildStamp.make(infoDictionary: Bundle.main.infoDictionary),
</file context>
Suggested change
| session: session ?? Self.analyticsSession() | |
| session: uploadSession |
| // end line (duration only; the response status is not observable here) once | ||
| // the response has finished. `after` throws outside a request scope | ||
| // (tests); the request line above already covers that case. | ||
| try { |
There was a problem hiding this comment.
P3: recordVerboseDiagnosticsRequest schedules an after fallback on every flagged request even when the route is wrapped in withApiRouteSpan, which always calls completeVerboseDiagnosticsRequest (success or error) and sets entry.finished. For all current flagged traffic (every VM route and the diagnostics ingest route use withApiRouteSpan), the after() closure is a guaranteed no-op, so each flagged request pays for scheduling a next/server after-task that never does anything. Since recordVerboseDiagnosticsRequest runs before the route can signal whether it is span-wrapped, this is inherent to the sync design, but worth noting given flagged requests already burst several /api/vm calls per user action.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/services/observability/verboseDiagnostics.ts, line 108:
<comment>recordVerboseDiagnosticsRequest schedules an `after` fallback on every flagged request even when the route is wrapped in `withApiRouteSpan`, which always calls completeVerboseDiagnosticsRequest (success or error) and sets entry.finished. For all current flagged traffic (every VM route and the diagnostics ingest route use withApiRouteSpan), the after() closure is a guaranteed no-op, so each flagged request pays for scheduling a `next/server` after-task that never does anything. Since recordVerboseDiagnosticsRequest runs before the route can signal whether it is span-wrapped, this is inherent to the sync design, but worth noting given flagged requests already burst several /api/vm calls per user action.</comment>
<file context>
@@ -0,0 +1,160 @@
+ // end line (duration only; the response status is not observable here) once
+ // the response has finished. `after` throws outside a request scope
+ // (tests); the request line above already covers that case.
+ try {
+ after(() => {
+ if (entry.finished) return;
</file context>
teamleaderleo
added
area: auth
Collaborator
|
This remains a large App Review diagnostics stack with unresolved review findings, so I left it open and marked ready-to-land. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #11289 (
feat-review-demo-modeis the PR base): reuses that PR's server-writable StackclientReadOnlyMetadatatargeting mechanism for the App Review account. Mechanism and docs only; the flag is NOT set on any live account.What this adds
One new server-set flag,
cmuxVerboseDiagnostics: true, read fail-closed (literal JSONtrueonly) at sign-in and revalidation. It turns on two things for that account only, with zero client UI, zero user-facing strings, and no hardcoded email or user id anywhere.Server half.
verifyRequest(web/services/vms/auth.ts) captures the flag into theAuthedUserprojection (so the 30s native auth cache keeps it) and, for flagged users only, records the request throughweb/services/observability/verboseDiagnostics.ts: one[cmux-verbose-diag] {"kind":"request",...}console line (method, path, query keys only, user id, user agent) pluscmux.verbose_diagnostics/cmux.verbose_diagnostics_userattributes on the active OTel span.withApiRouteSpan(web/services/telemetry.ts) then emits the completion line{"kind":"response", route, status, durationMs, userId, errorName?, errorMessage?}; routes not wrapped in it get a fallback{"kind":"request_end", durationMs}line scheduled throughnext/server'safter. Unflagged users cost one boolean check and one WeakMap miss per request.Client half (iOS).
CMUXAuthUser.verboseDiagnosticsEnabledis parsed in the StackCurrentUseractor, persisted with the cached identity card, and exposed viaMobileIdentityProviding, exactly alongsidedemonstrationContentEnabled. The composition root fans a newVerboseDiagnosticsReporter(inCmuxMobileAnalytics) into the existingDiagnosticLogevent tap next toAppLogandTransportSentryReporter(Sentry is untouched), so every ring event (lifecycle, auth, pairing, dial, RPC, stream health) is covered. The reporter batches on a single consumer task and POSTs to/api/diagnostics/ingestwith the Stack bearer + refresh tokens, resolving each event to a wall-clock timestamp, a stable machine name, and anen_US_POSIXsummary.withObservationTrackingoverAuthCoordinator.currentUsermirrors the flag through sign-in, restore, late revalidation, and sign-out.Resilience and overhead.
ingestis nonisolated and returns after one lock-protected boolean read when the account is unflagged: nothing buffers, no timer runs, no task spawns. Batches are removed from the buffer before upload and are never retried; any failure (429, outage, 4xx rejection) opens an outage gate so further attempts happen at most once per 5s cadence with fresh events, and the backlog is hard-capped at 512 drop-oldest. Uploads run on a short-timeout session off every UI and connection path, and pending events flush on backgrounding.Server double gate.
/api/diagnostics/ingestrequires auth AND re-checks the server-written flag on the resolved user; unauthenticated uploads get 401 and unflagged accounts 403, so a stale or tampered client can never opt itself in. Body, batch (256 events), and every field are bounded; control characters are stripped so one event is always exactly one log line.Operating it
Enable/disable (server credentials required, other metadata keys preserved):
The flag takes effect on the reviewer's next sign-in or session revalidation, within 30s on the server side (native auth cache TTL).
Reading the logs afterwards
[cmux-verbose-diag]. Every line is that marker followed by one JSON object withkind(request,response,request_end,client_event,client_batch),userId, andat. Narrow to the reviewer with[cmux-verbose-diag]plus their user id in the query, e.g."[cmux-verbose-diag]" "<stack user id>".client_eventlines carry the iOS timeline:clientAt,code,name(e.g.transportDialFailed),summary,deviceId(per-install analytics id),buildStamp.cmux.verbose_diagnostics == trueorcmux.verbose_diagnostics_user == "<stack user id>"in the dataset Vercel's OTel export feeds. Console lines are the complete record; traces add timing detail when the trace was sampled.Tests
swift testPackages/Shared/CMUXAuthCore: 25 pass, including new fail-closed metadata parse, legacy identity-card decode, and Codable round-trip forverboseDiagnosticsEnabled.swift testPackages/iOS/CmuxMobileAnalytics: 33 pass, including 6 new reporter tests: unauthorized ingest uploads nothing, resolved timestamps/text, batch-size drain, failure drops the batch and opens the outage gate (never requeues), revocation discards pending, backlog hard cap.bun testweb/tests/verbose-diagnostics.test.ts+web/tests/diagnostics-ingest-route.test.ts: 13 pass, including marker emitted only for flagged users, no query values in logs, span-wrapper completion with status and with thrown errors, ingest 401/403 double gate, per-event marker lines, batch bounds, control-character stripping.vm-route-auth,vm-auth-cache,auth-errors,telemetry-sampler,vm-observability,analytics-events-route,devices-route,vm-route-input,vm-billing-limit-paywall): 129 pass, 0 fail.bun run typecheck(tsgo): clean.swift buildforCmuxAuthRuntimeandCmuxMobileShellModel: clean. The app-target wiring (AppCompositionRoot,cmuxFeature) compiles only in the full Xcode build (GhosttyKit), which runs on the fleet lane for this stack.Dictionary: native auth cache = 30-second server-side cache of verified bearer-token users, keyed by token hash, so client request bursts cost one Stack call; event tap = the single live observer callback on the app's bounded diagnostic ring; outage gate = reporter state entered after a failed upload that limits further attempts to the periodic cadence; span = one traced operation exported to Axiom via OpenTelemetry; runtime logs = Vercel's per-request console output for deployed functions.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds a server-controlled
cmuxVerboseDiagnosticsaccount flag that, when set, turns on structured request logging for that account and streams the iOS app's diagnostic events to a new/api/diagnostics/ingestendpoint. Unflagged accounts see no behavior change, the flag reads fail-closed (only literal JSONtrue), and it is not set on any live account yet — the mechanism is intended for App Review sessions, with no client UI, user-facing strings, or hardcoded user identifiers.Server side
verifyRequestcaptures the flag into theAuthedUserprojection and, for flagged users, emits one[cmux-verbose-diag]request line (method, path, query-key names, user id, user agent) and annotates the active OTel span;withApiRouteSpanadds aresponsecompletion line, with arequest_endfallback vianext/server'safter./api/diagnostics/ingestrequires auth and re-checks the server-written flag (unflagged uploads get 403), bounds the body, batch, and fields, strips control characters so one event is exactly one log line, and emits oneclient_eventline per event.bun scripts/set-verbose-diagnostics.ts <email> on|offsets or clears the flag while preserving every other metadata key; changes take effect on the next sign-in or within 30s (native auth cache TTL).Client side (iOS)
verboseDiagnosticsEnabledis parsed fail-closed inCMUXAuthUser, persisted with the cached identity card, and exposed viaMobileIdentityProvidingexactly alongsidedemonstrationContentEnabled.VerboseDiagnosticsReporterinCmuxMobileAnalyticstaps theDiagnosticLogring, batches events on a single consumer off every UI and connection path, and POSTs to/api/diagnostics/ingestwith the Stack bearer and refresh tokens.Written for commit 5e09fa4. Summary will update on new commits.