|
| 1 | +# REVIEW.md — altimate-code |
| 2 | + |
| 3 | +TypeScript/Bun monorepo (Effect-based) — an `opencode` fork shipping the `altimate` CLI/TUI: deterministic SQL/dbt/lineage/FinOps tooling embeddable under any LLM agent, plus a GitHub Action for automated dbt PR review. |
| 4 | + |
| 5 | +## Goal: catch what CI cannot |
| 6 | + |
| 7 | +CI here enforces type-safety and marker presence, not runtime correctness of async/Effect flows, cross-platform behavior, or security-sensitive edges (permission checks, auth, path/HTML escaping). Focus review effort there. |
| 8 | + |
| 9 | +## Don't duplicate CI — never flag |
| 10 | + |
| 11 | +- **`bun turbo typecheck` / `tsc`** — pure type errors, unused type imports. If it compiles, don't relitigate types. |
| 12 | +- **Marker Guard** — presence/format of `altimate_change` marker comments is enforced by a dedicated CI job (`marker-guard` in `ci.yml`, see #872, #904), not at compile time. Don't flag a *missing* marker as a guess; CI will fail the PR. Do still flag *misuse* (see Focus Areas below) since Marker Guard checks presence, not correctness. |
| 13 | +- **`anti-slop.yml`** — advisory only (made non-blocking after #741 auto-closed legitimate PRs); don't treat its output as a merge gate. |
| 14 | +- **PR title/standards (`pr-standards.yml`)** — conventional-commit-style title checks are automated; don't nitpick title format. |
| 15 | +- Pre-existing issues in code the diff doesn't touch. |
| 16 | +- If a PR only touches `.claude/` skills, docs, or trivial config, skip deep review — see Comment style. |
| 17 | + |
| 18 | +## Evidence standard — no speculation |
| 19 | + |
| 20 | +Before flagging: trace the actual caller and the actual shipped surface. This repo carries a **parallel, unshipped "V2" runtime** (`packages/server` HttpApi, V2 projectors) alongside the shipped one (`packages/opencode/src/server`). Reviewers repeatedly had to correct findings that assumed V2 code was live. Always confirm which surface a change lands on before asserting impact. |
| 21 | + |
| 22 | +## Severity calibration |
| 23 | + |
| 24 | +- **Critical**: permission/auth bypass, unawaited Effect discarding a check, path/HTML injection, data loss on the shipped session/trace path, race conditions in shared session/worker state. |
| 25 | +- **Warning**: env/config resolution edge cases, cache invalidation gaps, retry/timeout values not justified by evidence, cross-platform assumptions (macOS-only paths, bash `set -u` crashes), test isolation leaks. |
| 26 | +- **Nit**: naming, comment clarity, minor duplication, non-shipped-surface cosmetics. |
| 27 | + |
| 28 | +## Focus areas — bug classes this repo actually ships |
| 29 | + |
| 30 | +1. **Effect values awaited without running them** (Critical). `ctx.ask(...)`-style helpers return an `Effect`, not a Promise; `await effectValue` awaits the *object*, never executes it — this shipped a real permission-check bypass (fixed via `await AppRuntime.runPromise(ctx.ask(...))`). **Flag when** an `Effect.promise`/`Effect.gen`-returning function is passed directly to `await`. **Verify** the function's return type first — Effect wrapping is easy to miss because it still type-checks. |
| 31 | + |
| 32 | +2. **Race conditions in session/worker/tool lifecycle** (Critical, ~12 fix commits). Concurrent access to shared state (session cancel/idle flags, worker/server stop, file writes, dispatcher registries) without ordering guarantees: `cancel()` race + idle state on normal loop exit (#845), worker/server `stop()` moved into `finally` so an invalid `--session` no longer leaks them, stale-file race + error misclassification (#611), DuckDB concurrent-access retry. **Flag when** new code mutates session/worker/cache state from more than one async path without a lock or `finally`-guaranteed cleanup. **Verify** cleanup runs on both success and error/cancel paths. |
| 33 | + |
| 34 | +3. **Env-var / config resolution correctness** (Warning→Critical, 8 fix commits). MCP config `${VAR}` interpolation needs full/recursive resolution, not single-pass; config-key normalization (`mcpServers`→`mcp`) and installer env injection have shipped broken more than once. **Flag when** a diff touches config loading or `${VAR}` interpolation. **Verify** resolution is applied everywhere the config is consumed, not just at one call site. |
| 35 | + |
| 36 | +4. **Fork-merge hygiene: `altimate_change` marker misuse** (Warning). Every line diverging from upstream `opencode` must sit inside an `altimate_change start/end` marker (100% coverage is the current bar — 98 files/407 blocks). Marker Guard checks *presence*, not *correctness*. **Flag**: (a) redundant new markers nested inside an already-marked block (previously false-positived, #904); (b) upstream-only APIs (`LanguageModelV3*`, `specificationVersion: "v3"`) leaking into code that intentionally stays on `@ai-sdk/provider@2.0.1`/`LanguageModelV2*` — should be reverted/bridged, not silently upgraded. |
| 37 | + |
| 38 | +5. **Auth/permission surface confusion (V2 vs shipped)** (Critical to flag correctly, Nit to over-flag). The shipped permission/OAuth routes live under `packages/opencode/src/server`; a structurally similar but **unshipped** V2 HttpApi exists under `packages/server`. **Flag when** a change weakens the shipped surface's check. **Verify first** which surface a route lives under — past "bugs" here were false positives against the dead V2 path. |
| 39 | + |
| 40 | +6. **Telemetry/trace data loss across turns** (Warning). Trace waterfall, summary prompt, and chat tab have each independently lost data after an agent turn (trace corruption #865/#867, diff totals not persisted to session summary). **Flag when** new trace/telemetry-read state isn't confirmed to be re-published after summarize/snapshot/restart. |
| 41 | + |
| 42 | +7. **dbt/warehouse connector correctness** (Warning, dbt=15 + snowflake/databricks/clickhouse — largest keyword cluster). dbt YAML file-kind classification, surfacing real dbt errors instead of generic "Could not parse", and warehouse-specific SQL (BigQuery `INFORMATION_SCHEMA`, Snowflake Cortex gating) have each shipped wrong. **Flag when** a diff edits a dbt file-kind regex or warehouse-specific SQL template. **Verify** file-kind matching handles `.py`/`.md` (not just `.sql`/`.yml`) and matches the most specific path segment, not the first match from root. |
| 43 | + |
| 44 | +8. **SQL/HTML injection in generated strings** (Critical, rare but severe). Hand-built SQL/HTML instead of parameterized binds/escaping: parameterized binds replaced `escapeSqlString` in finops/schema (#277); XSS fixed in `oauth-callback.ts`/`plugin/codex.ts` via `escapeHtml()`; symlink escape in `plugin/shared.ts` fixed via `Filesystem.containsReal` (resolves symlinks) instead of `.contains`. **Flag** any new SQL/HTML string interpolation of external input, or path-containment checks that don't resolve symlinks first. |
| 45 | + |
| 46 | +9. **Schema/config cache invalidation** (Warning). `Config.update()`/`updateGlobal()` needed to explicitly invalidate the per-directory `ScopedCache` (disposal alone didn't clear it); webfetch 404s were cached too long. **Flag** new cached derived values from config/fetch without an explicit invalidation path. |
| 47 | + |
| 48 | +10. **Cross-platform shell/path assumptions** (Nit→Warning, dense in `.claude/skills/*` review comments). Scripts hardcode macOS paths (Homebrew, Chrome.app, `java_home`), assume Bash ≥4.4 array expansion under `set -u`, or skip forcing UTF-8 locale. **Flag** `.sh` edits using `set -u` with array expansions, single-OS hardcoded paths with no fallback, or legacy `java -version` parsing. |
| 49 | + |
| 50 | +11. **Test isolation leaks under parallel CI** (Warning). `mock.module`/dispatcher `reset()` leaked state across test files (52 CI failures fixed via #460). **Flag** new tests using global mocks/dispatchers without confirmed teardown under parallel `bun test`. |
| 51 | + |
| 52 | +## Repo invariants & landmines |
| 53 | + |
| 54 | +- Effect (`effect` v4 beta) is used pervasively; `Effect.promise`/`Effect.sync`-returning functions look like normal async functions but must be run via `AppRuntime.runPromise` (or equivalent) — a bare `await` silently no-ops the effect. |
| 55 | +- Ships two SDK generations: `LanguageModelV2*` (`@ai-sdk/provider@2.0.1`, current) vs `V3` (upstream-only) — V3 types/fields must not appear in code on the shipped path. |
| 56 | +- `packages/opencode/src/server` is the shipped server; `packages/server` is a not-yet-shipped V2 HttpApi — don't assume the latter affects users. |
| 57 | +- The binary ships as `altimate` (npm exposes both `altimate` and `altimate-code` on PATH); dbt/dbt-tools and skills are bundled into the published binary, so unbundled-dependency regressions (e.g. `.node` binaries, `@altimateai/altimate-core`) break the *published* artifact even when `bun run` in dev works fine. |
| 58 | +- SQLite is the storage engine (`bun:sqlite`, migrated off `better-sqlite3`); schema is core-owned — a fresh DB may lack columns the fork's queries assume (`permission.data` was one such case). |
| 59 | + |
| 60 | +## Known-intentional — don't flag |
| 61 | + |
| 62 | +- `@ts-expect-error`/documented `as any` casts left in provider/TUI bridge code for known upstream SDK-shape mismatches during a bridge-merge PR (e.g. opentui `traits`, markdown `fg` prop) — these are intentionally temporary and explained inline. |
| 63 | +- Tying cleanup lifecycles to `server.instance.disposed` instead of an `Effect` scope — reviewers explicitly declined this refactor as changing instance-bootstrap semantics for no correctness gain. |
| 64 | + |
| 65 | +## High-blast-radius — never auto-edit |
| 66 | + |
| 67 | +`packages/opencode/src/server/**` core routing, `db.ts`/storage migration code, `install`/`install.ps1`, `.github/workflows/*.yml`, `patches/`, `bun.lock`, `packages/*/package.json` version pins, anything under `test/upstream/` (bridge-merge regression suite). |
| 68 | + |
| 69 | +## Comment style |
| 70 | + |
| 71 | +Exact-line comments, one finding each, most-severe first. Prefer a `suggestion` block over prose when the fix is a small diff. Skip deep review of doc-only, `.claude/` skill, and CHANGELOG-only diffs — a clean diff gets `lgtm`. |
0 commit comments