feat(cli): align primitive funnel measurement - #3052

Reviewed at head 9fcf1d07ec7bb14c63f6b54b71d0f9e4b5621a86.

Audited (read end-to-end): telemetry/primitive-funnel.ts, telemetry/primitive-funnel-state.ts, telemetry/primitive-funnel-command.ts, telemetry/transport.ts, and the preview.ts / render.ts call-site changes.
Trusting (not read): commands/add.ts, commands/catalog.ts, cli.ts, registry/threadMessageStackAuthorization.ts, and the five test files. So the OAuth-continuity and catalog-delivery claims in the description are not independently checked here.

Stack note: the base is feat/thread-message-stack, and #3046 is currently conflicted, so the diff that eventually lands is not this diff. Everything below is against the PR's own base.

Strengths

• The claim is a real mutual exclusion, not an advisory flag. createClaimMarker uses openSync(path, "wx", 0o600) (primitive-funnel-state.ts:38), which is O_CREAT|O_EXCL - so the filesystem arbitrates and exactly one process wins. The emittedEventIds pre-check at :126 runs before it and is only a fast path; two processes that both pass it still resolve correctly at the marker. The comments at :133-134 and :168-169 name which of the two is authoritative, in both directions.
• flush()'s new boolean is contract-correct in the two cases that decide whether a claim gets burned. transport.ts:110 returns true for an empty queue - "nothing queued is not a delivery failure" - which is the case that would otherwise release a legitimate claim and re-emit it on the next command. The snapshot-then-filter at :107 and :126-127 also means events enqueued mid-flight are not swept into the delivered set.
• performance.now() rather than Date.now() for durations - monotonic, so a clock adjustment mid-render cannot produce a negative or wild interval.
• The async handleOutput conversion (preview.ts:934) latches detected before the await and says so in the comment. That is the exact re-entrancy question the change raises, answered correctly.
• funnel_step numbering matches the description: failure steps share their success step's number (preview 7/7, render 8/8), so ordering by step is stable and no number is doubled up.

important - "no product behavior changes" is not accurate: the terminal awaits now sit in front of the user's own output

flush() is bounded by FLUSH_TIMEOUT_MS = 5_000 (transport.ts:10), and the terminals are now awaited on paths that previously fired and forgot. Two shapes, and the second is the one that matters:

• Success. render.ts:759-760 calls markRenderSucceeded() and then awaits the flush, so an offline or proxied user watches a finished render sit for up to five silent seconds before the command returns. Same at :931-932 and at preview.ts:1142.
• Failure. render.ts:745-750 awaits trackPrimitiveRenderFailed before handleRenderError, so the user's error message is delayed by the same budget. Identically at :909, preview.ts:403, and :1103. Whatever the argument for blocking a success path on telemetry, blocking the diagnostic on it is hard to defend - the person is already waiting to find out what went wrong.

The fix is ordering, not removal: print first, emit the terminal after. Where that is awkward, a much shorter timeout for the terminal flush specifically would bound the damage.

Related, at preview.ts:949: child.on("error", async (err) => ...) hands an async function to an EventEmitter, so the returned promise has no handler and a rejection surfaces as an unhandled rejection. The spinner and the error print are also inside it, after the await, so a failed studio start reports late for the same reason.

important - boundedInteger sends NaN to the maximum, which is the worst of the available answers

primitive-funnel.ts:81-84:

if (!Number.isFinite(value)) return value < 0 ? minimum : maximum;

NaN < 0 is false, so NaN returns maximum. Measured:

duration_ms  <- NaN        => 86400000   (24 hours)
duration_ms  <- +Infinity  => 86400000
duration_ms  <- -Infinity  => 0
result_count <- NaN        => 1000
result_rank  <- NaN        => 1000

The infinities are right. NaN is not: the description says non-finite input "collapses to the nearest bound", and NaN has no nearest bound. Picking the max is the worst option available, because NaN is conspicuous downstream and gets filtered, whereas 86,400,000 is a plausible number that silently moves every duration percentile - in a PR whose whole purpose is that a downstream read of this funnel can be trusted.

This is currently unreachable from the four call sites - every durationMs is performance.now() - performance.now(), and the counts come from array lengths - so it is a latent defect rather than a live one. But this clamp is the designated guard for exactly that input, and it fails in the permissive direction. For a duration, dropping the property is the only honest answer (0 is also a lie, and it drags the median down); Number.isNaN(value) needs its own branch either way.

important - the crash window between claim and acknowledgement is still an unrecoverable loss, of the same shape as the bug being fixed

primitive-funnel-command.ts:30-32 claims, emits, awaits, and releases on a falsy flush. If the process dies between the claim at :30 and the release decision at :32 - Ctrl+C on a long render, an OOM kill, a CI timeout - the marker persists and no later command can ever emit that step. That is the failure the description opens with ("the claim is durable and the delivery was not"), narrowed from always-on-exit-flush to only-on-crash, but not closed.

The two halves of the reasoning at primitive-funnel-state.ts:145-151 are also in tension. :145-146 says the claim must precede the send "because its whole job is to stop two concurrent processes both emitting one terminal event", while :150-151 says duplicates are the cheap failure because event_id collapses them downstream. If the second sentence is true - and releasePrimitiveFunnelEvent exists on the premise that it is - then pre-send exclusivity is buying protection against something the design already tolerates, and paying for it with a crash-shaped permanent loss. Writing the marker after the acknowledgement makes it a pure dedupe record: concurrent processes may both send, event_id collapses them, and a crash costs nothing that cannot be retried. Worth saying which property you actually want, since the code currently claims both.

nit - duration_ms is measured from four different origins in one command

preview.ts starts a primitiveCommandStartedAt at :214 (inside run), and then again at :964 (runDevMode), :1018 (runLocalStudioMode) and :1058 (runEmbeddedMode). So the same primitive_preview_succeeded event reports time-since-command-start on some paths and time-since-mode-entry on others, and the mode functions are entered after argument resolution, project discovery and version checks. The number is comparable within a path and not across them, which is the sort of thing that only shows up as an unexplainable bimodal distribution months later. Threading the run() value through, as attachStudioReadyHandler already does for its own parameter, makes it one interval.

nit - claim markers are never pruned

releasePrimitiveFunnelEvent removes a single marker, and nothing else touches .hyperframes/primitive-funnel-claims/. Since eventId is ${installId}:${suffix}, every install leaves two permanent files in the user's project directory. Tiny individually, unbounded over a project's life, and there is no reaper. Worth knowing: this repo's own .gitignore carries .hyperframes/ with a comment recording that it had to be broadened once after preview-generated state got committed - I did not find an equivalent ignore in the project template, so a generated project may well commit these.

Checked, not a finding

• An empty queue does not release a claim - transport.ts:110 returns true before the fetch. This was the failure I most expected to find, since flush() returning false for "nothing to send" would release every legitimate claim and re-emit on the next command.
• A non-OK response drops the batch from the queue and returns false, so the claim is released and the terminal is re-emittable by a later command. Consistent, though the justifying comment at :124-125 ("PostHog will reject the retry the same way") holds for a 400 and not for a 429 or a 502, where the other events in that batch are dropped with no claim to give back. Pre-existing drop behaviour; only the return value is new.
• shouldTrack() is checked before the claim (primitive-funnel-command.ts:26), so an opted-out run cannot burn a claim - the comment at :24-25 names precisely why that ordering matters.
• The state file write is atomic and mode-correct: temp file at 0600, directory at 0700, renameSync into place (primitive-funnel-state.ts:87-96).
• claimPrimitiveFunnelEvent returning false conflates "someone else holds it" with "the state file is unreadable", and the caller silently returns either way. That is the right call for telemetry that must never break the command, so I am noting it rather than filing it.

Verdict: APPROVE
Reasoning: This is careful work - the exclusion is real, the flush contract is right in both edge cases that matter, and the comments consistently name the trade-off rather than hiding it. Three things are worth fixing: the awaited terminals now delay the user's own error output, the NaN clamp resolves in the permissive direction on the one input it exists to catch, and the claim still has a crash-shaped permanent-loss window that the design's own duplicate tolerance would let you close. None of them is a defect in the instrumentation the PR set out to build.

— Rames Jusso

Reviewed at exact head. Extends the primitive funnel measurement: transport batching, funnel context persistence across preview/render, event alignment between add/catalog flows. Tests cover the full funnel lifecycle — auth→install→preview→render. The transport test verifies batch delivery and the command-level test verifies context propagation through preview and render.

— Miga