feat: [AI-8448] count installs from the shell installers, not just npm - #1096
feat: [AI-8448] count installs from the shell installers, not just npm#1096saravmajestic wants to merge 1 commit into
Conversation
The install dashboard dipped when the advertised install path moved from npm to `altimate.sh/install`. The installs did not stop — the instrumentation did. `first_launch` is the only install metric, and it fires off a marker file rather than any network call from the installer. That marker was written in exactly one place, `script/postinstall.mjs`, so every user arriving through `install` or `install.ps1` emitted nothing at all. Both shell installers now write the same marker, and `first_launch` carries a new `install_method` so the recovered volume is separable from npm rather than folded into one number. `altimate upgrade` on the curl path re-runs `install`, so curl upgrades become visible too. Brand-new installs remain `is_upgrade: false` — the field probes whether `~/.altimate/machine-id` existed before this launch: count(distinct machine_id) where type = "first_launch" and is_upgrade = false Details worth knowing, since each one fails silently rather than loudly: - The marker goes to `$XDG_DATA_HOME` (default `~/.local/share/altimate-code`) on every platform including Windows. `welcome.ts` resolves the data dir through Node's `os.homedir()` and never consults `%LOCALAPPDATA%`, so a marker written there would be ignored at read time. - `install.ps1` writes with `-Encoding ascii`. The documented entrypoint is `powershell -c "irm ... | iex"`, i.e. Windows PowerShell 5.1, where `-Encoding utf8` prepends a BOM. `.trim()` happens to strip a leading BOM (U+FEFF is JS whitespace), but `install_method` is matched against a fixed allowlist and must not depend on that. - An unresolved version falls back to `unknown` instead of empty: an empty marker is deleted unread, which would lose the install outright. That is the state `check_version` leaves whenever the GitHub API is unreachable. - The marker is written after the install dispatch, so a version that was already present (`check_version` exits 0 early) does not report an install, and neither does a failed download. - `install_method` is allowlisted to `curl`/`powershell`/`npm`, so a hand-edited or truncated file cannot mint a new dimension. It reads `unknown` when the marker predates the field — expected on the first upgrade after this ships. - The source file is consumed on read, including on the empty-marker path, so an orphan cannot be attributed to a later install. - Marker writes are non-fatal in both installers: a read-only `$HOME` costs the event, never the install. No new network call and no new identifier. The installers only record a version and their own name to a local file; the CLI's existing opt-out gates still decide whether anything is transmitted. Tests cover the two fields the dashboard reads, the allowlist, source-file consumption, and the shell installers' marker paths. The load-bearing one is the ordering invariant: `is_upgrade` is only correct because `index.ts` fires `Telemetry.init()` unawaited and `doInit()` yields at `await Config.get()` before minting the machine-id. An await added ahead of that mint would make every install report `is_upgrade: true` and silently empty the brand-new-install metric, so that ordering is now asserted directly — including that the mint does happen once awaited, so the assertion cannot pass vacuously. Verified `install`'s marker writer by executing it: XDG override, `v` stripping, the `unknown` fallback, and exit 0 on a read-only `$HOME`. `install.ps1` is asserted at source level only — no `pwsh` on the dev machine.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
📝 WalkthroughWalkthroughInstallers now write local version and source markers. The CLI consumes these markers during first launch and includes ChangesInstaller attribution telemetry
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🟠 High · up to The shell installers can record or transmit first-launch telemetry and create a machine ID even when the config-only telemetry opt-out is enabled during cold start, contrary to the documented privacy behavior. One test also depends on the environment's second opt-out variable. Merge should wait for the opt-out path and test isolation to be fixed. Sequence Diagram(s)sequenceDiagram
participant Installer
participant MarkerFiles
participant WelcomeBanner
participant Telemetry
Installer->>MarkerFiles: Write version and install-source markers
WelcomeBanner->>MarkerFiles: Read and remove install-source marker
MarkerFiles-->>WelcomeBanner: Return validated install method
WelcomeBanner->>Telemetry: Track first_launch with install_method
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@install`:
- Around line 502-510: Fix the cold-start telemetry opt-out gap by ensuring
telemetry initialization fails closed or is deferred until Config.get() is
available, then re-initialized after Instance.provide(); do not mint a machine
ID or enable first-launch telemetry when telemetry.disabled is configured. Apply
this to the write_install_marker flow in install (lines 502-510) and the
equivalent PowerShell install flow in install.ps1 (lines 321-336). After runtime
behavior is corrected, update docs/docs/reference/security-faq.md (lines
146-148) to accurately state the config-only opt-out guarantee.
In `@packages/opencode/test/install/install-telemetry.test.ts`:
- Around line 118-150: Update the telemetry test setup and cleanup around
Telemetry.init to snapshot, delete, and restore OPENCODE_DISABLE_TELEMETRY
alongside ALTIMATE_TELEMETRY_DISABLED. Ensure both opt-out variables are cleared
before initialization and restored in the finally block, preserving the existing
environment cleanup behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 03f14851-1932-45c7-b893-f18e4a0d4942
📒 Files selected for processing (11)
docs/docs/reference/security-faq.mddocs/docs/reference/telemetry.mdinstallinstall.ps1packages/opencode/script/postinstall.mjspackages/opencode/src/altimate/telemetry/index.tspackages/opencode/src/cli/welcome.tspackages/opencode/test/cli/welcome.test.tspackages/opencode/test/install/install-telemetry.test.tspackages/opencode/test/install/postinstall.test.tspackages/opencode/test/telemetry/telemetry.test.ts
| write_install_marker() { | ||
| local data_dir="${XDG_DATA_HOME:-$HOME/.local/share}/altimate-code" | ||
| # An empty marker is deleted unread by the CLI, so fall back to "unknown" | ||
| # rather than losing the install: $specific_version is empty whenever the | ||
| # GitHub API could not be reached (see check_version). | ||
| local marker_version="${specific_version:-unknown}" | ||
| mkdir -p "$data_dir" 2>/dev/null || return 0 | ||
| printf '%s' "${marker_version#v}" > "$data_dir/.installed-version" 2>/dev/null || return 0 | ||
| printf '%s' "curl" > "$data_dir/.install-source" 2>/dev/null || return 0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Fail closed for config-only telemetry opt-out before enabling shell-install telemetry.
If a user sets telemetry.disabled: true without an environment flag, early Telemetry.init() can run before Instance.provide() makes Config.get() available. Its catch path proceeds with telemetry enabled. These new markers then queue and send first_launch for curl and PowerShell installs, and can mint a machine ID despite the user’s configuration.
Defer telemetry initialization until configuration is available, or fail closed and re-initialize after instance setup. Do not state that config opt-out controls transmission until this path is fixed.
install#L502-L510: do not enable curl first-launch telemetry while config-only opt-out can be bypassed.install.ps1#L321-L336: do not enable PowerShell first-launch telemetry while config-only opt-out can be bypassed.docs/docs/reference/security-faq.md#L146-L148: correct this opt-out guarantee after the runtime behavior is fixed.
Based on learnings, the config-only telemetry opt-out cold-start gap occurs when doInit() runs before Instance.provide() makes Config.get() available, and its configuration catch path can mint a machine ID despite telemetry.disabled.
📍 Affects 3 files
install#L502-L510(this comment)install.ps1#L321-L336docs/docs/reference/security-faq.md#L146-L148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install` around lines 502 - 510, Fix the cold-start telemetry opt-out gap by
ensuring telemetry initialization fails closed or is deferred until Config.get()
is available, then re-initialized after Instance.provide(); do not mint a
machine ID or enable first-launch telemetry when telemetry.disabled is
configured. Apply this to the write_install_marker flow in install (lines
502-510) and the equivalent PowerShell install flow in install.ps1 (lines
321-336). After runtime behavior is corrected, update
docs/docs/reference/security-faq.md (lines 146-148) to accurately state the
config-only opt-out guarantee.
Source: Learnings
| const origCs = process.env.APPLICATIONINSIGHTS_CONNECTION_STRING | ||
| const origDisabled = process.env.ALTIMATE_TELEMETRY_DISABLED | ||
| const tmpHome = mkdtempSync(join(os.tmpdir(), "install-telemetry-home-")) | ||
| spyOn(os, "homedir").mockImplementation(() => tmpHome) | ||
| spyOn(global, "fetch").mockImplementation((async () => new Response("", { status: 200 })) as any) | ||
|
|
||
| try { | ||
| // The baked-in sink is refused under a test runner, and doInit returns before minting | ||
| // when it has no connection string — which would make this test vacuously pass. | ||
| delete process.env.ALTIMATE_TELEMETRY_DISABLED | ||
| process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = | ||
| "InstrumentationKey=k;IngestionEndpoint=https://example.invalid" | ||
| // init() is `initPromise ??= doInit()`; shutdown() is the only seam that clears it, so | ||
| // an earlier init in this process would otherwise be handed back already resolved. | ||
| await Telemetry.shutdown() | ||
|
|
||
| const pending = Telemetry.init() | ||
| const machineIdPath = path.join(tmpHome, ".altimate", "machine-id") | ||
|
|
||
| // The instant that matters — the same turn of the event loop in which index.ts calls | ||
| // showWelcomeBannerIfNeeded(). | ||
| expect(existsSync(machineIdPath)).toBe(false) | ||
|
|
||
| await pending | ||
| // Proves the assertion above is not vacuous: this path really does mint, just later. | ||
| expect(existsSync(machineIdPath)).toBe(true) | ||
| } finally { | ||
| await Telemetry.shutdown() | ||
| if (origCs !== undefined) process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = origCs | ||
| else delete process.env.APPLICATIONINSIGHTS_CONNECTION_STRING | ||
| if (origDisabled !== undefined) process.env.ALTIMATE_TELEMETRY_DISABLED = origDisabled | ||
| else delete process.env.ALTIMATE_TELEMETRY_DISABLED | ||
| rmSync(tmpHome, { recursive: true, force: true }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore and clear both telemetry opt-out variables.
This test clears only ALTIMATE_TELEMETRY_DISABLED. If OPENCODE_DISABLE_TELEMETRY is set in the test environment, Telemetry.init() exits before it creates the machine ID and Line 143 fails. Snapshot, delete, and restore OPENCODE_DISABLE_TELEMETRY in the same try/finally block.
Based on learnings, telemetry opt-out uses both ALTIMATE_TELEMETRY_DISABLED and OPENCODE_DISABLE_TELEMETRY, and opt-out tests must snapshot and restore both. As per coding guidelines, “Tests using global mock.module, dispatchers, or similar shared state must provide teardown and isolation safe for parallel bun test execution.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/test/install/install-telemetry.test.ts` around lines 118 -
150, Update the telemetry test setup and cleanup around Telemetry.init to
snapshot, delete, and restore OPENCODE_DISABLE_TELEMETRY alongside
ALTIMATE_TELEMETRY_DISABLED. Ensure both opt-out variables are cleared before
initialization and restored in the finally block, preserving the existing
environment cleanup behavior.
Sources: Coding guidelines, Learnings
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| # Only reached when an install actually happened: check_version exits 0 early | ||
| # when the requested version is already present. | ||
| write_install_marker |
There was a problem hiding this comment.
SUGGESTION: --binary installs are misattributed as curl
write_install_marker runs after both install branches, including install_from_binary (the install --binary <path> path). That branch sets specific_version="local", so the marker records install_method: "curl" and version "local" for a local dev build rather than a curl download. Guarding the call keeps the curl metric clean.
| write_install_marker | |
| [ -z "$binary_path" ] && write_install_marker |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Merge - 1 optional suggestion (non-blocking) Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (11 files)
Fix these issues in Kilo Cloud Reviewed by glm-5.2 · Input: 75.6K · Output: 28.5K · Cached: 735K Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 11 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="install">
<violation number="1" location="install:503">
P2: On the Windows bash path this marker lands where the CLI never reads it. The `install` script explicitly supports Windows (MINGW/MSYS/CYGWIN resolve `os="windows"`, seen around line 92), but `write_install_marker` resolves the data dir from `$HOME`, while welcome.ts resolves it via Node's `os.homedir()` (`getDataDir()`: `process.env.XDG_DATA_HOME || path.join(os.homedir(), ".local", "share")`). Under MSYS2/Cygwin `$HOME` is the POSIX home (`/home/<user>`), which does not match Windows' `os.homedir()` (`%USERPROFILE%`), so the `.installed-version`/`.install-source` files would be written to a location the CLI never checks — the exact silent failure this PR intends to fix. The in-diff comment claims the path "MUST match ... on every platform, including Windows", which is not guaranteed on the bash path. Consider deriving the fallback from the user profile on the Windows branches (e.g. `test "$os" = windows` using `$USERPROFILE` instead of `$HOME`), or document/limit the bash installer's Windows support.</violation>
<violation number="2" location="install:507">
P2: Binary installs now report the literal version `local` in telemetry. On the `--binary` path `specific_version="local"` is set (install line 77), so `write_install_marker` writes `.installed-version` = `local`. welcome.ts then emits `first_launch` with `version: "local"` (and the banner reads `vlocal installed`). Since this change is specifically about *counting/measuring* installs, `local` pollutes the version dimension for every `--binary` install. Either skip the marker on the binary path, or map it to `unknown` rather than `local`.</violation>
<violation number="3" location="install:510">
P2: When users use the documented `./install --binary ...` mode, this line records `curl` even though the download branch was skipped. Write `unknown` for local-binary installs; otherwise `first_launch.install_method` misattributes them as curl.</violation>
<violation number="4" location="install:522">
P2: When `--binary` points to the already-installed file, `install_from_binary` copies nothing but this unconditional call still creates telemetry markers. Skip marker creation for that no-op path so the next launch does not emit a false `first_launch`.</violation>
<violation number="5" location="install:522">
P1: The new curl/powershell install markers cause a first_launch telemetry event and machine-id generation on next CLI launch. If Telemetry.init() executes before Instance.provide() loads Config.get() (e.g., cold start), and its error-handling path defaults to telemetry enabled, config-only telemetry.disabled settings (without an env var) will be bypassed for these newly-instrumented install paths. Verify Telemetry.init() fails closed when config is not yet available, or defer initialization until config is loaded, before relying on marker-triggered first_launch events here.</violation>
</file>
<file name="packages/opencode/src/cli/welcome.ts">
<violation number="1" location="packages/opencode/src/cli/welcome.ts:30">
P2: When the CLI starts between the two installer writes, this reader consumes the version marker without a committed source marker and misclassifies `install_method`. Publish both values through one atomic metadata record, or write a complete pair before exposing the version marker.</violation>
<violation number="2" location="packages/opencode/src/cli/welcome.ts:31">
P2: In `readInstallMethod`, the read and unlink share one try block. If `readFileSync` succeeds but `unlinkSync` throws, the catch returns "unknown" and leaves the stale `.install-source` on disk — contradicting the function's own "stale value must never be attributed" invariant, since a later install that writes the version marker but fails to write a source would pick up the leftover value. Separate the unlink from the read (e.g. unlink in its own try) so a valid method is still reported and the file is still removed when possible.</violation>
</file>
<file name="packages/opencode/test/install/install-telemetry.test.ts">
<violation number="1" location="packages/opencode/test/install/install-telemetry.test.ts:119">
P3: This test only saves, deletes, and restores `ALTIMATE_TELEMETRY_DISABLED`, but telemetry opt-out is also gated by `OPENCODE_DISABLE_TELEMETRY`. If that variable happens to be set in the CI/test environment, `Telemetry.init()` will exit before minting the machine ID, and the `existsSync(machineIdPath)` assertion will fail spuriously. Snapshot, delete, and restore `OPENCODE_DISABLE_TELEMETRY` alongside `ALTIMATE_TELEMETRY_DISABLED` in the same try/finally block for proper test isolation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| # Only reached when an install actually happened: check_version exits 0 early | ||
| # when the requested version is already present. | ||
| write_install_marker |
There was a problem hiding this comment.
P1: The new curl/powershell install markers cause a first_launch telemetry event and machine-id generation on next CLI launch. If Telemetry.init() executes before Instance.provide() loads Config.get() (e.g., cold start), and its error-handling path defaults to telemetry enabled, config-only telemetry.disabled settings (without an env var) will be bypassed for these newly-instrumented install paths. Verify Telemetry.init() fails closed when config is not yet available, or defer initialization until config is loaded, before relying on marker-triggered first_launch events here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At install, line 522:
<comment>The new curl/powershell install markers cause a first_launch telemetry event and machine-id generation on next CLI launch. If Telemetry.init() executes before Instance.provide() loads Config.get() (e.g., cold start), and its error-handling path defaults to telemetry enabled, config-only telemetry.disabled settings (without an env var) will be bypassed for these newly-instrumented install paths. Verify Telemetry.init() fails closed when config is not yet available, or defer initialization until config is loaded, before relying on marker-triggered first_launch events here.</comment>
<file context>
@@ -487,13 +487,40 @@ install_from_binary() {
+# Only reached when an install actually happened: check_version exits 0 early
+# when the requested version is already present.
+write_install_marker
+
</file context>
| local marker_version="${specific_version:-unknown}" | ||
| mkdir -p "$data_dir" 2>/dev/null || return 0 | ||
| printf '%s' "${marker_version#v}" > "$data_dir/.installed-version" 2>/dev/null || return 0 | ||
| printf '%s' "curl" > "$data_dir/.install-source" 2>/dev/null || return 0 |
There was a problem hiding this comment.
P2: When users use the documented ./install --binary ... mode, this line records curl even though the download branch was skipped. Write unknown for local-binary installs; otherwise first_launch.install_method misattributes them as curl.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At install, line 510:
<comment>When users use the documented `./install --binary ...` mode, this line records `curl` even though the download branch was skipped. Write `unknown` for local-binary installs; otherwise `first_launch.install_method` misattributes them as curl.</comment>
<file context>
@@ -487,13 +487,40 @@ install_from_binary() {
+ local marker_version="${specific_version:-unknown}"
+ mkdir -p "$data_dir" 2>/dev/null || return 0
+ printf '%s' "${marker_version#v}" > "$data_dir/.installed-version" 2>/dev/null || return 0
+ printf '%s' "curl" > "$data_dir/.install-source" 2>/dev/null || return 0
+}
+
</file context>
| printf '%s' "curl" > "$data_dir/.install-source" 2>/dev/null || return 0 | |
| if [ -n "$binary_path" ]; then | |
| printf '%s' "unknown" > "$data_dir/.install-source" 2>/dev/null || return 0 | |
| else | |
| printf '%s' "curl" > "$data_dir/.install-source" 2>/dev/null || return 0 | |
| fi |
|
|
||
| # Only reached when an install actually happened: check_version exits 0 early | ||
| # when the requested version is already present. | ||
| write_install_marker |
There was a problem hiding this comment.
P2: When --binary points to the already-installed file, install_from_binary copies nothing but this unconditional call still creates telemetry markers. Skip marker creation for that no-op path so the next launch does not emit a false first_launch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At install, line 522:
<comment>When `--binary` points to the already-installed file, `install_from_binary` copies nothing but this unconditional call still creates telemetry markers. Skip marker creation for that no-op path so the next launch does not emit a false `first_launch`.</comment>
<file context>
@@ -487,13 +487,40 @@ install_from_binary() {
+# Only reached when an install actually happened: check_version exits 0 early
+# when the requested version is already present.
+write_install_marker
+
</file context>
| write_install_marker | |
| if [ -z "$binary_path" ] || ! [ "$binary_path" -ef "${INSTALL_DIR}/$(basename "$binary_path")" ]; then | |
| write_install_marker | |
| fi |
| function readInstallMethod(dataDir: string): InstallMethod { | ||
| const sourcePath = path.join(dataDir, SOURCE_FILE) | ||
| try { | ||
| const raw = fs.readFileSync(sourcePath, "utf-8").trim() |
There was a problem hiding this comment.
P2: When the CLI starts between the two installer writes, this reader consumes the version marker without a committed source marker and misclassifies install_method. Publish both values through one atomic metadata record, or write a complete pair before exposing the version marker.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/cli/welcome.ts, line 30:
<comment>When the CLI starts between the two installer writes, this reader consumes the version marker without a committed source marker and misclassifies `install_method`. Publish both values through one atomic metadata record, or write a complete pair before exposing the version marker.</comment>
<file context>
@@ -9,6 +9,32 @@ import { Telemetry } from "../altimate/telemetry"
+function readInstallMethod(dataDir: string): InstallMethod {
+ const sourcePath = path.join(dataDir, SOURCE_FILE)
+ try {
+ const raw = fs.readFileSync(sourcePath, "utf-8").trim()
+ fs.unlinkSync(sourcePath)
+ return (INSTALL_METHODS as readonly string[]).includes(raw) ? (raw as InstallMethod) : "unknown"
</file context>
| const sourcePath = path.join(dataDir, SOURCE_FILE) | ||
| try { | ||
| const raw = fs.readFileSync(sourcePath, "utf-8").trim() | ||
| fs.unlinkSync(sourcePath) |
There was a problem hiding this comment.
P2: In readInstallMethod, the read and unlink share one try block. If readFileSync succeeds but unlinkSync throws, the catch returns "unknown" and leaves the stale .install-source on disk — contradicting the function's own "stale value must never be attributed" invariant, since a later install that writes the version marker but fails to write a source would pick up the leftover value. Separate the unlink from the read (e.g. unlink in its own try) so a valid method is still reported and the file is still removed when possible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/cli/welcome.ts, line 31:
<comment>In `readInstallMethod`, the read and unlink share one try block. If `readFileSync` succeeds but `unlinkSync` throws, the catch returns "unknown" and leaves the stale `.install-source` on disk — contradicting the function's own "stale value must never be attributed" invariant, since a later install that writes the version marker but fails to write a source would pick up the leftover value. Separate the unlink from the read (e.g. unlink in its own try) so a valid method is still reported and the file is still removed when possible.</comment>
<file context>
@@ -9,6 +9,32 @@ import { Telemetry } from "../altimate/telemetry"
+ const sourcePath = path.join(dataDir, SOURCE_FILE)
+ try {
+ const raw = fs.readFileSync(sourcePath, "utf-8").trim()
+ fs.unlinkSync(sourcePath)
+ return (INSTALL_METHODS as readonly string[]).includes(raw) ? (raw as InstallMethod) : "unknown"
+ } catch {
</file context>
| # An empty marker is deleted unread by the CLI, so fall back to "unknown" | ||
| # rather than losing the install: $specific_version is empty whenever the | ||
| # GitHub API could not be reached (see check_version). | ||
| local marker_version="${specific_version:-unknown}" |
There was a problem hiding this comment.
P2: Binary installs now report the literal version local in telemetry. On the --binary path specific_version="local" is set (install line 77), so write_install_marker writes .installed-version = local. welcome.ts then emits first_launch with version: "local" (and the banner reads vlocal installed). Since this change is specifically about counting/measuring installs, local pollutes the version dimension for every --binary install. Either skip the marker on the binary path, or map it to unknown rather than local.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At install, line 507:
<comment>Binary installs now report the literal version `local` in telemetry. On the `--binary` path `specific_version="local"` is set (install line 77), so `write_install_marker` writes `.installed-version` = `local`. welcome.ts then emits `first_launch` with `version: "local"` (and the banner reads `vlocal installed`). Since this change is specifically about *counting/measuring* installs, `local` pollutes the version dimension for every `--binary` install. Either skip the marker on the binary path, or map it to `unknown` rather than `local`.</comment>
<file context>
@@ -487,13 +487,40 @@ install_from_binary() {
+ # An empty marker is deleted unread by the CLI, so fall back to "unknown"
+ # rather than losing the install: $specific_version is empty whenever the
+ # GitHub API could not be reached (see check_version).
+ local marker_version="${specific_version:-unknown}"
+ mkdir -p "$data_dir" 2>/dev/null || return 0
+ printf '%s' "${marker_version#v}" > "$data_dir/.installed-version" 2>/dev/null || return 0
</file context>
| # version it was installed at. Whether anything is ever sent remains entirely up | ||
| # to the CLI's existing telemetry opt-out gates. | ||
| write_install_marker() { | ||
| local data_dir="${XDG_DATA_HOME:-$HOME/.local/share}/altimate-code" |
There was a problem hiding this comment.
P2: On the Windows bash path this marker lands where the CLI never reads it. The install script explicitly supports Windows (MINGW/MSYS/CYGWIN resolve os="windows", seen around line 92), but write_install_marker resolves the data dir from $HOME, while welcome.ts resolves it via Node's os.homedir() (getDataDir(): process.env.XDG_DATA_HOME || path.join(os.homedir(), ".local", "share")). Under MSYS2/Cygwin $HOME is the POSIX home (/home/<user>), which does not match Windows' os.homedir() (%USERPROFILE%), so the .installed-version/.install-source files would be written to a location the CLI never checks — the exact silent failure this PR intends to fix. The in-diff comment claims the path "MUST match ... on every platform, including Windows", which is not guaranteed on the bash path. Consider deriving the fallback from the user profile on the Windows branches (e.g. test "$os" = windows using $USERPROFILE instead of $HOME), or document/limit the bash installer's Windows support.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At install, line 503:
<comment>On the Windows bash path this marker lands where the CLI never reads it. The `install` script explicitly supports Windows (MINGW/MSYS/CYGWIN resolve `os="windows"`, seen around line 92), but `write_install_marker` resolves the data dir from `$HOME`, while welcome.ts resolves it via Node's `os.homedir()` (`getDataDir()`: `process.env.XDG_DATA_HOME || path.join(os.homedir(), ".local", "share")`). Under MSYS2/Cygwin `$HOME` is the POSIX home (`/home/<user>`), which does not match Windows' `os.homedir()` (`%USERPROFILE%`), so the `.installed-version`/`.install-source` files would be written to a location the CLI never checks — the exact silent failure this PR intends to fix. The in-diff comment claims the path "MUST match ... on every platform, including Windows", which is not guaranteed on the bash path. Consider deriving the fallback from the user profile on the Windows branches (e.g. `test "$os" = windows` using `$USERPROFILE` instead of `$HOME`), or document/limit the bash installer's Windows support.</comment>
<file context>
@@ -487,13 +487,40 @@ install_from_binary() {
+# version it was installed at. Whether anything is ever sent remains entirely up
+# to the CLI's existing telemetry opt-out gates.
+write_install_marker() {
+ local data_dir="${XDG_DATA_HOME:-$HOME/.local/share}/altimate-code"
+ # An empty marker is deleted unread by the CLI, so fall back to "unknown"
+ # rather than losing the install: $specific_version is empty whenever the
</file context>
| // every install would then report is_upgrade: true and brand-new installs would vanish | ||
| // from the metric without a single test failing. Hence this test. | ||
| const origCs = process.env.APPLICATIONINSIGHTS_CONNECTION_STRING | ||
| const origDisabled = process.env.ALTIMATE_TELEMETRY_DISABLED |
There was a problem hiding this comment.
P3: This test only saves, deletes, and restores ALTIMATE_TELEMETRY_DISABLED, but telemetry opt-out is also gated by OPENCODE_DISABLE_TELEMETRY. If that variable happens to be set in the CI/test environment, Telemetry.init() will exit before minting the machine ID, and the existsSync(machineIdPath) assertion will fail spuriously. Snapshot, delete, and restore OPENCODE_DISABLE_TELEMETRY alongside ALTIMATE_TELEMETRY_DISABLED in the same try/finally block for proper test isolation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/install/install-telemetry.test.ts, line 119:
<comment>This test only saves, deletes, and restores `ALTIMATE_TELEMETRY_DISABLED`, but telemetry opt-out is also gated by `OPENCODE_DISABLE_TELEMETRY`. If that variable happens to be set in the CI/test environment, `Telemetry.init()` will exit before minting the machine ID, and the `existsSync(machineIdPath)` assertion will fail spuriously. Snapshot, delete, and restore `OPENCODE_DISABLE_TELEMETRY` alongside `ALTIMATE_TELEMETRY_DISABLED` in the same try/finally block for proper test isolation.</comment>
<file context>
@@ -0,0 +1,153 @@
+ // every install would then report is_upgrade: true and brand-new installs would vanish
+ // from the metric without a single test failing. Hence this test.
+ const origCs = process.env.APPLICATIONINSIGHTS_CONNECTION_STRING
+ const origDisabled = process.env.ALTIMATE_TELEMETRY_DISABLED
+ const tmpHome = mkdtempSync(join(os.tmpdir(), "install-telemetry-home-"))
+ spyOn(os, "homedir").mockImplementation(() => tmpHome)
</file context>
1 participant
Fixes AI-8448.
The dip was measurement, not installs
first_launchis the only install metric. It fires off a marker file rather than any network call from the installer — and that marker was written in exactly one place,packages/opencode/script/postinstall.mjs. Neitherinstallnorinstall.ps1wrote it, so once the advertised path moved from npm toaltimate.sh/install, those installs stopped being counted. Nothing changed about how many people were installing.What this does
Both shell installers now write the same marker
postinstall.mjswrites, andfirst_launchcarries a newinstall_method(curl|powershell|npm|unknown) so the recovered volume is separable from npm instead of folded into one number.altimate upgradeon the curl path re-runsinstall, so curl upgrades become visible too.Brand-new installs stay
is_upgrade: false— that field probes whether~/.altimate/machine-idexisted before this launch:Expect
install_method: "unknown"for the first upgrade after this ships — those markers predate the field.Details that fail silently rather than loudly
$XDG_DATA_HOME, default~/.local/share/altimate-code, on every platform including Windows.welcome.tsresolves the data dir through Node'sos.homedir()and never consults%LOCALAPPDATA%; a marker written there would be ignored at read time.-Encoding asciiininstall.ps1. The documented entrypoint ispowershell -c "irm ... | iex"— Windows PowerShell 5.1, where-Encoding utf8prepends a BOM..trim()does strip a leading BOM (U+FEFF is JS whitespace), so this was latent rather than broken, butinstall_methodis matched against a fixed allowlist and shouldn't depend on that.unknownversion fallback. An empty marker is deleted unread, so an unresolved version would lose the install outright. That's the statecheck_versionleaves whenever the GitHub API is unreachable.check_versionexits 0 early) reports no install, and neither does a failed download.install_method. A hand-edited or truncated source file readsunknownrather than minting a new dimension.$HOMEcosts the event, never the install.Privacy
No new network call and no new identifier. The installers record a version and their own name to a local file; the CLI's existing opt-out gates (
ALTIMATE_TELEMETRY_DISABLED,OPENCODE_DISABLE_TELEMETRY,telemetry.disabled) still decide whether anything is transmitted.docs/docs/reference/security-faq.mdanddocs/docs/reference/telemetry.mdare updated to say so.Tests
Followed the touchpoint set from #1064 (event union → docs → emitter → unit tests → install-script assertions).
test/cli/welcome.test.ts—is_upgradeboth ways,install_methodattribution, allowlist rejection, source-file consumption, empty-marker path.test/install/install-telemetry.test.ts— marker path/fallback/ordering/non-fatality for both installers, no-BOM, plus the ordering invariant below.test/install/postinstall.test.ts— npm writes.install-source.The load-bearing test is the ordering invariant.
is_upgradeis only correct becausesrc/index.tsfiresTelemetry.init()unawaited anddoInit()yields atawait Config.get()before minting the machine-id, so the synchronous banner call on the next line still sees pre-launch state. Anawaitadded ahead of that mint would make every install reportis_upgrade: trueand silently empty the brand-new-install metric without a single existing test failing. The test asserts the machine-id is absent at that instant and present once the promise resolves, so it can't pass vacuously.598 pass / 0 fail across
test/cli/welcome.test.ts test/install/ test/telemetry/telemetry.test.ts test/branding/; typecheck clean.Verification
install's marker writer was executed directly: XDG override honored,vprefix stripped,unknownfallback, exit 0 on a read-only$HOME.install.ps1is asserted at source level only — nopwshon the dev machine, so its runtime behavior rides on CI.Not in scope
Installs that never launch the CLI remain uncounted, so download→launch conversion is still unmeasurable. That needs a beacon from the install script itself — a new event plus opt-out handling in bash — and is deliberately deferred.
Two pre-existing things noticed but left alone:
welcome.ts:70returns before printing the welcome box whenisUpgradeis false, so the box only ever shows on upgrades. Plausibly intentional (the TUI has its own first-run flow), but it reads backwards for a "welcome" banner.test/altimate/review/telemetry.test.tsredirects$HOMEto keep the suite from minting a machine-id in the developer's real home — but Bun resolvesos.homedir()at startup and ignores laterprocess.env.HOMEmutation, so that protection doesn't currently work. This PR's tests usespyOn(os, "homedir"), the convention already used intest/mcp/discover.test.ts.🤖 Generated with Claude Code
Summary by cubic
Counts installs from the shell installers and attributes their source in telemetry. Previously only npm
postinstall.mjswrote the install marker, so installs viaaltimate.sh/installandinstall.ps1were uncounted; now both write the same marker and the CLI sendsfirst_launchwithinstall_methodto separatecurl/powershell/npmvolume. Aligns with AI-8448.installandinstall.ps1: write.installed-versionand.install-sourceto$XDG_DATA_HOME/altimate-code(fallback~/.local/share/altimate-code), after a successful install; use"unknown"when version cannot be resolved; writes are non-fatal. PowerShell uses-Encoding asciito avoid BOM.packages/opencode/script/postinstall.mjs: also writes.install-source("npm").packages/opencode/src/cli/welcome.ts: reads and consumes.install-source, allowlistscurl/powershell/npm, falls back to"unknown", and includesinstall_methodonfirst_launch.packages/opencode/src/altimate/telemetry/index.ts: addsinstall_methodto thefirst_launchevent schema.docs/docs/reference/security-faq.md,docs/docs/reference/telemetry.md) to clarify installers send no telemetry themselves.is_upgradecorrect; do not await telemetry init before the welcome banner.install_method: "unknown"for pre-existing markers; no user action required.Written for commit 10b2df6. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes