From a3be9aedd9f7e26fb85d92ae42bc99a5645b269a Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sat, 4 Jul 2026 20:51:09 -0700 Subject: [PATCH] docs: add REVIEW.md for KiloCode automated review guidance Built by mining 172 fix commits (18mo) and ~90 human PR review comments from this repo. Top focus areas: Effect values awaited without being run (shipped a real permission bypass), session/worker race conditions, altimate_change marker/fork-merge hygiene, V2-vs-shipped server surface confusion, and dbt/warehouse connector correctness. Co-Authored-By: Claude Opus 4.8 --- REVIEW.md | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 REVIEW.md diff --git a/REVIEW.md b/REVIEW.md new file mode 100644 index 000000000..7498c650d --- /dev/null +++ b/REVIEW.md @@ -0,0 +1,71 @@ +# REVIEW.md — altimate-code + +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. + +## Goal: catch what CI cannot + +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. + +## Don't duplicate CI — never flag + +- **`bun turbo typecheck` / `tsc`** — pure type errors, unused type imports. If it compiles, don't relitigate types. +- **Marker Guard** — presence/format of `altimate_change` marker comments is CI-enforced at compile time (see #872, #904). 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. +- **`anti-slop.yml`** — advisory only (made non-blocking after #741 auto-closed legitimate PRs); don't treat its output as a merge gate. +- **PR title/standards (`pr-standards.yml`)** — conventional-commit-style title checks are automated; don't nitpick title format. +- Pre-existing issues in code the diff doesn't touch. +- If a PR only touches `.claude/` skills, docs, or trivial config, skip deep review — see Comment style. + +## Evidence standard — no speculation + +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. + +## Severity calibration + +- **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. +- **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. +- **Nit**: naming, comment clarity, minor duplication, non-shipped-surface cosmetics. + +## Focus areas — bug classes this repo actually ships + +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. + +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. + +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. + +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. + +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. + +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. + +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. + +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. + +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. + +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. + +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`. + +## Repo invariants & landmines + +- 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. +- 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. +- `packages/opencode/src/server` is the shipped server; `packages/server` is a not-yet-shipped V2 HttpApi — don't assume the latter affects users. +- 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. +- 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). + +## Known-intentional — don't flag + +- `@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. +- 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. + +## High-blast-radius — never auto-edit + +`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). + +## Comment style + +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`.