-
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
base: main
Are you sure you want to change the base?
Changes from all commits
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,13 +487,40 @@ 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. | ||||||||||||||
| 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}" | ||||||||||||||
|
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 | ||||||||||||||
| 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 | ||||||||||||||
|
Comment on lines
+502
to
+510
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. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift Fail closed for config-only telemetry opt-out before enabling shell-install telemetry. If a user sets 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.
Based on learnings, the config-only telemetry opt-out cold-start gap occurs when 📍 Affects 3 files
🤖 Prompt for AI AgentsSource: Learnings 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: When users use the documented Prompt for AI agents
Suggested change
|
||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if [ -n "$binary_path" ]; then | ||||||||||||||
| install_from_binary | ||||||||||||||
| else | ||||||||||||||
| check_version | ||||||||||||||
| download_and_install | ||||||||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION:
Suggested change
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. 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 agentsThere 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: When Prompt for AI agents
Suggested change
|
||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| add_to_path() { | ||||||||||||||
| local config_file=$1 | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,32 @@ import { Telemetry } from "../altimate/telemetry" | |
|
|
||
| const APP_NAME = "altimate-code" | ||
| const MARKER_FILE = ".installed-version" | ||
| // altimate_change start — written alongside MARKER_FILE by whichever installer ran | ||
| // (postinstall.mjs, install, install.ps1) so first_launch can attribute the install. | ||
| const SOURCE_FILE = ".install-source" | ||
| const INSTALL_METHODS = ["curl", "powershell", "npm"] as const | ||
| type InstallMethod = (typeof INSTALL_METHODS)[number] | "unknown" | ||
|
|
||
| /** | ||
| * Read the installer that wrote the marker, then remove the file so it stays in | ||
| * lockstep with MARKER_FILE — a stale value must never be attributed to a later | ||
| * install whose installer did not write one. | ||
| * | ||
| * Returns "unknown" for a missing, unreadable, or unrecognized value: the marker | ||
| * predates this field on upgrade from an older version, and an unrecognized | ||
| * string must not reach the event as a free-form value. | ||
| */ | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: When the CLI starts between the two installer writes, this reader consumes the version marker without a committed source marker and misclassifies Prompt for AI agents |
||
| fs.unlinkSync(sourcePath) | ||
|
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: In Prompt for AI agents |
||
| return (INSTALL_METHODS as readonly string[]).includes(raw) ? (raw as InstallMethod) : "unknown" | ||
| } catch { | ||
| return "unknown" | ||
| } | ||
| } | ||
| // altimate_change end | ||
|
|
||
| /** Resolve the data directory at call time (respects XDG_DATA_HOME changes in tests). */ | ||
| function getDataDir(): string { | ||
|
|
@@ -27,12 +53,18 @@ function getDataDir(): string { | |
| */ | ||
| export function showWelcomeBannerIfNeeded(): void { | ||
| try { | ||
| const markerPath = path.join(getDataDir(), MARKER_FILE) | ||
| const dataDir = getDataDir() | ||
| const markerPath = path.join(dataDir, MARKER_FILE) | ||
| if (!fs.existsSync(markerPath)) return | ||
|
|
||
| const installedVersion = fs.readFileSync(markerPath, "utf-8").trim() | ||
| if (!installedVersion) { | ||
| fs.unlinkSync(markerPath) | ||
| // altimate_change — clear the companion file too, so an orphaned source value | ||
| // cannot be attributed to a later install. Both install scripts write "unknown" | ||
| // rather than an empty version, so this path should now only be reachable from | ||
| // a truncated or hand-edited marker. | ||
| readInstallMethod(dataDir) | ||
| return | ||
| } | ||
|
|
||
|
|
@@ -64,6 +96,7 @@ export function showWelcomeBannerIfNeeded(): void { | |
| session_id: "", | ||
| version: installedVersion, | ||
| is_upgrade: isUpgrade, | ||
| install_method: readInstallMethod(dataDir), | ||
| }) | ||
| // altimate_change end | ||
|
|
||
|
|
||
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.
P2: On the Windows bash path this marker lands where the CLI never reads it. The
installscript explicitly supports Windows (MINGW/MSYS/CYGWIN resolveos="windows", seen around line 92), butwrite_install_markerresolves the data dir from$HOME, while welcome.ts resolves it via Node'sos.homedir()(getDataDir():process.env.XDG_DATA_HOME || path.join(os.homedir(), ".local", "share")). Under MSYS2/Cygwin$HOMEis the POSIX home (/home/<user>), which does not match Windows'os.homedir()(%USERPROFILE%), so the.installed-version/.install-sourcefiles 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" = windowsusing$USERPROFILEinstead of$HOME), or document/limit the bash installer's Windows support.Prompt for AI agents