fix(onboarding): consensus review fixes for #1049

Addresses the findings from the review that fall inside this PR.

MAJOR — markSetupComplete() ran before the model-availability check
(dialog-provider). The branch below it deliberately refuses to claim success when
the gateway connects but offers nothing usable, and said so in a comment — but
completion had already been marked, so telemetry reported a finished onboarding
while the user was told to go pick a model. Now marked where the model is
actually set. This matters more since the scan gate moved onto the same signal.

MAJOR — the flushTimer leak. doInit() assigned a new interval without clearing
the previous handle, and shutdown() only ever clears the current one, so a second
doInit() stranded a timer for the life of the process. doShutdown also nulled
initPromise unconditionally, discarding a doInit() that init() had chained onto
the in-flight shutdown. Both fixed; see the note in doShutdown on why only the
first is covered by a test.

MAJOR — instance_connected and onboarding_abandoned could both be reported for
one launch. The gateway success events are emitted on the worker thread while
abandonment state is main-thread-owned, so a user who finished in the browser and
quit before the TUI observed the new provider was reported as abandoning at
gateway_auth. The exit path now checks whether credentials landed.

MAJOR — environment_scan_completed fired on every project_scan, including
/discover and any model-initiated call, so a funnel query could exceed 100%
conversion. Now guarded on isOnboardingSession.

MINOR — command.execute.before created a tracking record for every slash command
in every session, churning the capped map and evicting genuine onboarding
sessions, after which their remaining activation events were silently dropped.
noteCommandSubmission now only touches sessions already tracked, and
/onboard-connect marks the session before flagging its own submission.

MINOR — the cross-package event parity test that two comments claimed but which
did not exist. Now a compile-time assertion pinning the packages/tui event union
to the Telemetry variants; verified by renaming a property and watching the build
fail. Required exporting the context subpath from packages/tui.

MINOR — a raw HOME path could reach LLM-visible tool output on a sample-setup
failure. Paths are masked in `output`; the full message stays in metadata, which
the model never sees.

MINOR — documented why sample_setup_completed uses a success boolean instead of a
_failed sibling event.

NIT — launchId() no longer writes process.env. The worker receives the id
explicitly through WorkerOptions.env, so the write only leaked it into every
subprocess the CLI spawns.

Also removes the stray `// scratch` line at the end of cli/cmd/tui.ts.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018SLUQF3xgZHsGZHSjxe7vb

Author: Haider

packages/opencode/src/altimate/plugin/onboarding-telemetry.ts

@@ -81,13 +81,17 @@ function isJobCompletion(tool: string, output: { metadata?: unknown }): boolean
 export async function OnboardingTelemetryPlugin(_input: PluginInput): Promise<Hooks> {
   return {
     "command.execute.before": async (input) => {
+      // Mark the session BEFORE flagging the submission: noteCommandSubmission only touches
+      // sessions already tracked (so ordinary slash commands cannot churn the capped map), and
+      // /onboard-connect is the command that creates the record in the first place.
+      if (input.command === ONBOARD_CONNECT) OnboardingTelemetry.markOnboardingSession(input.sessionID)
+
       // Any slash command means the next user message was not typed by the user — needed so
       // `first_prompt_sent` measures a real first prompt rather than the scan gate's hidden
       // `/onboard-connect` submission.
       OnboardingTelemetry.noteCommandSubmission(input.sessionID)
 
       if (input.command !== ONBOARD_CONNECT) return
-      OnboardingTelemetry.markOnboardingSession(input.sessionID)
 
       // `skip` renders the menu immediately, with no scan to wait for, so the variant is known
       // now. The `scan` branch cannot be resolved here — the menu follows the scan, and the

packages/opencode/src/altimate/telemetry/index.ts

@@ -850,6 +850,11 @@ export namespace Telemetry {
         job: "sample_duck_db" | "breaks_downstream" | "sql_review" | "cost" | "something_else"
       }
     | {
+        /** Deliberately a `success` boolean rather than a `sample_setup_failed` sibling, unlike
+         *  the gateway_auth_completed/_failed pair. The gateway has genuinely distinct failure
+         *  modes worth their own enum (timeout / denied / error); this tool either materialised
+         *  the sample or did not, and the useful breakdown is `reused`, which only exists on the
+         *  success path. Splitting it would duplicate the counts an analyst has to add back up. */
         type: "sample_setup_completed"
         timestamp: number
         session_id: string
@@ -1366,12 +1371,15 @@ export namespace Telemetry {
   //     so the worker computes a different start time than the main thread.
   const LAUNCH_ID_ENV = "ALTIMATE_LAUNCH_ID"
 
+  let cachedLaunchId: string | undefined
+
   export function launchId(): string {
-    const existing = process.env[LAUNCH_ID_ENV]
-    if (existing) return existing
-    const created = randomUUID()
-    process.env[LAUNCH_ID_ENV] = created
-    return created
+    // The worker reads the value the TUI handed it through WorkerOptions.env; the main thread
+    // generates it. Cached in module scope rather than written back to process.env — the worker
+    // is given it explicitly, so writing it would only leak the id into every subprocess the CLI
+    // spawns, for no benefit.
+    if (!cachedLaunchId) cachedLaunchId = process.env[LAUNCH_ID_ENV] || randomUUID()
+    return cachedLaunchId
   }
   // altimate_change end
 
@@ -1537,6 +1545,11 @@ export namespace Telemetry {
       }
       enabled = true
       log.info("telemetry initialized", { mode: "appinsights" })
+      // altimate_change — clear any existing interval before installing a new one. doInit() can
+      // run more than once per process (init/shutdown cycles per session in prompt.ts), and
+      // without this each extra run strands the previous timer: shutdown() only ever clears the
+      // current handle, so orphans accumulate for the life of a `serve` process.
+      if (flushTimer) clearInterval(flushTimer)
       const timer = setInterval(flush, FLUSH_INTERVAL_MS)
       if (typeof timer === "object" && timer && "unref" in timer) (timer as any).unref()
       flushTimer = timer
@@ -1671,6 +1684,7 @@ export namespace Telemetry {
   }
 
   async function doShutdown(timeoutMs?: number) {
+    const initPromiseAtShutdown = initPromise
     // Wait for init to complete so we know whether telemetry is enabled
     // and have a valid endpoint to flush to.  init() is fire-and-forget
     // in CLI middleware, so it may still be in-flight when shutdown runs.
@@ -1693,8 +1707,19 @@ export namespace Telemetry {
     sessionId = ""
     projectId = ""
     machineId = ""
-    initPromise = undefined
-    initDone = false
+    // altimate_change — only clear initPromise if it is still the one this shutdown began with.
+    // init() can set `initPromise = shutdownPromise.then(doInit)`; nulling that unconditionally
+    // discards a doInit() which has not run yet, so the next init() starts a second one.
+    //
+    // Not covered by a test: that assignment requires initPromise to be undefined while
+    // shutdownPromise is still live, and those two are cleared one statement apart — a window I
+    // could not reach deterministically. Kept because it is free and obviously correct; the
+    // clear-before-assign in doInit() is what actually prevents an orphaned interval, whatever
+    // path leads to a second doInit.
+    if (initPromise === initPromiseAtShutdown) {
+      initPromise = undefined
+      initDone = false
+    }
   }
   // altimate_change end
 }

packages/opencode/src/altimate/telemetry/onboarding.ts

@@ -70,6 +70,9 @@ type DistributiveOmit<T, K extends PropertyKey> = T extends unknown ? Omit<T, K>
 
 type EmitInput = DistributiveOmit<OnboardingEventInput, "timestamp" | "session_id">
 
+/** Exported so the cross-package parity assertion in the tests can pin the TUI union to this. */
+export type OnboardingEmitInput = EmitInput
+
 // Module-global, per thread. Resets on every process launch, which is correct: a fresh launch
 // is a fresh onboarding attempt.
 let furthestStage: OnboardingStage | undefined
@@ -154,8 +157,18 @@ export function isCompleted() {
  * Call on the exit path, before the final flush. No-ops when the user never started (a
  * returning user with credentials), already completed, or when it has already fired.
  */
-export async function emitAbandonedIfIncomplete(): Promise<void> {
+export async function emitAbandonedIfIncomplete(opts?: { connected?: boolean }): Promise<void> {
   if (completed || abandonedEmitted || !furthestStage) return
+  // A gateway sign-in that succeeded is not an abandonment, even if the TUI never observed it.
+  // `gateway_auth_completed` and `instance_connected` are emitted on the WORKER thread, and this
+  // state lives on the main thread — so a user who finishes in the browser and quits before the
+  // TUI notices the new provider would otherwise be reported as abandoning at `gateway_auth`, in
+  // the same launch that already reported a successful connection. Two contradictory terminal
+  // states for one run, and gateway auth is the slowest step so it is the likeliest to hit this.
+  //
+  // The caller passes whether credentials now exist, which is the main thread's own view of the
+  // same fact and needs no cross-thread channel.
+  if (opts?.connected) return
   abandonedEmitted = true
   await emit({ type: "onboarding_abandoned", last_stage: furthestStage })
 }
@@ -220,9 +233,20 @@ function claim(sessionID: string, key: "menuShown" | "jobSelected" | "jobComplet
   return true
 }
 
-/** Record that the next user message in this session comes from a slash command. */
+/**
+ * Record that the next user message in this session comes from a slash command.
+ *
+ * Only touches sessions already tracked. `command.execute.before` fires for EVERY slash command
+ * in every session, so creating a record here made ordinary `/discover`, `/model` and so on churn
+ * the capped map and evict genuine onboarding sessions — after which `isOnboardingSession()`
+ * returns false and the rest of that user's activation events are silently dropped.
+ *
+ * An untracked session has no onboarding state to protect, so skipping it loses nothing:
+ * `first_prompt_sent` is gated on `isOnboardingSession` anyway.
+ */
 export function noteCommandSubmission(sessionID: string) {
-  record(sessionID).commandSubmission = true
+  const entry = sessions.get(sessionID)
+  if (entry) entry.commandSubmission = true
 }
 
 /** True (once) if this session's pending user message was command-submitted. */

packages/opencode/src/altimate/tools/project-scan.ts

@@ -945,18 +945,25 @@ export const ProjectScanTool = Tool.define("project_scan", {
     // otherwise be recorded as having none.
     //
     // degraded[] is a sorted list of short detection-failure keys — no paths, hosts, or messages.
-    void OnboardingTelemetry.emit({
-      type: "environment_scan_completed",
-      has_dbt: dbtProject.found,
-      has_warehouse: totalConnections > 0,
-      is_repo: git.isRepo,
-      connections_found: totalConnections,
-      degraded: degradedList,
-      // Explicit session: emit() otherwise falls back to the process-global telemetry context,
-      // which is set per prompt loop. Two concurrent sessions in `serve` or in the TUI worker
-      // overwrite each other's context, so a scan would be attributed to whichever session most
-      // recently started a turn.
-    }, ctx.sessionID)
+    // Only for onboarding runs. project_scan is also reachable via /discover and any
+    // model-initiated call; without this guard an event in the onboarding taxonomy fires for all
+    // of them, and a funnel query on scan_gate_shown → environment_scan_completed can exceed 100%.
+    //
+    // Explicit session id as well: emit() otherwise falls back to the process-global telemetry
+    // context, which is set per prompt loop, so two concurrent sessions overwrite each other's.
+    if (OnboardingTelemetry.isOnboardingSession(ctx.sessionID)) {
+      void OnboardingTelemetry.emit(
+        {
+          type: "environment_scan_completed",
+          has_dbt: dbtProject.found,
+          has_warehouse: totalConnections > 0,
+          is_repo: git.isRepo,
+          connections_found: totalConnections,
+          degraded: degradedList,
+        },
+        ctx.sessionID,
+      )
+    }
     // altimate_change end
 
     // Build metadata

packages/opencode/src/altimate/tools/sample-setup.ts

@@ -205,7 +205,11 @@ export const SampleSetupTool = Tool.define("sample_setup", {
       return {
         title: "Starter materialization failed",
         metadata: { success: false, error: message, targetPath: "", reused: false, suffix: 0, note: "" },
-        output: `status: error\nreason: materialize_failed\n\n${message}`,
+        // Paths are masked in `output` but kept verbatim in `metadata.error`. `output` is what
+        // the model sees (session/message-v2.ts), so it lands in conversation context and is sent
+        // to the provider on every later turn — and rejectUnsafeHome messages embed HOME
+        // verbatim. metadata never reaches the model, so the full text stays available locally.
+        output: `status: error\nreason: materialize_failed\n\n${redactPaths(message)}`,
       }
     }
   },
@@ -229,6 +233,18 @@ export const SampleSetupTool = Tool.define("sample_setup", {
 //
 // Best-effort by construction: telemetry must never fail a sample setup, so any fs error
 // yields 0 rather than propagating.
+/**
+ * Replace absolute filesystem paths with a placeholder.
+ *
+ * Deliberately local rather than reusing Telemetry.maskString: importing the telemetry module
+ * here drags in Config/Account and hangs this tool's tests. This only needs to handle paths.
+ */
+function redactPaths(message: string): string {
+  return message
+    .replace(/(?:[A-Za-z]:)?[\\/](?:[\w.\-~ ]+[\\/])+[\w.\-~ ]*/g, "<path>")
+    .replace(/~[\\/][^\s'"]*/g, "<path>")
+}
+
 function countFilesWithExtension(dir: string, extension: string): number {
   let total = 0
   let entries: fs.Dirent[]

packages/opencode/src/cli/cmd/tui.ts

@@ -19,6 +19,7 @@ import { win32InstallCtrlCGuard } from "@opencode-ai/tui/terminal-win32"
 // altimate_change start — onboarding telemetry: main-thread flush on the TUI exit path
 import { Telemetry } from "@/altimate/telemetry"
 import * as OnboardingTelemetry from "@/altimate/telemetry/onboarding"
+import { AltimateApi } from "@/altimate/api/client"
 // altimate_change end
 
 declare global {
@@ -268,7 +269,12 @@ export const TuiThreadCommand = cmd({
       // flush() would otherwise block for REQUEST_TIMEOUT_MS (10s) on a blackholed network — a
       // visible hang between the user quitting and the shell prompt returning.
       try {
-        await OnboardingTelemetry.emitAbandonedIfIncomplete()
+        // Ask whether gateway credentials landed. The success events are emitted on the worker
+        // thread and this state is main-thread-owned, so without this a browser sign-in that
+        // completed just before the user quit is reported as an abandonment in the same launch
+        // that already reported instance_connected.
+        const connected = await AltimateApi.isConfigured().catch(() => false)
+        await OnboardingTelemetry.emitAbandonedIfIncomplete({ connected })
         await Telemetry.shutdown({ timeoutMs: 2000 })
       } catch {
         // Never let telemetry delay or break exit.
@@ -278,4 +284,3 @@ export const TuiThreadCommand = cmd({
     process.exit(0)
   },
 })
-// scratch

packages/opencode/test/altimate/telemetry/onboarding.test.ts

@@ -11,6 +11,7 @@ import { describe, expect, test, beforeEach, afterEach, spyOn, mock } from "bun:
 import { Telemetry } from "@/altimate/telemetry"
 import * as Onboarding from "@/altimate/telemetry/onboarding"
 import { OnboardingTelemetryPlugin } from "@/altimate/plugin/onboarding-telemetry"
+import type { OnboardingTelemetryEvent } from "@opencode-ai/tui/context/onboarding-telemetry"
 
 type Tracked = Telemetry.Event
 
@@ -268,28 +269,45 @@ describe("activation events", () => {
 // Slash-command suppression, which first_prompt_sent depends on
 // ---------------------------------------------------------------------------
 describe("command submission tracking", () => {
-  test("a command-submitted message is flagged, once", () => {
+  test("a command-submitted message in an onboarding session is flagged, once", () => {
+    Onboarding.markOnboardingSession("ses_a")
     Onboarding.noteCommandSubmission("ses_a")
     expect(Onboarding.consumeCommandSubmission("ses_a")).toBe(true)
     expect(Onboarding.consumeCommandSubmission("ses_a")).toBe(false)
   })
 
   test("a session that never ran a command is not flagged", () => {
+    Onboarding.markOnboardingSession("ses_never")
     expect(Onboarding.consumeCommandSubmission("ses_never")).toBe(false)
   })
 
   test("the flag does not leak between sessions", () => {
+    Onboarding.markOnboardingSession("ses_a")
+    Onboarding.markOnboardingSession("ses_b")
     Onboarding.noteCommandSubmission("ses_a")
     expect(Onboarding.consumeCommandSubmission("ses_b")).toBe(false)
     expect(Onboarding.consumeCommandSubmission("ses_a")).toBe(true)
   })
 
-  test("every slash command is flagged, not just /onboard-connect", async () => {
+  test("an untracked session is not created just by running a slash command", () => {
+    // The anti-churn property. command.execute.before fires for every slash command in every
+    // session; if that created a record, ordinary /discover and /model traffic in a long-lived
+    // `serve` process would evict genuine onboarding sessions from the capped map and silently
+    // drop those users' remaining activation events.
+    Onboarding.noteCommandSubmission("ses_unrelated")
+    expect(Onboarding.consumeCommandSubmission("ses_unrelated")).toBe(false)
+    expect(Onboarding.isOnboardingSession("ses_unrelated")).toBe(false)
+  })
+
+  test("/onboard-connect marks the session before flagging its own submission", async () => {
+    // Ordering matters: the command that creates the record must be flagged too, or the hidden
+    // scan-gate submission counts as the user's first typed prompt.
     const hooks = await OnboardingTelemetryPlugin({} as any)
     await hooks["command.execute.before"]!(
-      { command: "discover", sessionID: "ses_c", arguments: "" },
+      { command: "onboard-connect", sessionID: "ses_c", arguments: "skip" },
       { parts: [] } as any,
     )
+    expect(Onboarding.isOnboardingSession("ses_c")).toBe(true)
     expect(Onboarding.consumeCommandSubmission("ses_c")).toBe(true)
   })
 })
@@ -343,3 +361,26 @@ describe("launch correlation id", () => {
     }
   })
 })
+
+// ---------------------------------------------------------------------------
+// Cross-package event parity
+// ---------------------------------------------------------------------------
+//
+// packages/tui cannot import the Telemetry event union, so it declares its own mirror and the
+// host remaps `name` → `type` through an unchecked cast in cli/cmd/tui.ts. Without this, a rename
+// or an added property on either side compiles clean and ships malformed events.
+//
+// Compile-time only: tsgo runs over test files, so drift is a build failure. There is nothing to
+// assert at runtime — a type is not a value.
+type TuiEventAsEmitInput<E> = E extends { name: infer N } ? Omit<E, "name"> & { type: N } : never
+
+// Fails to compile if any TUI event lacks a matching Telemetry variant, or if their property
+// names or enum values diverge.
+const _tuiEventsMatchTelemetry: Onboarding.OnboardingEmitInput = null as unknown as TuiEventAsEmitInput<OnboardingTelemetryEvent>
+void _tuiEventsMatchTelemetry
+
+describe("cross-package event parity", () => {
+  test("is enforced by the type assertion above, not at runtime", () => {
+    expect(true).toBe(true)
+  })
+})

packages/tui/package.json

@@ -17,6 +17,7 @@
     "./context/epilogue": "./src/context/epilogue.tsx",
     "./context/exit": "./src/context/exit.tsx",
     "./context/kv": "./src/context/kv.tsx",
+    "./context/onboarding-telemetry": "./src/context/onboarding-telemetry.tsx",
     "./context/project": "./src/context/project.tsx",
     "./context/runtime": "./src/context/runtime.tsx",
     "./context/sdk": "./src/context/sdk.tsx",

packages/tui/src/component/dialog-provider.tsx

@@ -398,8 +398,10 @@ function AutoMethod(props: AutoMethodProps) {
     await sdk.client.instance.dispose()
     await sync.bootstrap()
     if (disposed) return
-    // altimate_change start — mark setup complete (flips useReady → unlocks first-run chat/tips)
-    markSetupComplete()
+    // altimate_change start — setup is marked complete once a model is actually selected, not
+    // here. The branch below deliberately refuses to claim success when the gateway connects but
+    // offers nothing usable ("Connected, but no model is available yet"); marking completion up
+    // front contradicted that, and it drives onboarding_completed and the Part 2 scan gate.
     // The gateway sign-in already shows the auth URL + "Waiting for authorization…".
     // On success, confirm inline (green) and auto-close after a moment rather than
     // opening the model picker. Auto-select a model so the user can chat right away.
@@ -419,6 +421,9 @@ function AutoMethod(props: AutoMethodProps) {
         return
       }
       local.model.set({ providerID: props.providerID, modelID: model }, { recent: true })
+      // A model is chosen — this is the real end of setup (flips useReady → unlocks first-run
+      // chat/tips, and opens the Part 2 scan gate).
+      markSetupComplete()
       setConnected(true)
       closeTimer = setTimeout(() => {
         if (!disposed) dialog.clear()
@@ -427,6 +432,7 @@ function AutoMethod(props: AutoMethodProps) {
     }
     // altimate_change end
     toast.show({ message: `Connected to ${props.title}`, variant: "success" })
+    // No markSetupComplete() here: this opens the model picker, which marks it on selection.
     dialog.replace(() => <DialogModel providerID={props.providerID} />)
   })