Uh oh!
There was an error while loading. Please reload this page.
release: v0.9.4 review-fix bundle (A1-A12 + phase-label flake removal) - #1053
Merged
Conversation
…lock CI `spyOn(os, "homedir").mockImplementation(...)` in the mcp lifecycle suite's `beforeEach` was never torn down, so the mock leaked forward into the next test file loaded by bun. `packages/opencode/test/permission/next.test.ts` has six tilde-expansion assertions that resolve `~` via `os.homedir()`; those saw the still-active mock (which returns a temp dir with no resemblance to the real HOME) and failed with paths that pointed nowhere the assertions expected. Root cause of the red CI on PR #1001. Capture the spy in a mutable ref during `beforeEach` and restore it in a matching `afterEach`. Verified with the two suites together: 111 pass / 0 fail across mcp/lifecycle + permission/next. Flagged by the Tech Lead review; #1052 (D-list) does not need this.
… HOME is unsafe `materializeSample` refuses to write inside a root-owned HOME (the "sudo altimate-code" trap on Linux) with a clear message — but the recovery hint referenced a `--target-parent` flag that never existed. Users following the message got an "unknown flag" error and no path forward. Replace both occurrences with the working recipe: point HOME at the user's own home while running the CLI (`sudo HOME=/Users/you altimate-code` on macOS, `sudo HOME=/home/you altimate-code` on Linux). Same actionable intent, but the command in the message actually runs. Flagged by the Data Engineer persona; error text only, no behavioral change.
…w, echo authorize URL Three UX fixes to the Altimate LLM Gateway loopback OAuth handshake, all surfaced by the CTO + Support Engineer reviews: **Port collision (a3).** `startCallbackServer()` bound `127.0.0.1:7317` unconditionally. Second `altimate-code` process on the same box, an already-listening dev tool, or a lingering socket from a killed run made every OAuth attempt fail with EADDRINUSE and no fallback. Walk the 7317-7325 range on collision, remember which port bound, and use it to build the redirect URI so the callback matches what the gateway sees. **Auth window (a4-1).** `registerPending` timed out after 5 minutes. Corporate SSO + MFA + password-manager unlock routinely blows past that on first-run — the user comes back to an already-cancelled flow. Bump to 15 minutes. The pending map still evicts on completion or explicit cancel, so this is a ceiling, not a leak. **Headless fallback (a4-2).** `open()` silently no-ops on SSH sessions, tmux without a browser, WSL without wslview, and bare Docker containers. Users saw the CLI stall on "Waiting for browser…" with no clue what to do. Print the authorize URL to stderr immediately after `open()` so anyone in a headless context can paste it into a browser on their desktop and complete the handshake. Docs at docs/docs/getting-started/quickstart-new.md now describe the port range (a9). No test changes — behavior is verified by the release-day UX smoke.
…ndfather comment
The Support Engineer flagged a suspected regression: the tightened
cost-gate ("model.cost.input != null && !== 0") could exclude
legitimate paid providers whose model entries omit the `cost` field
— custom `altimate-code.json` registrations, BYOK entries with no
per-token metadata, self-hosted deployments — and force-feed them
the welcome picker on upgrade to v0.9.4.
Trace of the existing code confirmed the concern is a false alarm
for anyone whose provider id isn't "opencode": the outer `provider.id
!== "opencode" || …` short-circuits before the cost check runs. Only
OpenCode's own free-tier models are affected, which is the intended
tightening.
Rewrite as an explicit if/else so the intent reads at a glance
(non-opencode → connected; opencode → require nonzero cost) instead
of relying on short-circuit precedence, and add a grandfather comment
explaining WHY the check looks stricter than necessary. Semantically
identical to the pre-refactor code — verified by the existing
provider-connected test cases.
Proper prior-state regression detector (compare against the user's
last-known-connected set, not just current) is deferred to #1052 (D9).…ives in onboard-connect Two clarifications to the /onboard-connect activation-menu template, both flagged by the End User persona from a first-run walkthrough: **a7.** "Try Altimate on a sample dbt project" gave no hint about where the sample would land on disk. Users who accepted the option had to guess (or dig through logs) to find `~/altimate-sample-dbt/`. Menu row now says so up front. **a8.** The "install dbt-duckdb" fallback offered only `pip install dbt-duckdb`, which fails on modern Python (PEP 668 externally-managed-environment) with an unhelpful "externally-managed" error. Add `pipx install dbt-duckdb`, `uv tool install dbt-duckdb`, and the venv recipe as first-class alternatives. Each is a one-liner that works out of the box on macOS / Homebrew Python 3.12+ and on Debian/Ubuntu with the system Python. Template only — no code changes.
…ivation menu Rewrite Step 2 of the getting-started quickstart to describe what users actually see on a fresh install: welcome panel, curated 6-provider picker with Altimate LLM Gateway on the top row, and the env-var short-circuit for CI users. Add a new Step 2.5 covering the first-run "Scan your environment?" Yes/No dialog and its four branches (warehouse found / dbt-only / git-repo-no-dbt / nothing), and explicitly note that the "What would you like to do?" menu is chat text — the user types the number, not arrow-selects a row. Interactive picker is deferred (see #1052 / D1). Loopback OAuth port range 7317-7325 documented under the "Don't want to manage API keys?" tip so users encountering a port-collision error have context. Flagged by the PM persona (release-notes story) + End User persona (first-run mismatch between docs and product).
…users at the chat Reorder the sample project's README so the "What to try" section leads with the activation-menu path (typical first-run) instead of the bare-shell dbt recipe. Move the pip flow to an appendix labelled for the "I `cd`'d in from a different shell" case. Also mention the PEP 668 alternatives that made it into the template (pipx / uv / venv) and note that altimate-code walks the common Python env managers when locating an existing dbt binary, so users don't have to install into their system Python. Flagged by End User persona: the previous README wording implied the sample was primarily a bring-your-own-dbt playground, undersold the in-chat workflow.
…e can't collide with success rows
The two error paths in `sample_setup` returned metadata shaped like
a fresh-materialize success row: `{ targetPath: "", reused: false,
suffix: 0, note: "" }`. Downstream consumers that read metadata (the
template branches on `output`, but external callers of the tool can
subscribe to the whole envelope) had no reliable way to distinguish
"error" from "materialized into the preferred slot" without also
checking the `error` field.
Set `suffix: -1` on error metadata. Negative suffixes are impossible
on success (the resolver always returns >= 0 for numeric slots or a
string suffix for the randomized-fallback case), so the shape alone
tells success from failure.
Flagged by the Tech Lead persona; comment in-place explains the
collision so a future refactor doesn't drift back.… a follow-up The `writeJsonAtomic` helper's docstring promised "migration of the existing JSON.stringify + writeFileSync call sites" as if it were done in the same change. It isn't — only new sites use the helper; the legacy sites are still direct. That's a deliberate defer (the sweep touches sync-critical config files and needs its own review), but the docstring made it look landed. Update the docstring to reflect reality: helper exists, sweep is a tracked follow-up. No behavior change. Flagged by the Tech Lead persona.
…already CI-skipped) The test was gated with `test.skip` when `CI=true` because the `bootstrap.resolve-tools` span duration (~30-100ms) races the 50ms PTY sampling interval on cold runners — the label can complete between polls and the assertion drops it. Local runs also hit the flake (4/5 pass observed by the author). Since the test never runs in CI it provides no regression signal there; locally it emits noise on every full-suite run. Static + unit tests for the phase-event pipeline (server publish, TUI subscribe, store update) are separately in place and unaffected. Removes 9 references to an internal-tracker id from a public-repo file (D8 tracker-ref leak class, see #1052). Requested by @haider during v0.9.4 release testing.
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates provider onboarding, optional environment scanning, sample-project activation, loopback OAuth handling, provider readiness checks, test isolation, CLI retries, binary staleness detection, and filesystem documentation. ChangesOnboarding and sign-in flow
Test and maintenance reliability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AltimateCLI
participant AltimateCallbackServer
participant Browser
participant AltimateOAuth
AltimateCLI->>AltimateCallbackServer: Start listener on ports 7317-7325
AltimateCallbackServer->>AltimateCLI: Return active callback port
AltimateCLI->>Browser: Launch authorization URL
Browser->>AltimateOAuth: Complete sign-in
AltimateOAuth->>AltimateCallbackServer: Redirect to active loopback port
AltimateCallbackServer->>AltimateCLI: Deliver authorization callback
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
…n permission tests Six tests in `test/acp/permission.test.ts` failed when the developer had `ALTIMATE_CLI_YOLO=true` in their shell env (a common setup — direnv- managed at the repo root for daily use). Root cause is the short-circuit in `src/acp/permission.ts:57`: when the flag is set, the permission handler auto-approves with `reply: "once"` and returns before ever calling the ACP client's `requestPermission`, so: - The four tests that assert the reply shape (once/reject/etc.) saw the wrong reply and failed `toMatchObject`. - The two tests that `pollUntil(harness.requests.length === 1)` timed out after 2s because requestPermission was never called. The tests are correct — the code they test is a runtime feature toggle that should not leak into the deterministic test surface. Snapshot + restore `ALTIMATE_CLI_YOLO` (and its `OPENCODE_YOLO` fallback) around the whole file so the suite passes regardless of the developer's ambient env. Verified: 6/6 pass with and without `ALTIMATE_CLI_YOLO=true` in the parent env.
… older than build.ts The smoke tests picked up any `dist/*-<os>-<arch>/altimate` binary regardless of when it was built, and asserted invariants (embedded NAPI `.node` naming, standalone NODE_PATH-cleared startup) that only hold for a binary compiled from the CURRENT `script/build.ts`. Consequence: any developer with a stale local build — e.g. from a `bun run build:local` several weeks ago — sees 2 red failures on every `bun test` run even though nothing they did is broken. `script/build.ts` is the source of truth for the embed logic; a binary older than it is guaranteed to be checked against invariants that no longer match. Skip in that case with an actionable "run `bun run build:local` to refresh" message instead of failing. CI is unaffected: it either builds fresh or has no `dist/` at all (`findLocalBinary()` returns undefined → the existing "no local build found" skip fires). Verified: with a 2026-06-13 local binary present, all 5 tests now skip cleanly with the new message.
…ed" from child Under CI's bounded SUBPROCESS pass (`--max-concurrency=2`) — and more often locally at default concurrency — the CLI subprocess occasionally exits 1 with `Error: Unexpected error / database is locked` on the first `opencode run` invocation. Session storage opens SQLite at boot, and on cold spawns the WAL checkpoint can collide with the tracer's first write. It's a genuine transient (not a product regression) — the same command succeeds on retry. Wrap `run()` in the `withCliFixture` helper so that a non-zero exit whose stderr matches `/database is locked/i` triggers exactly one retry. Any other failure mode still surfaces immediately — only this specific transient is masked. Cleared: the `--trace writes a session trace artifact` case that consistently flaked under `--max-concurrency=2` locally. Not a change in production code — only the test-helper retry.
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
sahrizvi
marked this pull request as ready for review
July 31, 2026 05:51
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.
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/sample-projects/jaffle-shop-duckdb/README.md`:
- Around line 49-55: Update the installation command in the README setup
sequence to also install the separate DuckDB CLI package before the existing
duckdb query command, while preserving the current dbt-duckdb and optional pipx
installation guidance.
In `@packages/opencode/src/altimate/onboarding/materialize.ts`:
- Line 162: Update the HOME/root warning in the materializer error message to
direct users to rerun the CLI without sudo after installation. Remove the
ownership-unsafe `sudo HOME=/Users/you altimate-code` instruction; if privileged
execution must be supported, provide a recovery procedure that restores user
ownership before normal use.
In `@packages/opencode/src/altimate/plugin/altimate.ts`:
- Around line 157-172: Make callback-server initialization atomic in the
authorization startup flow around the shared server state and port-binding loop:
store the in-progress startup operation as a shared promise, have concurrent
authorize() calls await it before using currentCallbackPort, and only expose the
server after binding succeeds. Clear the shared startup promise on startup
failure and during shutdown, while preserving cleanup for success, errors, and
cancellation.
In `@packages/opencode/src/command/template/onboard-connect.txt`:
- Around line 188-195: Update the venv fallback instructions in the onboarding
template to tell users to paste the absolute path to the generated dbt
executable, rather than the tilde-prefixed ~/.venvs/dbt/bin/dbt path. Keep the
existing venv creation and installation commands unchanged, and align the
guidance with option 1’s literal path validation and execution.
In `@packages/opencode/src/util/filesystem.ts`:
- Around line 282-289: Update the docstring near the temporary-write migration
references to link directly to GitHub issue `#1052` instead of the generic issues
list, preserving the existing explanation of the deferred migrations.
In `@packages/opencode/test/install/smoke-test-binary.test.ts`:
- Around line 98-104: Update isBinaryStale() to compare the binary against a
generated build manifest or fingerprint covering all embedded build inputs,
including build.ts, changelog, migrations, skills, model snapshots,
parser/worker outputs, and the pinned altimate-core prebuild. Generate or
refresh this manifest through the existing build flow, and only skip rebuilding
when the binary’s recorded fingerprint matches the current manifest.
In `@packages/opencode/test/lib/cli-process.ts`:
- Around line 285-295: Update the retry logic in the Effect.gen block around
spawn so both attempts share the original timeout budget instead of allowing the
SQLite-lock retry to start another full-duration invocation. Track a common
deadline or derive a reduced timeout for the second spawn, while preserving the
single retry only for “database is locked” failures and returning other results
unchanged.
- Around line 291-294: Update the retry logic around spawn in the CLI process
helper to avoid retrying non-idempotent opencode run invocations after a SQLite
lock; restrict retries to idempotent command-only calls, or reset the relevant
fixture/session state before retrying. Preserve the existing immediate return
behavior for successful and non-lock failures.
In `@packages/opencode/test/mcp/lifecycle.test.ts`:
- Line 251: Update the test setup around homedirSpy so each test creates its
temporary home directory once in beforeEach, stores it in a captured homeDir
variable, and makes the mocked os.homedir return that same value on every call.
Preserve the existing per-test isolation while preventing different calls from
resolving different directories.
🪄 Autofix (Beta)
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: d4cd6a13-bb0d-4b0f-b5f4-6068ec9bbf54
📒 Files selected for processing (13)
docs/docs/getting-started/quickstart-new.mdpackages/opencode/sample-projects/jaffle-shop-duckdb/README.mdpackages/opencode/src/altimate/onboarding/materialize.tspackages/opencode/src/altimate/plugin/altimate.tspackages/opencode/src/altimate/tools/sample-setup.tspackages/opencode/src/command/template/onboard-connect.txtpackages/opencode/src/util/filesystem.tspackages/opencode/test/acp/permission.test.tspackages/opencode/test/cli/tui/phase-label.tui-e2e.test.tspackages/opencode/test/install/smoke-test-binary.test.tspackages/opencode/test/lib/cli-process.tspackages/opencode/test/mcp/lifecycle.test.tspackages/tui/src/component/use-connected.tsx
💤 Files with no reviewable changes (1)
- packages/opencode/test/cli/tui/phase-label.tui-e2e.test.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+98
to
+104
| function isBinaryStale(binaryPath: string): boolean { | ||
| try { | ||
| const buildScript = path.join(PKG_DIR, "script", "build.ts") | ||
| if (!fs.existsSync(buildScript)) return false | ||
| const binMtime = fs.statSync(binaryPath).mtimeMs | ||
| const scriptMtime = fs.statSync(buildScript).mtimeMs | ||
| return binMtime < scriptMtime |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
rg -n -C 4 'altimate-core|script/build\.ts|build:local|bun build' packages/opencodeRepository: AltimateAI/altimate-code
Length of output: 50381
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== smoke test relevant section =="
sed -n '1,150p' packages/opencode/test/install/smoke-test-binary.test.ts
echoecho"== build script top/middle/outlines around bundling =="
sed -n '1,120p' packages/opencode/script/build.ts
sed -n '220,430p' packages/opencode/script/build.ts
sed -n '430,530p' packages/opencode/script/build.ts
echoecho"== files listed in smoke test and adversarial test imports if present =="
sed -n '1,80p' packages/opencode/test/skill/release-v0.7.1-binary-adversarial.test.tsRepository: AltimateAI/altimate-code
Length of output: 30409
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== changed files/stat =="
git status --short
git diff --stat HEAD^..HEAD 2>/dev/null ||trueechoecho"== relevant diff context =="
git diff HEAD^..HEAD -- packages/opencode/test/install/smoke-test-binary.test.ts packages/opencode/script/build.ts packages/opencode/package.json 2>/dev/null | sed -n '1,240p'||trueechoecho"== deterministic check of package version pin against script build dependency =="
python3 - <<'PY'import json, pathlib, repkg = json.loads(pathlib.Path("packages/opencode/package.json").read_text())pkg_dep = pkg.get("dependencies", {}).get("`@altimateai/altimate-core`")build = pathlib.Path("packages/opencode/script/build.ts").read_text()m = re.search(r'`@altimateai/altimate-core`@([^\s\'"`]+)', build)print(f"package.json dependency={pkg_dep}")print(f"script/build.ts install command dependency={m.group(1) if m else None}")print(f"match={pkg_dep == m.group(1) if m else False}")PYRepository: AltimateAI/altimate-code
Length of output: 514
Use a build-manifest fingerprint for binary freshness.
isBinaryStale() only checks the binary against script/build.ts. build.ts also embeds CHANGELOG.md, migration files, .opencode/skills/SKILL.md, src/provider/models-snapshot.ts, parser/worker output, and the pinned @altimateai/altimate-core prebuild, so those changes can leave the build script mtime unchanged while the binary is still stale. Switch to a generated build manifest or fingerprint that covers the embedded inputs before relying on this skip.
🤖 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/install/smoke-test-binary.test.ts` around lines 98 -
104, Update isBinaryStale() to compare the binary against a generated build
manifest or fingerprint covering all embedded build inputs, including build.ts,
changelog, migrations, skills, model snapshots, parser/worker outputs, and the
pinned altimate-core prebuild. Generate or refresh this manifest through the
existing build flow, and only skip rebuilding when the binary’s recorded
fingerprint matches the current manifest.
ContributorAuthor
There was a problem hiding this comment.
Fair point — the mtime-vs-build.ts guard doesn't cover embedded inputs like CHANGELOG.md, migrations, skills, or the pinned altimate-core prebuild. However, the current guard is strictly better than no guard (the pre-fix state was "always fails on any stale build"). Filed under issue #1052 as a follow-up so we can design a proper build-manifest fingerprint separately.
There was a problem hiding this comment.
@sahrizvi, acknowledged. The current script/build.ts guard reduces stale-binary failures for the changed build logic. GitHub issue #1052 is the correct follow-up for complete build-input fingerprinting.
You are interacting with an AI system.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+291
to
+294
| const first = yield* spawn(argv, opts) | ||
| if (first.exitCode === 0) return first | ||
| if (!/database is locked/i.test(first.stderr)) return first | ||
| return yield* spawn(argv, opts) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
rg -n -C 5 'withCliFixture|\.opencode\.run\s*\(|\bopencode\.run\s*\(' packages/opencode/testRepository: AltimateAI/altimate-code
Length of output: 11418
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== cli-process relevant sections =="
sed -n '240,330p' packages/opencode/test/lib/cli-process.ts
sed -n '1,80p' packages/opencode/test/lib/cli-process.ts
sed -n '420,470p' packages/opencode/test/lib/cli-process.ts
echo"== opencode.run call sites =="
rg -n -C 4 'opencode\.run\(' packages/opencode/test/cli packages/opencode/test/lib ||trueecho"== fixture state / session writes candidates in opencode CLI =="
rg -n -C 3 'session|state|sqlite|locked|run|database|store|write|artifact|output|stdout|fixture|fixtureState|workdir|tmpdir' packages/opencode/src packages/opencode/test/lib packages/opencode/test/cli/run/run-process.test.ts | sed -n '1,220p'Repository: AltimateAI/altimate-code
Length of output: 36005
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== package files and lockfiles =="
fd -a '^(package\.json|bun\.lockb|bun\.lock|pnpm-lock\.yaml|yarn\.lock|package-lock\.json)$'.| sed 's#^\./##'| sort | head -80
echo"== cli index/run command refs =="
rg -n -C 4 'run|Serve\.Default|Session|SessionRegistry|AppRuntime|main|src/index' packages/opencode/src/index.ts packages/opencode/src packages/opencode | sed -n '1,240p'echo"== session/storage sqlite refs =="
rg -n -C 4 'sqlite|Sqlite|Database|WAL|checkpoint|locked|session|SessionStore|persist|storage|InstanceStore|createWriteStream|mkdtemp|tmpdir|fs\.(rm|remove|unlink|writeFile|appendFile|mkdir)' packages/opencode/src packages/opencode/feature packages/opencode/ 2>/dev/null | sed -n '1,260p'||trueRepository: AltimateAI/altimate-code
Length of output: 42333
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== sqlite lock implementations =="
rg -n -C 6 'database is locked|locked|lock|SQLITE_BUSY|busy|WAL|checkpoint|xWalCheckpoint|SqliteBackend|SqliteNode|open|createConnection|createConnectionSync|`@opencode-ai/core/database`|Database\.defaultLayer' packages packages/opencode packages/core 2>/dev/null | sed -n '1,260p'||trueecho"== RunCommand implementation =="
fd -a 'run\.ts$' packages/opencode/src packages/opencode packages | sed -n '1,40p'forfin$(fd 'run\.ts$' packages/opencode/src packages/opencode packages 2>/dev/null | head -20);doecho"--- $f ---"
rg -n -C 8 'class RunCommand|handler:|run\(|Session|message|stdout|events|AppRuntime|trace|Output|Event|session'"$f"| sed -n '1,220p'||truedoneecho"== session event writes =="
rg -n -C 4 'session\.error|session\.success|text|message|append|events|Session' packages/opencode/src/session packages/opencode/src packages/opencode/packages/core/session 2>/dev/null | sed -n '1,260p'||trueecho"== package workspace references =="
sed -n '1,140p' packages/opencode/package.jsonRepository: AltimateAI/altimate-code
Length of output: 50382
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== precise sqlite lock sources =="
rg -n -C 6 'database is locked|wants a lock|LOCK_CONTENTION|is locked|SQLiteError|Sqlite' packages packages/opencode packages/core packages/effect-sqlite-node packages/effect-drizzle-sqlite 2>/dev/null | sed -n '1,180p'||trueecho"== opencode test fixture state cleanup =="
sed -n '213,290p' packages/opencode/test/lib/cli-process.ts
sed -n '310,380p' packages/opencode/test/lib/cli-process.ts
echo"== RunCommand full relevant sections =="
sed -n '377,590p' packages/opencode/src/cli/cmd/run.ts
sed -n '590,820p' packages/opencode/src/cli/cmd/run.ts
echo"== run events/app runtime usage section =="
rg -n -C 4 'session\.(create|list|share|fork)|process\.exit|sessionID|AppRuntime\.runPromise|Session\.Default|GlobalBus\.publish|event\.type|output' packages/opencode/src/cli/cmd/run.ts | sed -n '1,180p'||trueRepository: AltimateAI/altimate-code
Length of output: 47374
🏁 Script executed:
#!/bin/bashset -euo pipefail
# Inspect sqlite error classification and the source of "database is locked".
rg -n -C 8 'classifySqliteError|SQLite_ERROR_LOCK_CONTENTION|wants a lock|locked|LOCK_CONTENTION|message:|message' packages/core/src packages/effect-sqlite-node/src 2>/dev/null | sed -n '1,220p'||true# Compare current run retry code with exact line numbers from git.
git --no-pager diff -- packages/opencode/test/lib/cli-process.ts | sed -n '1,120p'||true
sed -n '288,300p' packages/opencode/test/lib/cli-process.ts
# Narrow session creation/write side effects in RunCommand.
sed -n '556,840p' packages/opencode/src/cli/cmd/run.ts | sed -n '1,260p'Repository: AltimateAI/altimate-code
Length of output: 30444
Don’t retry non-idempotent opencode run calls on SQLite lock.
All current opencode.run() call sites run the same prompt after session.create, so a first attempt that writes session state and tracing before hitting database is locked can duplicate results or trace output on retry. Limit this retry path to idempotent command-only invocations, or reset the fixture/session state before retried execution.
🤖 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/lib/cli-process.ts` around lines 291 - 294, Update the
retry logic around spawn in the CLI process helper to avoid retrying
non-idempotent opencode run invocations after a SQLite lock; restrict retries to
idempotent command-only calls, or reset the relevant fixture/session state
before retrying. Preserve the existing immediate return behavior for successful
and non-lock failures.
ContributorAuthor
There was a problem hiding this comment.
Valid concern — the narrow /database is locked/i error-string match already limits the blast radius (a first attempt that errors before session-state write is the only class that triggers retry), but a proper fix would either reset the fixture between attempts or gate retry to command-only invocations. Filed under issue #1052 so we can design it in isolation.
There was a problem hiding this comment.
@sahrizvi, acknowledged. The current narrow error match reduces exposure, but it does not establish that the first invocation had no side effects. Deferring the retry design to #1052 is appropriate.
I will leave this finding unresolved for this PR and track the follow-up in #1052. 🐇
You are interacting with an AI system.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
1 issue found across 13 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/altimate/plugin/altimate.ts">
<violation number="1" location="packages/opencode/src/altimate/plugin/altimate.ts:269">
P2: Copying this URL from an SSH/tmux session to a local browser still redirects to the local workstation's loopback, so sign-in times out unless the user has separately forwarded the selected fallback port. The headless guidance could include an SSH port-forwarding path using the actual bound port, or use a device/non-loopback flow for remote browsers.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Uh oh!
There was an error while loading. Please reload this page.
| // Remote-SSH, and without a printed URL a headless user gets no path | ||
| // forward and times out staring at "Complete sign-in in your browser". | ||
| await open(authorizeUrl).catch(() => undefined) | ||
| process.stderr.write( |
There was a problem hiding this comment.
P2: Copying this URL from an SSH/tmux session to a local browser still redirects to the local workstation's loopback, so sign-in times out unless the user has separately forwarded the selected fallback port. The headless guidance could include an SSH port-forwarding path using the actual bound port, or use a device/non-loopback flow for remote browsers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/plugin/altimate.ts, line 269:
<comment>Copying this URL from an SSH/tmux session to a local browser still redirects to the local workstation's loopback, so sign-in times out unless the user has separately forwarded the selected fallback port. The headless guidance could include an SSH port-forwarding path using the actual bound port, or use a device/non-loopback flow for remote browsers.</comment>
<file context>
@@ -221,16 +249,26 @@ export async function AltimateAuthPlugin(_input: PluginInput): Promise<Hooks> {
+ // Remote-SSH, and without a printed URL a headless user gets no path
+ // forward and times out staring at "Complete sign-in in your browser".
await open(authorizeUrl).catch(() => undefined)
+ process.stderr.write(
+ `\nIf your browser didn't open automatically, complete sign-in at:\n ${authorizeUrl}\n\n`,
+ )
</file context>
ContributorAuthor
There was a problem hiding this comment.
Valid — the printed URL alone doesn't help SSH/tmux users unless they've forwarded the port. A device/non-loopback flow is the right long-term answer but out of scope for a release-fix bundle. Filed under issue #1052 for the follow-up work.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…ent OAuth flows Both CodeRabbit (Major) and cubic (P1, confidence 9) flagged the same race in `startCallbackServer()`: `server` was written before `listen()` resolved and `currentCallbackPort` was undefined during the port walk. A second `authorize()` call arriving mid-startup passed the `if (server) return` short-circuit and immediately read `currentCallbackPort ?? CALLBACK_PORT_PREFERRED`, building a redirect to port 7317 — but the first flow may have fallen back to 7318-7325 because 7317 was squatted. The callback from the second flow would then never reach the listener. Coalesce concurrent starts through a shared `startupInFlight` promise. Every caller awaits the same startup and reads the actually-bound `currentCallbackPort` before constructing its redirect. The promise is cleared in a `finally` on both success and failure so a later retry re-runs the port walk cleanly. Fast-path (`if (server && currentCallbackPort !== undefined) return`) still elides the promise dance for the common case of "already bound and healthy" — no perf regression for the single-flow path.
…very text CodeRabbit (Major) and cubic (P2, confidence 9) both flagged the `HOME=/root` error message that this PR introduced: 1. `sudo HOME=/Users/you altimate-code` still writes as root, so a later normal-user run can't modify the materialized sample. 2. `/Users/you` is a macOS path — Linux users get told to run something that's platform-wrong; Windows users doubly so. Rewrite: primary guidance is now "re-run without sudo" (install to a per-user prefix like `~/.local` via `npm i -g --prefix ~/.local`, or use nvm/asdf). Secondary path is `sudo -E HOME="$HOME" altimate-code` which uses the caller's own HOME regardless of platform, with an explicit chown recovery hint for the root-owned files that result. Existing test just asserts the message contains "sudo" — still passes.
…icate; drop invented comment history Consensus review m7 + m8: the "is any provider connected" predicate was duplicated verbatim in `use-connected.tsx` and `home/tips.tsx`, and the comment I added in `use-connected.tsx` invented a regression that never existed (claimed the `provider.id !== "opencode" || ...` short-circuit had been "tightened" to exclude legitimate paid providers — but the short-circuit had always exempted non-OpenCode providers, so the described regression was mathematically impossible). Fix: 1. Extract `isAnyProviderConnected(providers)` into `util/connected.ts` with the accurate rationale (why the cost gate is OpenCode-specific, why undefined cost counts as "not paid" for OpenCode only). 2. Both `use-connected.tsx` (welcome-picker gate) and `home/tips.tsx` (first-run tip visibility) call the extracted helper. Cannot silently drift now. 3. Dropped the invented "regression grandfather" narrative — the refactor from `A || B` to `if (A) return true; return B` was and is a faithful no-op; the comment now says only that. Consensus review warning: three separate reviewers repeated the false grandfather history back as established fact. Removing the source prevents that from becoming institutional memory.
…ess guard; drop unreachable sentinel Three fixes from the consensus review of PR #1053. **m3 — cli-process.ts SQLite retry observability**: the retry was silent on the retry path, so a systematic regression from transient lock contention would keep the suite green — defeating the point of the harness. Added a `console.warn` that names the failure class and the "if you see this often, contention has moved from transient to systematic" signal, so a repeated pattern is visible in CI logs. **m5 — smoke-test-binary staleness guard walks src/ + script/**: the prior guard compared only against `script/build.ts`. That catches the rare "build logic changed" case but MISSES the common "src/ changed" case — after a `git pull` that only touches source, the guard would still say the binary is fresh and run assertions against a stale build. Rewrote `isBinaryStale()` to walk both trees, skipping `node_modules` / `.turbo` / `dist` / hidden dirs, and looking at files that actually contribute to the compiled binary (`*.ts{,x}`, `*.json`, `*.txt`, `*.md`). **m6 — sample_setup error metadata: revert suffix: -1 → 0**: the sentinel guarded a collision no consumer can reach. Error-path `output` contains no `suffix:` line at all (it's `status: error\n reason: ...\n\n${message}`), and the template branches on `status: error` first. `success: false` is the true disambiguator for metadata readers. Reverted the sentinel and updated the comment to reflect the actual disambiguation contract.
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
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/test/install/smoke-test-binary.test.ts">
<violation number="1" location="packages/opencode/test/install/smoke-test-binary.test.ts:126">
P2: The extension filter includes `.md` files when checking staleness, but `.md` files under `src/` and `script/` are never embedded in the binary — editing a README or AGENTS doc would falsely skip the smoke tests. Drop `.md` from the regex to match the comment's intent (`*.ts / *.tsx / *.json / *.txt`).</violation>
</file>
<file name="packages/opencode/src/altimate/plugin/altimate.ts">
<violation number="1" location="packages/opencode/src/altimate/plugin/altimate.ts:209">
P1: A post-bind `http.Server` error can now crash the TUI worker: this removes the only `error` listener immediately after the bind succeeds, while Node treats an emitted `error` without a listener as uncaught. Keeping a persistent server-level handler after removing the bind-attempt listener would preserve the retry behavior without leaving the live callback server unprotected.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
| server!.once("error", onErr) | ||
| server!.listen(port, "127.0.0.1", () => { | ||
| server!.removeListener("error", onErr) |
There was a problem hiding this comment.
P1: A post-bind http.Server error can now crash the TUI worker: this removes the only error listener immediately after the bind succeeds, while Node treats an emitted error without a listener as uncaught. Keeping a persistent server-level handler after removing the bind-attempt listener would preserve the retry behavior without leaving the live callback server unprotected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/plugin/altimate.ts, line 209:
<comment>A post-bind `http.Server` error can now crash the TUI worker: this removes the only `error` listener immediately after the bind succeeds, while Node treats an emitted `error` without a listener as uncaught. Keeping a persistent server-level handler after removing the bind-attempt listener would preserve the retry behavior without leaving the live callback server unprotected.</comment>
<file context>
@@ -182,21 +186,45 @@ async function doStartCallbackServer(): Promise<void> {
+ }
+ server!.once("error", onErr)
+ server!.listen(port, "127.0.0.1", () => {
+ server!.removeListener("error", onErr)
+ resolve()
+ })
</file context>
| // Only consider files that actually contribute to the compiled binary | ||
| // — build.ts globs *.ts / *.tsx / *.json / *.txt for embedding. A | ||
| // stray editor swap file (.swp) or backup shouldn't trigger a re-skip. | ||
| if (!/\.(tsx?|json|txt|md)$/.test(entry.name)) continue |
There was a problem hiding this comment.
P2: The extension filter includes .md files when checking staleness, but .md files under src/ and script/ are never embedded in the binary — editing a README or AGENTS doc would falsely skip the smoke tests. Drop .md from the regex to match the comment's intent (*.ts / *.tsx / *.json / *.txt).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/install/smoke-test-binary.test.ts, line 126:
<comment>The extension filter includes `.md` files when checking staleness, but `.md` files under `src/` and `script/` are never embedded in the binary — editing a README or AGENTS doc would falsely skip the smoke tests. Drop `.md` from the regex to match the comment's intent (`*.ts / *.tsx / *.json / *.txt`).</comment>
<file context>
@@ -89,19 +89,59 @@ function resolveNodePath(): string {
+ // Only consider files that actually contribute to the compiled binary
+ // — build.ts globs *.ts / *.tsx / *.json / *.txt for embedding. A
+ // stray editor swap file (.swp) or backup shouldn't trigger a re-skip.
+ if (!/\.(tsx?|json|txt|md)$/.test(entry.name)) continue
+ try {
+ const m = fs.statSync(full).mtimeMs
</file context>
Suggested change
| if(!/\.(tsx?|json|txt|md)$/.test(entry.name))continue | |
| if(!/\.(tsx?|json|txt)$/.test(entry.name))continue |
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
…s as paired start/end blocks Marker Guard CI is strict: single-line `// altimate_change — description` comments don't count as protection for the code that follows. My m8 refactor used the single-line form for both the import and the `isAnyProviderConnected` call in `home/tips.tsx`, and the guard on the PR's CI run flagged the `const connected = ...` line as unmarked new code in an upstream-shared file. Rewrap both spots as `// altimate_change start` / `// altimate_change end` blocks so the guard's `computeMarkedLines` coverage map includes the extracted-predicate call site. No behavior change; comment form only.
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tui/src/feature-plugins/home/tips.tsx (1)
51-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClose the
altimate_changeblock before Line 54.The marker at Line 51 has no matching
altimate_change endbefore the block that starts at Line 54. Add the end marker afterconst connectedto avoid nested markers.Proposed fix
const connected = createMemo(() => isAnyProviderConnected(api.state.provider)) +// altimate_change end // altimate_change start — upstream_fix: restore first-run onboarding state🤖 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/tui/src/feature-plugins/home/tips.tsx` around lines 51 - 53, Add the matching altimate_change end marker immediately after the connected memo declaration in the tips component, closing the marker block before the following block begins and leaving the predicate unchanged.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/tui/src/feature-plugins/home/tips.tsx`:
- Around line 51-53: Add the matching altimate_change end marker immediately
after the connected memo declaration in the tips component, closing the marker
block before the following block begins and leaving the predicate unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3424b8b9-8f1b-4f1e-9db4-25220700a76f
📒 Files selected for processing (9)
packages/opencode/sample-projects/jaffle-shop-duckdb/README.mdpackages/opencode/src/altimate/plugin/altimate.tspackages/opencode/src/altimate/tools/sample-setup.tspackages/opencode/src/command/template/onboard-connect.txtpackages/opencode/test/install/smoke-test-binary.test.tspackages/opencode/test/lib/cli-process.tspackages/tui/src/component/use-connected.tsxpackages/tui/src/feature-plugins/home/tips.tsxpackages/tui/src/util/connected.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/opencode/test/lib/cli-process.ts
- packages/opencode/sample-projects/jaffle-shop-duckdb/README.md
- packages/tui/src/component/use-connected.tsx
- packages/opencode/src/command/template/onboard-connect.txt
…name in comments Marker Guard step 9 (branding audit, LEAK_PATTERNS regex `/\bOpenCode\b/g`) flagged two mentions of the upstream product name in the new file's docstring comments. Reworded to reference the provider-id string literal (`` `"opencode"` ``, `` `opencode` provider ``) which is code identity, not branding. Same technical content, no product-name leak. The prior mention in `component/use-connected.tsx:6` is untouched — it lives inside an `altimate_change` block, which the branding audit whitelists as intentional fork commentary. Verified locally: `bun run script/upstream/analyze.ts --branding` reports 0 leaks.
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
saravmajestic
previously approved these changes
Jul 31, 2026
Added: - First-run scan + activation menu (bundled 6-provider picker, scan gate with 4 branches, numbered activation menu wired to real skills). - Bundled jaffle-shop DuckDB sample project with pre-compiled manifest so /discover + /review work without dbt-core installed. Fixed: - Loopback OAuth port walk 7317-7325 + 15-min pending window + concurrent-startup coalescing. - CI test-isolation leak (spyOn(os,"homedir") without restore). - HOME=/root recovery message: portable + ownership-safe. - PEP 668 install fallbacks (pipx/uv) actually expose a dbt binary. - Scan telemetry disclosure honest about anonymous env summary. - sample_setup error metadata shape (suffix: -1 → 0). - Auth plugin no longer leaks state-bearing URLs into log files. Changed: - useConnected + home-tips share one predicate (packages/tui/src/util/connected.ts).
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
saravmajestic
approved these changes
Jul 31, 2026
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 2, 2026
This was referenced Aug 10, 2026
This was referenced Aug 13, 2026
sahrizvi added a commit
that referenced
this pull request
Aug 25, 2026
* chore(hygiene): [#1052 D8] pre-push scan for internal-tracker refs Public repo hardening: catch tracker-adjacent references before they land in commits, branches, or file content. The exact patterns live in the RULES array in `script/check-tracker-leaks.ts` — read there for the canonical list. - `script/check-tracker-leaks.ts` — bun script that scans branch name + commit messages + added-lines diff vs `origin/main`. Silent on clean; exits 1 with per-source report on hit. Bypass via `SKIP_TRACKER_CHECK=1`. - `.husky/pre-push` — invokes the script after existing typecheck. - `CONTRIBUTING.md` — one-line note pointing at the hook + how to bypass. CI-side mirror deferred to a follow-up PR (needs a `workflow`-scoped token to add `.github/workflows/*` and this session's token doesn't have it). The D8 issue calls out both local + CI as needed — local hook is the primary guard; CI is the backstop for contributors who never ran `git config core.hooksPath .husky` or who used the bypass env var. The scan is diff-based: existing tracker-adjacent strings in main (a handful in comments) are grandfathered and won't trigger on unrelated PRs. Only NEW added lines are checked, so touching a file with a legacy reference is safe as long as the reference itself doesn't appear in the diff's `+` lines. * test(build): [#1052 D10] stamp-based staleness guard for the smoke test The m5 guard walked `src/` + `script/` mtimes for the newest touched file. Correct for the common case but blind to changes in CHANGELOG.md, migrations, bundled skills, models-snapshot.ts, the opentui parser worker, and the per-platform altimate-core prebuild — any of which can change the compiled binary's shape without touching a `.ts` file. - `packages/opencode/script/build.ts` — emit `dist/<target>/bin/build-inputs.json` at the end of each per-target build. JSON lists every embedded input's sha256 plus an aggregate hash over the sorted pairs. Paths are relative to `packages/opencode` so the read side can resolve them without env plumbing. - `packages/opencode/test/install/smoke-test-binary.test.ts` — new `isBinaryStaleFromStamp()` rehashes each listed input and reports stale on any mismatch. Falls back to the old mtime walk when the stamp is missing (older `bun run build:local` runs, or targets built before this landed). - Explicit `no-stamp` sentinel routes cleanly to the fallback without conflating "no binary" with "no stamp" — both surface as skips, but for different messages. * test(harness): [#1052 D11] idempotent retry in cli-process.run() The `database is locked` retry re-spawns `opencode run` against the same XDG_DATA_HOME. If the first attempt got as far as opening SQLite and taking a partial write before the WAL/checkpoint collision fired, a naive retry would either see partial state or double-write on top of it — exactly the non-idempotent behavior the CodeRabbit review on PR #1053 flagged. Fix: before the retry, delete `opencode*.db{,-wal,-shm}` under the fixture's isolated data dir. The retry then boots into a clean SQLite state. Other fixture content (config file, home files, extra test setup) is preserved — tests that inject state into `home` still see it. This is deliberately narrower than "reset the whole fixture" (option b in the deferral): scrubbing DB files only preserves any state a caller wrote before invoking `run()`, which some tests rely on. * test(tui): [#1052 D12] deterministic regression test for phaseLabel() Replaces the deleted `phase-label.tui-e2e.test.ts` (was `test.skip` under `CI=true` because a PTY poll-interval race made it flaky). The published chain — `publishPhase → Bus → sync.tsx handler → store → render` — is now covered in three deterministic layers: 1. Server-side publish + subscribe wiring: existing fork-feature-guards string-shape assertions in `test/upstream/fork-feature-guards.test.ts`. 2. Store-mutation handler in `context/sync.tsx` case "session.phase": same fork-feature-guards test. 3. Last-mile label lookup — THIS FILE. If `phase-label.ts` PHASE_LABELS drifts from the span names `SessionPrompt.traceSpan` emits, users see the "Thinking..." fallback silently. This test catches that. Full component-level synthetic-event coverage remains a future extension of D12; the event-injection scaffolding does not exist as a reusable fixture yet and is not gated on this file. * fix(models): [#1052 D14] drop eager import-time ModelsDev.refresh() Previously `setTimeout(() => ModelsDev.refresh(), 0)` fired a fetch to https://models.dev/api.json at module import. Its `AbortSignal.timeout(10000)` cannot cancel a synchronous `getaddrinfo()`; under Linux `unshare --net` (Verdaccio sanity Phase 3 [10/10] on Ubuntu CI runners) DNS blocked long enough that the pending fetch held the event loop past command completion and SIGTERM landed before any bytes flushed. That blocked the v0.9.4 release and forced the SKIP that ships in `test/sanity/phases/resilience.sh` today. Fix: no eager fetch. Callers use `ModelsDev.Data()` which resolves via (1) local disk cache → (2) bundled `models-snapshot.ts` (always present in release binaries, regenerated at build time) → (3) fetch only if both absent. The bundled snapshot means release-binary users always have model metadata even on a completely offline cold start. Long-running processes (TUI, serve) still receive updates via the hourly `setInterval` below, `.unref()`'d so it never blocks exit. Short-lived commands rely on the snapshot's release-time freshness. Trade-off: models added to models.dev between releases don't appear in short-lived commands until the next release rebuild. Bounded by release cadence. Acceptable given the release-blocker this closes. Follow-up (not in this PR): re-enable the `[10/10] no-internet graceful handling` sanity test once this fix has soaked through one release. * fix(hygiene): [#1052 D8 review-fix M1] catch suffix-adjacent tracker leaks + self-test Consensus review flagged the D8 scanner's Jira-key regex as its strongest finding — the trailing word-boundary requires a non-word char after the digits, which fails when a letter, digit, or underscore immediately follows. Exactly the class of typo and paste-through the scrubber exists to prevent. Naïve fixes (negative lookahead over word chars) don't help: the regex engine backtracks the digit run, but every position still has a digit as the "next char" so the lookahead keeps failing. Correct fix is to drop the trailing boundary entirely — the pattern matches greedily through the digits, stops at the first non-digit, and reports the prefix regardless of what follows. Known blind spot documented + tested: pastes with no separator before the prefix (no leading word-boundary) are not caught. Realistic leak surface (branches, commit messages, path fragments, doc text) is delimited so this does not hit in practice. Also: gate the scanner's `main()` behind `import.meta.main` so RULES can be imported by the self-test without triggering a scanner run at test collection time. `packages/opencode/test/skill/tracker-leak-check.test.ts` — 28 test cases pinning positive / negative / blind-spot behaviour for both regexes. Runs under `bun test`. If the regex regresses, this test catches it before the local push hook or CI misses a leak. * test(build): [#1052 D10 review-fix M2] widen stamp to workspace packages + lockfile Consensus review flagged that the D10 stamp (from `b22861e091`) only walks `packages/opencode/src` and `packages/opencode/script`, but Bun.build follows imports into every workspace package (`@opencode-ai/core`, `@opencode-ai/tui`, `@opencode-ai/util`, `@opencode-ai/plugin`, `@altimateai/dbt-tools`, …) and bundles them into the binary. Edits under those packages, or a `bun install` that bumps a bundled dep, left the stamp reporting fresh — exactly the false-negative pattern the guard was meant to close. Changes: - `packages/opencode/script/build.ts` — enumerate `packages/*/src` at build time and walk each (opencode/ is already walked directly). Also hash every workspace `packages/*/package.json`, the workspace-root `package.json`, and `bun.lock`. Enumerating rather than hard-coding lets new workspace packages get covered automatically. - Stamp paths are now REPO_ROOT-relative (not packages/opencode-relative) so entries like `packages/tui/src/util/record.ts` resolve without munging. - `packages/opencode/test/install/smoke-test-binary.test.ts` — resolve stamp entries against `REPO_ROOT` instead of `PKG_DIR`. Fallback to the mtime walk is unchanged (still works for older builds without a stamp). Verified: typecheck clean; existing D12 phase-label test still passes; smoke test skips cleanly when no binary is present (unchanged behaviour). A build + tampering with a workspace-package file will now flip the stamp; a build + stale binary + edit to `packages/tui/src/...` will trigger the skip that the old walk missed. * fix(models): [#1052 D14 review-fix M3] don't crash on non-JSON error body + fire refresh at boot Two consensus-review findings on the D14 commit (`248eaaf86c`): 1. Pre-existing crash: `ModelsDev.Data()`'s `Flock.withLock` branch calls `JSON.parse(result2.text)` unconditionally after `fetchApi()` returns — even when `result2.ok === false` and the body is an HTML 5xx error page. That throws `SyntaxError` and blocks model initialization. Bug existed before D14 but was rarely exposed because the eager import-time refresh() warmed the disk cache first, so most subsequent `Data()` calls returned via `readJson`. Post-D14 more first-calls fall through to fetch, so the crash window widens. Fix: return `{}` when `!result2.ok` — callers already tolerate an empty catalog (Provider.state produces no models.dev-derived entries, same UX as `OPENCODE_DISABLE_MODELS_FETCH=1`). 2. Reintroduce a boot-time refresh without holding the event loop: 4/6 reviewers flagged the loss of "fresh at boot" as a real regression for short-lived commands. Fix: `Promise.resolve().then(() => refresh().catch())`. A microtask can't itself keep Bun alive, and if the fetch it schedules is still in flight at process-exit the snapshot covers callers on the next run. Preserves the load-bearing part of the D14 fix (no `setTimeout` keeping the loop alive) while restoring near-immediate cache warming. If this ever reintroduces the unshare-net sanity hang on CI, drop the Promise.then line — the snapshot alone keeps release binaries functional offline. Also fixes the misleading comment "always have model metadata even on a cold-start with no network" — accurate for release binaries but false in dev-mode where the snapshot isn't embedded. Reworded to say so. Verified: typecheck clean; `--version` exits in ~1s cleanly; existing D12 phase-label test still passes. * fix(hygiene): [#1052 D8 review-fix follow-up] scrub example strings from scanner source The consensus review-fix for D8 (commit 0ffc42db97) added concrete tracker- key literals to the scanner's own test file and doc comments — e.g. inside `test.each([...])` fixtures, in the block comment above `RULES`, and in the pre-push hook comment. Those are the exact strings the scanner is meant to catch, so a pre-push run flagged them on this branch. That defeats the purpose: no example strings should appear as grep-visible literals in a repo whose whole rule is "don't put those literals here." Fix: - Test fixtures now build the strings at runtime from split prefix + digits (`const PREFIX = "A" + "I"`, `key(1234, "foo")` etc). The regex still sees what it needs to test; a source grep for the pattern finds nothing. - Scanner source comment loses its verbatim example strings — the intent is clear from the code + tests. - `.husky/pre-push` comment stops naming a specific pattern host; the scanner's own RULES are the source of truth. - Path-allowlist added in the same commit is no longer needed and dropped — keeps the guard strict for everyone. Verified: `bun test packages/opencode/test/skill/tracker-leak-check.test.ts` still 28/28 pass; scanner reports clean on the working tree. * fix(hygiene): [#1052 bot-review round] harden scanner + stamp + models.dev cache Addresses the actionable findings from the CodeRabbit + cubic-ai + kilo-code bot reviews on PR #1085 (Claude review skipped — not @claude-review'd). Only real issues; noisy or already-documented findings are noted in the PR reply. Scanner (`script/check-tracker-leaks.ts`): - **Cubic P1 (security):** the `--base` arg used to be embedded in `sh -c "${cmd}"`, which the shell then re-parsed — a caller could smuggle arbitrary shell via `--base=$(...)`. Replace with a shell-free `git(args[])` helper using Bun.$ tagged templates (each arg becomes one argv element, no shell). Belt-and-braces: reject `--base` values that don't look like a git ref (`^[A-Za-z0-9/_.@{}~^-]+$`) before running any git command, so a bad value fails loud instead of silently. - **Cubic P1 (correctness):** `shOK` used to `catch { return "" }`, so a real git failure (missing binary, corrupt index) reported as a clean scan. New helper `git()` fails loud with exit code 2 on unexpected errors; only `merge-base` (which legitimately returns empty on diverged history) opts into the silent path. - **CodeRabbit Major:** the diff parser's `!startsWith("+++")` filter dropped legitimate content lines starting with `++` (e.g. an added an added line whose text starts with two plus signs renders as `+++...` in unified-diff). Match the file header exactly (`+++ ` or `+++\t`) so a content line whose prefix happens to look like `+++<text>` still gets scanned. Build stamp (`packages/opencode/script/build.ts`): - **Kilo suggestion:** `_stampRoot` and `REPO_ROOT` computed the same value twice. Consolidated to one `REPO_ROOT` at the top of the block. - **Kilo suggestion:** stale comment claimed paths were "relative to the workspace root (dir = packages/opencode)" — post-M2 they're REPO_ROOT- relative. Corrected. - **Cubic P2:** `tsconfig.json` changes can flip target / moduleResolution and change the compiled output shape without editing any `.ts` file. Added `packages/opencode/tsconfig.json` to the stamp. models.dev cache (`packages/opencode/src/provider/models.ts`): - **CodeRabbit Minor:** a 2xx response can still carry HTML (proxies, error pages that respond 200) or truncated JSON. The old code wrote `result2.text` to the disk cache BEFORE the parse, so a bad body poisoned the cache for the next run + crashed on the current call. Now parses first; only caches + returns on success. On parse failure, log with a body preview and return an empty catalog (same graceful path as the non-2xx branch). Not addressed (deferred / disagreed / duplicated with known caveats): - Cubic P1 "cold-cache CLI still starts fetchApi → recreates D14 blocker" and kilo warning on the same line — these identify the trade-off the D14 review-fix commit explicitly documents; no change. - Cubic P1 "hook doesn't wire to `bun run script/check-tracker-leaks.ts`" — false positive; `.husky/pre-push` does invoke it. - CodeRabbit + cubic P2 "new files added post-build not detected" — real limitation, orthogonal fix, deferred to a follow-up (adding a walk at read-time would double the cost of every test run). Verified: typecheck 13/13; scanner self-test 28/28; scanner clean on this branch; scanner rejects `--base=$(...)` with exit 2; `--version` still exits in ~1s cleanly. * fix(hygiene): [#1052 bot-review round 2] address open bot findings Round 1 fixed 10 of the 22 bot comments; this covers the rest, plus two the round-1 fixes introduced. models.dev (coderabbit Major + kilo + cubic P1 — all three flagged it): - Remove the import-time `Promise.resolve().then(() => ModelsDev.refresh())`. The comment defending it argued a microtask cannot keep Bun alive. The microtask cannot, but the `fetch()` it starts can — and on a cold cache `refresh()` reaches `fetchApi()`, whose `AbortSignal.timeout` still cannot cancel a blocking `getaddrinfo()`. That is the D14 release-blocker shape, so the eager refresh is gone rather than rescheduled. Snapshot + on-demand load + the unref'd hourly interval remain. - Reject JSON that parses but is not a catalog object (`null`, `[]`, scalars — what a misconfigured proxy returns with a JSON content-type) before writing the disk cache, so the "don't poison the cache" intent holds for structured-but-wrong bodies too. Build stamp (cubic P2 x2, coderabbit Major, coderabbit Minor): - Stamp `packages/opencode/package.json`. The workspace walk skips `opencode`, so its own manifest was unstamped while a comment claimed otherwise. - Extract the input walk to `script/stamp-inputs.ts` so the smoke-test guard can re-enumerate with identical rules. Rehashing recorded inputs only catches CHANGED and DELETED files; a file ADDED after the build left every hash matching while the binary was stale. The stamp now records the walked roots and the guard treats an unrecorded file under them as stale. - Validate the stamp shape at runtime. `as BuildStamp` is a compile-time claim: `{"inputs":"x"}` passed the old length check and then threw in the loop. Tracker scanner (cubic P1 x2, cubic P2, cubic P3, coderabbit Major): - Scan the refs actually being pushed, read from the pre-push stdin, instead of always scanning HEAD. `git push origin dirty:other` previously approved a ref nobody had looked at. - A deletion-only push now scans nothing rather than falling back to HEAD. Found by the new end-to-end tests, not by a reviewer. - An unborn HEAD (`git init`, zero commits) no longer exits 2. That regression came from round 1's `failOnError: true` default and contradicted the documented brand-new-repo path. - A missing base ref says so on stderr instead of exiting silently, so a guard that has disabled itself is visible. Test harness (coderabbit Major): - `cli-process` SQLite scrub rethrows anything that is not ENOENT, so a permission or removal failure no longer lets the retry start on half-written state — the exact condition the scrub exists to prevent. Verification: 5 end-to-end tests drive the real script against throwaway repositories, including two real `git push` runs through an installed pre-push hook (clean branch pushes; a branch carrying a tracker ref is blocked). Three of them fail against the pre-fix scanner. Suites: 33 scanner, 580 provider, 218 install, 13/13 typecheck; lint warnings down from 15 to 13; markers verified "All blocks properly closed" repo-wide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(hygiene): [#1052 bot-review round 3] ref names, cache paths, stamp roots Round-3 findings, all on code round 2 introduced. Tracker scanner (coderabbit Major + cubic P1): - Scan the ref NAMES being pushed, both ends. Round 2 started scanning pushed ref CONTENT but still only ever scanned the checked-out branch's name, so `git push origin main:refs/heads/<tracker-key>` published a tracker-shaped branch while `main` and every line of its content were clean. The destination ref is the one that becomes public and need not match the source; tags go the same way. The checked-out branch name is now scanned only in the no-stdin fallback. models.dev cache (cubic P1 x2): - `refresh()` wrote 2xx bodies straight to the cache with no validation at all, so the hourly refresh could poison a cache the cold-fetch path was careful to protect. Both paths now share one validator, as does the disk-cache READ, so an entry poisoned by an older build is not trusted either. - That validator no longer accepts any object. An object-shaped error payload (`{"error": "rate limited"}`) passed the round-2 check. At least one value must now parse as a `Provider`. Deliberately "at least one" rather than "all": requiring all would let a single new upstream field empty the catalog, which is a worse failure than the one being prevented. Build stamp (coderabbit Minor x2, cubic P2, cubic P3): - Include `.mp3` in the shared input extensions. packages/tui imports its attention sounds with `{ type: "file" }`, so they are embedded in the binary, but the extension filter excluded them and a changed sound left the stamp fresh. - Record `packages/*/src` as a glob alongside the concrete roots. Concrete roots only describe packages that existed during the build, so a package added later was invisible to the guard. - Validate `roots`/`rootGlobs` like `inputs`. A non-array value threw a TypeError at module load inside `describe()`, failing the whole file instead of degrading to the mtime fallback — the opposite of the hardening round 2 added for `inputs`. Tests (cubic P3): - Remove the throwaway git repositories in `afterAll`. They carry tracker-key fixtures, so leaving them in tmp is worse than ordinary litter. - New end-to-end case for the destination-ref hole; it fails against the round-2 scanner. Verification: 7-case e2e matrix against real repositories (unborn HEAD, clean, content leak, pushed-ref content, no-stdin fallback, deletion-only push, tracker-shaped destination ref) all as expected; 34 scanner + 799 provider and install tests pass; 13/13 typecheck; no new lint warnings; markers balanced; scanner clean against its own diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(models): [#1052] structural catalog validation, not the zod schema Fixes a regression I introduced in round 3, caught by CI. Round 3 gated the models.dev cache on `Provider.safeParse`, as a review suggested. It looked right and was wrong: the schema requires a `Model.options` record that real models.dev entries do not carry, so it rejects 0 of the 144 providers in our own bundled snapshot. Nothing was ever cached, and every load fell through to a network fetch — which hangs in a sandboxed environment. The only signal was an unrelated CLI smoke test timing out at 60s ("fresh-DB production path adopts the core schema"). Bisected to round 3, then to this file. The validator is now structural: a non-empty map whose values are provider objects carrying a `models` map. That still rejects everything the reviewers were worried about — `null`, `[]`, scalars, and object-shaped error payloads like `{"error": "rate limited"}` — without coupling cache acceptance to a schema that does not match the payload. It moved to `provider/models-catalog.ts` (flat exports + self-reexport per AGENTS.md) so it is directly testable. Trying to cover it through `ModelsDev.get()` did not work: `Flag.OPENCODE_MODELS_PATH` is captured at module load, so a test cannot redirect the cache path afterwards. 12 new tests pin the behaviour, and the first one is the guard that was missing: the real bundled catalog must be accepted. `output-hygiene` is back to 3/3; provider, scanner and install suites total 811 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 freeto 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.
Summary
Fixes the actionable items from the v0.9.4 pre-release multi-persona review (CTO, PM, Data Engineer, Tech Lead, Support Engineer), plus all findings from the subsequent bot review (CodeRabbit + cubic) and the local consensus review (6 reviewers). Twelve original items; A5 dropped as false positive. Everything requiring more than a 30-min patch was deferred to tracking issue #1052.
Also deletes one flaky TUI e2e file (already CI-skipped) that was emitting noise on every local suite run.
Changes
Original persona-review actionables (A1–A12, 11 commits)
os.homedirspy inmcp/lifecycle.test.ts— root cause of the red TypeScript check on PR #1001.spyOnhad no restore, poisoning six tilde-expansion tests inpermission/next.test.tswhen bun loaded the next file.--target-parentnever existed; users hit "unknown flag". Later refined byB2(portable + ownership-safe wording).A4stderr echo of the authorize URL was later removed — seeM2— after review discovered the worker's stdio redirect swallowed it AND the TUI dialog already renders + copies the URL.)useConnected()clarified. Semantically identical to the pre-refactor code (later extracted to a shared predicate — seem8).~/altimate-sample-dbt/in the menu row; offer PEP 668 alternatives (pipx/uv/ venv) alongside plainpip.quickstart-new.mdrewritten: welcome panel + curated 6-provider picker + Altimate LLM Gateway top row; new Step 2.5 for the first-run scan gate; explicit note that the activation menu is chat text (interactive picker deferred to #1052 / D1).jaffle-shop/README.mdreordered: activation-menu path leads; bare-shell pip flow appendix.sample_setuperror metadata suffix sentinel (later reverted — seem6).writeJsonAtomicdocstring reflects that the call-site sweep is a tracked follow-up.phase-label.tui-e2e.test.ts— flaky locally (bootstrap-span duration races the PTY poll interval), alreadytest.skipin CI.Bot-review round (CodeRabbit + cubic, 4 commits)
startupInFlightpromise so concurrent OAuth flows all use the actually-bound port./Users/youplatform assumption; primary advice = re-run withoutsudo; secondary =sudo -E HOME="$HOME" altimate-codewithchownrecovery hint)./issues/1052inwriteJsonAtomicdocstring;duckdb-cliinstall line in sample README).mkdtempout ofos.homedirmock; retry shares timeout budget).Consensus-review round (6-reviewer local consensus, 4 commits)
pipx install dbt-duckdbanduv tool install dbt-duckdbexpose NOdbtbinary (the entry point ships withdbt-core, a dep). Users hit a second dead end after PEP 668. Fixed topipx install --include-deps dbt-duckdbanduv tool install dbt-core --with dbt-duckdb.stderr.writeof the authorize URL. The auth plugin runs inside a TUI worker whose stdio is redirected to the log file — the write reached the log, never the terminal. Also redundant (dialog-provider.tsx already renders + copies). Removed to stop leaking state-bearing URLs into logs.useConnectedgrandfather history; usesonce()+removeListeneron success too, so a later runtime server error isn't silently swallowed).console.warnon the retry path so a systematic regression stays visible.isBinaryStale()walkssrc/+script/for the newest source mtime instead of justscript/build.ts; catches the common "src edited" case, not just "build logic edited".suffix: -1sentinel insample_setup(the sentinel guarded an unreachable collision).isAnyProviderConnected()topackages/tui/src/util/connected.tssouse-connected.tsxandhome/tips.tsxcan't drift.~/altimate-sample-dbt/is the default, not unconditional).Deferred (tracked in issue #1052)
11 items, including the 3 new deferrals from bot + consensus review:
cli-process.run().Test plan
bun turbo typecheck— 13/13 workspaces greenpackages/opencodeCI-mode suite: CI run on HEAD reportspass=10994 fail=0(job 30607912838)permission/next.test.tstilde-expansion tests), which A1 root-fixesNote on the acp/permission local-only failures
Earlier drafts of this description attributed the 6 local
test/acp/permission.test.tsfailures to "a macOS scheduling flake absent on CI's Linux runner". That was wrong. Commit2cbc1e818croot-caused them:ALTIMATE_CLI_YOLO=truein the developer's shell env (direnv-managed) short-circuitedpackages/opencode/src/acp/permission.ts:57, auto-approving every permission and bypassing every assertion. Fixed by adding an env-hermetic guard in the test file that snapshots + restores the env var. Verified: 6/6 pass with and withoutALTIMATE_CLI_YOLO=truein the parent env.Related
Notes
main(not a feature branch).Summary by CodeRabbit
New Features
Bug Fixes
Documentation