From 233bb0e94b9f5f3ae5425c99b1b4f300e8f73501 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 10 Aug 2026 19:06:26 +0530 Subject: [PATCH 01/10] fix(telemetry): accept `1`/case-insensitive `true` on `ALTIMATE_TELEMETRY_DISABLED` + honor `OPENCODE_DISABLE_TELEMETRY` fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of the v0.9.5 review reviewers flagged the same shape: - The two telemetry-disable gates (`altimate/plugin/altimate.ts::buildCliContext` and `altimate/telemetry/index.ts::doInit`) each did `=== "true"`. Users setting `ALTIMATE_TELEMETRY_DISABLED=1` (or `=TRUE`) were silently ignored — telemetry stayed on. - The v0.9.4 CHANGELOG advertised `OPENCODE_DISABLE_TELEMETRY=1` as an opt-out env var, but that name was wired into test fixtures only. Users who set it based on the CHANGELOG were not opted out. Both call sites now route through a shared `Flag.truthyEnv` helper that accepts `"true"` / `"TRUE"` / `"1"` and checks both env var names. Runtime evaluation is preserved (Flag.* constants freeze at import — wrong shape for gates the caller re-reads). Test coverage: `telemetry-opt-out-flag.test.ts` (14 tests) locks the accepted-value matrix down so a future edit to `truthy()` can't silently narrow it again. --- .../opencode/src/altimate/plugin/altimate.ts | 7 ++- .../opencode/src/altimate/telemetry/index.ts | 5 +- packages/opencode/src/flag/flag.ts | 9 +++ .../telemetry/telemetry-opt-out-flag.test.ts | 61 +++++++++++++++++++ 4 files changed, 79 insertions(+), 3 deletions(-) create mode 100644 packages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts diff --git a/packages/opencode/src/altimate/plugin/altimate.ts b/packages/opencode/src/altimate/plugin/altimate.ts index b57b4233ef..6b61d01d32 100644 --- a/packages/opencode/src/altimate/plugin/altimate.ts +++ b/packages/opencode/src/altimate/plugin/altimate.ts @@ -8,6 +8,7 @@ import * as OnboardingTelemetry from "../telemetry/onboarding" // altimate_change — shared machine-id helper (race-safe, UUID-validated, size-capped) import { getOrCreateMachineId } from "../util/machine-id" import { Config } from "@/config/config" +import { Flag } from "@/flag/flag" import { InstallationVersion } from "@opencode-ai/core/installation/version" import { Log } from "@/altimate/util/log" @@ -76,9 +77,11 @@ const log = Log.create({ service: "altimate-plugin" }) export async function buildCliContext(machineIdPath?: string): Promise { // altimate_change start — honour both telemetry opt-out gates, mirroring // telemetry/index.ts::doInit: - // 1. ALTIMATE_TELEMETRY_DISABLED=true env var (always-works hard opt-out) + // 1. ALTIMATE_TELEMETRY_DISABLED / OPENCODE_DISABLE_TELEMETRY env vars + // ("true"/"TRUE"/"1", case-insensitive — see Flag.truthyEnv) // 2. config.telemetry.disabled (resolved via the async Config.get()) - let disabled = process.env.ALTIMATE_TELEMETRY_DISABLED === "true" + let disabled = + Flag.truthyEnv("ALTIMATE_TELEMETRY_DISABLED") || Flag.truthyEnv("OPENCODE_DISABLE_TELEMETRY") if (!disabled) { try { const userConfig = (await Config.get()) as any diff --git a/packages/opencode/src/altimate/telemetry/index.ts b/packages/opencode/src/altimate/telemetry/index.ts index 61db828430..e7f14db537 100644 --- a/packages/opencode/src/altimate/telemetry/index.ts +++ b/packages/opencode/src/altimate/telemetry/index.ts @@ -1683,7 +1683,10 @@ export namespace Telemetry { async function doInit() { try { - if (process.env.ALTIMATE_TELEMETRY_DISABLED === "true") { + // altimate_change — accept "true"/"TRUE"/"1" (case-insensitive) via truthyEnv, + // and honor the OPENCODE_DISABLE_TELEMETRY fallback promised by v0.9.4's CHANGELOG + // (previously only wired in test fixtures, silent no-op in product). + if (Flag.truthyEnv("ALTIMATE_TELEMETRY_DISABLED") || Flag.truthyEnv("OPENCODE_DISABLE_TELEMETRY")) { buffer = [] return } diff --git a/packages/opencode/src/flag/flag.ts b/packages/opencode/src/flag/flag.ts index 5f59df3304..a5603215db 100644 --- a/packages/opencode/src/flag/flag.ts +++ b/packages/opencode/src/flag/flag.ts @@ -19,6 +19,15 @@ function altEnv(altKey: string, openKey: string) { // altimate_change end export namespace Flag { + // altimate_change start — runtime-evaluated env-truthy helper for callers that need + // current process.env state. Module-level Flag.* constants freeze their value at + // import time; that's the wrong semantics for gates the caller re-reads on each + // invocation (e.g. telemetry.doInit()). Accepts "true" / "TRUE" / "1" — case-insensitive + // — so one convention covers every telemetry/onboarding opt-out env var. + export function truthyEnv(key: string): boolean { + return truthy(key) + } + // altimate_change end // altimate_change start - ALTIMATE_CLI_CLIENT flag with OPENCODE_CLIENT fallback export declare const ALTIMATE_CLI_CLIENT: string // altimate_change end diff --git a/packages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts b/packages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts new file mode 100644 index 0000000000..bd8131d243 --- /dev/null +++ b/packages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts @@ -0,0 +1,61 @@ +// v0.9.5 review — Chaos gremlin P1 + End-user P2. +// +// Before this release the two telemetry-disable gates +// (`altimate/plugin/altimate.ts::buildCliContext` and `altimate/telemetry/index.ts::doInit`) +// each checked `process.env.ALTIMATE_TELEMETRY_DISABLED === "true"`. A user +// running `ALTIMATE_TELEMETRY_DISABLED=1 altimate ...` (the shape most users +// reach for) was silently ignored — telemetry stayed on. v0.9.4's CHANGELOG +// also referenced `OPENCODE_DISABLE_TELEMETRY=1` as an opt-out env var, but +// that name was wired into test fixtures only and never checked in product. +// +// Both call sites now route through `Flag.truthyEnv`, which accepts +// "true"/"TRUE"/"1" and honors the OPENCODE_DISABLE_TELEMETRY fallback. +// This file locks the contract in place so a future edit to `truthy()` (or +// to either call site) can't silently narrow it again. + +import { afterEach, describe, expect, test } from "bun:test" +import { Flag } from "../../src/flag/flag" + +const VAR = "ALTIMATE_TELEMETRY_DISABLED" + +afterEach(() => { + delete process.env[VAR] +}) + +describe("Flag.truthyEnv — telemetry opt-out shape", () => { + test("unset env var → false (default: telemetry enabled)", () => { + delete process.env[VAR] + expect(Flag.truthyEnv(VAR)).toBe(false) + }) + + test("empty string → false", () => { + process.env[VAR] = "" + expect(Flag.truthyEnv(VAR)).toBe(false) + }) + + test.each([ + ["true", true], + ["TRUE", true], + ["True", true], + ["1", true], + ["false", false], + ["FALSE", false], + ["0", false], + ["yes", false], + ["on", false], + [" true ", false], // no trimming — matches existing truthy() semantics + ["2", false], + ])("value %j → %s", (value, expected) => { + process.env[VAR] = value + expect(Flag.truthyEnv(VAR)).toBe(expected) + }) + + test("re-reads process.env on each call (not frozen at import)", () => { + delete process.env[VAR] + expect(Flag.truthyEnv(VAR)).toBe(false) + process.env[VAR] = "1" + expect(Flag.truthyEnv(VAR)).toBe(true) + process.env[VAR] = "false" + expect(Flag.truthyEnv(VAR)).toBe(false) + }) +}) From 078876be896aa0d3047a67f59beebf3961abd6da Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 10 Aug 2026 19:06:50 +0530 Subject: [PATCH 02/10] docs(configure): document `ctrl+y` YOLO mid-session toggle in permissions + keybinds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v0.9.5 review — PM P1. The mid-session YOLO toggle (introduced in #1078, `bfb5a7cebe`) is a security-relevant control — it auto-approves prompts for `rm -rf`, `git push --force`, `.env` reads, etc. — but zero external docs mentioned it existed. Discoverability was in-app only (a persistent hint next to the prompt + entry in the command palette), so a user who never noticed the hint could not find it via docs/search. - `docs/docs/configure/permissions.md`: added a "Mid-session toggle (TUI)" paragraph under the existing "Yolo Mode" section, documenting `Ctrl+Y`, confirmation-on-enable / instant-off, session/subagent scope + in-memory lifetime, and the deny-rules-still-apply guarantee. - `docs/docs/configure/keybinds.md`: added a `Ctrl+Y` row to the "UI Toggles" table and appended `session_yolo_toggle` to the Session identifier reference list. --- docs/docs/configure/keybinds.md | 3 ++- docs/docs/configure/permissions.md | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/docs/configure/keybinds.md b/docs/docs/configure/keybinds.md index 8628446d47..dba58308f1 100644 --- a/docs/docs/configure/keybinds.md +++ b/docs/docs/configure/keybinds.md @@ -61,6 +61,7 @@ Override it in your config: | Leader + `k` | Keybind list | | Leader + `e` | Open editor | | Leader + `q` | Quit | +| `Ctrl+Y` | Toggle YOLO mode for this session (confirms when enabling; instant when disabling) | ### Input Editing @@ -114,7 +115,7 @@ All configurable keybind identifiers: ### Session -`session_export`, `session_new`, `session_list`, `session_timeline`, `session_fork`, `session_rename`, `session_delete`, `session_child_cycle`, `session_parent`, `session_share`, `session_unshare`, `session_interrupt`, `session_compact` +`session_export`, `session_new`, `session_list`, `session_timeline`, `session_fork`, `session_rename`, `session_delete`, `session_child_cycle`, `session_parent`, `session_share`, `session_unshare`, `session_interrupt`, `session_compact`, `session_yolo_toggle` ### Messages diff --git a/docs/docs/configure/permissions.md b/docs/docs/configure/permissions.md index 3b4e7e7557..aafacf38af 100644 --- a/docs/docs/configure/permissions.md +++ b/docs/docs/configure/permissions.md @@ -132,6 +132,8 @@ The fallback `OPENCODE_YOLO` env var is also supported. When both are set, `ALTI When yolo mode is active in the TUI, a `△ YOLO` indicator appears in the footer status bar. +**Mid-session toggle (TUI):** Press `Ctrl+Y` inside the TUI to toggle yolo mode for the current session without restarting. Enabling requires a one-tap confirmation; disabling is instant. The toggle is **session and subagent scoped and lives in memory only** — restart the CLI and yolo defaults back to whatever `--yolo` / `ALTIMATE_CLI_YOLO` was at launch. Deny rules stay enforced. + ## Recommended Configurations ### Data Engineering (Default, Balanced) From 28fb476604f973d7494ffad378f633ef96ff8837 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 10 Aug 2026 19:07:10 +0530 Subject: [PATCH 03/10] test(telemetry): cover `classifyProvider` allowlist + prototype-pollution defense MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v0.9.5 review — Tech Lead P1. `Telemetry.classifyProvider` (`telemetry/index.ts:1028`) shipped this release with zero test coverage despite sitting on the `provider_selected` privacy path. Its guarantees are: - `CURATED_PROVIDER_ENUM` uses `Object.create(null)` — a plain literal would resolve `["constructor"]`, `["toString"]`, `["valueOf"]` to inherited functions (all truthy), and the branch `if (curated) return { provider: curated, ... }` would ship a JS built-in as a curated provider slug. This test asserts the null-prototype defense holds. - Only `KNOWN_PROVIDER_IDS` carry a raw `provider_id` on the wire; customer-named custom providers fall through to `{ provider: "other" }` with no id (no PII leak). - The `opencode` + `big-pickle` pair returns `"big_pickle"` only when BOTH args match — proves a regression that ignored `modelID` would be caught. 29 tests, all pure — no fixtures, no side effects. --- .../test/telemetry/classify-provider.test.ts | 113 ++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100644 packages/opencode/test/telemetry/classify-provider.test.ts diff --git a/packages/opencode/test/telemetry/classify-provider.test.ts b/packages/opencode/test/telemetry/classify-provider.test.ts new file mode 100644 index 0000000000..53bf1b31b4 --- /dev/null +++ b/packages/opencode/test/telemetry/classify-provider.test.ts @@ -0,0 +1,113 @@ +// v0.9.5 review — Tech Lead P1. +// +// classifyProvider (packages/opencode/src/altimate/telemetry/index.ts) sits on the +// `provider_selected` telemetry path. It is the point where a caller-supplied provider +// id becomes an enum value on our wire, so its allowlist is load-bearing: +// +// - CURATED_PROVIDER_ENUM must be a null-prototype record. A plain `{}` inherits +// Object.prototype, and `record["constructor"]` / ["toString"] / ["valueOf"] +// resolve to inherited functions — those functions are truthy, so with a plain +// object the branch `if (curated) return { provider: curated, ... }` would +// ship the string form of a JS built-in as a "provider" name (or worse, whatever +// the caller-supplied id was, since normalizeCustomProviderID upstream permits +// lowercase letters). The null-prototype defense makes those lookups return +// undefined, forcing the "not curated" path. +// +// - Only ids in KNOWN_PROVIDER_IDS should carry a raw provider_id on the wire. +// Everything else falls through to `{ provider: "other" }` with NO id attached — +// that's what keeps a customer-named custom provider from leaking to telemetry. +// +// - The `opencode` + `big-pickle` pair is the one hard-coded case that returns +// "big_pickle" rather than one of the curated slugs, and it depends on BOTH +// args matching. A regression that ignored modelID would cause every +// `providerID="opencode"` to still ship as `big_pickle`, misattributing traffic. +// +// This file locks each of those three behaviors down. + +import { describe, expect, test } from "bun:test" +import { Telemetry } from "../../src/altimate/telemetry" + +describe("Telemetry.classifyProvider — allowlist + prototype defense", () => { + describe("curated providers", () => { + test.each([ + ["altimate-backend", "altimate_gateway"], + ["anthropic", "anthropic"], + ["openai", "openai"], + ["google", "google"], + ])("providerID %j → provider %j, keeps raw id", (providerID, expected) => { + const result = Telemetry.classifyProvider(providerID) + expect(result).toEqual({ provider: expected, provider_id: providerID }) + }) + }) + + describe("prototype-pollution defense", () => { + // The three inherited-property names most likely to appear as a "provider id" + // in the wild (they're plain lowercase identifiers, so they slip past + // normalizeCustomProviderID). Without the Object.create(null) barrier, + // `CURATED_PROVIDER_ENUM["constructor"]` returns the JS constructor Function, + // which is truthy — and the branch would ship it as a curated provider. + test.each(["constructor", "toString", "valueOf", "hasOwnProperty", "__proto__"])( + "prototype key %j must NOT be treated as a curated match", + (key) => { + const result = Telemetry.classifyProvider(key) + // The one guarantee this test cares about: the result is not one of the + // curated enum values. What it collapses to (usually "other" without an id) + // is fine — the defect being guarded against is exactly the shipping of a + // prototype method as a curated enum. + expect(["altimate_gateway", "anthropic", "openai", "google", "big_pickle"]).not.toContain(result.provider) + expect(result.provider).toBe("other") + }, + ) + }) + + describe("known-but-not-curated providers", () => { + test.each([ + "opencode", + "github-copilot", + "azure", + "amazon-bedrock", + "openrouter", + "mistral", + "groq", + "deepseek", + "xai", + "snowflake-cortex", + "databricks", + "ollama", + "lmstudio", + ])("providerID %j → provider 'other', keeps raw id (safe to publish)", (providerID) => { + const result = Telemetry.classifyProvider(providerID) + expect(result).toEqual({ provider: "other", provider_id: providerID }) + }) + }) + + describe("unknown / customer-named providers", () => { + test.each(["acme-corp", "my-internal-gateway", "team-eng-shared-llm", ""])( + "providerID %j → provider 'other', DROPS raw id (no PII leak)", + (providerID) => { + const result = Telemetry.classifyProvider(providerID) + expect(result.provider).toBe("other") + expect(result.provider_id).toBeUndefined() + }, + ) + }) + + describe("opencode + big-pickle hard-coded pair", () => { + test("both provider and model must match — provider only ≠ big_pickle", () => { + const result = Telemetry.classifyProvider("opencode") + // opencode is known-not-curated → "other" + id, NOT "big_pickle" + expect(result).toEqual({ provider: "other", provider_id: "opencode" }) + }) + + test("both provider and model must match — model only ≠ big_pickle", () => { + const result = Telemetry.classifyProvider("anthropic", "big-pickle") + // Anthropic-with-a-strange-model is still anthropic, not big_pickle + expect(result).toEqual({ provider: "anthropic", provider_id: "anthropic" }) + }) + + test("both matching → big_pickle", () => { + const result = Telemetry.classifyProvider("opencode", "big-pickle") + expect(result).toEqual({ provider: "big_pickle", provider_id: "opencode" }) + }) + }) +}) From 66fb1658b74d8bbcbefa617f940afccbfacfa811 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 10 Aug 2026 19:07:31 +0530 Subject: [PATCH 04/10] test(sample-setup): cover `redactPaths` + `countSampleContents` docstring claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v0.9.5 review — Tech Lead P1. ~90 new lines in `sample-setup.ts` shipped this release without direct unit coverage. `redactPaths` in particular lists three specific bugs in its docstring that the implementation was written to fix — sentence-swallowing regex, `/root/…` prefix leak, and partial redaction of surnames containing an apostrophe or accented character (José, O'Connor). None were asserted. - `redactPaths` + `countSampleContents` promoted from module-local to exports for direct testing. Pure functions, no additional risk. - New `sample-setup-helpers.test.ts` (16 tests) asserts each docstring claim: sentence terminates on whitespace/quote; O'Connor and José redact cleanly; adjacent `` markers collapse; short/empty known values don't shatter the input; user-supplied `extra` list works. - `countSampleContents` gets a real dir-tree fixture (models/staging, models/marts/core, seeds/) and asserts the recursive `.sql` count + top-level `.csv` count that feeds the `sample_setup_completed` telemetry event. --- .../src/altimate/tools/sample-setup.ts | 7 +- .../altimate/sample-setup-helpers.test.ts | 171 ++++++++++++++++++ 2 files changed, 176 insertions(+), 2 deletions(-) create mode 100644 packages/opencode/test/altimate/sample-setup-helpers.test.ts diff --git a/packages/opencode/src/altimate/tools/sample-setup.ts b/packages/opencode/src/altimate/tools/sample-setup.ts index bfaee9654e..d0c16aa751 100644 --- a/packages/opencode/src/altimate/tools/sample-setup.ts +++ b/packages/opencode/src/altimate/tools/sample-setup.ts @@ -268,7 +268,9 @@ export const SampleSetupTool = Tool.define("sample_setup", { * apply a conservative pattern that stops at whitespace and quotes rather than trying to guess * where a path ends. Full detail is kept in `metadata.error`, which the model never sees. */ -function redactPaths(message: string, extra: (string | undefined)[] = []): string { +// altimate_change — exported for direct unit testing (v0.9.5 review, Tech Lead P1). +// Pure function; no additional risk from exposing it. +export function redactPaths(message: string, extra: (string | undefined)[] = []): string { let out = message for (const known of [os.homedir(), process.cwd(), os.tmpdir(), ...extra]) { if (!known || known.length < 2) continue @@ -306,8 +308,9 @@ function countFilesWithExtension(dir: string, extension: string): number { return total } +// altimate_change — exported for direct unit testing (v0.9.5 review, Tech Lead P1). /** dbt models (`models/**\/*.sql`) and seed tables (`seeds/*.csv`) in the shipped sample. */ -function countSampleContents(sampleSourcePath: string): { models: number; tables: number } { +export function countSampleContents(sampleSourcePath: string): { models: number; tables: number } { return { models: countFilesWithExtension(path.join(sampleSourcePath, "models"), ".sql"), tables: countFilesWithExtension(path.join(sampleSourcePath, "seeds"), ".csv"), diff --git a/packages/opencode/test/altimate/sample-setup-helpers.test.ts b/packages/opencode/test/altimate/sample-setup-helpers.test.ts new file mode 100644 index 0000000000..656a8dfaff --- /dev/null +++ b/packages/opencode/test/altimate/sample-setup-helpers.test.ts @@ -0,0 +1,171 @@ +// v0.9.5 review — Tech Lead P1. +// +// sample-setup.ts::redactPaths has a docstring listing three specific bugs the +// implementation was written to fix (regex swallowing surrounding sentence, +// `/root/…` prefix leaking, José/O'Connor surnames leaking on the first pass). +// None of them had test coverage this release. This file asserts the fixes so a +// future edit to the regex or the known-value substitution loop can't silently +// re-introduce them. +// +// countSampleContents is a tiny counting helper — but it's called on every +// sample_setup invocation and its output feeds a telemetry event, so a +// silent-zero bug (typo in the extension filter, wrong subdirectory name) +// would misreport onboarding activity. The fixture below builds a real dir tree +// per test and asserts the count. + +import { describe, expect, test, beforeAll, afterAll } from "bun:test" +import fs from "fs" +import os from "os" +import path from "path" + +import { redactPaths, countSampleContents } from "../../src/altimate/tools/sample-setup" + +describe("redactPaths", () => { + test("redacts an absolute POSIX path", () => { + const out = redactPaths("failed at /usr/local/bin/dbt") + expect(out).toBe("failed at ") + }) + + test("redacts a Windows drive path", () => { + const out = redactPaths("failed at C:\\Users\\alice\\dbt.exe") + // Windows paths pattern matches `C:\` opener and consumes until whitespace/quote. + expect(out).toBe("failed at ") + expect(out).not.toContain("Users") + expect(out).not.toContain("alice") + }) + + test("redacts a home-relative path (~/)", () => { + // Note: `~/.altimate/…` is redacted by the POSIX-`/` pattern first, which + // starts at the leading slash and leaves the `~` as a harmless prefix. + // What the assertion cares about is that no path segment leaks — the exact + // shape of the redaction marker is secondary. + const out = redactPaths("cannot read ~/.altimate/machine-id") + expect(out).not.toContain(".altimate") + expect(out).not.toContain("machine-id") + expect(out).toContain("") + }) + + test("redacts a bare tilde-only path (~/x with no preceding slash)", () => { + // The dedicated `~\/…` pattern is what catches this shape — the POSIX-`/` + // one starts inside the path and can leave a `~` behind. + const input = "opening ~/opt/dbt for read" + const out = redactPaths(input) + expect(out).not.toContain("opt/dbt") + expect(out).toContain("") + }) + + test("terminates at whitespace, does NOT swallow the surrounding sentence", () => { + // The docstring calls out this exact class of bug: an early implementation + // used a character class that included `.`, so the regex would consume the + // rest of the sentence past the path. Reader ends up with just "". + const input = "Underlying error: /Users/alice/projects/dbt-demo failed to compile" + const out = redactPaths(input) + expect(out).toContain("failed to compile") + expect(out).toContain("Underlying error:") + expect(out).not.toContain("/Users") + }) + + test("terminates at a double-quote", () => { + const out = redactPaths('opening "/Users/alice/dbt_project.yml" for read') + expect(out).toContain("for read") + expect(out).not.toContain("alice") + }) + + test("handles a path containing an apostrophe (O'Connor)", () => { + // Docstring bug: an early character class excluded `'`, so the regex would + // stop at the apostrophe and leak the substring after it. Now apostrophes + // are permitted inside the redacted run. + const out = redactPaths("failed at /Users/O'Connor/projects/x") + expect(out).not.toContain("O'Connor") + expect(out).not.toContain("Connor") + expect(out).toBe("failed at ") + }) + + test("handles a path containing an accented character (José)", () => { + const out = redactPaths("failed at /Users/José/dbt") + expect(out).not.toContain("José") + expect(out).toBe("failed at ") + }) + + test("collapses adjacent segments so double-redaction reads clean", () => { + // The known-value pass replaces os.homedir() etc first; the greedy pattern + // then may match the "" tail and re-redact. The collapse rule keeps + // the output from becoming "". + const home = os.homedir() + const out = redactPaths(`failed at ${home}/dbt/models/foo.sql`) + expect(out).toBe("failed at ") + expect(out).not.toMatch(//) + }) + + test("passes short/empty known values without exploding", () => { + // Guard for `known.length < 2` — empty string or single-char known values + // used to `split("")` and shatter every character. The guard keeps them out + // of the substitution loop. + const out = redactPaths("hello world", ["", "a", undefined]) + expect(out).toBe("hello world") + }) + + test("substitutes user-supplied extras", () => { + const out = redactPaths("clone failed at /tmp/checkout-xyz", ["/tmp/checkout-xyz"]) + expect(out).toBe("clone failed at ") + }) + + test("returns the message unchanged when nothing path-shaped is present", () => { + expect(redactPaths("dbt run completed in 3s")).toBe("dbt run completed in 3s") + }) +}) + +describe("countSampleContents", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "sample-setup-helpers-")) + + beforeAll(() => { + // Real dir tree. Not mocked — the helper does synchronous fs.readdirSync, + // and a mocked fs would drift out of shape from what the caller sees. + fs.mkdirSync(path.join(root, "models", "staging"), { recursive: true }) + fs.mkdirSync(path.join(root, "models", "marts", "core"), { recursive: true }) + fs.mkdirSync(path.join(root, "seeds"), { recursive: true }) + + fs.writeFileSync(path.join(root, "models", "top.sql"), "select 1") + fs.writeFileSync(path.join(root, "models", "staging", "stg_orders.sql"), "select 1") + fs.writeFileSync(path.join(root, "models", "staging", "stg_users.sql"), "select 1") + fs.writeFileSync(path.join(root, "models", "marts", "core", "dim_customers.sql"), "select 1") + + // Non-.sql alongside .sql, must NOT be counted. + fs.writeFileSync(path.join(root, "models", "readme.md"), "hi") + fs.writeFileSync(path.join(root, "models", "schema.yml"), "version: 2") + + fs.writeFileSync(path.join(root, "seeds", "country_codes.csv"), "code,name\n") + fs.writeFileSync(path.join(root, "seeds", "regions.csv"), "id,name\n") + // Non-.csv seed — not counted. + fs.writeFileSync(path.join(root, "seeds", "notes.md"), "hi") + }) + + afterAll(() => { + fs.rmSync(root, { recursive: true, force: true }) + }) + + test("counts .sql files recursively under models/", () => { + const { models } = countSampleContents(root) + expect(models).toBe(4) + }) + + test("counts .csv files under seeds/ (top level; matches production sample layout)", () => { + const { tables } = countSampleContents(root) + expect(tables).toBe(2) + }) + + test("returns zeros when the sample dir is missing the expected subdirs", () => { + const empty = fs.mkdtempSync(path.join(os.tmpdir(), "sample-setup-empty-")) + try { + expect(countSampleContents(empty)).toEqual({ models: 0, tables: 0 }) + } finally { + fs.rmSync(empty, { recursive: true, force: true }) + } + }) + + test("returns zeros when the sample dir does not exist at all", () => { + // countFilesWithExtension swallows the readdirSync error and returns 0 — + // this is the graceful-degradation shape the telemetry event depends on. + expect(countSampleContents(path.join(root, "does-not-exist"))).toEqual({ models: 0, tables: 0 }) + }) +}) From 4759a751c9d5e9beb2631c6603d91bc133f27f04 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 10 Aug 2026 19:07:56 +0530 Subject: [PATCH 05/10] test(onboarding): assert `claimEnvironmentScan` idempotency + session isolation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v0.9.5 review — Tech Lead P1. The `environment_scan_completed` emission at `project-scan.ts:952-964` fires only when `isOnboardingSession(sessionID) && claimEnvironmentScan(sessionID)`. The claim call is the one thing preventing a second `project_scan` run inside the same onboarding session from pushing `scan_gate_shown → environment_scan_completed` above 100% in the funnel dashboard. That claim wasn't directly tested. Full end-to-end coverage through `project-scan.ts` needs git/dbt/docker detection stubs and is disproportionately expensive for the guarantee at stake, so this file tests the load-bearing behavior at its actual home (`onboarding.ts`): 1. First claim → true, subsequent claims for same session → false 2. Claims are session-scoped — new sessionID claims independently 3. `isOnboardingSession` is false for untracked sessions 4. The composed guard `isOnboardingSession && claim` fires exactly once across N `project_scan` runs in an onboarding session 5. A non-onboarding session running the same guard chain never fires, and the claim stays unspent (proving the short-circuit works) 6 tests, `resetForTest()` between each. --- .../telemetry/environment-scan-claim.test.ts | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 packages/opencode/test/telemetry/environment-scan-claim.test.ts diff --git a/packages/opencode/test/telemetry/environment-scan-claim.test.ts b/packages/opencode/test/telemetry/environment-scan-claim.test.ts new file mode 100644 index 0000000000..12431995ab --- /dev/null +++ b/packages/opencode/test/telemetry/environment-scan-claim.test.ts @@ -0,0 +1,100 @@ +// v0.9.5 review — Tech Lead P1. +// +// project-scan.ts (packages/opencode/src/altimate/tools/project-scan.ts:952-964) emits +// `environment_scan_completed` guarded by: +// +// if (OnboardingTelemetry.isOnboardingSession(ctx.sessionID) +// && OnboardingTelemetry.claimEnvironmentScan(ctx.sessionID)) { void OnboardingTelemetry.emit(...) } +// +// The claim call is the only thing preventing double-fire of the funnel event +// (a second project_scan invocation inside the same onboarding session would +// otherwise push `scan_gate_shown → environment_scan_completed` above 100%, +// which is the exact metric the comment on that emission block calls out as +// worth protecting). +// +// End-to-end coverage through project-scan requires shelling to git/dbt/docker +// detection and is disproportionately expensive for the guarantee at stake. +// The load-bearing behavior is the once-per-session claim + the session-scope +// isolation of that claim, both of which live in onboarding.ts and are +// independently testable. + +import { afterEach, beforeEach, describe, expect, test } from "bun:test" +import * as OnboardingTelemetry from "../../src/altimate/telemetry/onboarding" + +beforeEach(() => { + OnboardingTelemetry.resetForTest() +}) +afterEach(() => { + OnboardingTelemetry.resetForTest() +}) + +describe("environment_scan_completed guard", () => { + test("claimEnvironmentScan returns true on first call, false on subsequent calls", () => { + const session = "sess-1" + expect(OnboardingTelemetry.claimEnvironmentScan(session)).toBe(true) + expect(OnboardingTelemetry.claimEnvironmentScan(session)).toBe(false) + expect(OnboardingTelemetry.claimEnvironmentScan(session)).toBe(false) + }) + + test("claims are session-scoped — a second session claims independently", () => { + expect(OnboardingTelemetry.claimEnvironmentScan("sess-A")).toBe(true) + expect(OnboardingTelemetry.claimEnvironmentScan("sess-B")).toBe(true) + // ...and each session's claim stays exhausted after its first success + expect(OnboardingTelemetry.claimEnvironmentScan("sess-A")).toBe(false) + expect(OnboardingTelemetry.claimEnvironmentScan("sess-B")).toBe(false) + }) + + test("isOnboardingSession is false for sessions that were never marked", () => { + // The AND-guard on the emission means a non-onboarding session that runs + // project_scan (via /discover, or a model-initiated call) will NOT emit + // the onboarding-funnel event, even though the claim call would succeed + // on its own. This is what stops the funnel-taxonomy event from firing + // for routine `/discover` runs from returning users. + expect(OnboardingTelemetry.isOnboardingSession("random-session")).toBe(false) + }) + + test("isOnboardingSession is true only after markOnboardingSession", () => { + const s = "onboarding-sess" + expect(OnboardingTelemetry.isOnboardingSession(s)).toBe(false) + OnboardingTelemetry.markOnboardingSession(s) + expect(OnboardingTelemetry.isOnboardingSession(s)).toBe(true) + }) + + test("the AND-guard (isOnboardingSession && claimEnvironmentScan) fires exactly once", () => { + // Mirrors the exact shape at project-scan.ts:952. A single onboarding + // session that runs project_scan twice must see the emission gate open + // once and stay closed on the retry. + const s = "funnel-sess" + OnboardingTelemetry.markOnboardingSession(s) + const fires: number[] = [] + for (let i = 0; i < 3; i++) { + if ( + OnboardingTelemetry.isOnboardingSession(s) && + OnboardingTelemetry.claimEnvironmentScan(s) + ) { + fires.push(i) + } + } + expect(fires).toEqual([0]) + }) + + test("a non-onboarding session running the same guard chain never fires", () => { + const s = "returning-user-sess" + // Note: no markOnboardingSession call. Guard should short-circuit at + // isOnboardingSession → false and never even reach the claim call, so + // the claim stays unspent (verifiable below). + const fires: number[] = [] + for (let i = 0; i < 3; i++) { + if ( + OnboardingTelemetry.isOnboardingSession(s) && + OnboardingTelemetry.claimEnvironmentScan(s) + ) { + fires.push(i) + } + } + expect(fires).toEqual([]) + // If the guard had short-circuited correctly, the claim is still available. + // Prove it by explicitly calling claimEnvironmentScan and observing true. + expect(OnboardingTelemetry.claimEnvironmentScan(s)).toBe(true) + }) +}) From 388f41bc12be37a4b5da0dfdc7ae712aff3a3001 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 10 Aug 2026 19:08:17 +0530 Subject: [PATCH 06/10] chore(welcome): replace vague "tracked separately" claim with explicit `FIXME` anchor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v0.9.5 review — PM P2. The comment at `welcome.ts:46` referenced a "pre-existing telemetry-init gap (tracked separately)" — a claim that implied a tracking issue existed. It didn't (verified by searching open issues on `AltimateAI/altimate-code`). Rewritten to: - Anchor the gap with `FIXME(telemetry-init-config-opt-out)` so it shows up in code searches. - Describe the exact failure mode: `doInit()` may run before `Instance.provide()` has made `Config.get()` resolvable — the catch branch in `telemetry/index.ts::doInit` proceeds with telemetry enabled, so a user who opted out via `telemetry.disabled` config (env var not set) can still have a machine-id minted on cold-start. - Explicitly note the env-var opt-out (`ALTIMATE_TELEMETRY_DISABLED` / `OPENCODE_DISABLE_TELEMETRY`) is unaffected — that check doesn't need Instance context. - Honestly state no tracking issue currently exists rather than repeat the earlier false claim. Pre-existing (not introduced by this release). Filed as part of the release/v0.9.5 deferred-items tracker. --- packages/opencode/src/cli/welcome.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/opencode/src/cli/welcome.ts b/packages/opencode/src/cli/welcome.ts index 650a851a31..dab265049a 100644 --- a/packages/opencode/src/cli/welcome.ts +++ b/packages/opencode/src/cli/welcome.ts @@ -43,10 +43,16 @@ export function showWelcomeBannerIfNeeded(): void { // launch. Probe existence with existsSync only — do NOT mint here. Minting is left to // Telemetry.doInit() (its job, not the welcome banner's); the first_launch machine_id is // attached at flush time from telemetry module state, so it does not depend on minting here. - // NOTE: doInit's early call runs before an Instance is available, so its CONFIG opt-out gate - // fails open and it can still mint for a config-only opt-out user. That is a pre-existing - // telemetry-init gap (tracked separately), not something this banner can fix — this code just - // stops adding a SECOND minting site under an even weaker (env-only) gate. + // + // FIXME(telemetry-init-config-opt-out): doInit() may run before Instance.provide() has made + // Config.get() resolvable (see the try/catch around Config.get in telemetry/index.ts::doInit + // — the catch branch proceeds with telemetry enabled). A user who opted out via the + // `telemetry.disabled` config key — with no env var set — can therefore still get a + // machine-id minted on first launch. The env-var opt-out (ALTIMATE_TELEMETRY_DISABLED / + // OPENCODE_DISABLE_TELEMETRY) is unaffected — that check does not need Instance context. + // Pre-existing (not introduced by this release); calling it out explicitly here rather than + // leaving the earlier "(tracked separately)" wording, which claimed a tracking issue that + // does not currently exist. const machineIdPath = path.join(os.homedir(), ".altimate", "machine-id") const isUpgrade = fs.existsSync(machineIdPath) // altimate_change end From 2407ededa60d8284f02e4f69797ec56424111977 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 10 Aug 2026 20:23:10 +0530 Subject: [PATCH 07/10] docs(permissions): include `OPENCODE_YOLO` in YOLO mid-session toggle restart-state sentence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coderabbit review on PR #1086: the "Mid-session toggle (TUI)" paragraph added in commit `078876be89` listed only `--yolo` and `ALTIMATE_CLI_YOLO` as the launch-time sources whose value the toggle reverts to. Line 129 immediately above already documents `OPENCODE_YOLO` as a supported fallback, so the omission was inconsistent — a user who set only `OPENCODE_YOLO=true` had no docs describing what happens after a `Ctrl+Y` toggle + restart. --- docs/docs/configure/permissions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/docs/configure/permissions.md b/docs/docs/configure/permissions.md index aafacf38af..852b8ac1a8 100644 --- a/docs/docs/configure/permissions.md +++ b/docs/docs/configure/permissions.md @@ -132,7 +132,7 @@ The fallback `OPENCODE_YOLO` env var is also supported. When both are set, `ALTI When yolo mode is active in the TUI, a `△ YOLO` indicator appears in the footer status bar. -**Mid-session toggle (TUI):** Press `Ctrl+Y` inside the TUI to toggle yolo mode for the current session without restarting. Enabling requires a one-tap confirmation; disabling is instant. The toggle is **session and subagent scoped and lives in memory only** — restart the CLI and yolo defaults back to whatever `--yolo` / `ALTIMATE_CLI_YOLO` was at launch. Deny rules stay enforced. +**Mid-session toggle (TUI):** Press `Ctrl+Y` inside the TUI to toggle yolo mode for the current session without restarting. Enabling requires a one-tap confirmation; disabling is instant. The toggle is **session and subagent scoped and lives in memory only** — restart the CLI and yolo defaults back to whatever `--yolo`, `ALTIMATE_CLI_YOLO`, or `OPENCODE_YOLO` was at launch. Deny rules stay enforced. ## Recommended Configurations From 37430a5c0f824b0c2b49a5060b05d5912cc2a236 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 10 Aug 2026 20:23:31 +0530 Subject: [PATCH 08/10] test(telemetry): drop redundant `not.toContain` before `toBe("other")` in prototype-defense test Cubic P3 on PR #1086: `expect([...curated...]).not.toContain(result.provider)` is a strict weakening of the very next line's `expect(result.provider).toBe("other")`. `toBe("other")` already excludes every curated enum value; the array-not-contain check adds no signal and implies the test enforces something broader than the exact-match assertion actually does. --- .../opencode/test/telemetry/classify-provider.test.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/opencode/test/telemetry/classify-provider.test.ts b/packages/opencode/test/telemetry/classify-provider.test.ts index 53bf1b31b4..82e85d17d0 100644 --- a/packages/opencode/test/telemetry/classify-provider.test.ts +++ b/packages/opencode/test/telemetry/classify-provider.test.ts @@ -50,11 +50,9 @@ describe("Telemetry.classifyProvider — allowlist + prototype defense", () => { "prototype key %j must NOT be treated as a curated match", (key) => { const result = Telemetry.classifyProvider(key) - // The one guarantee this test cares about: the result is not one of the - // curated enum values. What it collapses to (usually "other" without an id) - // is fine — the defect being guarded against is exactly the shipping of a - // prototype method as a curated enum. - expect(["altimate_gateway", "anthropic", "openai", "google", "big_pickle"]).not.toContain(result.provider) + // The guarantee: a prototype key must not resolve to any curated enum. + // `toBe("other")` implies it's none of `altimate_gateway|anthropic|openai|google|big_pickle`, + // so no separate `not.toContain` guard is needed. expect(result.provider).toBe("other") }, ) From a5c1ca7e91df29e96b43bf0508534ca2b8b9d99d Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 10 Aug 2026 20:23:51 +0530 Subject: [PATCH 09/10] test(sample-setup): switch to per-test `tmpdir()` fixture + assert whitespace-in-path documented gap Two bot-review follow-ups on PR #1086: - Coderabbit + cubic (P3): the `countSampleContents` describe used one shared `mkdtempSync` at module scope with cleanup only in `afterAll`. Two problems: the fixture leaks when the suite is filtered (only redactPaths tests run) or when a `beforeAll` throws before `afterAll` registers, and it disagrees with the repo's `await using tmp = await tmpdir()` convention. Reworked each test to own its own tmp dir via the shared `tmpdir` fixture from `test/fixture`. - Codex: the redactPaths tests exercised each listed path shape but not paths containing whitespace. The greedy pattern terminates at the first `\s`, so a real CWD like `/Users/alice/My Documents/dbt` leaks the middle segment `Documents` between two `` markers. Added a test that: 1. asserts the raw pass DOES leak `Documents` (documents the limitation) 2. asserts the guarded pass (`redactPaths(msg, [cwd])`) collapses cleanly Production callers already pass the CWD as a known-value extra, so the guarded path is the one the wire sees. The test locks that in. --- .../altimate/sample-setup-helpers.test.ts | 114 +++++++++++------- 1 file changed, 69 insertions(+), 45 deletions(-) diff --git a/packages/opencode/test/altimate/sample-setup-helpers.test.ts b/packages/opencode/test/altimate/sample-setup-helpers.test.ts index 656a8dfaff..1327326c29 100644 --- a/packages/opencode/test/altimate/sample-setup-helpers.test.ts +++ b/packages/opencode/test/altimate/sample-setup-helpers.test.ts @@ -1,4 +1,4 @@ -// v0.9.5 review — Tech Lead P1. +// v0.9.5 review — Tech Lead P1 (initial pass) + coderabbit / cubic follow-ups. // // sample-setup.ts::redactPaths has a docstring listing three specific bugs the // implementation was written to fix (regex swallowing surrounding sentence, @@ -12,12 +12,19 @@ // silent-zero bug (typo in the extension filter, wrong subdirectory name) // would misreport onboarding activity. The fixture below builds a real dir tree // per test and asserts the count. - -import { describe, expect, test, beforeAll, afterAll } from "bun:test" +// +// Fixture ownership note (bot-review follow-up, coderabbit + cubic): +// countSampleContents originally shared one `mkdtempSync` at module scope with +// afterAll cleanup. That leaks if the suite is filtered (only redactPaths tests +// selected) or if a `beforeAll` throws before `afterAll` registers. Switched to +// the repo's `await using tmp = await tmpdir()` pattern so each test owns its +// fixture and cleanup is bound to the test scope. + +import { describe, expect, test } from "bun:test" import fs from "fs" -import os from "os" import path from "path" +import { tmpdir } from "../fixture/fixture" import { redactPaths, countSampleContents } from "../../src/altimate/tools/sample-setup" describe("redactPaths", () => { @@ -87,11 +94,35 @@ describe("redactPaths", () => { expect(out).toBe("failed at ") }) + test("path segment terminates at the first whitespace, so a path containing a space leaks the tail", () => { + // codex-review gap: the greedy pattern stops at the first `\s`, so a real + // CWD like `/Users/alice/My Documents/dbt` gets split — only `/Users/alice/My` + // is redacted; `Documents/dbt` is left in the output. + // + // The `extra` list is what production callers use to close this gap (they + // pass the exact CWD to `redactPaths(msg, [cwd])`), so this test also + // asserts the compensating behavior — with the CWD known-value, the whole + // path collapses cleanly. + const cwd = "/Users/alice/My Documents/dbt" + const raw = redactPaths(`failed at ${cwd}/models/foo.sql`) + // Documented limitation of the pattern-only pass: the greedy path pattern + // terminates at the first whitespace, so `/Users/alice/My` and + // `/dbt/models/foo.sql` each redact cleanly but the middle segment + // `Documents` sits between two `` markers. + expect(raw).toContain("Documents") + expect(raw).not.toBe("failed at ") + // With the CWD passed as a known value the whole path collapses cleanly: + const guarded = redactPaths(`failed at ${cwd}/models/foo.sql`, [cwd]) + expect(guarded).toBe("failed at ") + expect(guarded).not.toContain("Documents") + expect(guarded).not.toContain("alice") + }) + test("collapses adjacent segments so double-redaction reads clean", () => { // The known-value pass replaces os.homedir() etc first; the greedy pattern // then may match the "" tail and re-redact. The collapse rule keeps // the output from becoming "". - const home = os.homedir() + const home = require("os").homedir() const out = redactPaths(`failed at ${home}/dbt/models/foo.sql`) expect(out).toBe("failed at ") expect(out).not.toMatch(//) @@ -115,57 +146,50 @@ describe("redactPaths", () => { }) }) -describe("countSampleContents", () => { - const root = fs.mkdtempSync(path.join(os.tmpdir(), "sample-setup-helpers-")) - - beforeAll(() => { - // Real dir tree. Not mocked — the helper does synchronous fs.readdirSync, - // and a mocked fs would drift out of shape from what the caller sees. - fs.mkdirSync(path.join(root, "models", "staging"), { recursive: true }) - fs.mkdirSync(path.join(root, "models", "marts", "core"), { recursive: true }) - fs.mkdirSync(path.join(root, "seeds"), { recursive: true }) +// Shared helper — each test uses its own tmp dir via `await using`, so cleanup +// is scoped to the test itself (bot-review follow-up). +async function seedSampleTree(dir: string) { + fs.mkdirSync(path.join(dir, "models", "staging"), { recursive: true }) + fs.mkdirSync(path.join(dir, "models", "marts", "core"), { recursive: true }) + fs.mkdirSync(path.join(dir, "seeds"), { recursive: true }) - fs.writeFileSync(path.join(root, "models", "top.sql"), "select 1") - fs.writeFileSync(path.join(root, "models", "staging", "stg_orders.sql"), "select 1") - fs.writeFileSync(path.join(root, "models", "staging", "stg_users.sql"), "select 1") - fs.writeFileSync(path.join(root, "models", "marts", "core", "dim_customers.sql"), "select 1") + fs.writeFileSync(path.join(dir, "models", "top.sql"), "select 1") + fs.writeFileSync(path.join(dir, "models", "staging", "stg_orders.sql"), "select 1") + fs.writeFileSync(path.join(dir, "models", "staging", "stg_users.sql"), "select 1") + fs.writeFileSync(path.join(dir, "models", "marts", "core", "dim_customers.sql"), "select 1") - // Non-.sql alongside .sql, must NOT be counted. - fs.writeFileSync(path.join(root, "models", "readme.md"), "hi") - fs.writeFileSync(path.join(root, "models", "schema.yml"), "version: 2") + // Non-.sql alongside .sql, must NOT be counted. + fs.writeFileSync(path.join(dir, "models", "readme.md"), "hi") + fs.writeFileSync(path.join(dir, "models", "schema.yml"), "version: 2") - fs.writeFileSync(path.join(root, "seeds", "country_codes.csv"), "code,name\n") - fs.writeFileSync(path.join(root, "seeds", "regions.csv"), "id,name\n") - // Non-.csv seed — not counted. - fs.writeFileSync(path.join(root, "seeds", "notes.md"), "hi") - }) + fs.writeFileSync(path.join(dir, "seeds", "country_codes.csv"), "code,name\n") + fs.writeFileSync(path.join(dir, "seeds", "regions.csv"), "id,name\n") + // Non-.csv seed — not counted. + fs.writeFileSync(path.join(dir, "seeds", "notes.md"), "hi") +} - afterAll(() => { - fs.rmSync(root, { recursive: true, force: true }) - }) - - test("counts .sql files recursively under models/", () => { - const { models } = countSampleContents(root) - expect(models).toBe(4) +describe("countSampleContents", () => { + test("counts .sql files recursively under models/", async () => { + await using tmp = await tmpdir() + await seedSampleTree(tmp.path) + expect(countSampleContents(tmp.path).models).toBe(4) }) - test("counts .csv files under seeds/ (top level; matches production sample layout)", () => { - const { tables } = countSampleContents(root) - expect(tables).toBe(2) + test("counts .csv files under seeds/ (top level; matches production sample layout)", async () => { + await using tmp = await tmpdir() + await seedSampleTree(tmp.path) + expect(countSampleContents(tmp.path).tables).toBe(2) }) - test("returns zeros when the sample dir is missing the expected subdirs", () => { - const empty = fs.mkdtempSync(path.join(os.tmpdir(), "sample-setup-empty-")) - try { - expect(countSampleContents(empty)).toEqual({ models: 0, tables: 0 }) - } finally { - fs.rmSync(empty, { recursive: true, force: true }) - } + test("returns zeros when the sample dir is missing the expected subdirs", async () => { + await using tmp = await tmpdir() + expect(countSampleContents(tmp.path)).toEqual({ models: 0, tables: 0 }) }) - test("returns zeros when the sample dir does not exist at all", () => { + test("returns zeros when the sample dir does not exist at all", async () => { // countFilesWithExtension swallows the readdirSync error and returns 0 — // this is the graceful-degradation shape the telemetry event depends on. - expect(countSampleContents(path.join(root, "does-not-exist"))).toEqual({ models: 0, tables: 0 }) + await using tmp = await tmpdir() + expect(countSampleContents(path.join(tmp.path, "does-not-exist"))).toEqual({ models: 0, tables: 0 }) }) }) From 3bd7f6475c75b1f4729b606306cc3bf4a4f45b6d Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 10 Aug 2026 20:24:11 +0530 Subject: [PATCH 10/10] test(telemetry): cover `OPENCODE_DISABLE_TELEMETRY` fallback + gate-site source anchor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three PR #1086 reviewers (codex, coderabbit, cubic) named the same gap: the initial telemetry-opt-out test file only exercised the shared `Flag.truthyEnv` helper against `ALTIMATE_TELEMETRY_DISABLED`. A regression that dropped the `OPENCODE_DISABLE_TELEMETRY` fallback OR removed the `truthyEnv(A) || truthyEnv(B)` OR-composition at either call site (`altimate/plugin/altimate.ts::buildCliContext`, `altimate/telemetry/index.ts::doInit`) would still pass every existing test — the exact regression this suite exists to prevent. Three coverage layers now, one per describe block: 1. **Parser semantics** (existing, unchanged): `Flag.truthyEnv` accepts "true"/"TRUE"/"1", rejects the everything-else surface. 2. **Consumer-boundary composition**: reproduces the `truthyEnv(A) || truthyEnv(B)` shape via a local helper and exercises each branch — only-primary, only-fallback, both-set, neither-set. Proves the OPENCODE fallback works. 3. **Gate-site source anchor**: reads `altimate.ts` and `telemetry/index.ts`, strips line comments, and asserts both env-var names appear inside a two-line window (allowing prettier line-wraps of the OR expression). Fails loudly if a future edit deletes the fallback branch. Brittle by design — but the alternative (booting `doInit()` / `buildCliContext` in-process to observe the effect) requires Config, machine-id, and a sink, which is disproportionate for what this test proves. Env-mutation isolation: bot-review follow-up (coderabbit). Each test now snapshots both env vars in `beforeEach` and restores what it found in `afterEach`, so this suite is safe against other tests in the same process reading either variable. --- .../telemetry/telemetry-opt-out-flag.test.ts | 176 +++++++++++++++--- 1 file changed, 148 insertions(+), 28 deletions(-) diff --git a/packages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts b/packages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts index bd8131d243..d985834736 100644 --- a/packages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts +++ b/packages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts @@ -1,36 +1,63 @@ -// v0.9.5 review — Chaos gremlin P1 + End-user P2. +// v0.9.5 review — Chaos gremlin P1 + End-user P2, plus bot-review follow-ups +// (codex + coderabbit + cubic all named the same gap: the initial version of +// this file only tested the shared `Flag.truthyEnv` helper against +// `ALTIMATE_TELEMETRY_DISABLED`. That meant a regression that dropped the +// `OPENCODE_DISABLE_TELEMETRY` fallback OR removed the OR-composition at either +// call site (`altimate/plugin/altimate.ts::buildCliContext`, +// `altimate/telemetry/index.ts::doInit`) would still pass every test — the +// exact failure this file exists to prevent). // -// Before this release the two telemetry-disable gates -// (`altimate/plugin/altimate.ts::buildCliContext` and `altimate/telemetry/index.ts::doInit`) -// each checked `process.env.ALTIMATE_TELEMETRY_DISABLED === "true"`. A user -// running `ALTIMATE_TELEMETRY_DISABLED=1 altimate ...` (the shape most users -// reach for) was silently ignored — telemetry stayed on. v0.9.4's CHANGELOG -// also referenced `OPENCODE_DISABLE_TELEMETRY=1` as an opt-out env var, but -// that name was wired into test fixtures only and never checked in product. +// The file now covers three layers: +// 1. Parser semantics — `Flag.truthyEnv` accepts "true"/"TRUE"/"1", rejects +// the everything-else surface (case sensitivity, whitespace, non-1 digits). +// 2. Consumer-boundary composition — the exact `truthyEnv(A) || truthyEnv(B)` +// shape used at both gate sites returns true when EITHER env var is set. +// 3. Gate-site source anchor — both `altimate.ts` and `telemetry/index.ts` +// reference both env-var names on the same line. Brittle by design: if a +// future edit deletes the fallback branch, this test catches it without +// needing to boot the full telemetry init in a unit test. // -// Both call sites now route through `Flag.truthyEnv`, which accepts -// "true"/"TRUE"/"1" and honors the OPENCODE_DISABLE_TELEMETRY fallback. -// This file locks the contract in place so a future edit to `truthy()` (or -// to either call site) can't silently narrow it again. +// Env-mutation isolation: each test snapshots the two vars before mutating +// them and restores what it found. This keeps the suite safe against other +// tests in the same process reading `process.env.ALTIMATE_TELEMETRY_DISABLED` +// or `OPENCODE_DISABLE_TELEMETRY` and against parallel suites in other +// packages (bot-review follow-up, coderabbit). -import { afterEach, describe, expect, test } from "bun:test" +import { afterEach, beforeEach, describe, expect, test } from "bun:test" +import fs from "fs" +import path from "path" import { Flag } from "../../src/flag/flag" -const VAR = "ALTIMATE_TELEMETRY_DISABLED" +const ALTIMATE_VAR = "ALTIMATE_TELEMETRY_DISABLED" +const OPENCODE_VAR = "OPENCODE_DISABLE_TELEMETRY" + +// Snapshot both env vars and restore them after each test — protects against +// leakage into or out of this suite regardless of what set them. +let snapshot: { altimate: string | undefined; opencode: string | undefined } + +beforeEach(() => { + snapshot = { + altimate: process.env[ALTIMATE_VAR], + opencode: process.env[OPENCODE_VAR], + } +}) afterEach(() => { - delete process.env[VAR] + if (snapshot.altimate === undefined) delete process.env[ALTIMATE_VAR] + else process.env[ALTIMATE_VAR] = snapshot.altimate + if (snapshot.opencode === undefined) delete process.env[OPENCODE_VAR] + else process.env[OPENCODE_VAR] = snapshot.opencode }) -describe("Flag.truthyEnv — telemetry opt-out shape", () => { +describe("Flag.truthyEnv — parser semantics on ALTIMATE_TELEMETRY_DISABLED", () => { test("unset env var → false (default: telemetry enabled)", () => { - delete process.env[VAR] - expect(Flag.truthyEnv(VAR)).toBe(false) + delete process.env[ALTIMATE_VAR] + expect(Flag.truthyEnv(ALTIMATE_VAR)).toBe(false) }) test("empty string → false", () => { - process.env[VAR] = "" - expect(Flag.truthyEnv(VAR)).toBe(false) + process.env[ALTIMATE_VAR] = "" + expect(Flag.truthyEnv(ALTIMATE_VAR)).toBe(false) }) test.each([ @@ -46,16 +73,109 @@ describe("Flag.truthyEnv — telemetry opt-out shape", () => { [" true ", false], // no trimming — matches existing truthy() semantics ["2", false], ])("value %j → %s", (value, expected) => { - process.env[VAR] = value - expect(Flag.truthyEnv(VAR)).toBe(expected) + process.env[ALTIMATE_VAR] = value + expect(Flag.truthyEnv(ALTIMATE_VAR)).toBe(expected) }) test("re-reads process.env on each call (not frozen at import)", () => { - delete process.env[VAR] - expect(Flag.truthyEnv(VAR)).toBe(false) - process.env[VAR] = "1" - expect(Flag.truthyEnv(VAR)).toBe(true) - process.env[VAR] = "false" - expect(Flag.truthyEnv(VAR)).toBe(false) + delete process.env[ALTIMATE_VAR] + expect(Flag.truthyEnv(ALTIMATE_VAR)).toBe(false) + process.env[ALTIMATE_VAR] = "1" + expect(Flag.truthyEnv(ALTIMATE_VAR)).toBe(true) + process.env[ALTIMATE_VAR] = "false" + expect(Flag.truthyEnv(ALTIMATE_VAR)).toBe(false) + }) +}) + +describe("Consumer-boundary composition — both env vars route through the same OR gate", () => { + // The two production callers use `truthyEnv(A) || truthyEnv(B)`. These tests + // exercise both branches of that OR expression so a regression that hard-wires + // one branch to false would fail here (codex/coderabbit/cubic finding). + // + // We reproduce the composition inline via a local helper so the tests read + // as one unit rather than four permutations. The "gate-site anchor" tests + // below then prove the production callers actually contain this shape. + const gate = () => + Flag.truthyEnv(ALTIMATE_VAR) || Flag.truthyEnv(OPENCODE_VAR) + + test("neither set → gate is closed (telemetry enabled)", () => { + delete process.env[ALTIMATE_VAR] + delete process.env[OPENCODE_VAR] + expect(gate()).toBe(false) + }) + + test("only ALTIMATE_TELEMETRY_DISABLED set → gate is open (primary branch)", () => { + delete process.env[OPENCODE_VAR] + process.env[ALTIMATE_VAR] = "1" + expect(gate()).toBe(true) }) + + test("only OPENCODE_DISABLE_TELEMETRY set → gate is open (fallback branch)", () => { + // v0.9.4 CHANGELOG advertised this name; before v0.9.5 it was silently + // ignored in product. Removing the second `truthyEnv(...)` from either + // call site would silently regress this — the source-anchor test below + // catches that additional shape. + delete process.env[ALTIMATE_VAR] + process.env[OPENCODE_VAR] = "1" + expect(gate()).toBe(true) + }) + + test.each([ + ["true", "true"], + ["1", "1"], + ["TRUE", "true"], + ])( + "both set (%j / %j) → gate is open", + (altimateVal, opencodeVal) => { + process.env[ALTIMATE_VAR] = altimateVal + process.env[OPENCODE_VAR] = opencodeVal + expect(gate()).toBe(true) + }, + ) + + test("both set with non-truthy values → gate is closed", () => { + process.env[ALTIMATE_VAR] = "false" + process.env[OPENCODE_VAR] = "0" + expect(gate()).toBe(false) + }) +}) + +describe("Gate-site anchor — both call sites reference both env-var names", () => { + // codex/coderabbit/cubic all pointed out that testing the shared helper in + // isolation doesn't prove the CALLER retains the fallback branch. Without an + // integration-level runner for `doInit()` / `buildCliContext` (both touch + // Config, machine-id, and a network sink), the closest deterministic proof is + // a source-shape assertion: both call sites must mention both env-var names + // within a small window. A future edit that deletes the fallback fails here + // and gives a clear name for what regressed. + // + // The window (a single non-comment line, or up to two adjacent non-comment + // lines) is deliberately narrow — enough for the OR expression to wrap to a + // second line for prettier, but not enough for the two names to sit in + // unrelated pieces of the file. + const GATE_FILES = [ + "src/altimate/plugin/altimate.ts", + "src/altimate/telemetry/index.ts", + ] + + test.each(GATE_FILES)( + "%s references both %s and %s in the same gate", + (relativePath) => { + const absolute = path.resolve(__dirname, "../..", relativePath) + const source = fs.readFileSync(absolute, "utf8") + // Strip line comments so a `// mentions X and Y` comment doesn't count. + const codeLines = source + .split("\n") + .map((l) => l.replace(/\/\/.*$/, "").trim()) + const window = 2 + let matched = false + for (let i = 0; i < codeLines.length && !matched; i++) { + const slice = codeLines.slice(i, i + window).join(" ") + if (slice.includes(ALTIMATE_VAR) && slice.includes(OPENCODE_VAR)) { + matched = true + } + } + expect(matched).toBe(true) + }, + ) })