chore(release): v0.9.5 pre-release review fixes - #1086
Merged
Conversation
added 6 commits
August 10, 2026 19:06
…ETRY_DISABLED` + honor `OPENCODE_DISABLE_TELEMETRY` fallback 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.
…ions + keybinds 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.
…tion defense
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.
…ring claims 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 `<path>` 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.
… isolation 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.
…t `FIXME` anchor 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.
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
full receipts (4 sessions)
orchestrator ·
|
| subagent | cost |
|---|---|
| You are the Tech Lead reviewer for altimate-code v0.9.5.Repo: `/Users/haide… | ≥ $1.8691 |
| You are the Chaos Gremlin reviewer for altimate-code v0.9.5. Persona this r… | ≥ $1.4699 |
| You are the Product Manager reviewer for altimate-code v0.9.5 (patch releas… | ≥ $0.8184 |
| You are the End-User reviewer for altimate-code v0.9.5. Persona: a working… | ≥ $0.7898 |
| You are the CTO reviewer for altimate-code v0.9.5 (a patch release cut from… | ≥ $0.6577 |
Get an independent review from codex on PR #1086 (release/v0.9.5 → main) in `… |
≥ $0.2307 |
codex · 59b04ccf
- - - - - - - - - - - - - - - - - - - - - - - - -
AIRECEIPTS
“Read PLAN-review-quality.md in full. Also rea…”
Codex · Aug 10 2026 13:45:07 UTC · 3m 07s
gpt-5.6-sol 100%
cache served 85% of input tokens
pre-edit: no named edit tool observed
(share before the first named edit tool)
exec.........................≥ $0.9913 (12 calls)
caveat: Codex trace omits GPT-5.6 cache-write tokens — floor excludes any write premium
--------------------------------------------------
KNOWN PRICED SUBTOTAL....................≥ $0.9913
standard API-equivalent floor; not an invoice
partial pricing coverage; invoice total unknown
same tokens on gpt-5.4-mini..............≥ $0.1487
(85% lower observable floor)
(arithmetic, not a prediction)
- - - - - - - - - - - - - - - - - - - - - - - - -
npx aireceipts-cli
github.com/anandgupta42/receipts
- - - - - - - - - - - - - - - - - - - - - - - - -
codex · 165cdbf2
- - - - - - - - - - - - - - - - - - - - - - - - -
AIRECEIPTS
“The current branch (`release/v0.9.5`) adds se…”
Codex · Aug 10 2026 13:50:14 UTC · 17s
gpt-5.6-sol 100%
cache served 78% of input tokens
pre-edit: no named edit tool observed
(share before the first named edit tool)
exec..........................≥ $0.0622 (2 calls)
caveat: Codex trace omits GPT-5.6 cache-write tokens — floor excludes any write premium
--------------------------------------------------
KNOWN PRICED SUBTOTAL....................≥ $0.0622
standard API-equivalent floor; not an invoice
partial pricing coverage; invoice total unknown
same tokens on gpt-5.4-mini..............≥ $0.0093
(85% lower observable floor)
(arithmetic, not a prediction)
- - - - - - - - - - - - - - - - - - - - - - - - -
npx aireceipts-cli
github.com/anandgupta42/receipts
- - - - - - - - - - - - - - - - - - - - - - - - -
codex · 614544ad
- - - - - - - - - - - - - - - - - - - - - - - - -
AIRECEIPTS
“The current branch (`release/v0.9.5`) adds se…”
Codex · Aug 10 2026 13:50:57 UTC · 3m 15s
gpt-5.6-sol 100%
cache served 87% of input tokens
pre-edit: no named edit tool observed
(share before the first named edit tool)
exec.........................≥ $1.0896 (13 calls)
caveat: Codex trace omits GPT-5.6 cache-write tokens — floor excludes any write premium
--------------------------------------------------
KNOWN PRICED SUBTOTAL....................≥ $1.0896
standard API-equivalent floor; not an invoice
partial pricing coverage; invoice total unknown
same tokens on gpt-5.4-mini..............≥ $0.1634
(85% lower observable floor)
(arithmetic, not a prediction)
- - - - - - - - - - - - - - - - - - - - - - - - -
npx aireceipts-cli
github.com/anandgupta42/receipts
- - - - - - - - - - - - - - - - - - - - - - - - -
Generated by aireceipts
📝 WalkthroughWalkthroughThe PR documents the session YOLO toggle, adds shared telemetry opt-out parsing for two environment variables, exports sample setup helpers, and adds tests for telemetry and sample setup behavior. ChangesSession YOLO controls
Telemetry opt-out handling
Sample setup helper testing
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sahrizvi
marked this pull request as ready for review
August 10, 2026 13:58
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the opt-out at both consumer boundaries.
This suite calls
Flag.truthyEnvdirectly with onlyALTIMATE_TELEMETRY_DISABLED. It does not testOPENCODE_DISABLE_TELEMETRY,buildCliContext, orTelemetry.init(). Therefore, it cannot enforce the contract claimed in Lines 11-14. Add fallback cases in the existing consumer tests.Also applies to: 19-20, 36-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts` around lines 11 - 14, Extend the existing telemetry opt-out tests to cover both consumer boundaries, `buildCliContext` and `Telemetry.init()`, rather than only calling `Flag.truthyEnv` directly. Add cases proving `OPENCODE_DISABLE_TELEMETRY` acts as the fallback when `ALTIMATE_TELEMETRY_DISABLED` is absent, while preserving the existing truthy-value coverage and expected opt-out behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/docs/configure/permissions.md`:
- Around line 135-136: Update the mid-session toggle restart-state description
to include OPENCODE_YOLO alongside --yolo and ALTIMATE_CLI_YOLO as supported
launch-time sources for the default YOLO value.
In `@packages/opencode/src/altimate/telemetry/index.ts`:
- Around line 1686-1689: Update doInit() to resolve configuration before
initializing telemetry, honoring telemetry.disabled even when Instance.provide()
is not yet available. If Config.get() cannot be resolved during early startup,
fail closed by deferring initialization without minting machineId or sending
events, then retry telemetry initialization after instance setup; preserve the
existing environment-variable opt-outs.
In `@packages/opencode/test/altimate/sample-setup-helpers.test.ts`:
- Around line 119-145: Replace the shared root and beforeAll/afterAll filesystem
setup with a per-test await using tmp = await tmpdir() fixture in each test.
Import tmpdir from fixture/fixture.ts, create the required models and seeds tree
under the fixture directory, and use that directory as root so each test has
isolated, scoped cleanup.
In `@packages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts`:
- Around line 21-23: Update the telemetry test cleanup around VAR to capture its
original environment value before each test and restore that value afterward,
deleting VAR only when it was initially unset. Ensure environment mutations in
this suite are isolated from concurrent environment-sensitive tests by using the
test runner’s supported serialization or isolation mechanism.
---
Nitpick comments:
In `@packages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts`:
- Around line 11-14: Extend the existing telemetry opt-out tests to cover both
consumer boundaries, `buildCliContext` and `Telemetry.init()`, rather than only
calling `Flag.truthyEnv` directly. Add cases proving
`OPENCODE_DISABLE_TELEMETRY` acts as the fallback when
`ALTIMATE_TELEMETRY_DISABLED` is absent, while preserving the existing
truthy-value coverage and expected opt-out behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d467f083-77dd-40ca-9e0a-0e08f0b0c1ff
📒 Files selected for processing (11)
docs/docs/configure/keybinds.mddocs/docs/configure/permissions.mdpackages/opencode/src/altimate/plugin/altimate.tspackages/opencode/src/altimate/telemetry/index.tspackages/opencode/src/altimate/tools/sample-setup.tspackages/opencode/src/cli/welcome.tspackages/opencode/src/flag/flag.tspackages/opencode/test/altimate/sample-setup-helpers.test.tspackages/opencode/test/telemetry/classify-provider.test.tspackages/opencode/test/telemetry/environment-scan-claim.test.tspackages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts
Comment on lines
+1686
to
+1689
| // 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")) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
fd -a 'index.ts|telemetry.ts|instance-state.ts|welcome.ts' packages/opencode/src 2>/dev/null | sed 's#^\./##' | head -100
echo
echo "Telemetry outline:"
ast-grep outline packages/opencode/src/altimate/telemetry/index.ts 2>/dev/null | sed -n '1,220p' || true
echo
echo "Relevant telemetry lines:"
nl -ba packages/opencode/src/altimate/telemetry/index.ts | sed -n '1640,1780p'
echo
echo "Welcome references:"
nl -ba packages/opencode/src/cli/welcome.ts | sed -n '1,180p' 2>/dev/null | rg -n "FIXME|telemetry|Instance|opencode" -C 3 || trueRepository: AltimateAI/altimate-code
Length of output: 3149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search telemetry.disabled and opt-out usages:"
rg -n "telemetry\.disabled|DISABLE_TELEMETRY|ALTIMATE_TELEMETRY_DISABLED|open(d|c)ode_disable_telemetry|Instance\.provide|doInit|ensureTelemetryEnabled" packages/opencode/src packages/opencode -g '*.ts' -g '*.tsx' | sed -n '1,240p'Repository: AltimateAI/altimate-code
Length of output: 27109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("packages/opencode/src/altimate/telemetry/index.ts")
text = p.read_text()
for needle in [
"async function doInit()",
"Flag.truthyEnv(\"ALTIMATE_TELEMETRY_DISABLED\")",
"Flag.truthyEnv(\"OPENCODE_DISABLE_TELEMETRY\")",
"Config.get()",
"process.env.ALTIMATE_TELEMETRY_FORCE",
"process.env.ALTIMATE_TELEMETRY_DISABLED",
"ENABLED_TELEMETRY",
"machineId",
"readMachineId",
"writeMachineId",
"doInit()"
]:
print(f"\n== {needle} ==")
idx = text.find(needle)
if idx == -1:
print("NOT FOUND")
continue
line = text[:idx].count("\n") + 1
print(f"first at line {line}")
start = max(0, idx - 800)
end = min(len(text), idx + 1000)
print(text[start:end])
PY
echo
echo "Welcome lines:"
python3 - <<'PY'
from pathlib import Path
for path in ["packages/opencode/src/cli/welcome.ts", "packages/opencode/src/altimate/telemetry/index.ts", "packages/opencode/src/altimate/plugin/altimate.ts"]:
p=Path(path)
print(f"\n== {path} ==")
txt=p.read_text()
for i,line in enumerate(txt.splitlines(),1):
if "FIXME" in line or "telemetry.disabled" in line or "doInit" in line or "Instance.provide" in line or "readMachineId" in line or "writeMachineId" in line or "ENABLED_TELEMETRY" in line:
print(f"{i}: {line}")
PYRepository: AltimateAI/altimate-code
Length of output: 14958
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in ["packages/opencode/src/altimate/telemetry/index.ts", "packages/opencode/src/cli/welcome.ts", "packages/opencode/src/altimate/plugin/altimate.ts", "packages/opencode/src/config/config.ts":
p=Path(path)
print(f"\n== {path} ==")
for i,line in enumerate(p.read_text().splitlines(),1):
if "doInit" in line or "enable" in line.lower() or "disabled" in line or "Config.get" in line or "writeMachine" in line or "readMachine" in line or "FIXME" in line or "telemetry.disabled" in line:
print(f"{i}: {line}")
PY
echo
echo "Read relevant telemetry sections by computed ranges:"
python3-script<<'PY'
from pathlib import Path
p=Path("packages/opencode/src/altimate/telemetry/index.ts")
lines=p.read_text().splitlines()
for start,end in [(1700,1880),(1880,1990)]:
print(f"\n-- {start}-{end} --")
for i in range(start,end+1):
if i<=len(lines):
print(f"{i}: {lines[i-1]}")
PYRepository: AltimateAI/altimate-code
Length of output: 625
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
paths = [
"packages/opencode/src/altimate/telemetry/index.ts",
"packages/opencode/src/cli/welcome.ts",
"packages/opencode/src/altimate/plugin/altimate.ts",
]
for path in paths:
p = Path(path)
print(f"\n== {path} keyword lines ==")
for i, line in enumerate(p.read_text().splitlines(), 1):
if any(k in line for k in ["doInit", "ENABLE_TELEMETRY", "enabled", "disabled", "writeMachineId", "readMachineId", "FIXME", "telemetry.disabled", "Config.get"]):
print(f"{i}: {line}")
PY
echo
python3 - <<'PY'
from pathlib import Path
p = Path("packages/opencode/src/altimate/telemetry/index.ts")
lines = p.read_text().splitlines()
for start,end in [(1700,1880),(1880,1990)]:
print(f"\n-- telemetry {start}-{end} --")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i}: {lines[i-1]}")
PY
echo
echo "Config key definitions/usages:"
rg -n "telemetry|TELEMETRY|disabled|writeMachineId|readMachineId|ENABLE_TELEMETRY" packages/opencode/src packages/opencode/test -g '*.ts' | sed -n '1,260p'Repository: AltimateAI/altimate-code
Length of output: 48007
Protect config-based telemetry opt-outs during early initialization.
doInit() catches unavailable Config.get() and proceeds as enabled, so telemetry.disabled is ignored on startup paths before Instance.provide(). packages/opencode/src/cli/welcome.ts documents this, but it still mints machineId and sends events. Delay initialization until configuration is resolvable and fail closed, then retry after instance setup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/src/altimate/telemetry/index.ts` around lines 1686 - 1689,
Update doInit() to resolve configuration before initializing telemetry, honoring
telemetry.disabled even when Instance.provide() is not yet available. If
Config.get() cannot be resolved during early startup, fail closed by deferring
initialization without minting machineId or sending events, then retry telemetry
initialization after instance setup; preserve the existing environment-variable
opt-outs.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/flag/flag.ts">
<violation number="1" location="packages/opencode/src/flag/flag.ts:27">
P3: The new `Flag.truthyEnv` is presented as the shared env-truthy convention, but an identical private `truthyEnv` (same `=== "true" || === "1"` lowercase logic) and the same dual-env-var fallback pattern already exist in `src/cli/upgrade.ts` (`isAutoupdateDisabledByEnv`). That's a second copy of the same convention that won't stay in sync with this one if the accepted values ever change. Consider having `upgrade.ts` route through `Flag.truthyEnv` (or the inverse) so the telemetry and autoupdate opt-outs share one implementation rather than two independent ones.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // 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 { |
There was a problem hiding this comment.
P3: The new Flag.truthyEnv is presented as the shared env-truthy convention, but an identical private truthyEnv (same === "true" || === "1" lowercase logic) and the same dual-env-var fallback pattern already exist in src/cli/upgrade.ts (isAutoupdateDisabledByEnv). That's a second copy of the same convention that won't stay in sync with this one if the accepted values ever change. Consider having upgrade.ts route through Flag.truthyEnv (or the inverse) so the telemetry and autoupdate opt-outs share one implementation rather than two independent ones.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/flag/flag.ts, line 27:
<comment>The new `Flag.truthyEnv` is presented as the shared env-truthy convention, but an identical private `truthyEnv` (same `=== "true" || === "1"` lowercase logic) and the same dual-env-var fallback pattern already exist in `src/cli/upgrade.ts` (`isAutoupdateDisabledByEnv`). That's a second copy of the same convention that won't stay in sync with this one if the accepted values ever change. Consider having `upgrade.ts` route through `Flag.truthyEnv` (or the inverse) so the telemetry and autoupdate opt-outs share one implementation rather than two independent ones.</comment>
<file context>
@@ -19,6 +19,15 @@ function altEnv(altKey: string, openKey: string) {
+ // 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)
+ }
</file context>
added 4 commits
August 10, 2026 20:23
… restart-state sentence 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.
…` 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.
…itespace-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 `<path>` 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.
…ite source anchor 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.
sahrizvi
changed the title
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
Contributor
Author
Bot-review round 2 — addressed / deferredThanks @coderabbitai, @cubic-dev-ai, and codex (invoked out-of-band). Findings triaged: Fixed (commits 2407ede → 3bd7f64)
For the last one — since the OR-composition is inlined at both call sites rather than extracted to a helper, the test suite now covers it two ways: (1) reproduces the Deferred — tracked in #1087
Not applied
|
anandgupta42
self-requested a review
August 10, 2026 14:56
@sahrizvi That triage reads right to me. On my two items:
On the source-anchor test for the OR-composition: I get why you didn't boot One thing I'd flag for the deferred #1087 work: the Nothing further from me. Ship it. Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
anandgupta42
approved these changes
Aug 10, 2026
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Changed-file status =='
git status --short
printf '%s\n' '== Relevant commit subjects =='
git log --oneline -12
printf '%s\n' '== Test and documentation structure =='
ast-grep outline packages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts --items all
ast-grep outline packages/opencode/test/altimate/sample-setup-helpers.test.ts --items all
ast-grep outline docs/docs/configure/permissions.md --items all || true
printf '%s\n' '== Telemetry gate references =='
rg -n -C 4 'ALTIMATE_TELEMETRY_DISABLED|OPENCODE_DISABLE_TELEMETRY|truthyEnv' \
packages/opencode/src/altimate/plugin/altimate.ts \
packages/opencode/src/altimate/telemetry/index.ts \
packages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts
printf '%s\n' '== Fixture and whitespace-path coverage =='
rg -n -C 3 'tmpdir|whitespace|space|redactPaths' \
packages/opencode/test/altimate/sample-setup-helpers.test.ts
printf '%s\n' '== YOLO restart documentation =='
rg -n -C 3 'OPENCODE_YOLO|Ctrl\+Y|restart' docs/docs/configure/permissions.md
printf '%s\n' '== Deferred telemetry anchor =='
rg -n -C 5 'telemetry-init-config-opt-out|Config\.get|FIXME' \
packages/opencode/src/cli/welcome.ts \
packages/opencode/src/altimate/telemetry/index.ts
printf '%s\n' '== GitHub issue `#1087` =='
gh issue view 1087 --repo AltimateAI/altimate-code --json number,title,state,body,urlLength of output: 33004
The reported fixes are present. This includes the Issue I did not run the test suite in this inspection environment. ᕱᕱ ♡ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/test/altimate/sample-setup-helpers.test.ts`:
- Around line 97-119: Update redactPaths so filesystem paths containing
whitespace are redacted as a complete path during the pattern-only pass, without
requiring the path in the extra list. Then change the raw assertion in the test
to expect exactly "failed at <path>" and remove expectations documenting the
partial-redaction limitation, while preserving the guarded known-path coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b74419a3-7b1e-4154-a53f-313ae4b0032a
📒 Files selected for processing (4)
docs/docs/configure/permissions.mdpackages/opencode/test/altimate/sample-setup-helpers.test.tspackages/opencode/test/telemetry/classify-provider.test.tspackages/opencode/test/telemetry/telemetry-opt-out-flag.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/opencode/test/telemetry/classify-provider.test.ts
- docs/docs/configure/permissions.md
Comment on lines
+97
to
+119
| 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 `<path>` markers. | ||
| expect(raw).toContain("Documents") | ||
| expect(raw).not.toBe("failed at <path>") | ||
| // 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 <path>") | ||
| expect(guarded).not.toContain("Documents") | ||
| expect(guarded).not.toContain("alice") | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not accept partial path redaction.
redactPaths leaves the segment after whitespace visible. A directory name can contain project or user data. Passing [cwd] only protects callers that provide the exact path.
Update redactPaths to redact the complete path, then expect raw to equal "failed at <path>".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/test/altimate/sample-setup-helpers.test.ts` around lines 97
- 119, Update redactPaths so filesystem paths containing whitespace are redacted
as a complete path during the pattern-only pass, without requiring the path in
the extra list. Then change the raw assertion in the test to expect exactly
"failed at <path>" and remove expectations documenting the partial-redaction
limitation, while preserving the guarded known-path coverage.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ] | ||
|
|
||
| test.each(GATE_FILES)( | ||
| "%s references both %s and %s in the same gate", |
There was a problem hiding this comment.
SUGGESTION: test.each title has three %s placeholders but only one row argument
GATE_FILES is a flat string[], so each row passes only relativePath to the callback. The first %s resolves to the file path, but the two trailing %s have no corresponding argument and render literally in the test name (e.g. src/altimate/plugin/altimate.ts references both %s and %s in the same gate), so the env-var names the title advertises never appear. Interpolating the constants directly restores the intent.
Suggested change
| "%s references both %s and %s in the same gate", | |
| `%s references both ${ALTIMATE_VAR} and ${OPENCODE_VAR} in the same gate`, |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (11 files)
Fix these issues in Kilo Cloud Reviewed by glm-5.2 · Input: 115.2K · Output: 23.5K · Cached: 1.7M Review guidance: REVIEW.md from base branch |
3 tasks
sahrizvi
added a commit
that referenced
this pull request
Aug 10, 2026
* docs(changelog): v0.9.5 entry The v0.9.5 release PR (#1086) merged without a CHANGELOG entry because I mistakenly split it out. `CHANGELOG.md` is baked into the compiled binary at build time (`script/build.ts` embeds it as `OPENCODE_CHANGELOG`) and copied into every platform-specific npm package (`script/publish.ts`), so shipping v0.9.5 without this entry would give users an artifact whose embedded changelog stops at v0.9.4. Entry incorporates PM's release-notes feedback from the pre-release persona review: leads with the Windows grep fix (~16% of Windows users broken since v0.9.2, hard number from telemetry), collapses the welcome-panel two-round fix into a single bullet, includes the YOLO deny-rules-still-apply safety caveat, and files the three telemetry / instrumentation commits under Changed (observability) rather than Added (user-visible feature) so the release isn't overstated. * test(skill): adversarial coverage for v0.9.5 surfaces Per the release skill's Step 6, every release ships with an adversarial test file under `test/skill/release-v{X}-adversarial.test.ts` — one per release since v0.5.14. The v0.9.5 file was written but not committed; adding it here so the release PR carries the intended coverage. Categories per the skill (empty/null/undefined, boundary values, type confusion, injection attacks, error propagation), applied to the v0.9.5 code surface: - **Flag.truthyEnv:** 32KB env values, embedded null bytes, whitespace, unknown keys — all must not enable and must not throw. - **Telemetry.classifyProvider:** 10KB providerIDs, control characters, JSON-stringified inputs, prototype keys (`__proto__` / `prototype` / `hasOwnProperty` / `isPrototypeOf`), unusual modelID values — all must fall through to "other" without leaking a raw id or extending Object. - **OnboardingTelemetry.claimEnvironmentScan:** empty / 10KB / path-shaped / SQL-shaped sessionIDs — idempotent and session-scoped; the composed gate short-circuits correctly when the session was never marked. - **redactPaths / countSampleContents:** empty input, 100KB input in <1s, regex metacharacters in `extras`, user text already containing `<path>`, fs symlink loops on `countSampleContents`. - **buildCliContext / buildAuthorizeUrl:** base64url shape, `v=1` payload, redirect / state encoding on hostile inputs (newlines, tabs, fragment chars), fragment-goes-last invariant, graceful degradation when the machine-id path is unreadable (payload valid, `machine_id` omitted). 22 tests, all pass locally. No `mock.module()`; real helpers, real fs where needed, real `process.env` mutation with `beforeEach` / `afterEach` snapshot-and-restore. * test(skill): escape null bytes as `\x00` so git treats the file as text Previous commit landed the adversarial file with three raw null bytes embedded in string literals meant to test null-byte handling. Git classified the file as binary because of them, showing `Bin 0 -> 13936 bytes` and 0 insertions/deletions on the diff — reviewers on the PR couldn't see the file contents. Runtime behavior of the tests is unchanged; the JS string `"true\x00"` compiles to the same "true"+NUL sequence as a literal 0x00 byte in the source, without tripping git's binary detector. 22/22 tests still pass after the escape. * fix(pr-review): address coderabbit + cubic findings on PR #1088 Three inline findings, all valid, all small: - **CHANGELOG.md:25** (coderabbit + cubic, both P3) — repo-relative telemetry doc path was `docs/reference/telemetry.md`; the file actually lives at `docs/docs/reference/telemetry.md` (content root is `docs/` per `docs/mkdocs.yml`, published at `reference/telemetry/` on the docs site). Fixed the path so a maintainer following the link in the changelog finds the file. - **adversarial.test.ts:180** (coderabbit) — the 100KB stress test used `repeat(1000)` which is ~20KB, not 100KB as advertised, and asserted a wall-clock `performance.now()` budget which is flaky under host load / parallel test runners. Built exactly 100_000 characters via `Math.ceil(100_000 / segment.length)` slicing, and dropped the timing assertion — the test now proves only what it claims (redaction shape survives large input). - **adversarial.test.ts:225** (coderabbit) — the symlink-loop test had a bare `catch { return }` that would swallow every setup failure into a passing test, and `toBeGreaterThanOrEqual(0)` accepted incorrect positive counts. Narrowed the catch to the one known unsupported- symlink error class (`EPERM` on Windows without dev-mode), re-throw everything else, and tightened the assertion to exact `.toBe(0)` — a nonzero count would mean the helper is following the loop. 22/22 adversarial tests still pass after the changes. * test(skill): scope `ALTIMATE_TELEMETRY_DISABLED` snapshot hooks to their describe block Kilo review on PR #1088: `beforeEach` / `afterEach` for the `ALTIMATE_TELEMETRY_DISABLED` env-var snapshot were declared at file scope, so they ran for all 22 tests even though only the four tests in `Flag.truthyEnv adversarial` ever set that variable. Moved the `const`, the `let`, and both hooks inside that describe block so scope matches usage. Behavior unchanged (still 22/22 pass); noise removed from unrelated tests' setup/teardown timeline. --------- Co-authored-by: Haider <haider@altimate.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue for this PR
Deferred items tracker: #1087 (not "closes" — kept separately as a stub for ongoing work).
Type of change
(Bundle of test/docs improvements + one small env-var fix — no user-facing feature change.)
What does this PR do?
Fixes surfaced by the v0.9.5 pre-release multi-persona review (CTO, PM, End-user, Tech Lead, Chaos Gremlin) and then by a second review pass from codex + coderabbit + cubic on the PR itself. All persona verdicts came back SHIP or SHIP WITH NOTES; zero P0s. This PR lands the actionable findings before we tag v0.9.5.
Round 1 — persona review (commits 1-6):
233bb0e91/case-insensitivetrueonALTIMATE_TELEMETRY_DISABLED+ honorOPENCODE_DISABLE_TELEMETRYfallback (v0.9.4 CHANGELOG promised this env var but it was only wired in test fixtures)078876beCtrl+YYOLO mid-session toggle inpermissions.md+keybinds.md28fb4766classifyProviderallowlist + prototype-pollution defense againstconstructor/toString/valueOf66fb1658redactPaths+countSampleContentsdocstring claims (José, O'Connor, sentence-swallow)4759a751claimEnvironmentScanidempotency + session isolation388f41bcFIXME(telemetry-init-config-opt-out)anchor + honest description of the pre-existing gapRound 2 — bot review follow-ups (commits 7-10):
2407ededaOPENCODE_YOLOin the YOLO mid-session toggle restart-state sentence37430a5cnot.toContainbeforetoBe("other")in the prototype-defense testa5c1ca7etmpdir()fixture per repo convention + assert whitespace-in-path documented gap3bd7f647OPENCODE_DISABLE_TELEMETRYfallback + gate-site source anchor (proves both call sites retain the OR-composition, not just the shared helper)How did you verify your code works?
bun turbo typecheck)test/telemetry/,test/altimate/sample-setup-helpers.test.ts,test/altimate/altimate-plugin.test.ts): 258/258 greenorigin/main: clean — no upstream-shared files touchedPR reviewsection below): nomust-fixfindings; class-level gaps codex named were addressed in Round 2Screenshots / recordings
Not applicable — no UI change.
Checklist
Deferred items
Deferred (design/process/monitoring work) is tracked in #1087:
windows-ripgrep-e2e+ oneERR_STREAM_PREMATURE_CLOSEflake on merge commit092567a8d2)welcome.ts(pre-existing; would require movingdoInit()behindInstance.provide()— not a v0.9.5 hotfix)Coderabbit's Major finding on
telemetry/index.ts::doInitis the same gap — see #1087.Not action-needed:
@zip.js/zip.jssupply-chain concern is already exact-pinned at2.7.62in bothpackages/core/package.jsonandpackages/opencode/package.json.🤖 Generated with Claude Code
https://claude.ai/code/session_01Q8FGy89Qpr39k8nCSpCcK2
Summary by CodeRabbit
New Features
Ctrl+Yto toggle YOLO mode during a session.Privacy
trueor1values.Documentation