-
Notifications
You must be signed in to change notification settings - Fork 134
feat: [AI-8448] count installs from the shell installers, not just npm #1096
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
10b2df6
c70b2ec
7cf575e
df22213
b69530b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -487,11 +487,55 @@ install_from_binary() { | |
| chmod 755 "$dest_path" | ||
| } | ||
|
|
||
| # Write the same post-install marker that npm's postinstall.mjs writes, so the | ||
| # CLI emits its `first_launch` telemetry event on the next run. Without this the | ||
| # curl install path — the one advertised at altimate.sh/install — produces no | ||
| # install event at all, and every curl user is invisible in install metrics. | ||
| # | ||
| # The path MUST match welcome.ts's data-dir resolution ($XDG_DATA_HOME, falling | ||
| # back to ~/.local/share) on every platform, including Windows: the CLI reads it | ||
| # via Node's os.homedir() and never consults %LOCALAPPDATA%. | ||
| # | ||
| # No network call and no identifier is written here — this only hands the CLI the | ||
| # version it was installed at. Whether anything is ever sent remains entirely up | ||
| # to the CLI's existing telemetry opt-out gates. | ||
| # $1 — install_method to record. Must be a value in the CLI's allowlist | ||
| # (packages/opencode/src/cli/welcome.ts); anything else reports as "unknown". | ||
| write_install_marker() { | ||
| local marker_source="$1" | ||
| local data_dir="${XDG_DATA_HOME:-$HOME/.local/share}/altimate-code" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: On the Windows bash path this marker lands where the CLI never reads it. The Prompt for AI agents |
||
| # 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Binary installs now report the literal version Prompt for AI agents |
||
| mkdir -p "$data_dir" 2>/dev/null || return 0 | ||
| # Companion first, trigger last: the CLI returns early unless .installed-version | ||
| # exists, then consumes .install-source. Trigger-first would let a CLI starting in | ||
| # between report install_method "unknown", and a truncated .installed-version is | ||
| # deleted unread — losing the install rather than just its attribution. | ||
| printf '%s' "$marker_source" > "$data_dir/.install-source" 2>/dev/null || return 0 | ||
| # The trigger is published atomically. Companion-first alone only closes the | ||
| # "attribution lost" window; a plain redirect truncates before filling, so a CLI | ||
| # starting mid-write can still observe an EMPTY .installed-version, which it | ||
| # deletes unread — losing the install itself. mv within one directory is atomic. | ||
| local tmp="$data_dir/.installed-version.$$" | ||
| printf '%s' "${marker_version#v}" > "$tmp" 2>/dev/null || return 0 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Non-blocking (NIT) — the temp is not cleaned when the temp write itself fails
The same gap exists in the other two writers, where nothing is cleaned on either write or rename failure. A single best-effort cleanup path covering both stages would cover all three. |
||
| mv -f "$tmp" "$data_dir/.installed-version" 2>/dev/null || { rm -f "$tmp" 2>/dev/null; return 0; } | ||
| } | ||
|
|
||
| if [ -n "$binary_path" ]; then | ||
| install_from_binary | ||
| # Attributed as "local", not "curl": --binary installs a file the caller already | ||
| # had (dev build, air-gapped artifact) and sets specific_version="local", so | ||
| # folding it into the curl metric would misreport both source and version. | ||
| # Still recorded — it is a real install — just not a curl one. | ||
| write_install_marker "local" | ||
| else | ||
| check_version | ||
| download_and_install | ||
| # Only reached when an install actually happened: check_version exits 0 early | ||
| # when the requested version is already present. | ||
| write_install_marker "curl" | ||
| fi | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -304,6 +304,71 @@ if (-not $needsBaseline) { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| # --------------------------------------------------------------------------- | ||||||
| # Post-install marker (install telemetry) | ||||||
| # --------------------------------------------------------------------------- | ||||||
| # Mirrors npm's postinstall.mjs (and ./install's write_install_marker) so the CLI | ||||||
| # emits its `first_launch` event on the next run; without it this install path is | ||||||
| # invisible in install metrics. | ||||||
| # | ||||||
| # A function, not inline code, so the Pester suite can AST-extract and execute it | ||||||
| # against a temp profile the same way it does Test-Checksum. The subprocess tests | ||||||
| # deliberately stop the installer before this point, so inline code here would have | ||||||
| # no runtime coverage on the riskiest of the writers. | ||||||
| function Write-InstallMarker { | ||||||
| param([string]$Version) | ||||||
|
|
||||||
| # The directory MUST match welcome.ts's resolution - $XDG_DATA_HOME, else | ||||||
| # <home>\.local\share - because the CLI reads it through Node's os.homedir() and | ||||||
| # never looks at %LOCALAPPDATA%. Writing to LOCALAPPDATA here would be silently | ||||||
| # ignored at read time. | ||||||
| # | ||||||
| # No network call and no identifier is written; only the installed version is | ||||||
| # recorded. The CLI's existing telemetry opt-out gates still decide whether | ||||||
| # anything is ever sent. | ||||||
| # | ||||||
| # EVERYTHING is inside the try, path computation included. $ErrorActionPreference is | ||||||
| # "Stop", and Join-Path resolves provider-qualified paths - so a null or empty | ||||||
| # $env:USERPROFILE (pwsh on non-Windows, a stripped service profile) or an | ||||||
| # XDG_DATA_HOME naming a non-existent PSDrive raises a TERMINATING error. Computed | ||||||
| # outside the try, that error would abort the installer after the binary is placed | ||||||
| # but before the PATH registry write and the "Get started" output, leaving the user | ||||||
| # with an installed binary that is not on PATH. [IO.Path]::Combine also keeps | ||||||
| # PSDrive resolution out of it entirely. | ||||||
| try { | ||||||
| $dataRoot = if ($env:XDG_DATA_HOME) { $env:XDG_DATA_HOME } else { [IO.Path]::Combine($env:USERPROFILE, ".local", "share") } | ||||||
| $dataDir = [IO.Path]::Combine($dataRoot, "altimate-code") | ||||||
| New-Item -ItemType Directory -Force -Path $dataDir | Out-Null | ||||||
| # The CLI deletes an empty marker without reporting, so fall back to "unknown" | ||||||
| # when the version could not be resolved (GitHub API unreachable). | ||||||
| $markerVersion = if ($Version) { $Version -replace '^v', '' } else { "unknown" } | ||||||
| # -NoNewline: the CLI trims, but keep the file byte-identical to the npm path. | ||||||
| # | ||||||
| # -Encoding ascii, not utf8: the documented entrypoint is `powershell -c "irm ... | iex"`, | ||||||
| # i.e. Windows PowerShell 5.1, where `-Encoding utf8` prepends a UTF-8 BOM. Both values are | ||||||
| # ASCII by construction, so ascii is lossless here and cannot emit one. The CLI's .trim() | ||||||
| # happens to strip a leading BOM (U+FEFF is JS whitespace), but the install-source value is | ||||||
| # matched against a fixed allowlist and must not depend on that. | ||||||
| # | ||||||
| # Companion first, trigger last. The CLI returns early unless .installed-version | ||||||
| # exists, then consumes .install-source - so writing the trigger first would let a | ||||||
| # CLI starting in between report install_method "unknown". Set-Content also | ||||||
| # truncates before writing, and an empty .installed-version is deleted unread, | ||||||
| # which would lose the install outright. | ||||||
| Set-Content -Path ([IO.Path]::Combine($dataDir, ".install-source")) -Value "powershell" -NoNewline -Encoding ascii | ||||||
| # Trigger published atomically: Set-Content truncates before writing, so a CLI | ||||||
| # starting mid-write could observe an EMPTY .installed-version and delete it | ||||||
| # unread, losing the install. Move-Item within one directory is atomic. | ||||||
| $tmpMarker = [IO.Path]::Combine($dataDir, ".installed-version.tmp") | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Temp marker file is left behind when The bash installer removes its temp file on a failed publish ( Reply with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The temp marker uses a fixed name ( Prompt for AI agents
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Non-blocking (MINOR) — fixed temp name, and no temp cleanup in the
Concurrent There is also no cleanup of the temp when $tmpMarker = [IO.Path]::Combine($dataDir, ".installed-version.$PID.tmp")and in the Remove-Item -Force -ErrorAction SilentlyContinue $tmpMarkerA |
||||||
| Set-Content -Path $tmpMarker -Value $markerVersion -NoNewline -Encoding ascii | ||||||
| Move-Item -Force -Path $tmpMarker -Destination ([IO.Path]::Combine($dataDir, ".installed-version")) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: When Move-Item fails (destination locked, transient IO error), the catch silently swallows it and leaves Prompt for AI agents |
||||||
| } catch { | ||||||
| # Non-fatal - a missing marker only costs us the install event, never the install. | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| Write-InstallMarker -Version $specificVersion | ||||||
|
|
||||||
| # --------------------------------------------------------------------------- | ||||||
| # PATH (user scope, via registry + broadcast) | ||||||
| # --------------------------------------------------------------------------- | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -238,7 +238,20 @@ function writeUpgradeMarker(version) { | |||||||||||||||||||||||
| const xdgData = process.env.XDG_DATA_HOME || path.join(os.homedir(), ".local", "share") | ||||||||||||||||||||||||
| const dataDir = path.join(xdgData, "altimate-code") | ||||||||||||||||||||||||
| fs.mkdirSync(dataDir, { recursive: true }) | ||||||||||||||||||||||||
| fs.writeFileSync(path.join(dataDir, ".installed-version"), version.replace(/^v/, "")) | ||||||||||||||||||||||||
| // Companion first, trigger last. `.installed-version` is what the CLI keys on: | ||||||||||||||||||||||||
| // it returns early unless that file exists, then consumes `.install-source`. | ||||||||||||||||||||||||
| // Trigger-first left two windows — a CLI starting in between reports | ||||||||||||||||||||||||
| // install_method "unknown", and writeFileSync truncates before writing, so a | ||||||||||||||||||||||||
| // reader could observe an EMPTY `.installed-version` and delete it unread, | ||||||||||||||||||||||||
| // losing the install outright. Matches `install` and `install.ps1`. | ||||||||||||||||||||||||
| fs.writeFileSync(path.join(dataDir, ".install-source"), "npm") | ||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 BLOCKING MAJOR — the write-order fix was not applied to npm, though the commit says it was The response commit states: "Companion written before trigger, in all three writers." Two were changed. npm still writes the trigger first: fs.writeFileSync(path.join(dataDir, ".installed-version"), version.replace(/^v/, "")) // trigger
fs.writeFileSync(path.join(dataDir, ".install-source"), "npm") // companionBoth failure modes closed for
npm is the only channel that was ever counted before this PR, so this is not a leftover on a dead path. Both new order tests are per-writer source assertions ( Fix: swap the two lines, and add the matching assertion to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in b69530b — and you were right that the commit message overclaimed.
While fixing it I found the reorder was only half the fix. Companion-first closes the "attribution lost" window; it does nothing about the truncation window you also named. A plain write truncates before filling, so a reader mid-write can still observe an empty All four writers now publish the trigger via temp+rename: |
||||||||||||||||||||||||
| // Trigger published atomically: writeFileSync truncates before filling, so a CLI | ||||||||||||||||||||||||
| // starting mid-write could observe an EMPTY `.installed-version` and delete it | ||||||||||||||||||||||||
| // unread, losing the install. renameSync within one directory is atomic. | ||||||||||||||||||||||||
| const versionPath = path.join(dataDir, ".installed-version") | ||||||||||||||||||||||||
| const tmpPath = `${versionPath}.${process.pid}.tmp` | ||||||||||||||||||||||||
| fs.writeFileSync(tmpPath, version.replace(/^v/, "")) | ||||||||||||||||||||||||
| fs.renameSync(tmpPath, versionPath) | ||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Temp marker file is left behind when Unlike Reply with
Comment on lines
+253
to
+254
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: When the temp write or rename fails, the outer catch swallows the error without deleting Prompt for AI agents
Suggested change
|
||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||
| // Non-fatal — the CLI just won't show a welcome banner | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Non-blocking (NIT) — the new
unknowngloss overstates when it occurs"what every upgrade from a version predating this field reports" is not quite right. An upgrade performed by a current curl / PowerShell / npm writer records its current method — the writer is what decides, not the version being upgraded from.
unknownis what the reader falls back to when the companion is missing, unreadable, or unrecognised. The predating-the-field case reaches it only through an unconsumed marker written before the field existed.Listing
unknowncloses m7; only the explanation needs narrowing. Something like: "reported when the installer wrote no source marker, or wrote one this version does not recognise."