feat: append cli_context param to altimate auth URL for PostHog session correlation

[altimate-harness-bot]

Summary

Appends a cli_context query param to the browser auth URL opened by the CLI during sign-in.

• buildCliContext() encodes { v: 1, machine_id, cli_version } as a base64url JSON blob
• machine_id is read from ~/.altimate/machine-id (random UUID, non-PII) — the same value already sent to Azure App Insights telemetry
• On the frontend, cli_context is decoded and passed to posthog.register() + posthog.alias() to link the CLI device to the authenticated user

Companion PR: AltimateAI/altimate-frontend#3106

Requested by @saravmajestic via harness

---

Summary by cubic

Adds a base64url-encoded cli_context to the CLI auth URL (now in the URL fragment) so the web app can link the browser session to the CLI device in PostHog. Uses a shared, race‑safe machine_id helper with UUID v4 validation and an enforced 512‑byte cap; honors both env and config telemetry opt‑out.

• Refactors

  • Extracted getOrCreateMachineId() to altimate/util/machine-id.ts (exclusive-create, UUID v4 check, rejects symlinks/non-regular files; bounded descriptor read enforces the 512‑byte limit, including on race re-reads; returns "" and logs on errors).
  • Updated telemetry, altimate plugin, and cli/welcome to use it; buildCliContext() respects ALTIMATE_TELEMETRY_DISABLED=true and config.telemetry.disabled, fails closed (and logs) if config is unreadable; cli_context appended via buildAuthorizeUrl() in the URL fragment; docs updated to clarify the device/installation ID and PostHog vs App Insights; added tests.

• Bug Fixes

  • Fixed fresh‑install detection in cli/welcome.ts: probe file existence only (no minting), avoiding ID creation for config‑opt‑out users.
  • Prevented auth failures on read‑only $HOME by handling directory creation errors and returning an empty ID instead of throwing.

^{Written for commit 346df2c. Summary will update on new commits.}

[github-actions]

This PR doesn't fully meet our contributing guidelines and PR template.

What needs to be fixed:

• PR description is missing required template sections. Please use the PR template.

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.

[claude]

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.}

[sahrizvi]

Review summary

Verdict: request changes — 0 critical, 4 major, 6 minor, 3 nits.

The mechanics here are right: base64url is the correct codec (unpadded, no +//, so no escaping surprises), the payload is version-tagged with v: 1 before anyone needs it, and a failed read never blocks sign-in. Exporting buildCliContext with an injectable path also makes it testable against the real filesystem instead of a mocked fs.

Two things block merge. First, the change transmits a persistent device identifier in a way the product's own published privacy documentation says it will not. Second, the feature can silently fail to do the one thing it exists to do, because it reads a file that only the telemetry module knows how to create.

Detailed findings are inline. Below is what has no single line to attach to.

Documentation changes needed (files not in this diff)

• docs/docs/reference/telemetry.md:150 and docs/docs/reference/security-faq.md:133 state, without qualification, that "Both identifiers are only sent when telemetry is enabled." This PR sends the machine id on the auth URL regardless of the telemetry setting. Either gate the parameter on the opt-out or correct the promise — the current combination is a written commitment the code does not keep.
• docs/docs/reference/telemetry.md:147 and docs/docs/reference/security-faq.md:132 describe the machine id as serving "only to distinguish one machine from another in aggregate analytics" and "NOT tied to ... identity". Linking the CLI device to an authenticated user is worth disclosing there.

Cross-repo contract to confirm on the companion frontend PR

• Decode as base64url, not standard base64.
• Require v === 1; reject unknown versions rather than best-effort parsing.
• Cap decoded length and catch base64/JSON parse failures — this param is attacker-suppliable.
• Treat cli_version: "local" as a legitimate development value, not a release version.
• Decide explicitly what an empty or absent machine_id means. If it means "do not alias", enforce that — aliasing on an empty value would merge unrelated anonymous sessions into a single identity.

Nits (non-blocking)

• Sync I/O on the interactive auth path — readFileSync inside an async authorize(). The file is tiny and telemetry/index.ts already reads it synchronously, so this matches existing habit; worth changing only if the shared machine-id helper ends up async.
• Manual URL concatenation — the browser-OAuth plugins in this repo build authorize URLs with URLSearchParams (src/plugin/xai.ts, codex.ts, digitalocean.ts, snowflake-cortex.ts). The manual style predates this PR, which adds a second hand-escaped param to it. Follow-up cleanup, not a blocker.
• Weak version assertion — expect(typeof ctx["cli_version"]).toBe("string") does catch the key being dropped or renamed, so it is not vacuous, but it never checks the value. expect(ctx["cli_version"]).toBe(InstallationVersion) is strictly stronger. (InstallationVersion is typeof-guarded at packages/core/src/installation/version.ts:7 and can never be a non-string.)

Considered and not raised

• The optional machineIdPath? parameter is a legitimate testability seam, not a smell — a test-only env var would be worse, since it makes test behaviour reachable in production builds.
• The oversized-file concern is not a denial-of-service vector; the file sits in the user's own home directory. The real consequence is a broken auth URL, which is covered inline.
• The first-run window where telemetry has not yet written the machine id is real but narrow: src/index.ts:126 starts Telemetry.init() at CLI startup, long before a human clicks through sign-in. It is fire-and-forget and the write sits behind two awaits, so the race exists — but the deterministic failure is the opt-out path, not this.

Verified locally: bun test test/altimate/altimate-plugin.test.ts -> 3 pass, 7 expect calls.

[sahrizvi]

MAJOR — Logic Error / Design: machine-id lifecycle is split; this reads, only telemetry creates

buildCliContext() reads ~/.altimate/machine-id but never creates it. The only writer is Telemetry.doInit() (telemetry/index.ts:1702-1722), which carries race-safe exclusive-create (flag: "wx") logic this code does not share.

When the file is absent, this yields machine_id: "" while telemetry goes on to mint and use a real UUID — so the browser session and the CLI device carry different identities, and the correlation this PR exists to provide silently fails.

The path path.join(os.homedir(), ".altimate", "machine-id") is now constructed independently in three places:

• telemetry/index.ts:1702 — read + create
• plugin/altimate.ts:61 — read only (new here)
• cli/welcome.ts:46 — existence check

Fix: extract one shared helper with the existing read-or-create wx semantics and use it at all three call sites.

Note this alone does not resolve the opt-out issue or the empty-value issue flagged separately — the helper also needs to carry the consent decision and return an optional, validated id.

[sahrizvi]

MAJOR — Security (privacy): the machine ID is sent even when telemetry is disabled, contradicting the published docs

Telemetry.doInit() returns early — before the machine-id block — when ALTIMATE_TELEMETRY_DISABLED=true or config.telemetry.disabled is set (telemetry/index.ts:1667-1679). buildCliContext() checks neither, so this parameter is appended unconditionally.

A user who opted out but has a machine-id file from an earlier run still transmits that stable device identifier on every sign-in, specifically for product analytics.

The shipped documentation promises the opposite without qualification:

• docs/docs/reference/telemetry.md:150 — "Both identifiers are only sent when telemetry is enabled."
• docs/docs/reference/security-faq.md:133 — "Both identifiers are only sent when telemetry is enabled."

Fix: resolve the opt-out through the same config/env path telemetry uses, and omit machine_id entirely when the user has opted out.

(The mirror case — opted out with no pre-existing file, so machine_id is permanently "" — is reasonable behaviour, but it should be a deliberate documented decision rather than a side effect.)

[sahrizvi]

MAJOR — Security: a persistent device identifier lives in a URL query parameter

Query params land in browser history, app.myaltimate.com access logs, any CDN/WAF in front of it, the clipboard when a user copies this URL for an SSH/tmux sign-in, and potentially a Referer header if /register loads third-party resources.

That is a durable copy of a device identifier scattered across systems that have no retention policy for it — unlike the telemetry pipeline, which does.

Fix (any of):

• Confirm the param is scrubbed from access logs and that /register sets a restrictive Referrer-Policy.
• Move the payload to a URL fragment (#cli_context=...) — never transmitted to the server, still readable by the page, which fits this use case exactly.
• Stronger long-term: send a short-lived correlation token that maps to the device server-side, rather than the durable identifier itself.

[sahrizvi]

MAJOR — Bug / Security: no size or format validation on contents copied into the URL

readFileSync(idPath, "utf8").trim() accepts whatever is on disk — a multi-megabyte file, a symlink to another file, binary data (silently producing U+FFFD), embedded newlines — and all of it is base64-encoded into the authorize URL.

• A corrupt or oversized file produces a URL past browser/proxy length limits (~2 KB on older stacks, 8 KB on many servers), turning a working sign-in into an opaque browser error. The read is fail-open for missing files but not for malformed ones.
• A symlink planted at ~/.altimate/machine-id copies another file's contents into a URL sent to and logged by Altimate's servers. Not a privilege-boundary break — anyone who can write that path already controls the account — but a real exfiltration primitive that validation removes for free.

Fix: use lstat (not stat, which follows symlinks and would still accept a symlink to a regular file), reject anything oversized or not a regular file, then validate shape:

const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i
machineId = UUID_RE.test(raw) ? raw : ""

The canonical writer at telemetry/index.ts:1713 uses randomUUID(), so a UUID check rejects nothing legitimate.

[sahrizvi]

MINOR — Documentation: the comment does not disclose the CLI-to-account linkage

The raw value is indeed still a random UUID, so "never an email or real identity" is literally true. But the stated purpose of this change is to link that device to an authenticated user in analytics, and the user-facing docs describe the identifier as "purely random and serves only to distinguish one machine from another in aggregate analytics" (docs/docs/reference/telemetry.md:147) and "NOT tied to your hardware, OS, or identity" (docs/docs/reference/security-faq.md:132).

Fix: soften this comment and add the disclosure to both docs — that signing in associates the anonymous machine id with the account in analytics. This is an additive disclosure gap, distinct from the flat contradiction flagged on the URL line.

[sahrizvi]

MINOR — Design: an empty machine_id is transmitted rather than omitted

Every failure path converges on machine_id: "", and the empty value is still sent. The telemetry module already handles this correctly — ...(machineId && { machine_id: machineId }) at telemetry/index.ts:1570 omits the key — so this diverges from the pattern it imitates.

The test comment at altimate-plugin.test.ts:34 claims the empty string lets the frontend distinguish "error reading" from "no key", but the key is never omitted in any path, so that distinction does not exist.

If the companion frontend aliases on the empty value, unrelated anonymous sessions merge into one identity. That consequence lives in the other repository and is unverified here — worth confirming on that side.

Fix:

const ctx: Record<string, unknown> = { v: 1, cli_version: InstallationVersion }
if (machineId) ctx["machine_id"] = machineId

and make "absent means do not alias" explicit in the frontend contract.

[sahrizvi]

MINOR — Code Quality: the catch and its log describe a narrower failure than they handle

The bare catch swallows EACCES, EISDIR, ELOOP, ENOTDIR and I/O errors, but logs "machine-id file not found". It also says it "will omit machine_id" when it in fact sends "". Someone debugging a missing correlation caused by a permissions problem is actively misled.

Fix:

} catch (err) {
  const code = (err as NodeJS.ErrnoException)?.code
  if (code === "ENOENT") log.debug("machine-id not present for cli_context")
  else log.warn("machine-id read failed", { code, path: idPath })
}

Non-ENOENT codes indicate a real local problem and deserve more than debug.

[sahrizvi]

MINOR — Documentation: the frontend contract is unspecified, and this payload is untrusted input on that side

Nothing records what the consumer must do: decode base64url (not standard base64), require v === 1, cap decoded size, catch base64/JSON parse failures, validate field types, and treat cli_version: "local" as a legitimate dev value rather than a release.

From the frontend's perspective a URL query param is attacker-suppliable — anyone can hand-craft one and load /register.

Fix: add JSDoc here stating the contract, and make sure the companion PR validates rather than trusts.

[sahrizvi]

MINOR — Testing: the integration point is untested

All three tests exercise buildCliContext() in isolation. Nothing asserts that the authorize URL actually carries cli_context, that it decodes back, or that client / redirect / state survive alongside it.

Delete the &cli_context=... line in altimate.ts and this entire suite still passes — which is the definition of an untested feature.

Fix: extract a buildAuthorizeUrl() and assert on it via new URL() / URLSearchParams, then decode cli_context independently.

[sahrizvi]

MINOR — Testing: no failure-mode coverage, and the repo's temp-dir fixture is bypassed

Untested paths, several of which carry the bugs flagged elsewhere in this review: telemetry opted out, permission-denied on an existing file, empty file (0 bytes), non-UUID or binary contents, oversized file, path-is-a-directory, and symlink.

The fixture value "test-uuid-1234" is also not a UUID, which quietly blesses arbitrary contents as acceptable input.

Separately, this hand-rolls fs.mkdtempSync + manual fs.rmSync cleanup while the repo ships a tmpdir() fixture with await using auto-cleanup at test/fixture/fixture.ts:147. The manual version leaks temp directories whenever a test throws mid-run.

[sahrizvi]

Review round 2 — fix commit 3395c4c

Verdict: request changes — 6 major, 5 minor.

Real progress here. The omit-when-empty fix is exactly right and now matches the telemetry module's own pattern, the error classification inside buildCliContext does what was asked, and buildAuthorizeUrl is a clean extraction that makes the parameter genuinely guarded against silent removal.

But the commit touched only 2 files, so every documentation finding is untouched — and the central privacy issue came back in a worse form. buildCliContext now creates the machine-id file, while honouring only one of the two documented opt-out mechanisms.

Status of the round-1 findings

#	Item	Status
1	Machine-id lifecycle split / path duplication	Partial — helper added, but telemetry and welcome still carry their own copies
2	Sent despite telemetry opt-out	Partial — env var honoured, config opt-out ignored; now also creates the id
3	No size/format validation	Not addressed
4	Device ID in URL query param	Not addressed
5	Privacy docs stale	Not addressed
6	Empty machine_id transmitted	Resolved
7	Misleading catch/log	Partial — fixed in buildCliContext, reintroduced in the new helper (see minor 1)
8	Authorize URL untested	Resolved — though the harness has its own problem (see major 3)
9	Failure-mode coverage / tmpdir() fixture	Partial
10	Frontend decode contract	Not addressed — a comment names posthog.alias, but no contract is specified

---

Major

1. The config opt-out is ignored, and the fix now mints an identifier for users who used it

altimate.ts:93, :62-80; telemetry/index.ts:1667, :1676

buildCliContext guards on process.env.ALTIMATE_TELEMETRY_DISABLED !== "true" and nothing else. Telemetry.doInit() honours two independent opt-outs — the env var at telemetry/index.ts:1667 and userConfig.telemetry?.disabled at :1676 — and the docs present them as equally valid: "Disable telemetry entirely with ALTIMATE_TELEMETRY_DISABLED=true or the config option above" (docs/docs/reference/telemetry.md:150, config documented at :111).

The new part is the creation side effect. Previously this code only read the file, so a config-opted-out user with no prior machine id transmitted nothing. Now getOrCreateMachineId mints and persists one during sign-in, then sends it — the CLI creates a permanent tracking identifier for a user who used a documented opt-out.

Compounding it, the comment at :91 says this "matches the guard in telemetry/index.ts::doInit". It matches half of it. A partial opt-out that advertises itself as complete is worse than none, because it stops the next reader from checking.

Fix: resolve the full opt-out policy in one place and consult it here. Config.get() is async while buildCliContext is sync, so either make context construction async or pass a resolved telemetryEnabled flag in from the caller. Failing that, this function should go back to read-only and never create — leaving creation to telemetry, where consent is already resolved.

2. The "shared" helper is not shared, and a new comment claims it is

altimate.ts:61; telemetry/index.ts:1702-1722; cli/welcome.ts:46

Line 61 states the helper is "Used by both buildCliContext and the telemetry module's doInit()." The commit changed only altimate.ts and the test file. telemetry/index.ts:1702-1722 still holds its own inline read-or-create implementation, and welcome.ts:46 still builds the path independently.

The result is worse than the original finding: there are now two independent read-or-create implementations that must stay in sync, where before there was one creator and one reader — plus a comment asserting they are unified. Line 85's "written by the telemetry module" is stale for the same reason.

Fix: move the path and lifecycle into a neutral module (e.g. altimate/telemetry/machine-id.ts) imported by telemetry, auth, and welcome — a plugin importing from telemetry, or the reverse, is the wrong dependency direction. Keep "read existing" separate from "create" so callers do not inherit an unexpected persistence side effect. Until the migration happens, the comment should not claim it has.

3. The test suite writes a persistent machine-id into the runner's home directory

altimate-plugin.test.ts:156, :184; altimate.ts:115-122

Both buildAuthorizeUrl tests call the function with no path override. buildAuthorizeUrl calls buildCliContext() with no argument at :120, which reaches getOrCreateMachineId(undefined), defaults to os.homedir()/.altimate/machine-id at :63, and writes at :72-75.

Reproduced against a clean isolated HOME: the run created a 36-byte UUID file. So bun test on a developer laptop or a CI runner now mints an analytics identity that will later be reported as a real device. It also corrupts an existing signal — cli/welcome.ts:46-47 uses existsSync on that file as the "upgrade vs fresh install" proxy, so a machine that has only ever run the test suite is thereafter classified as an upgrade.

The tests look isolated but are not: both create a temp machine-id containing "url-test-uuid" at :149-151, and that file is never read. The comment at :153-155 admits the path is not plumbed through. Dead setup that disguises the problem.

There is already a pattern for this in the repo — test/altimate/telemetry/onboarding.test.ts:367-372 redirects HOME to a temp dir with a comment explaining this exact hazard.

Fix: give buildAuthorizeUrl an optional machineIdPath forwarded to buildCliContext, and/or redirect HOME the way onboarding.test.ts does.

4. No size, format, or symlink validation — unchanged

altimate.ts:65

fs.readFileSync(idPath, "utf8").trim() still accepts arbitrary contents: no lstat, no size cap, no UUID check. An oversized, multi-line, or symlinked file still ends up base64-encoded in the auth URL — breaking sign-in past URL length limits, and still usable to copy another file's text into a URL that gets logged server-side. (Invalid byte sequences are replaced by the utf8 decode rather than passed through verbatim, but the content is still unbounded and unvalidated.)

The case for validating is stronger now, not weaker: line 76 mints ids with randomUUID(), so writer and validator would agree by construction. The tests themselves feed "test-uuid-1234" and "expected-uuid", showing non-UUID content sails through.

Fix: lstat and reject non-regular files, cap size before reading, require the canonical UUID shape.

5. Persistent device identifier still travels in a URL query parameter

altimate.ts:115-120, :419

Unchanged. Base64url is an encoding, not confidentiality. The identifier still reaches browser history, access logs, CDN/WAF logs, the clipboard on SSH/tmux sign-in, and potentially a Referer header.

Fix: a short-lived opaque correlation nonce registered server-side, or delivery over the authenticated back-channel. A fragment would remove server-log exposure but not history or clipboard.

6. Privacy documentation is untouched and now inaccurate in a second way

docs/docs/reference/telemetry.md:150,154; security-faq.md:133; also telemetry.md:68,147, security-faq.md:132

Neither commit changed docs/. telemetry.md:150 and security-faq.md:133 still promise both identifiers "are only sent when telemetry is enabled", which major 1 shows is still false. Beyond the original finding, the docs name Azure Application Insights as the destination and state that no separate data store is maintained (:154), while the new comment at altimate.ts:86-87 describes the frontend aliasing the id into PostHog. Both the opt-out promise and the destination need correcting before this ships.

---

Minor

1. The new helper treats every write failure as a lost race

altimate.ts:76-79

try { fs.writeFileSync(idPath, candidate, { encoding: "utf8", flag: "wx" }); return candidate }
catch { return fs.readFileSync(idPath, "utf8").trim() }

The bare catch assumes EEXIST. On EACCES, EROFS or ENOSPC the re-read targets a file that was never created, throws ENOENT, and reaches buildCliContext's handler — which sees code === "ENOENT" and logs the benign "machine-id not present" at debug. So a real permissions or disk failure gets downgraded to the quietest log level. (Errors like EISDIR surface on the initial read at :65 and are correctly rethrown at :67, so the misreporting is limited to write-stage failures.) Auth still degrades gracefully by omitting the id.

Fix:

} catch (writeErr) {
  if ((writeErr as NodeJS.ErrnoException)?.code !== "EEXIST") throw writeErr
  return fs.readFileSync(idPath, "utf8").trim()
}

2. The concurrency test does not test concurrency

altimate-plugin.test.ts:131-144

getOrCreateMachineId is synchronous, so both calls run to completion before Promise.all receives anything. The first creates the file; the second takes the plain read path. The wx branch at :77-79 is never entered — this test passes unchanged if flag: "wx" becomes a plain write, which is precisely the property it claims to guard.

Fix: force EEXIST with a stubbed writeFileSync, or spawn two real processes. If neither is worthwhile, drop the test rather than keep a guard that guards nothing.

3. Conditional assertion cannot prove what the test is for

altimate-plugin.test.ts:172-175

if (hasOwnProperty(ctx, "machine_id")) { ... } passes when the key is absent, so it cannot establish that machine_id was emitted. The sibling tests already save and restore ALTIMATE_TELEMETRY_DISABLED, so the expected state is controllable — assert it directly.

4. Remaining untested paths, and the repo fixture is still bypassed

altimate-plugin.test.ts

Still uncovered: config-based opt-out, non-UUID contents, oversized file, symlink, permission-denied on an existing file, path-is-a-directory, and the real wx race. Every test still hand-rolls mkdtempSync + rmSync, leaking temp directories on failure, instead of the tmpdir() fixture at test/fixture/fixture.ts:147.

5. Frontend decode contract still unspecified

altimate.ts:83-87

The comment naming posthog.alias(email, machine_id) is welcome honesty, but there is still no stated contract: base64url rather than standard base64, require v === 1, cap decoded size, reject malformed input, treat cli_version: "local" as a dev value, and treat an absent machine_id as "do not alias". From the frontend's side this parameter is attacker-suppliable.

---

Nits

• buildAuthorizeUrl now mutates the filesystem two calls deep. A build… name that implies no side effects is what makes major 3 easy to miss — resolving the id before URL construction and passing it in would fix both.
• altimate.ts:85 — "written by the telemetry module" is stale now that this file writes it too.

Considered and not raised

• state is not URL-encoded — not an issue. It is randomBytes(16).toString("hex") at altimate.ts:387; hex is URL-safe by construction, and this predates the PR.

What the fix got right

• getOrCreateMachineId faithfully reproduces the wx exclusive-create pattern including the lost-race re-read, and correctly rethrows non-ENOENT read errors instead of swallowing them.
• Omit-when-empty is exactly right and now matches telemetry/index.ts:1570.
• buildCliContext's handler distinguishes ENOENT from real failures and logs the code and path.
• buildAuthorizeUrl is a clean extraction — removing the cli_context parameter would now fail a test.
• The env-var opt-out test is well built: it plants a value that must not appear, asserts key absence rather than emptiness, and saves/restores the variable properly.
• The code comment now states the posthog.alias linkage openly rather than describing the payload as simply non-PII.

Tests pass: 10 pass, 27 expect calls.

[github-actions]

👋 This PR was automatically closed by our quality checks.

Common reasons:

• New GitHub account with limited contribution history
• PR description doesn't meet our guidelines
• Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

[sahrizvi]

Review round 3 — 43fb343

Verdict: request changes — 1 new regression, 3 carry-overs untouched.

This round is a real refactor and most of it is good. util/machine-id.ts is the right module in the right place doing the right things: lstat rather than stat, a 512-byte cap checked before reading, a v4-specific UUID regex, EEXIST-only race handling, and a distinct warn log per failure class. It reads like someone understood the findings rather than pattern-matching them. Telemetry's inline copy was genuinely deleted, not wrapped. And moving cli_context from the query string to the URL fragment is a better answer to the leakage finding than the review asked for — the comment at altimate.ts:63-70 describes the benefit accurately and doesn't overclaim, naming access logs, CDN/WAF and Referer without pretending to solve browser history or clipboard.

One new problem can break sign-in, and three items called out last round are unchanged.

Status of the round-2 findings

#	Item	Status
1	Config opt-out ignored	Resolved in the sender; still missing in welcome.ts (see major 2)
2	"Shared" helper not shared	Resolved — telemetry, plugin and welcome all import it
3	No size/format/symlink validation	Resolved
4	Tests write into the runner's HOME	Not addressed — reproduced again
5	Device ID in URL query param	Resolved — moved to the fragment
6	Privacy docs	Partial — disclosed, but now internally inconsistent (minor 1)
7	Write catch treated all errors as a lost race	Resolved — EEXIST-only
8	"Concurrent callers" test isn't concurrent	Not addressed — byte-identical
9	Conditional assertion that can't fail	Not addressed
10	Untested paths / tmpdir() fixture	Partial — six good failure-mode tests added; fixture still bypassed
11	Frontend decode contract	Partial — docs and a comment, no written contract

---

Major

1. getOrCreateMachineId throws on a read-only home, and the one caller that used to catch no longer does

util/machine-id.ts:79; plugin/altimate.ts:92-94

fs.mkdirSync(path.dirname(idPath), { recursive: true }) at machine-id.ts:79 sits outside any try. Every other failure path in that module is guarded and returns "" — this one propagates.

Both the docstring and the call site state otherwise:

• machine-id.ts:34 — "@returns A v4 UUID string, or "" if the value is invalid or unreadable."
• altimate.ts:93 — "returns "" on all error conditions … no try/catch needed."

On the strength of that second comment, buildCliContext removed the try/catch it had previously. So on a read-only $HOME, a restricted container, a full disk, or any mkdirSync failure, the exception escapes buildCliContext → buildAuthorizeUrl → authorize() and sign-in fails outright. It previously degraded by omitting the field. Fail-open became fail-closed, on the one path where this feature is explicitly non-essential.

Confirmed by execution: calling the helper with a path under a chmod 555 directory returns THREW:EACCES, not "".

The other two callers are safe by accident rather than design — Telemetry.doInit() and showWelcomeBannerIfNeeded() (welcome.ts:31, catch at :99) each sit inside their own broad try/catch. Only the auth path is exposed.

Fix — bring mkdirSync inside the guarded region so the module honours its own contract:

try {
  fs.mkdirSync(path.dirname(idPath), { recursive: true })
  fs.writeFileSync(idPath, candidate, { encoding: "utf8", flag: "wx" })
  return candidate
} catch (writeErr) {
  const code = (writeErr as NodeJS.ErrnoException)?.code
  if (code !== "EEXIST") { log.warn("machine-id create failed", { code, path: idPath }); return "" }
  …
}

A test with a non-writable parent directory would have caught this and belongs in the suite.

2. welcome.ts mints the identifier under the env-var gate only

cli/welcome.ts:51

if (process.env.ALTIMATE_TELEMETRY_DISABLED !== "true") getOrCreateMachineId()

The config gate that was just added to buildCliContext is absent here, so a user who disabled telemetry via config.telemetry.disabled still gets ~/.altimate/machine-id created on first launch. Nothing transmits it today — both senders check config — so this is creation without disclosure rather than a leak. But it is the same inconsistency as last round reappearing in a new place, and it becomes a leak the moment a third caller is added.

Credit where due: the ordering hazard here was handled deliberately and correctly. existsSync is probed before minting, with a comment explaining that using the helper as the probe would report every new user as an upgrade. That is exactly the trap this change could have fallen into.

Fix: route both gates through one shared predicate (e.g. isTelemetryDisabled()) and call it from all three sites.

3. The fragment move is a breaking cross-repo change that nothing in this repo can verify

altimate.ts:105-115

cli_context moved from &cli_context= to #cli_context=. Technically the right call. But the companion frontend PR was written against the query string, and the comment at altimate.ts:69-70 asserts "The frontend reads it from the fragment (see useCliContext.ts)" — which cannot be checked from here. If that side still reads searchParams, correlation silently returns nothing: no error, no failed request, just a feature that quietly does nothing. No test on this side can catch it.

Two further properties worth confirming before merge, both invisible from this repo:

• Fragments never reach the server. If /register resolves cli_context during SSR rather than in the browser, it will never see it.
• Survival across the OAuth round trip. The user goes /register → identity provider → back. Browsers generally carry a fragment across a 3xx to a fragment-less target, but not reliably through a chain that sets its own fragment, and not through a client-side navigation that drops it. The value needs to be read and stashed before the provider hop.

Given how silent the failure mode is, an end-to-end check is worth more here than any unit test. Ideally land both sides together.

---

Minor

1. The new documentation is narrower than the code, and contradicts an unchanged line

docs/docs/reference/telemetry.md:150 vs :152+; security-faq.md:135

The new sections say suppression happens "when ALTIMATE_TELEMETRY_DISABLED=true" and name only the env var. The unchanged sentence at telemetry.md:150 says both identifiers "are only sent when telemetry is enabled. Disable telemetry entirely with ALTIMATE_TELEMETRY_DISABLED=true or the config option above."

The code now honours both, so the new text understates the protection — a config-opted-out reader would reasonably conclude the auth URL is exempt from their choice. One sentence fixes it.

2. Config.get() falls open in a context where it may routinely fail

altimate.ts:87-90

A Config.get() throw is treated as "not disabled", mirroring doInit. But doInit runs on the main thread where config is available, while the auth plugin runs inside the server worker — the module header in telemetry/onboarding.ts is explicit that the worker has a different initialization story. If Config.get() throws there, a config-opted-out user's identifier is transmitted anyway, which is exactly the case this round set out to close.

Fix: confirm Config.get() resolves in the plugin worker. If it can't be relied on, resolve the opt-out on the main thread and pass it in rather than failing open.

3. Carry-overs that were not touched

• Tests still write into the runner's HOME. Re-verified: running the suite against a clean isolated HOME produced $HOME/.altimate/machine-id containing a fresh UUID. Both buildAuthorizeUrl tests (:156, :191) still call the builder with no path override, and the temp machine-id written at :151 is still dead setup that the comment at :153-155 still admits is unused. buildAuthorizeUrl needs an optional machineIdPath forwarded to buildCliContext.
• The "concurrent callers" test (:131-144) is byte-identical to last round: two synchronous calls wrapped in Promise.resolve, so the wx branch is never entered and the test passes with a plain write.
• The conditional assertion (:179-183) still passes whether or not machine_id is present.
• The tmpdir() fixture at test/fixture/fixture.ts:147 is still bypassed in all 17 tests.
• Still untested: config-based opt-out, and the EACCES path that is major 1.

4. Production re-export that exists only for tests

altimate.ts:59

export { getOrCreateMachineId } from "../util/machine-id" is annotated as existing so old test imports keep working. Update the two test imports and drop it — the plugin re-exporting a utility it doesn't own will read as intentional API to the next person.

---

Considered and not raised

• Telemetry.track firing before doInit leaves first_launch without a machine id. The event is buffered and the id is attached at flush time from module state, which doInit populates before the first flush — so the claim is unproven. It is also pre-existing behaviour rather than something this PR introduced.

What this round got right

• The v4-specific regex is a nice touch — it rejects a well-formed v1 UUID, and there is a test for exactly that.
• Six new failure-mode tests covering precisely the paths flagged last round: non-UUID content, oversized file, wrong UUID version, empty file, directory-at-path, symlink.
• The empty-file case is handled thoughtfully — it returns "" rather than minting over the file, and the test name says so.
• The fragment tests assert searchParams.has("cli_context") === false, which genuinely guards the placement rather than just checking the value exists.
• Telemetry's inline implementation was deleted, not merely wrapped.

Tests pass: 17 pass, 37 expect calls.

[github-actions]

👋 This PR was automatically closed by our quality checks.

Common reasons:

• New GitHub account with limited contribution history
• PR description doesn't meet our guidelines
• Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

[saravmajestic]

Thanks for the thorough round-3 pass. All items addressed in ffe8b1fa.

Major

1. mkdirSync outside the guard → sign-in fails on read-only $HOME. Fixed — mkdirSync is now inside the try/catch in getOrCreateMachineId, so EACCES/EROFS/ENOSPC return "" and the module honours its own contract. Added a test that mocks mkdirSync to throw EACCES and asserts "" (not a throw).

2. welcome.ts mints under the env gate only. Fixed by removing the mint entirely — creation is delegated to Telemetry.doInit(), which resolves the full opt-out (env and config). welcome.ts keeps only the existsSync upgrade probe. The first_launch machine_id is attached at flush from telemetry module state, so it doesn't depend on minting here.

3. Fragment is a breaking cross-repo change unverifiable from this repo. Acknowledged. The companion frontend PR (monorepo #3106) reads location.hash first (query string kept as an older-CLI fallback) and stashes the value in sessionStorage before the OAuth hop so it survives the round trip. That's a real coupling that can't be proven from this repo — the two should land together, and an e2e check is worth more than any unit test here. Flagging for coordinated merge.

Minor

1. Docs understate the opt-out. Fixed — both the CLI-auth section and the identifier bullet now name ALTIMATE_TELEMETRY_DISABLED=true or the telemetry.disabled config option, and the destination line notes the auth-flow id is associated in PostHog, separate from the App Insights pipeline.

2. Config.get() falls open in the worker. Changed to fail closed — if Config.get() throws (confirmed it does in a non-Instance context), buildCliContext now omits machine_id rather than sending it. One honest caveat: if Config.get() routinely throws inside the authorize() worker, this means correlation silently never fires there. authorize() serves the loopback callback, likely within Instance.provide() where config resolves, but I haven't confirmed that in a running TUI. If it turns out not to resolve there, I'll follow your stronger suggestion — resolve the opt-out on the main thread and pass a flag in. Flagging so it's a deliberate decision, not an assumption.

3. Carry-overs:

• Tests writing into $HOME — fixed: buildAuthorizeUrl takes an optional machineIdPath forwarded to buildCliContext, and every test uses the repo tmpdir() await using fixture. Also guarded against a developer's exported ALTIMATE_TELEMETRY_DISABLED breaking the suite.
• "Concurrent callers" test — replaced with a real wx/EEXIST branch test (mock lstat→ENOENT, writeFileSync→EEXIST, assert it re-reads the winner).
• Conditional assertion that can't fail — replaced with an unconditional machine_id === <known uuid> round-trip.
• tmpdir() fixture — now used across all tests.
• Added config-opt-out and mkdir-EACCES tests.

4. Dead re-export. Removed export { getOrCreateMachineId }; the tests now import from util/machine-id.

Also encoded the state param in the authorize URL. Full test/altimate suite is green (4026/0).

[github-actions]

👋 This PR was automatically closed by our quality checks.

Common reasons:

• New GitHub account with limited contribution history
• PR description doesn't meet our guidelines
• Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

[sahrizvi]

Review round 4 — ffe8b1f

Verdict: request changes — 2 major, 3 minor. Close to done.

Nine of the ten round-3 items are fully resolved, most with regression tests that exercise the exact branch that was broken rather than the happy path around it. The mkdirSync fix ships with a test that mocks lstat→ENOENT and mkdir→EACCES and asserts "", so it fails if the fix is reverted. The fake-concurrency test was replaced with a real EEXIST branch test. buildAuthorizeUrl now threads machineIdPath, which fixes the $HOME pollution at its root rather than papering over it — verified: a run against a clean isolated HOME leaves no .altimate directory behind. 19 pass.

The welcome.ts fix is the cleanest of the set: rather than duplicating the two-gate opt-out, minting was removed entirely and ownership handed to Telemetry.doInit(). That instinct is right. The problem is that the owner it was handed to doesn't enforce the policy the comment claims.

Status of the round-3 findings

#	Item	Status
1	mkdirSync outside the try → helper threw, sign-in hard-failed	Resolved + regression test
2	welcome.ts minted under the env gate only	Resolved locally, but relocated — see major 1
3	Fragment move is an unverified cross-repo contract change	Partial — contract documented, behaviour still unverified
4	Tests wrote a real machine id into the runner's $HOME	Resolved — verified against an isolated HOME
5	Docs named only the env var	Resolved
6	"Concurrent callers" test wasn't concurrent	Resolved — real EEXIST branch test
7	Conditional assertion that couldn't fail	Resolved — now asserts the id round-trips
8	tmpdir() fixture bypassed	Resolved — await using throughout
9	Production re-export existing only for tests	Resolved — removed
10	No config-opt-out test, no EACCES test	Resolved — both added

---

Major

1. The config opt-out still doesn't hold — minting just moved to a call site that fails open

cli/welcome.ts:43-48; src/index.ts:126; altimate/telemetry/index.ts:1667-1682

The new welcome.ts comment says minting is "owned by Telemetry.doInit(), which resolves the full opt-out policy (env var AND config) before creating the file." That is not true of the call that actually does the minting.

The startup sequence:

1. src/index.ts:126 fires Telemetry.init().catch(() => {}) from the yargs middleware at index.ts:101 — before any Instance.provide.
2. doInit() reaches its config gate and calls Config.get().
3. Outside an Instance context that throws. doInit's own comment at telemetry/index.ts:1673-1675 says exactly this: "Config.get() may throw outside Instance context (e.g. CLI middleware before Instance.provide())".
4. Its catch treats the failure as not disabled — fail open.
5. Execution continues to machineId = getOrCreateMachineId() and enabled = true.
6. init() is deduplicated, so the later instance-aware call reuses this result rather than re-resolving the opt-out.

So a user whose only opt-out is "telemetry": { "disabled": true } in config still gets ~/.altimate/machine-id created on their disk at CLI startup, and telemetry enabled. This is the round-3 finding relocated rather than eliminated — with a comment asserting a guarantee that the receiving code does not provide.

buildCliContext independently re-checks config and does so on a path where it succeeds, so the id is not transmitted in the auth URL. The disk artefact and the enabled telemetry pipeline remain.

Fix: resolve the config opt-out before early telemetry initialization and pass an explicit decision into Telemetry.init(), or have the early doInit() defer minting until an instance-aware init can resolve the policy. Then correct the welcome.ts comment. A test starting from a fresh isolated home with config-only opt-out, asserting both that the file stays absent and that telemetry stays disabled, would lock this down.

2. The fragment contract still isn't verified on the consuming side

plugin/altimate.ts:62-77, :116-128

The decode contract is now written down properly — base64url not standard base64, require v === 1, validate field types, "local" is a valid dev build, treat the payload as untrusted, and an absent machine_id means "do not attribute". The move from posthog.alias(...) to registering a cli_machine_id super-property is also a better design: it removes the empty-value identity-merge hazard from round 1.

What the tests prove is that this CLI builds a fragment. They cannot prove the parts that decide whether the feature works:

• that the deployed frontend reads location.hash rather than searchParams — the previously reported consumer used the query string;
• that it reads and persists the value before navigating to the identity provider, since a fragment never reaches either the app server or the IdP;
• that it survives or is restored across the return leg;
• that it handles an absent machine_id without attributing.

This repository contains no useCliContext.ts, cliContext.ts, or cli_machine_id — the comment asserts the companion implementation exists, and that assertion is currently the only evidence.

Fix: link the companion PR and land both sides together, with one end-to-end pass covering /register#cli_context=… → IdP → return → decode → super-property registration. The failure mode is silent on both sides, so an integration check is worth more here than any further unit test.

---

Minor

1. The fail-closed branch is silent, and its comment describes the wrong condition

plugin/altimate.ts:86-95

Inverting the Config.get() failure to fail closed was the right call, and documenting it as a deliberate divergence from doInit is good practice. Two corrections:

The comment is inaccurate. It reads "this plugin can run in the server worker where Config.get() throws 'InstanceRef not provided'", implying the throw is normal during authorization. It is not. Server routes are wrapped in Instance.provide({ directory, init: InstanceBootstrap, fn }) at server/server.ts:288, Instance.current is AsyncLocalStorage-backed (project/instance.ts:84 → context.use()), and ALS propagates across awaits — so attach() (effect/run-service.ts:25-37) resolves the instance via its tryLegacyInstance() fallback even deep inside authorize() after await startCallbackServer(). Config.get() succeeds on that path.

But it isn't dead code either. ProvidersLoginCommand declares instance: (args) => !args.url (cli/cmd/providers.ts:303), so altimate auth login <url> deliberately skips instance bootstrap — the comment on that line says so. On that one path Config.get() does throw, the fail-closed branch fires, and machine_id is silently dropped.

Because the catch logs nothing, "user opted out via config" and "config was unreadable" are indistinguishable in the field. If correlation rates come back low, nothing points here.

Fix:

} catch (err) {
  log.warn("cli_context: config unreadable, omitting machine_id (fail-closed)", {
    code: (err as NodeJS.ErrnoException)?.code,
  })
  disabled = true
}

and reword the comment to describe an unexpected-config-unavailable fallback, naming the URL-login path as the known case.

2. The helper's stated guarantees exceed what it enforces

util/machine-id.ts:24-31, :45-54, :90-96

The docstring promises "regular-file-only" and "reads at most 512 bytes". Both are checked via lstatSync and then the read is performed separately by pathname with an unbounded readFileSync — so the file can be swapped or grown between the two calls, and the size cap is advisory rather than enforced. The EEXIST race-loser re-read at :91 performs no lstat or size check at all before reading.

Practical impact is limited, since UUID_RE rejects anything that isn't 36 well-formed characters — but a multi-gigabyte file is fully read into memory before the regex rejects it, which is the outcome the cap exists to prevent.

Fix: open once with no-follow semantics where available, inspect the descriptor, and read a bounded number of bytes — or soften the docstring to describe what is actually enforced.

3. Privacy terminology in the docs

docs/docs/reference/telemetry.md:154

The durable machine id is described as "an anonymized session identifier", but it is explicitly persisted and reused across sessions — that is an installation/device identifier. The same sentence says it is "never used for tracking, advertising, or cross-site identification" while the surrounding paragraph describes using it to associate CLI activity with an authenticated account. "Not used for advertising or cross-site tracking" would be accurate; "never used for tracking" reads as overclaiming against the feature's own description.

---

What this round got right

• Every round-3 fix landed with a test that drives the specific broken branch. The mkdirSync regression guard (test:193-213) and the EEXIST race test (test:165-191) both fail if their fixes are reverted, which is the entire point.
• buildAuthorizeUrl threading machineIdPath fixed $HOME pollution at the source rather than redirecting HOME in the tests.
• The fragment test is now non-vacuous: it asserts the written UUID round-trips through the URL and separately asserts absence from searchParams.
• Removing minting from welcome.ts outright, instead of duplicating the gate, was the right instinct — the ownership just needs to actually enforce the policy.
• Docs now name both opt-out mechanisms consistently across both files and disclose the PostHog pipeline as distinct from Azure Application Insights.
• The fail-closed decision is documented as a deliberate divergence with reasoning, rather than silently differing from doInit.

Tests: 19 pass, 39 expect calls.

[github-actions]

👋 This PR was automatically closed by our quality checks.

Common reasons:

• New GitHub account with limited contribution history
• PR description doesn't meet our guidelines
• Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

[github-actions]

👋 This PR was automatically closed by our quality checks.

Common reasons:

• New GitHub account with limited contribution history
• PR description doesn't meet our guidelines
• Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

[saravmajestic]

Thanks — the tracing on major 1 is precise. Minors fixed in 346df2cd; on the two majors I'd like to agree on scope so we can land this.

Minor — all fixed

1. Fail-closed comment inaccurate + silent. Corrected: the comment now says Config.get() resolves in a normal browser authorize() (server routes run inside Instance.provide, ALS-propagated) and names the auth login <url> path (instance: (args) => !args.url) as the one place it throws. Added the log.warn(...) so a low correlation rate is traceable to this branch rather than mistaken for a lost reply.
2. Advisory size cap / unbounded re-read. machine-id.ts now reads through a descriptor — fstat (regular file + size, on the same fd we read) then readSync of at most MAX_BYTES — so the cap is enforced at read time, and the EEXIST race re-read is bounded too (it previously had no check at all). A multi-GB file can no longer be slurped whole before UUID_RE rejects it.
3. Docs terminology. The machine id is now described as a device/installation identifier (persisted, reused across sessions), and "never used for tracking" is softened to "not used for advertising or cross-site tracking".

Also corrected the welcome.ts comment that overclaimed doInit's guarantee (see major 1).

Major 1 — real, but pre-existing; proposing a follow-up

You're right that the config opt-out doesn't hold at early init, and the welcome.ts comment shouldn't have claimed otherwise (fixed). But this behaviour predates this PR: doInit's config gate has always failed open before an Instance exists (its own comment at telemetry/index.ts:1673-1675 says so), and init() dedup means the later instance-aware call reuses that result. This PR didn't introduce the disk-mint-under-config-opt-out — it only removed welcome.ts's own (env-only) mint and, wrongly, described doInit as the enforcing owner.

The correct fix — resolving the config opt-out before early telemetry init and threading an explicit decision into Telemetry.init() — is a change to the telemetry-init path (high blast radius, unrelated to cli_context). I'd like to split that into its own issue/PR rather than fold a telemetry-subsystem fix into this one, and keep this PR to the accurate comment. If you'd prefer it fixed here, I'll do it, but wanted to flag the scope. Happy to open the follow-up issue with your index.ts:126 → doInit → dedup trace.

Major 2 — the consuming side is implemented; it's cross-repo

The companion frontend is AltimateAI/altimate-frontend#3106. It does exactly what the contract requires: reads location.hash first (query string kept only as an older-CLI fallback), stashes the value in sessionStorage before the IdP bounce so it survives the return leg, decodes with v === 1 + string field validation, and treats an absent machine_id as "do not attribute" (registers the cli_machine_id super-property; no alias). It genuinely can't be verified from this repo — as you say, the right check is one end-to-end pass, and the two should land together. Linking it here for that.

I think that leaves this PR mergeable once we agree major 1 is a separate follow-up. If not, say so and I'll fold the telemetry-init fix in.

[sahrizvi]

LGTM