feat: [AI-7520] Altimate LLM Gateway CLI signup + onboarding UX - #1001
feat: [AI-7520] Altimate LLM Gateway CLI signup + onboarding UX#1001saravmajestic wants to merge 22 commits into
Conversation
- Add an `oauth` `method:"auto"` to the Altimate auth plugin: bind a loopback server on `localhost:7317`, open the browser to the web authorize page, verify `state`, and save the gateway credential to `~/.altimate/altimate.json` - Surface `altimate-backend` first in provider selection (TUI + clack) with the "Recommended · best tool-calling · 10M free tokens" hint Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…uth success
- Add `WelcomePanel` boot box (readiness-aware tips + "What is Altimate Code")
on the home screen; first run is a welcoming panel, not an auto-opened modal
- Restructure the model picker into READY / NEEDS-SETUP; add the curated
`DialogModelWelcome` (top 5 providers + "Search all providers"), Big Pickle
fallback, and `useReady` / `markSetupComplete`
- `/connect` opens the curated picker
- Altimate LLM Gateway sign-in confirms inline ("Authentication successful") and
auto-closes (auto-selecting a model) instead of dropping into the model picker
- Don't force Google: land on the sign-up page and let the user choose
(drop `google_start`); reword the sign-in instruction
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds Altimate OAuth authentication, first-run onboarding and scan flows, credential-aware provider/model selection, readiness gates across the TUI, a packaged DuckDB starter sample with safe materialization, welcome/help presentation updates, logout support, and local installation tooling. ChangesAuthentication and onboarding
Starter sample and materialization
Welcome and navigation presentation
Repository support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TUI
participant AltimateAuthPlugin
participant AltimateApi
participant SampleSetupTool
participant Materializer
User->>TUI: Select Altimate sign-in
TUI->>AltimateAuthPlugin: authorize()
AltimateAuthPlugin->>AltimateApi: exchangeSocialToken()
AltimateApi-->>AltimateAuthPlugin: Return auth_token
AltimateAuthPlugin->>TUI: Save credentials and select model
TUI->>SampleSetupTool: Run sample_setup
SampleSetupTool->>Materializer: Materialize starter sample
Materializer-->>SampleSetupTool: Return path and status
Suggested reviewers: Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
|
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. |
- /auth: sign in to the Altimate LLM Gateway directly via the OAuth loopback, skipping the provider picker (new `DialogAltimateAuth`) - /logout: clear the stored gateway credential (`AltimateApi.clearCredentials`) and disconnect (dispose + bootstrap; the provider loader drops the now-stale auth-store entry on reload) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DialogModelWelcome now tags each row with its providerID (and modelID for Big Pickle) and compares against local.model.current(), rendering a bright-green ✓ plus a "· selected" note on the active provider — so the user can see at a glance that e.g. Altimate LLM Gateway is already selected. Bright green (diffHighlightAdded) is used because plain ANSI green renders dim in some terminals. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ebase safety) Shrink our footprint on the upstream opencode `dialog-model.tsx` so future upstream merges are easier to rebase: - Move `useReady`/`markSetupComplete`, `DialogModelWelcome`, and `DialogBigPickleConfirm` into a new altimate-owned `component/altimate-onboarding.tsx` (zero rebase surface — our file) - `dialog-model.tsx` (424 → 160 lines) now holds only pristine `useConnected` plus the `DialogModel` READY/NEEDS-SETUP restructure, fully wrapped in `altimate_change` markers so an upstream conflict is confined and human-resolvable - Repoint imports in `app.tsx`, `home.tsx`, `dialog-provider.tsx`, `welcome-panel.tsx` The onboarding file imports `DialogModel`/`useConnected` back from `dialog-model`, but only inside callbacks/JSX — the circular reference is runtime-only and safe. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Consensus code review — PR #1001
Independent reviews from 6 models (Claude, GPT 5.4 Codex, Kimi K2.5, MiniMax M2.7, Qwen 3.6, MiMo V2.5 Pro), converged and then re-validated line-by-line against the checked-out code.
Verdict: changes requested. The onboarding refactor and OAuth loopback design are well-structured (crypto-random state, CSRF state check, 0o600 credential file, cleanly isolated onboarding). But the new loopback auth server has correctness/reliability bugs and localhost security-hardening gaps — 7 major, 9 minor — all in the auth path. See inline comments.
Nits (not inlined): recent-models section dropped from the model picker; Big Pickle won't show a ✓ in the full picker after selection; error page doesn't window.close() like the success page; documented circular import between altimate-onboarding.tsx and dialog-model.tsx; silent open() failure (mitigated — the URL is still rendered); no ALTIMATE_CALLBACK_PORT override.
Positives: crypto-random state + CSRF check, 0o600 credentials, clean rebase-safe isolation of onboarding, reuse of existing provider onSelect handlers, reliable server teardown on the normal path.
Missing tests: the entire OAuth loopback flow (early callback, invalid/missing state, error branch, timeout, port collision, persistence) and the /auth · /logout · Big Pickle · mixed-row picker paths are untested.
| // Bind the port BEFORE opening the browser so the credential can | ||
| // only be delivered to this process. | ||
| const state = randomBytes(16).toString("hex") | ||
| await startCallbackServer() |
There was a problem hiding this comment.
The browser can hand back the login before we're listening for it
MAJOR — Race — pending is registered too late. startCallbackServer() binds the port and starts accepting requests here inside authorize(), but the module-level pending (state + resolve/reject) is only set later when the framework calls the returned callback() (waitForCallback, ~L114). If the browser redirects before callback() runs (an already-signed-in user gets an instant redirect, or any scheduling gap), /callback sees pending === undefined and rejects as "Invalid state — possible CSRF", silently dropping the credential. The singleton also can't support two concurrent /auth flows — the second overwrites the first.
Fix: register pending keyed by state (e.g. a Map) synchronously in authorize() before opening the browser; have callback() await that promise, and buffer an early callback by state until it's awaited.
There was a problem hiding this comment.
Fixed in e46fdd48b. registerPending(state) now runs synchronously inside authorize() before open(), keying the pending flow by the unguessable state. The /callback handler resolves/rejects by that state, so an instant redirect (already-signed-in user) is matched instead of dropped as CSRF. See altimate.ts:162-171.
| : undefined | ||
| if (model) local.model.set({ providerID: props.providerID, modelID: model }, { recent: true }) | ||
| setConnected(true) | ||
| setTimeout(() => dialog.clear(), 5000) |
There was a problem hiding this comment.
Backing out of the sign-in screen doesn't actually stop the sign-in
MAJOR — No cancellation/cleanup when the dialog is dismissed. Pressing esc (or unmounting) only calls dialog.clear(); it never cancels the pending callback, so the loopback server can stay open for ~5 minutes and a late browser callback can still mutate app state after dismissal. This success-path setTimeout(() => dialog.clear(), 5000) is also never retained or cleared on unmount, so it can fire later and dismiss an unrelated dialog opened in the meantime.
Fix: guard post-await UI updates with an onCleanup disposed flag, clearTimeout in onCleanup, and stop the callback server / abort the SDK callback when the dialog is dismissed.
There was a problem hiding this comment.
Addressed in f5596b40a (UI leak) + 40c80361c. AutoMethod now has an onCleanup/disposed guard and the success setTimeout is retained and cleared on unmount, so a late callback can no longer dismiss an unrelated dialog. The server-side half (eagerly cancelling the pending flow on dismiss) is tracked as NEW-3 below — the dismiss path can't reach the loopback pending map, so the 5-min timeout + callback()'s finally (stops the server when it's the last flow) is the backstop.
| let pending: Pending | undefined | ||
|
|
||
| async function startCallbackServer(): Promise<void> { | ||
| if (server) return |
There was a problem hiding this comment.
If port 7317 is busy, sign-in silently hangs forever with no retry
MAJOR — Port collision leaves a broken, silently-failing server. server is assigned by createServer() before listen(). On EADDRINUSE the listen promise rejects (L94-96) but server is never reset to undefined, so this if (server) return early-returns on the next attempt and the flow proceeds as if listening — the callback never arrives and the user waits out the 5-minute timeout with no message.
Fix: in the error handler set server = undefined (and close it), and surface a clear "port 7317 in use" error to the TUI. Consider an ephemeral port if the redirect contract allows.
There was a problem hiding this comment.
Fixed in e46fdd48b. On listen error we now reset server = undefined (clearing the if (server) return guard so a retry can re-bind) and throw an EADDRINUSE-specific message instead of hanging: "Port 7317 is already in use — close whatever is using it and try again." See altimate.ts:112-122.
| <h2>Signed in ✓</h2><p>You can return to your terminal.</p> | ||
| <script>setTimeout(()=>window.close(),1500)</script></body>` | ||
|
|
||
| const HTML_ERROR = (msg: string) => `<!doctype html><meta charset="utf-8"><title>Altimate Code</title> |
There was a problem hiding this comment.
The error page echoes attacker-controlled text straight into HTML
MAJOR — Reflected, unescaped input in the callback error page (localhost XSS). HTML_ERROR(msg) interpolates msg into HTML with no escaping, and the value ultimately comes from url.searchParams (the error branch at L57, which runs before the state check). Any page that drives the browser to http://localhost:7317/callback?error=<script>... during the auth window yields reflected XSS on the localhost origin. Impact is limited (transient page, no secrets/cookies there) but it's avoidable.
Fix: HTML-escape all reflected values, and validate state before handling the error branch.
There was a problem hiding this comment.
Fixed in e46fdd48b. Added escapeHtml() and applied it to the reflected value in HTML_ERROR (altimate.ts:20-25,37). The related root cause is also closed — state is now validated before the ?error= branch runs (see the abort-path comment), so untrusted input no longer reaches that branch without a valid state.
| }) | ||
|
|
||
| await new Promise<void>((resolve, reject) => { | ||
| server!.listen(CALLBACK_PORT, () => resolve()) |
There was a problem hiding this comment.
The sign-in callback server is reachable from the whole network, not just this machine
MAJOR — Callback server listens on all interfaces, not loopback. listen(CALLBACK_PORT) with no host binds to ::/0.0.0.0, so the auth callback server is reachable from the LAN — despite the comment claiming "the credential can only be delivered to this process." The 128-bit state still protects credential delivery, but the listener still processes remote requests (probing, plus the pre-state-check error abort below).
Fix: server!.listen(CALLBACK_PORT, "127.0.0.1", () => resolve()).
There was a problem hiding this comment.
Fixed in e46fdd48b. server.listen(CALLBACK_PORT, "127.0.0.1", ...) now binds loopback only, so the callback server is unreachable from the LAN. altimate.ts:109.
| // A provider is "ready" (usable now) when it has valid credentials: it is present | ||
| // in the live provider list with at least one model — and, for the free OpenCode | ||
| // provider, with at least one paid model (a Zen key entered). | ||
| function providerReady(id: string) { |
There was a problem hiding this comment.
A provider with unknown pricing can be treated as "ready" without a key
MINOR — providerReady("opencode") mishandles undefined cost. m.cost?.input !== 0 evaluates to true when cost is undefined, so OpenCode can be counted as "ready" (usable now) even without a Zen key entered.
Fix: m.cost?.input != null && m.cost.input !== 0. (Same defect exists in useConnected() — see the separate comment.)
There was a problem hiding this comment.
Fixed in f5596b40a. providerReady("opencode") now uses m.cost?.input != null && m.cost.input !== 0, so undefined cost is no longer treated as ready. The matching fix in useConnected() is in the same commit.
| // Persist to ~/.altimate/altimate.json — the provider loader | ||
| // reads this first (it carries the instance/tenant + api_url | ||
| // the generic auth.json store can't). | ||
| await AltimateApi.saveCredentials({ |
There was a problem hiding this comment.
Browser sign-in skips the tenant-name check that manual entry enforces
MINOR — OAuth callback skips the tenant-name validation the manual path enforces. The manual paste path runs validateCredentials() (tenant regex + API check) before saving; this OAuth path calls saveCredentials() directly on the callback instance. The source is the trusted web app, so risk is low, but the invariant differs between the two entry points.
Fix: validate instance against VALID_TENANT_REGEX before persisting, or document why the OAuth source is trusted.
There was a problem hiding this comment.
Fixed in f5596b40a. The OAuth path now validates AltimateApi.isValidInstanceName(creds.instance) against VALID_TENANT_REGEX before exchange/persist, matching the manual paste path's invariant. altimate.ts:195.
| // useConnected() (real credentials) via useReady(), it gates the first-run chat | ||
| // lock. Module-global so it is shared across the app and resets on every process | ||
| // launch (so a fresh relaunch is a clean fresh-user state). | ||
| const [setupComplete, setSetupComplete] = createSignal(false) |
There was a problem hiding this comment.
After signing out, the app still shows "you're all set" guidance
MINOR — setupComplete is never reset on /logout. This module-global signal is OR'd with connected() in useReady(). /logout clears credentials (so connected() → false) but leaves setupComplete === true, so useReady() stays true for the rest of the process. Verified scope: useReady() only drives tips and the update-toast (not the chat/prompt), so the real impact is that the post-logout panel keeps showing "ready" tips (/discover) instead of /connect until relaunch. (Reviewers split on severity; kept minor given the cosmetic impact.)
Fix (if desired): gate the post-logout tips on connected() rather than useReady(), or add a resetSetupComplete() called from /logout.
There was a problem hiding this comment.
Fixed in 9c481ecf8. /logout now calls resetSetupComplete(), so setupComplete is cleared alongside credentials and useReady() returns to the /connect guidance without a relaunch.
| res.end(body) | ||
| } | ||
|
|
||
| const error = url.searchParams.get("error") |
There was a problem hiding this comment.
Anyone on the network can cancel your sign-in without any secret
MAJOR — Unauthenticated ?error= can abort an in-progress sign-in. This error branch runs before state validation and rejects/clears the current pending flow. Combined with the all-interface bind above, any local or LAN requester can cancel a user's sign-in without knowing state — there's no CSRF protection on the abort path.
Fix: validate state before honoring the error branch (this also closes the reflected-XSS issue on the same branch).
There was a problem hiding this comment.
Fixed in e46fdd48b. state is now validated first — before the ?error= branch. An unknown/missing state returns 400 and can neither cancel an in-progress flow nor deliver anything, so the abort path is no longer reachable without the 128-bit secret. altimate.ts:76-89.
| // are used by the restructured DialogModel below. | ||
| import { markSetupComplete, DialogBigPickleConfirm } from "./altimate-onboarding" | ||
|
|
||
| export function useConnected() { |
There was a problem hiding this comment.
The same unknown-pricing bug also mislabels providers as connected
MINOR — useConnected() has the same undefined-cost defect as providerReady(). It treats OpenCode as connected when any model has cost?.input !== 0, which is also true for undefined cost. This one has a wider blast radius: it drives the suggested: !connected() flag on the /connect and /auth commands, not just the model picker.
Fix: apply the same != null && !== 0 guard here.
There was a problem hiding this comment.
Fixed in f5596b40a. useConnected() now applies the same m.cost?.input != null && m.cost.input !== 0 guard, so undefined cost no longer marks OpenCode connected (and the suggested: !connected() flag on /connect//auth is correct).
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Requesting changes — PR #1001
Consensus review by 6 models (Claude, GPT 5.4 Codex, Kimi K2.5, MiniMax M2.7, Qwen 3.6, MiMo V2.5 Pro), converged and re-validated line-by-line against the code. Formalizing the verdict from the inline review above (#pullrequestreview-4705485161).
7 major, 9 minor findings — all in the new OAuth loopback auth path. The design is sound (crypto-random state, CSRF check, 0o600 credentials, cleanly isolated onboarding), but the loopback server has correctness/reliability bugs and localhost hardening gaps that should be fixed before merge.
Blocking (major):
- Race —
pendingregistered after the server starts accepting requests; an instant redirect drops the credential (altimate.ts:140). - Dismissing the dialog doesn't cancel the flow; server +
setTimeoutleak (dialog-provider.tsx:209). - Port collision (
EADDRINUSE) leavesserverset → silent 5-min hang on retry (altimate.ts:43). - Reflected, unescaped
errorin the callback HTML — localhost XSS (altimate.ts:23). - Callback server binds all interfaces instead of
127.0.0.1(altimate.ts:94). - API key delivered in the URL query string (
altimate.ts:76). - Unauthenticated
?error=aborts an in-progress sign-in without knowingstate(altimate.ts:57).
Minor findings and fixes are in the inline comments. Nothing here blocks the onboarding UX direction — the issues are contained to auth-path correctness, security hardening, and error handling. Happy to walk through any of them.
Address sahrizvi's review on #1001: - Register the pending flow in a state-keyed Map synchronously in authorize() BEFORE opening the browser, and have callback() await that promise — an already-signed-in user's instant redirect is no longer dropped as CSRF, and concurrent /auth flows no longer clobber each other - Bind the callback server to 127.0.0.1 (was all-interfaces / LAN-reachable) - Validate `state` BEFORE honoring the `?error=` branch, so an unauthenticated request can't cancel an in-progress sign-in - HTML-escape reflected values in the callback error page (localhost XSS) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- altimate.ts: reset the callback server on a failed listen (EADDRINUSE no longer wedges the singleton) + surface the reason; validate the callback instance name before persisting; log the failure reason on the auth catch path - dialog-provider: DialogAltimateAuth onMount wrapped in try/catch (no more stuck "Starting sign-in…"); AutoMethod guards post-await updates + clears its auto-close timer on unmount (onCleanup); on connect-with-no-model, open the picker instead of faking a green ✓ - dialog-model: guard the favorite keybind against string (NEEDS-SETUP) rows; treat undefined model cost as not-paid in providerReady() and useConnected() - app.tsx /logout: try/catch with error toast + reset the setupComplete flag - altimate-onboarding: add resetSetupComplete() Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…oopback URL) The loopback now receives a short-lived `token` (login_token) instead of the raw key; callback() exchanges it via POST /auth/social/exchange (AltimateApi. exchangeSocialToken) for the auth_token and saves that. Single code path for both the Google and email/password connect flows. State validation, loopback bind, HTML escaping, and the pending-Map/one-time-server logic are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Follow-up review — PR #1001 (still requesting changes)
Consensus re-review of the fix commits (e46fdd4, f5596b4, 7e7ac7c) by 6 models. All 16 prior findings are resolved (15 fully, #2 partially) — thank you. The fixes surfaced new issues, and one is a blocker, so keeping this at request changes.
Blocker (top priority): the OAuth callback's api_url is trusted with no scheme/host validation, and the CLI POSTs the one-time login_token to it (altimate.ts:90 → client.ts exchangeSocialToken). Unlike the manual-paste path, the OAuth path validates neither scheme nor host. A malicious browser extension or a web-app open-redirect could exfiltrate the token + tenant. Fix: require https:// and allowlist known Altimate hosts before exchanging (except in an explicit dev mode).
The five inline comments below (NEW-2 … NEW-6) are the remaining items — NEW-2 is a quick correctness fix worth doing now; the rest are fast-follows. Once the api_url validation and NEW-2 land, this is good to go.
| const result = registerPending(state) | ||
|
|
||
| const webUrl = (process.env.ALTIMATE_WEB_URL || DEFAULT_WEB_URL).replace(/\/+$/, "") | ||
| const redirect = `http://localhost:${CALLBACK_PORT}/callback` |
There was a problem hiding this comment.
The browser is sent to localhost but we only listen on IPv4 — sign-in can miss
MAJOR — NEW-2 — loopback binds 127.0.0.1 but the redirect uses http://localhost. altimate.ts:106 binds IPv4 loopback only, while the redirect here is http://localhost:${CALLBACK_PORT}/callback. On hosts where localhost resolves to ::1 first, the browser callback can hit a closed IPv6 port. Browser Happy-Eyeballs fallback often masks this, but it's fragile.
Fix: use http://127.0.0.1:${CALLBACK_PORT}/callback in the redirect to match the bind (or bind both loopback families).
Flagged by: GPT (MAJOR)
There was a problem hiding this comment.
Fixed in 40c80361c (NEW-2). The redirect is now http://127.0.0.1:${CALLBACK_PORT}/callback to match the IPv4 loopback bind, so a localhost→::1 resolution can't hit a closed port. altimate.ts:176.
| await startCallbackServer() | ||
| // Register the pending flow BEFORE opening the browser so an instant | ||
| // redirect can be matched by state rather than dropped as CSRF. | ||
| const result = registerPending(state) |
There was a problem hiding this comment.
Backing out of sign-in leaves the server and pending flow running for 5 minutes
MINOR — NEW-3 (residual of prior #2) — dismissing the dialog doesn't cancel the server-side flow. The onCleanup+disposed guard fixes the UI leak, but nothing rejects this eagerly-created registerPending promise on dismiss, so callback() keeps awaiting, the loopback server stays up, and the pending entry lingers until the 5-minute timeout.
Fix: on dismiss, reject/cancel the pending flow by state (and stop the server if it was the last one). Also attach a no-op .catch() to this registerPending promise so a never-awaited rejection can't become an unhandled rejection.
Flagged by: Kimi (MAJOR), MiMo, Claude — consensus
There was a problem hiding this comment.
Partially addressed in 40c80361c (NEW-3). Added void result.catch(() => {}) so the eagerly-created registerPending promise can't surface as an unhandled rejection when the dialog is dismissed. The eager reject-by-state on dismiss isn't feasible from here: the TUI dismiss path never receives state, so it can't reach the loopback pending map. The backstop remains the 5-min timeout (rejects the pending entry) and callback()'s finally (stops the server when it was the last flow). Happy to open a follow-up if we want true immediate cancel — it'd require threading state back out to the dialog.
| // Log the reason (CSRF / timeout / invalid instance / …). Runs in the | ||
| // server process, so this goes to the log, not the TUI display. | ||
| console.error("[altimate] gateway sign-in failed:", err instanceof Error ? err.message : err) | ||
| return { type: "failed" } |
There was a problem hiding this comment.
When sign-in fails, the user sees a generic error with no reason
MINOR — NEW-4 (UX) — auth/callback failures are opaque to the user. The plugin callback() logs the reason via console.error but returns bare { type: "failed" }; OauthCallbackFailed carries no message (auth-service.ts), so the TUI shows a generic failure with no reason (port busy, timeout, exchange 401, invalid instance).
Fix: thread a reason string through the failed result (or surface it via toast.error in the callback path).
Flagged by: MiniMax (rated CRITICAL), GPT, Kimi, MiMo — consensus
There was a problem hiding this comment.
Fixed in 40c80361c (NEW-4). AutoMethod now calls toast.error(...) on result.error instead of clearing silently, so port-busy/timeout/exchange-401/invalid-instance failures are visible in the TUI. The precise reason is still logged server-side via console.error.
| }[provider.id], | ||
| // altimate_change end | ||
| category: provider.id in PROVIDER_PRIORITY ? "Popular" : "Other", | ||
| async onSelect() { |
There was a problem hiding this comment.
The recommended /connect path can still hang on a startup failure
MINOR — NEW-5 (UX) — /connect welcome path lacks the try/catch that /auth got. The onboarding fix wrapped DialogAltimateAuth, but the recommended /connect path routes through this createDialogProviderOptions().onSelect → oauth.authorize() (just below, ~L95) with no equivalent guard, so a port-collision/startup failure there still fails without the new toast.
Fix: wrap the OAuth authorize block in createDialogProviderOptions() with the same toast/clear behavior as DialogAltimateAuth.
Flagged by: GPT (MAJOR)
There was a problem hiding this comment.
Fixed in 40c80361c (NEW-5). The OAuth authorize block in createDialogProviderOptions() (the /connect path) is now wrapped in try/catch with the same toast.error(...) + dialog.clear() behavior as DialogAltimateAuth.
| } | ||
|
|
||
| entry.resolve({ api_url: apiUrl, instance, token }) | ||
| html(200, HTML_SUCCESS) |
There was a problem hiding this comment.
The browser says "Signed in" before we've actually finished signing in
MINOR — NEW-6 — browser shows "Signed in ✓" before the token exchange/persist succeeds. The loopback returns HTML_SUCCESS here as soon as it receives the token; if exchangeSocialToken()/save then fails, the browser says success while the terminal reports failure.
Fix: use neutral "authorization received, return to terminal" copy, or delay success until exchange + persist succeed.
Flagged by: GPT, MiniMax
There was a problem hiding this comment.
Fixed in 40c80361c (NEW-6). HTML_SUCCESS is now neutral — "Authorization received / Return to your terminal to finish connecting." — so the browser no longer claims success before the exchange + persist complete. The terminal remains the source of truth for actual success/failure.
- NEW-2: callback redirect uses http://127.0.0.1 to match the loopback bind (a plain `localhost` redirect can resolve to ::1 and hit a closed port) - NEW-6: neutral browser copy ("Authorization received") — the loopback replies before the CLI has exchanged/persisted, so it must not claim "Signed in" - NEW-5: guard the /connect OAuth authorize path with try/catch → toast + clear (parity with DialogAltimateAuth; port-collision no longer fails silently) - NEW-4: AutoMethod surfaces a toast on callback failure instead of clearing silently (the precise reason is logged server-side) - NEW-3: swallow a never-awaited pending rejection (void result.catch) to avoid an unhandled rejection when the dialog is dismissed before callback() runs Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The welcome/top-5 picker already leads with the Altimate LLM Gateway. Order the full DialogModel list the same way: export `PROVIDER_PRIORITY` and sort the READY section's providers by it (the NEEDS-SETUP section already used it), so the gateway leads whether or not it is connected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve conflicts from upstream's TUI extraction (packages/opencode/src/cli/cmd/tui → packages/tui) and fork-features-as-plugins re-architecture, re-applying the AI-7520 CLI onboarding onto the new structure: - dialog-provider: export/reorder PROVIDER_PRIORITY (Altimate LLM Gateway first), brand the gateway option, add WARNLIST + DialogAltimateAuth, harden the OAuth authorize/AutoMethod UX (try/catch → toast, inline green success + auto-close) - dialog-model: keep the READY / NEEDS-SETUP restructure + Big Pickle, wired through upstream's new DialogVariant flow; gateway leads the full list - use-connected + home tips: fix undefined-cost mislabeling OpenCode as connected - app.tsx: /connect (welcome picker), /auth (gateway), /logout (plugin command + resetSetupComplete); gate the update-available dialog on onboarding readiness - home.tsx: render the WelcomePanel in the home_logo slot - provider-credentials plugin: add altimate.provider.logout - providers.ts: Altimate LLM Gateway first in the login picker - relocate altimate-onboarding + welcome-panel into packages/tui with relative imports Verified: `bun run typecheck` green in packages/tui and packages/opencode; the onboarding/provider/model/app-lifecycle tests pass. Pre-existing sync*.test.tsx failures come from origin/main's own merge (files untouched here). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cherry-pick two chunks from plg-onboarding-prototype-v2, re-applied onto the relocated packages/tui (excludes the prototype stub-server, device-flow, demo flags, and Part 3 sample-env per scope): Part 2 — scan gate: - dialog-scan-gate.tsx: one-time "Scan your environment?" Yes/No gate - register hidden `onboard-connect` command + Part-2-only template (hands off to the existing /discover; discover.txt unchanged) - app.tsx: fire the gate exactly once on a genuine not-ready→ready transition, wired to the existing onboardingReady/useReady signal TUI polish: - prompt/index.tsx: Claude-style input bar (thin rules, `›`, meta row below); first-run submit opens the welcome picker instead of erroring; ⚠ unreliable model chip driven by WARNLIST - autocomplete.tsx + command-palette.tsx: first-run filtering to local/safe commands until a model is ready; hide onboard-connect from the slash menu - sidebar footer: JTBD "here's what you can do" panel + community/docs links, dotted dividers; sidebar left border Verified: bun run typecheck green in packages/tui and packages/opencode; onboarding/provider/model/app-lifecycle tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- welcome-panel: drop the "Tips for getting started" section; the header now shows only "What is Altimate Code" with updated positioning copy. - app: auto-open the curated provider picker (DialogModelWelcome) on a fresh launch with no usable model — the first-run onboarding entry point. Fires exactly once and only after startup settles (`ready()`), so a returning user with valid credentials never sees it; chat input stays visible with submit gated in the prompt until setup completes. Replaces the removed "/connect" tip as the way first-run users are guided to connect. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Previously "No" fired `/onboard-connect skip`, which had the agent post an intent question. Instead, declining now simply closes the gate and drops the user into the empty chat input — the gate already clears itself, so the app-level handler treats `skip` as a no-op and only "Yes" starts the scan flow. - `app.tsx`: onChoose ignores `skip`; only `scan` submits the command - `onboard-connect.txt`: drop the now-unreachable skip branch and the `$ARGUMENTS` dispatch — the template is scan-only - add interaction coverage for `DialogScanGate` (copy renders; `y`→scan, `n`→skip, Enter on default Yes→scan) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Marker Guard flags altimate additions to upstream-shared files that sit outside `altimate_change start … end` blocks (single-line `// altimate_change —` comments do not cover the following line). Wrap each flagged region so the guard passes and the code survives upstream merges: - app.tsx: `connected` + `onboardingReady` readiness signals - dialog-provider.tsx: move `end` past the PROVIDER_PRIORITY close brace - prompt/autocomplete.tsx, prompt/index.tsx: `useReady` gate + WARNLIST chip - use-connected.tsx: undefined-cost "not paid" fix - home/tips.tsx: connected() undefined-cost fix - sidebar/footer.tsx: rewritten View signature + Dotted helper - routes/session/sidebar.tsx: left-edge border attributes Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Second Marker Guard pass — wrap the next uncovered altimate regions: - app.tsx: /connect command entry (curated welcome picker) - dialog-provider.tsx: authorize try/catch guard - prompt/autocomplete.tsx: first-run `if (ready())` server-command gate - sidebar/footer.tsx: sidebar_footer slot registration Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Third Marker Guard pass: - app.tsx: onboarding-aware update-available dialog gate - dialog-provider.tsx: `const local = useLocal()` in AutoMethod (sets the connected model as the post-connect default) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wrap the entire AutoMethod success/auto-close flow (inline green confirm + model auto-select instead of opening the picker) in one outer altimate_change block; the existing inner markers nest. Clears the last Marker Guard finding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…MATE_API_URL) The social token exchange used the browser callback's `url` param or, when absent, fell back to the hardcoded prod API base — so in local/dev (where the web has no BACKEND_API_URL to deliver) the CLI exchanged the token against prod, which can't verify a locally-minted token, and the sign-in failed with a generic UnknownError. Honor `ALTIMATE_API_URL` before the prod fallback (mirrors the existing `ALTIMATE_WEB_URL` override), so the exchange can be pointed at a local backend for dev/staging. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
packages/tui/test/cli/cmd/tui/provider-options.test.ts (1)
17-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case covering the new
altimate-backendbranding/ordering.The ordering update is correct for the new
PROVIDER_PRIORITY, but nothing pins the behavior this PR actually introduces:altimate-backendsorting to the front and being retitled "Altimate LLM Gateway" with its description. A one-liner keeps that from regressing on the next rebase.💚 Suggested test
test("does not collide with a configured provider named other", () => {test("puts the Altimate LLM Gateway first and rebrands it", () => { const options = providerOptions([ { id: "openai", name: "OpenAI" }, { id: "altimate-backend", name: "altimate-backend" }, ]) expect(options[0]).toMatchObject({ value: "altimate-backend", title: "Altimate LLM Gateway", category: "Popular", }) })🤖 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/test/cli/cmd/tui/provider-options.test.ts` around lines 17 - 30, Add a focused test alongside the existing providerOptions ordering test that includes altimate-backend with openai, then assert altimate-backend is first and is rebranded with value "altimate-backend", title "Altimate LLM Gateway", and category "Popular".packages/opencode/src/altimate/api/client.ts (1)
199-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider parsing the exchange response with zod and sharing the timeout helper.
exchangeSocialTokenandvalidateCredentialsnow duplicate the sameAbortController+setTimeout(15_000)+.finally(clearTimeout)boilerplate, and the response is only cast (as { auth_token?: string }) rather than validated like the rest of this module does with zod. A smallfetchWithTimeout()helper plusz.object({ auth_token: z.string().min(1) }).parse(await res.json())would keep behavior identical while removing the cast and the copy.Also note
res.text()/res.json()run after the timer is cleared, so a stalled body read is still unbounded — fine for these small payloads, but worth knowing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/api/client.ts` around lines 199 - 226, Refactor exchangeSocialToken and validateCredentials to use a shared fetchWithTimeout helper for the 15-second AbortController timeout and cleanup, removing duplicated timeout boilerplate while preserving request behavior. In exchangeSocialToken, replace the auth_token type cast with zod validation using an object schema requiring a non-empty string, consistent with the module’s existing validation patterns.packages/opencode/src/plugin/tui/altimate/provider-credentials.tsx (1)
87-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePartial failure reports as "Sign-out failed" even though credentials were deleted.
clearCredentials()is irreversible; ifinstance.dispose()then throws, the toast tells the user sign-out failed while the credential file is already gone (and the in-memory provider stays live). Separating the two steps makes the message accurate.♻️ Suggested split
async function logout(api: TuiPluginApi) { try { await AltimateApi.clearCredentials() - await api.client.instance.dispose() - api.ui.toast({ variant: "success", message: "Signed out of Altimate LLM Gateway" }) } catch (err) { api.ui.toast({ variant: "error", message: err instanceof Error ? err.message : "Sign-out failed" }) + return } + try { + await api.client.instance.dispose() + } catch { + api.ui.toast({ variant: "warning", message: "Signed out — restart to fully reload providers" }) + return + } + api.ui.toast({ variant: "success", message: "Signed out of Altimate LLM Gateway" }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/plugin/tui/altimate/provider-credentials.tsx` around lines 87 - 95, Update logout so AltimateApi.clearCredentials() completes in its own error-handling path and reports credential deletion accurately, then attempt api.client.instance.dispose() separately without labeling the already-completed sign-out as failed. Preserve success feedback when both steps succeed and provide a distinct warning or error message when disposal alone fails.packages/tui/src/component/prompt/autocomplete.tsx (1)
457-482: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNested
altimate_changemarker inside an already-marked block.Lines 462-464 open a new
altimate_change start/endpair that is fully nested inside the outeraltimate_change start/endblock spanning 457-482. As per coding guidelines, "Keep altimate_change markers non-redundant; do not nest new markers inside an already-marked block."♻️ Suggested fix
if (ready()) { for (const serverCommand of sync.data.command) { if (serverCommand.source === "skill") continue - // altimate_change start — keep one autocomplete row per slash command name + // keep one autocomplete row per slash command name if (localSlashNames.has(serverCommand.name)) continue - // altimate_change end // altimate_change — onboard-connect is invoked programmatically by the Part 2🤖 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/component/prompt/autocomplete.tsx` around lines 457 - 482, Remove the nested altimate_change start/end markers around the localSlashNames check in the ready() command loop, while preserving the check itself and the outer marker pair surrounding the server-command block.Source: Coding guidelines
packages/tui/src/component/use-connected.tsx (1)
6-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated "is provider paid/connected" predicate across files — extract a shared helper.
The same
undefined-vs-0bug fix formodel.cost?.inputhad to be applied independently in both of these files (and, per surrounding graph context, appears duplicated again indialog-provider.tsx/dialog-model.tsx's ownuseConnected/providerReady). Copy-pasted business logic like this is exactly why the fix had to land in multiple places at once instead of one.
packages/tui/src/component/use-connected.tsx#L6-L15: extract theprovider.id !== "opencode" || Object.values(provider.models).some(m => m.cost?.input != null && m.cost.input !== 0)predicate into a small exported pure function (e.g.isProviderReady(provider)) that other Solid components (dialog-provider.tsx,dialog-model.tsx) can import instead of re-implementing.packages/tui/src/feature-plugins/home/tips.tsx#L48-L57: since this runs againstapi.state.provider(TuiPluginApi) rather thansync.data.provider, have this call the same extracted pure predicate (parameterized on a provider/model shape) instead of re-declaring the check inline, so future fixes only need to land once.🤖 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/component/use-connected.tsx` around lines 6 - 15, The provider readiness predicate is duplicated across connected-state checks. In packages/tui/src/component/use-connected.tsx lines 6-15, extract and export a pure isProviderReady-style helper accepting the shared provider/model shape and preserving the null-safe nonzero input-cost condition; update the local useConnected logic to call it, and update dialog-provider.tsx/dialog-model.tsx consumers to import it. In packages/tui/src/feature-plugins/home/tips.tsx lines 48-57, replace the inline predicate with the same helper, adapting its provider shape as needed so future readiness fixes are centralized.packages/tui/src/routes/home.tsx (1)
30-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the orphaned onboarding hint export.
HomeFirstTimeOnboardingHintis the only remaining reference to this helper, while the home route renders throughWelcomePanelinstead. Drop the function and its associated comment to avoid leaving dead, misleading code behind.🤖 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/routes/home.tsx` around lines 30 - 52, Remove the HomeFirstTimeOnboardingHint function and its surrounding altimate_change comments from the home route module, leaving the WelcomePanel-based rendering flow unchanged.
🤖 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/src/altimate/plugin/altimate.ts`:
- Around line 94-104: Validate the callback-supplied URL before resolving the
entry in the callback handler: allow only HTTPS URLs or the configured
DEFAULT_API_URL/ALTIMATE_API_URL hosts, and fall back to the trusted default or
reject invalid values. Ensure exchangeSocialToken receives only the validated
apiUrl, preventing the token from being posted to an arbitrary host.
- Around line 62-64: Update startCallbackServer to track an in-flight startup
promise so concurrent authorize flows await the same listen result instead of
returning when server is merely created. Assign server only after listen
succeeds, clear the startup promise on both success and failure, and also reset
it in stopCallbackServer while preserving existing startup error propagation.
In `@packages/tui/src/app.tsx`:
- Around line 580-592: Update the DialogScanGate onChoose handler so a non-skip
choice is not silently discarded when promptRef.current is unavailable. When the
ref is null, surface a toast directing the user to /discover (or defer the
submission until the prompt ref mounts), while preserving the existing ref.set
and ref.submit flow when the ref exists.
- Around line 558-565: Update the first-run picker effect guarded by
firstRunPickerHandled to wait for sync.status === "complete" instead of relying
on ready(). Preserve the existing one-shot guard and onboardingReady() bypass,
and only set firstRunPickerHandled and replace the dialog after synchronization
completes.
- Around line 809-821: Update the run handler for the altimate.logout command to
clear the command palette dialog before or alongside dispatching
altimate.provider.logout. Preserve the existing resetSetupComplete behavior and
ensure the palette closes after /logout executes.
In `@packages/tui/src/component/altimate-onboarding.tsx`:
- Around line 62-68: Update connectProvider to fall back to the full provider
catalog when providers() has no matching option or its onSelect is unavailable,
while preserving the curated option’s handler when present. Invoke the resolved
provider action so selecting a curated row never silently does nothing.
In `@packages/tui/src/component/dialog-provider.tsx`:
- Around line 288-319: Update DialogAltimateAuth’s onMount flow to track
disposal and guard all post-await UI actions, including toast, dialog.replace,
and dialog.clear, so dismissing the dialog while authorize is pending cannot
resurrect or alter the current UI. Replace the hardcoded method: 0 in
sdk.client.provider.oauth.authorize with the index of the entry whose type is
"oauth" from sync.data.provider_auth["altimate-backend"], reusing the existing
sync data hook/context.
In `@packages/tui/src/component/prompt/index.tsx`:
- Around line 1047-1059: Update submitInner() so /connect command input is
dispatched to the registered provider.connect command before the !ready()
first-run gate executes. Preserve the existing DialogModelWelcome, input
cleanup, and false-return behavior for other unready normal messages, while
ensuring a trailing-space /connect invocation reaches provider.connect.
In `@packages/tui/src/component/welcome-panel.tsx`:
- Around line 30-44: Update WelcomePanel’s fixed 65-column wordmark layout to
respond to terminal width using useTerminalDimensions(). Below an appropriate
narrow-terminal threshold, collapse or stack the left panel (and ensure it no
longer reserves 65 columns), while preserving the existing side-by-side layout
on wider terminals so the onboarding content and CTA remain usable.
In `@replace.sh`:
- Line 15: Update replace.sh so the current-platform build does not hard-code
the darwin-arm64 artifact: derive DIST_BIN from the platform/architecture
selected by the build flow in packages/opencode/script/build.ts, and execute
codesign only when the host target is Darwin. Preserve the existing artifact
existence validation and add an explicit unsupported-platform fallback if a
target cannot be resolved.
- Around line 34-35: Update the installation flow around the rm/cp commands in
replace.sh to create the parent directory of $DEST before copying $DIST_BIN.
Ensure the directory creation succeeds for fresh installations, then preserve
the existing removal and copy behavior.
---
Nitpick comments:
In `@packages/opencode/src/altimate/api/client.ts`:
- Around line 199-226: Refactor exchangeSocialToken and validateCredentials to
use a shared fetchWithTimeout helper for the 15-second AbortController timeout
and cleanup, removing duplicated timeout boilerplate while preserving request
behavior. In exchangeSocialToken, replace the auth_token type cast with zod
validation using an object schema requiring a non-empty string, consistent with
the module’s existing validation patterns.
In `@packages/opencode/src/plugin/tui/altimate/provider-credentials.tsx`:
- Around line 87-95: Update logout so AltimateApi.clearCredentials() completes
in its own error-handling path and reports credential deletion accurately, then
attempt api.client.instance.dispose() separately without labeling the
already-completed sign-out as failed. Preserve success feedback when both steps
succeed and provide a distinct warning or error message when disposal alone
fails.
In `@packages/tui/src/component/prompt/autocomplete.tsx`:
- Around line 457-482: Remove the nested altimate_change start/end markers
around the localSlashNames check in the ready() command loop, while preserving
the check itself and the outer marker pair surrounding the server-command block.
In `@packages/tui/src/component/use-connected.tsx`:
- Around line 6-15: The provider readiness predicate is duplicated across
connected-state checks. In packages/tui/src/component/use-connected.tsx lines
6-15, extract and export a pure isProviderReady-style helper accepting the
shared provider/model shape and preserving the null-safe nonzero input-cost
condition; update the local useConnected logic to call it, and update
dialog-provider.tsx/dialog-model.tsx consumers to import it. In
packages/tui/src/feature-plugins/home/tips.tsx lines 48-57, replace the inline
predicate with the same helper, adapting its provider shape as needed so future
readiness fixes are centralized.
In `@packages/tui/src/routes/home.tsx`:
- Around line 30-52: Remove the HomeFirstTimeOnboardingHint function and its
surrounding altimate_change comments from the home route module, leaving the
WelcomePanel-based rendering flow unchanged.
In `@packages/tui/test/cli/cmd/tui/provider-options.test.ts`:
- Around line 17-30: Add a focused test alongside the existing providerOptions
ordering test that includes altimate-backend with openai, then assert
altimate-backend is first and is rebranded with value "altimate-backend", title
"Altimate LLM Gateway", and category "Popular".
🪄 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: 739d66e6-5a0c-4d22-99ae-43d118f0756c
📒 Files selected for processing (26)
.github/meta/merge-commit.txtpackages/dbt-tools/test/fixture/.user.ymlpackages/opencode/src/altimate/api/client.tspackages/opencode/src/altimate/plugin/altimate.tspackages/opencode/src/cli/cmd/providers.tspackages/opencode/src/command/index.tspackages/opencode/src/command/template/onboard-connect.txtpackages/opencode/src/plugin/tui/altimate/provider-credentials.tsxpackages/tui/src/app.tsxpackages/tui/src/component/altimate-onboarding.tsxpackages/tui/src/component/command-palette.tsxpackages/tui/src/component/dialog-model.tsxpackages/tui/src/component/dialog-provider.tsxpackages/tui/src/component/dialog-scan-gate.tsxpackages/tui/src/component/prompt/autocomplete.tsxpackages/tui/src/component/prompt/index.tsxpackages/tui/src/component/use-connected.tsxpackages/tui/src/component/welcome-panel.tsxpackages/tui/src/feature-plugins/home/tips.tsxpackages/tui/src/feature-plugins/sidebar/footer.tsxpackages/tui/src/routes/home.tsxpackages/tui/src/routes/session/index.tsxpackages/tui/src/routes/session/sidebar.tsxpackages/tui/test/cli/cmd/tui/provider-options.test.tspackages/tui/test/cli/tui/dialog-scan-gate.test.tsxreplace.sh
| async function startCallbackServer(): Promise<void> { | ||
| if (server) return | ||
| server = createServer((req, res) => { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Second concurrent authorize() returns before the server is listening.
server is assigned synchronously by createServer() and only then awaited on listen(). A second authorize() that runs during that await sees server truthy and returns immediately from startCallbackServer(), so it opens the browser while the socket may not be accepting yet (and it never learns about an EADDRINUSE/startup failure, which the first caller reports by resetting server = undefined — leaving the second flow to wait out the 5-minute timeout).
Store the in-flight startup promise and await it, so all callers share one readiness result.
🔒️ Proposed fix: share the startup promise
-let server: ReturnType<typeof createServer> | undefined
+let server: ReturnType<typeof createServer> | undefined
+let starting: Promise<void> | undefined async function startCallbackServer(): Promise<void> {
- if (server) return
- server = createServer((req, res) => {
+ if (starting) return starting
+ starting = (async () => {
+ const created = createServer((req, res) => {…assign server = created only after listen resolves, and clear starting = undefined in both the success and failure paths (and in stopCallbackServer()).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/src/altimate/plugin/altimate.ts` around lines 62 - 64,
Update startCallbackServer to track an in-flight startup promise so concurrent
authorize flows await the same listen result instead of returning when server is
merely created. Assign server only after listen succeeds, clear the startup
promise on both success and failure, and also reset it in stopCallbackServer
while preserving existing startup error propagation.
| dialog.replace(() => ( | ||
| <DialogScanGate | ||
| onChoose={(arg) => { | ||
| // No → the gate already cleared itself; just drop the user into the | ||
| // empty chat input. Only Yes kicks off the scan flow. | ||
| if (arg === "skip") return | ||
| const ref = promptRef.current | ||
| if (!ref) return | ||
| ref.set({ input: `/onboard-connect ${arg}`, parts: [] }) | ||
| ref.submit() | ||
| }} | ||
| /> | ||
| )) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scan choice is silently dropped when the prompt ref isn't mounted.
scanGateShown is already true, so if promptRef.current is null (e.g. still on a route without a mounted prompt) the user's "Yes" vanishes with no feedback and the gate never returns. Surface a toast pointing at /discover, or defer submission until the ref resolves.
🤖 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/app.tsx` around lines 580 - 592, Update the DialogScanGate
onChoose handler so a non-skip choice is not silently discarded when
promptRef.current is unavailable. When the ref is null, surface a toast
directing the user to /discover (or defer the submission until the prompt ref
mounts), while preserving the existing ref.set and ref.submit flow when the ref
exists.
| { | ||
| name: "altimate.logout", | ||
| title: "Sign out of Altimate LLM Gateway", | ||
| slashName: "logout", | ||
| run: () => { | ||
| // altimate_change — the credential clear + instance dispose + toast happen | ||
| // opencode-side (AltimateApi is unreachable from packages/tui); see | ||
| // packages/opencode/src/plugin/tui/altimate/provider-credentials.tsx. | ||
| keymap.dispatchCommand("altimate.provider.logout") | ||
| resetSetupComplete() | ||
| }, | ||
| category: "Provider", | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
/logout leaves the palette dialog open.
Every other terminal palette action either dialog.clear()s or replaces the dialog; this one does neither, so the command palette stays on screen after signing out.
🛠️ Proposed fix
run: () => {
keymap.dispatchCommand("altimate.provider.logout")
resetSetupComplete()
+ dialog.clear()
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| name: "altimate.logout", | |
| title: "Sign out of Altimate LLM Gateway", | |
| slashName: "logout", | |
| run: () => { | |
| // altimate_change — the credential clear + instance dispose + toast happen | |
| // opencode-side (AltimateApi is unreachable from packages/tui); see | |
| // packages/opencode/src/plugin/tui/altimate/provider-credentials.tsx. | |
| keymap.dispatchCommand("altimate.provider.logout") | |
| resetSetupComplete() | |
| }, | |
| category: "Provider", | |
| }, | |
| { | |
| name: "altimate.logout", | |
| title: "Sign out of Altimate LLM Gateway", | |
| slashName: "logout", | |
| run: () => { | |
| // altimate_change — the credential clear + instance dispose + toast happen | |
| // opencode-side (AltimateApi is unreachable from packages/tui); see | |
| // packages/opencode/src/plugin/tui/altimate/provider-credentials.tsx. | |
| keymap.dispatchCommand("altimate.provider.logout") | |
| resetSetupComplete() | |
| dialog.clear() | |
| }, | |
| category: "Provider", | |
| }, |
🤖 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/app.tsx` around lines 809 - 821, Update the run handler for
the altimate.logout command to clear the command palette dialog before or
alongside dispatching altimate.provider.logout. Preserve the existing
resetSetupComplete behavior and ensure the palette closes after /logout
executes.
| export function DialogAltimateAuth() { | ||
| const { theme } = useTheme() | ||
| const sdk = useSDK() | ||
| const dialog = useDialog() | ||
| const toast = useToast() | ||
|
|
||
| onMount(async () => { | ||
| const providerID = "altimate-backend" | ||
| try { | ||
| const result = await sdk.client.provider.oauth.authorize({ providerID, method: 0 }) | ||
| if (result.error) { | ||
| toast.show({ variant: "error", message: JSON.stringify(result.error) }) | ||
| dialog.clear() | ||
| return | ||
| } | ||
| if (result.data?.method === "auto") { | ||
| dialog.replace(() => ( | ||
| <AutoMethod providerID={providerID} title="Altimate LLM Gateway" index={0} authorization={result.data!} /> | ||
| )) | ||
| } else if (result.data?.method === "code") { | ||
| dialog.replace(() => ( | ||
| <CodeMethod providerID={providerID} title="Altimate LLM Gateway" index={0} authorization={result.data!} /> | ||
| )) | ||
| } else { | ||
| dialog.clear() | ||
| } | ||
| } catch (err) { | ||
| // e.g. the loopback port is busy — don't hang on "Starting sign-in…". | ||
| toast.error(err instanceof Error ? err : new Error("Failed to start sign-in")) | ||
| dialog.clear() | ||
| } | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
DialogAltimateAuth can resurrect a dismissed dialog, and hardcodes method: 0.
Two things here:
- Unlike
AutoMethod(Lines 351-357), thisonMounthas nodisposedguard — if the user presses esc whileauthorize()is in flight, the laterdialog.replace(...)re-opens the sign-in UI over whatever is on screen. method: 0couples this entry point to the ordering of themethodsarray inpackages/opencode/src/altimate/plugin/altimate.ts; adding a method above the oauth one silently sends/authto the wrong flow. Prefer resolving the index fromsync.data.provider_auth["altimate-backend"]bytype === "oauth".
🛡️ Disposal guard
export function DialogAltimateAuth() {
const { theme } = useTheme()
const sdk = useSDK()
const dialog = useDialog()
const toast = useToast()
+ let disposed = false
+ onCleanup(() => {
+ disposed = true
+ })
onMount(async () => {
const providerID = "altimate-backend"
try {
const result = await sdk.client.provider.oauth.authorize({ providerID, method: 0 })
+ if (disposed) return
if (result.error) {🤖 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/component/dialog-provider.tsx` around lines 288 - 319,
Update DialogAltimateAuth’s onMount flow to track disposal and guard all
post-await UI actions, including toast, dialog.replace, and dialog.clear, so
dismissing the dialog while authorize is pending cannot resurrect or alter the
current UI. Replace the hardcoded method: 0 in
sdk.client.provider.oauth.authorize with the index of the entry whose type is
"oauth" from sync.data.provider_auth["altimate-backend"], reusing the existing
sync data hook/context.
| // altimate_change start — first-run gate without dead chat / error copy: a normal | ||
| // message with no provider ready opens the welcome picker (the message is | ||
| // discarded) with a friendly line, rather than erroring. | ||
| if (!ready()) { | ||
| dialog.replace(() => ( | ||
| <DialogModelWelcome intro="First, let's connect your AI model — then I'll get right on that." /> | ||
| )) | ||
| input.clear() | ||
| input.extmarks.clear() | ||
| setStore("prompt", { input: "", parts: [] }) | ||
| return false | ||
| } | ||
| // altimate_change end |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how local slash commands (slashName-based) are dispatched relative to submitInner,
# and whether DialogModelWelcome exposes an equivalent connect action.
rg -n --type=ts -C5 'useCommandSlashes' packages/tui/src
rg -n --type=tsx -C10 'DialogModelWelcome' packages/tui/src/component/altimate-onboarding.tsxRepository: AltimateAI/altimate-code
Length of output: 2984
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files 'packages/tui/src/component/altimate-onboarding.tsx' 'packages/tui/src/component/prompt/index.tsx' 'packages/tui/src/component/prompt/autocomplete.tsx' 'packages/tui/src/keymap.tsx'
echo
echo "== altimate-onboarding relevant =="
wc -l packages/tui/src/component/altimate-onboarding.tsx
sed -n '1,240p' packages/tui/src/component/altimate-onboarding.tsx
echo
echo "== prompt/index relevant sections =="
wc -l packages/tui/src/component/prompt/index.tsx
sed -n '1000,1230p' packages/tui/src/component/prompt/index.tsx
echo
echo "== prompt/autocomplete relevant =="
wc -l packages/tui/src/component/prompt/autocomplete.tsx
sed -n '80,190p' packages/tui/src/component/prompt/autocomplete.tsx
echo
echo "== keymap useCommandSlashes =="
wc -l packages/tui/src/keymap.tsx
sed -n '230,290p' packages/tui/src/keymap.tsx
echo
echo "== useCommandSlashes usages/imports =="
rg -n 'useCommandSlashes|CommandSlash|commandEntry|slashName|connect' packages/tui/src/component packages/tui/src/util packages/tui/src/keymap.tsx -g '*.ts' -g '*.tsx'
echo
echo "== find slash command registration files =="
rg -n 'CommandSlash|commandEntry|namespace: "palette"|connect' packages/tui/src -g '*.ts' -g '*.tsx' | sed -n '1,220p'Repository: AltimateAI/altimate-code
Length of output: 45534
Move the /connect escape-hatch before the first-run gate.
/connect is registered as the provider.connect palette command, but submitInner() returns false and opens DialogModelWelcome before the slash-command dispatch. A user who dismisses autocomplete with a trailing space and presses Enter will never hit /connect; replace this early return with a /connect-aware path, or dispatch provider.connect first for command input.
🤖 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/component/prompt/index.tsx` around lines 1047 - 1059, Update
submitInner() so /connect command input is dispatched to the registered
provider.connect command before the !ready() first-run gate executes. Preserve
the existing DialogModelWelcome, input cleanup, and false-return behavior for
other unready normal messages, while ensuring a trailing-space /connect
invocation reaches provider.connect.
| <box | ||
| width={65} | ||
| flexShrink={0} | ||
| alignItems="center" | ||
| justifyContent="center" | ||
| gap={1} | ||
| paddingTop={1} | ||
| paddingBottom={1} | ||
| paddingLeft={1} | ||
| > | ||
| <text fg={theme.text} attributes={TextAttributes.BOLD}> | ||
| Welcome to Altimate Code | ||
| </text> | ||
| <Logo /> | ||
| </box> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether `@opentui/core` box clips, wraps, or overflows when a flexShrink:0 fixed-width
# child exceeds the parent's available width.
rg -n --type=ts -C5 'flexShrink' packages/tui/src/component | head -50Repository: AltimateAI/altimate-code
Length of output: 5031
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'welcome-panel.tsx|home.tsx|index.tsx' packages/tui/src | sed 's#/workspace/##' | sed 's#/Users/[^/]*/##' | head -50
echo "== welcome-panel outline =="
ast-grep outline packages/tui/src/component/welcome-panel.tsx || true
echo "== welcome-panel contents =="
cat -n packages/tui/src/component/welcome-panel.tsx | sed -n '1,220p'
echo "== session index outline around imports/use =="
ast-grep outline packages/tui/src/session/index.tsx --view compact || true
rg -n --type=tsx -C8 'WelcomePanel|Welcome|dimensions|useTerminalDimensions|wide|sidebarVisible' packages/tui/src/session/index.tsx packages/tui/src -C4 | head -220Repository: AltimateAI/altimate-code
Length of output: 4170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== session/routes usages =="
rg -n -C8 'WelcomePanel|Welcome|dimensions|useTerminalDimensions|useTerminal|sidebarVisible|wide' packages/tui/src/utils packages/tui/src/context packages/tui/src/routes packages/tui/src/component packages/tui/src/session || true
echo "== relevant source files =="
for f in packages/tui/src/routes/home.tsx packages/tui/src/routes/session/index.tsx packages/tui/src/component/welcome-panel.tsx; do
echo "--- $f ---"
wc -l "$f"
sed -n '1,220p' "$f" | cat -n
done
echo "== opentui/core package source if present =="
git ls-files | rg 'opentui/core|opentui' | head -100
rg -n -C4 'function compute|flexShrink|flexGrow|width=|flexDirection|overflow|clip' packages packages/tui -g '*.ts' -g '*.tsx' | head -240 || trueRepository: AltimateAI/altimate-code
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== opentui/core package manifests =="
for f in package.json packages/opencode/package.json; do
[ -f "$f" ] && { echo "--- $f ---"; sed -n '80,140p' "$f" | cat -n; }
done
echo "== git files likely opentui core =="
git ls-files | rg 'packages/opencv|opencode/open|opentui|tui/src' | head -200
echo "== search opentui core box implementation if vendored =="
rg -n -C8 'class .*Box|BoxRenderable|flexShrink|flexGrow|wrapMode|overflow|clip|compute.*box|render.*box' packages -g '*.ts' -g '*.tsx' | head -260 || true
echo "== candidate implementation files =="
while IFS= read -r f; do
if rg -q 'flexShrink|flexGrow|BoxRenderable|renderBox|wrapMode|overflow' "$f"; then
echo "--- $f ---"
wc -l "$f"
fi
done < <(git ls-files | rg '\.ts$|\.tsx$' | head -200)Repository: AltimateAI/altimate-code
Length of output: 34778
🌐 Web query:
@opentui/core flexShrink fixedWidth box overflow wrap behavior
💡 Result:
In @opentui/core, layout behavior is powered by the Yoga flexbox engine [1][2]. The interaction between flexShrink, fixed dimensions, and overflow is governed by the following rules: Flex Shrink Defaults: By default, OpenTUI applies a specific heuristic for flexShrink [3][4]: - If a component has an explicit numeric width or height, flexShrink defaults to 0 [3][4]. This prevents the element from shrinking below its specified size when space is constrained [5][4]. - If a component does not have explicit dimensions (e.g., auto-sized), flexShrink defaults to 1, allowing it to shrink to fit available space [5][4]. Overflow and Wrapping: - Flex Wrap: You can control whether children wrap to new lines using the flexWrap property on the parent container (supported values: "nowrap", "wrap", "wrap-reverse") [5][1]. - Overflow Behavior: The overflow property (supported values: "visible", "hidden", "scroll") determines how content that exceeds the parent's boundaries is handled [5][4]. When set to "hidden" or "scroll", child elements are clipped to the parent's bounds [5]. - Known Issues with Text: Without explicit flexShrink={0}, text elements may shrink and cause rendering artifacts (such as overlapping characters) when they exceed the capacity of a fixed-size container [6]. Explicitly setting flexShrink={0} on children is the recommended way to ensure they maintain their size and respect layout boundaries within a fixed-width/height parent [6][7]. To ensure a box with fixed dimensions handles content appropriately without unwanted shrinking or overlapping, you should explicitly define flexShrink={0} on the child components or the container itself if necessary [6].
Citations:
- 1: https://anomalyco-opentui.mintlify.app/api/renderable
- 2: https://anomalyco-opentui.mintlify.app/components/box
- 3: https://github.com/anomalyco/opentui/blob/406bed2d791e62ba600a5c9f2c072a3b246b6b01/packages/core/src/Renderable.ts
- 4: https://mintlify.wiki/anomalyco/opentui/core-concepts/layout-system
- 5: https://anomalyco-opentui.mintlify.app/core-concepts/layout-system
- 6: feature: default flexShrink=0 for text elements anomalyco/opentui#435
- 7: https://mintlify.wiki/anomalyco/opentui/api/yoga-layout
🌐 Web query:
opentui core BoxRenderable width overflow calculation flexShrink
💡 Result:
In OpenTUI, the BoxRenderable component utilizes the Yoga layout engine to manage sizing and overflow [1][2]. The calculation for flexShrink and width handling is governed by specific internal logic within the Renderable core [3][4]. Default flexShrink Logic OpenTUI applies automatic default values for flexShrink based on whether a Renderable has explicit dimensions defined [3][4]: 1. If an explicit numeric width or height is provided, the component defaults to flexShrink: 0 [3][4]. 2. If the dimensions are not explicitly set (e.g., set to auto), the component follows the web default of flexShrink: 1 [3][4]. This behavior ensures that elements with fixed sizes do not shrink unintentionally when space is constrained [5][6]. Users can explicitly override this default by passing a flexShrink value in the layout options [5][6]. Handling Overflow When content exceeds the boundaries of a parent container, the overflow property determines how the layout engine handles the extra content [5][6]. The supported values are [7]: - visible: The default behavior; content is shown even if it extends beyond the parent bounds [5][6]. - hidden: Clips content to the parent's bounds [5][6]. - scroll: Clips content and enables scrolling functionality [5][6]. For elements like text that might otherwise shrink and cause garbled output when inside a constrained container, explicitly setting flexShrink: 0 is recommended to maintain the intended layout [8]. This prevents the layout engine from force-shrinking the element to fit into the parent's defined bounds [8].
Citations:
- 1: https://mintlify.wiki/anomalyco/opentui/components/box
- 2: https://anomalyco-opentui.mintlify.app/components/box
- 3: https://github.com/anomalyco/opentui/blob/406bed2d791e62ba600a5c9f2c072a3b246b6b01/packages/core/src/Renderable.ts
- 4: https://github.com/sst/opentui/blob/3dcc7730/packages/core/src/Renderable.ts
- 5: https://anomalyco-opentui.mintlify.app/core-concepts/layout-system
- 6: https://mintlify.wiki/anomalyco/opentui/core-concepts/layout-system
- 7: https://anomalyco-opentui.mintlify.app/api/yoga-layout
- 8: feature: default flexShrink=0 for text elements anomalyco/opentui#435
🌐 Web query:
opentui renderable layout overflowWidth flexWrap grow shrink default
💡 Result:
OpenTUI uses the Yoga layout engine to manage component positioning and sizing, supporting various Flexbox properties [1][2][3]. Below are the details for the specific properties requested: flexGrow This property determines how much a flex item should grow relative to the rest of the flex items in the container [1][2]. It accepts a number value [4][2]. flexShrink This property determines how much a flex item should shrink relative to the rest of the flex items in the container if there is insufficient space [1][2]. It accepts a number value [4][2]. In the implementation, if no value is provided, it defaults to 1 [4]. flexWrap This property controls whether flex items wrap onto multiple lines or stay on a single line [2][3]. Available options include: - "nowrap" (Default): Items stay on a single line [2][3]. - "wrap": Items wrap to new lines [2][3]. - "wrap-reverse": Items wrap to new lines in reverse order [2][3]. overflow This property governs the behavior of content that exceeds the component's boundaries [2][3]. It accepts the following string values: - "visible" - "hidden" - "scroll" Note on overflowWidth: While the standard Flexbox/Yoga model used by OpenTUI includes an "overflow" property, there is no distinct "overflowWidth" property in the layout options [2][3]. Sizing is generally handled through properties like width, minWidth, and maxWidth [2][3].
Citations:
- 1: https://anomalyco-opentui.mintlify.app/api/renderable
- 2: https://anomalyco-opentui.mintlify.app/core-concepts/layout-system
- 3: https://mintlify.wiki/anomalyco/opentui/core-concepts/layout-system
- 4: https://github.com/anomalyco/opentui/blob/406bed2d791e62ba600a5c9f2c072a3b246b6b01/packages/core/src/Renderable.ts
🌐 Web query:
opentui Renderable.ts flexShrink 0 overflow clip implementation
💡 Result:
In OpenTUI, the layout system is powered by Yoga (Flexbox) [1][2], and Renderable is the base class providing these capabilities [3]. Regarding flexShrink and overflow: 1. Flex Shrink Implementation: flexShrink is a property available within the LayoutOptions interface of Renderable [1][4]. By default, Yoga nodes may have a default flexShrink value of 1, which can cause elements (especially text) to shrink and potentially overlap or become garbled when space is constrained [5][6]. To prevent this behavior, developers explicitly set flexShrink={0} on elements that should maintain their size regardless of available space [5][7][8]. 2. Overflow Implementation: The overflow property controls how content that exceeds the bounds of a container is handled [7][9]. It supports the following values: - "visible": The default behavior, where content is rendered outside the parent's bounds [7][9]. - "hidden": Clips the content to the parent's bounds [7][9]. - "scroll": Enables scrolling behavior, which both clips the content and allows it to be scrolled [7][9]. These properties are managed by mapping standard CSS-like Flexbox and overflow options to the underlying Yoga layout engine, which then informs the rendering process [1][7]. [1][5][7]
Citations:
- 1: https://github.com/sst/opentui/blob/3dcc7730/packages/core/src/Renderable.ts
- 2: https://mintlify.wiki/anomalyco/opentui/api/renderable
- 3: https://anomalyco-opentui.mintlify.app/api/renderable
- 4: https://github.com/anomalyco/opentui/blob/406bed2d791e62ba600a5c9f2c072a3b246b6b01/packages/core/src/Renderable.ts
- 5: feature: default flexShrink=0 for text elements anomalyco/opentui#435
- 6: Text and Boxes need flexShrink=0 where they should not shrink anomalyco/opencode#3775
- 7: https://mintlify.wiki/anomalyco/opentui/core-concepts/layout-system
- 8: https://mintlify.wiki/anomalyco/opentui/api/yoga-layout
- 9: https://anomalyco-opentui.mintlify.app/core-concepts/renderables
Make the welcome wordmark panel responsive to terminal width.
WelcomePanel is mounted on home.tsx and above the session scrollback, but its 65-column left wordmark column has flexShrink={0} while the container is width="100%". On narrower terminals, the right column with the onboarding copy and CTA can be squeezed to near-zero width. Use useTerminalDimensions() and either collapse the fixed left panel, switch to a stacked layout, or increase the threshold below which the left column no longer demands 65 columns.
🤖 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/component/welcome-panel.tsx` around lines 30 - 44, Update
WelcomePanel’s fixed 65-column wordmark layout to respond to terminal width
using useTerminalDimensions(). Below an appropriate narrow-terminal threshold,
collapse or stack the left panel (and ensure it no longer reserves 65 columns),
while preserving the existing side-by-side layout on wider terminals so the
onboarding content and CTA remain usable.
| # curl -fsSL https://www.altimate.sh/install | bash | ||
|
|
||
| REPO_DIR="$(cd "$(dirname "$0")" && pwd)" | ||
| DIST_BIN="$REPO_DIR/packages/opencode/dist/@altimateai/altimate-code-darwin-arm64/bin/altimate" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not hard-code the macOS arm64 artifact for a current-platform build.
bun run build --single is documented as building the current platform, but DIST_BIN always targets darwin-arm64 and codesign is always executed. Other hosts can produce a different dist/${name}/bin artifact, as defined by packages/opencode/script/build.ts, and then this script fails at the existence check or signing step. Derive the target from the build selection and sign only on Darwin, or explicitly reject unsupported hosts before building.
As per coding guidelines, shell scripts must provide fallbacks for single-OS paths rather than assuming one platform.
Also applies to: 27-30
🤖 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 `@replace.sh` at line 15, Update replace.sh so the current-platform build does
not hard-code the darwin-arm64 artifact: derive DIST_BIN from the
platform/architecture selected by the build flow in
packages/opencode/script/build.ts, and execute codesign only when the host
target is Darwin. Preserve the existing artifact existence validation and add an
explicit unsupported-platform fallback if a target cannot be resolved.
Source: Coding guidelines
| rm -f "$DEST" | ||
| cp "$DIST_BIN" "$DEST" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Create the destination directory before copying.
On a fresh installation, $HOME/.altimate/bin may not exist, causing the replacement to fail before the binary is installed. Create the parent directory first.
Suggested fix
+mkdir -p "$(dirname "$DEST")"
rm -f "$DEST"
cp "$DIST_BIN" "$DEST"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rm -f "$DEST" | |
| cp "$DIST_BIN" "$DEST" | |
| mkdir -p "$(dirname "$DEST")" | |
| rm -f "$DEST" | |
| cp "$DIST_BIN" "$DEST" |
🤖 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 `@replace.sh` around lines 34 - 35, Update the installation flow around the
rm/cp commands in replace.sh to create the parent directory of $DEST before
copying $DIST_BIN. Ensure the directory creation succeeds for fresh
installations, then preserve the existing removal and copy behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
14 issues found across 26 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/tui/src/component/prompt/index.tsx">
<violation number="1" location="packages/tui/src/component/prompt/index.tsx:255">
P3: The unreliable-model chip is missing for models with display names because it looks up `WARNLIST` by `parsed().model`, not the model ID used as the warning-map key. Use `local.model.current()?.modelID` for this lookup so `qwen-plus` remains warned regardless of its display name.</violation>
<violation number="2" location="packages/tui/src/component/prompt/index.tsx:1050">
P2: First-run model gating also intercepts slash commands: `/auth` and `/logout` cannot run when no model is ready because this branch executes before command dispatch and clears the input. Restrict the welcome gate to non-command normal prompts, and allow model-independent onboarding commands to dispatch before requiring `selectedModel`.</violation>
</file>
<file name="packages/tui/src/feature-plugins/sidebar/footer.tsx">
<violation number="1" location="packages/tui/src/feature-plugins/sidebar/footer.tsx:39">
P2: The community and docs links reinvent the Link component inline — use the existing `<Link href="...">` from `src/ui/link.tsx` instead, so click behavior stays in one place.</violation>
</file>
<file name="packages/tui/src/component/dialog-model.tsx">
<violation number="1" location="packages/tui/src/component/dialog-model.tsx:36">
P2: A provider with only deprecated models is treated as ready but produces no selectable model, so it disappears from both READY and NEEDS-SETUP. Readiness should use the same non-deprecated model predicate as the READY list, including in the OpenCode cost check.</violation>
<violation number="2" location="packages/tui/src/component/dialog-model.tsx:63">
P2: OpenCode `*-nano` models are now selectable even though the picker previously blocked them, allowing users to choose models that the existing UI intentionally treated as unusable. Preserving the OpenCode nano guard keeps these entries visible but non-selectable.</violation>
<violation number="3" location="packages/tui/src/component/dialog-model.tsx:109">
P3: When a user already has an OpenCode/Zen model ready, Big Pickle is appended to the end of NEEDS SETUP instead of appearing before the lower-priority providers. In a long provider list this breaks the promised ordering and makes the fallback harder to find.</violation>
</file>
<file name="replace.sh">
<violation number="1" location="replace.sh:35">
P2: `cp` will fail if `$HOME/.altimate/bin/` doesn't exist yet (e.g. first run before the install script). Add `mkdir -p "$(dirname "$DEST")"` before the `cp` so the script works without prerequisites.</violation>
</file>
<file name="packages/tui/src/component/dialog-provider.tsx">
<violation number="1" location="packages/tui/src/component/dialog-provider.tsx:309">
P2: When the gateway authorization returns the supported `method: "code"` variant, `/auth` falls back to the generic code flow, so a successful sign-in opens the model picker and skips the promised inline success/automatic model selection. A gateway-specific code completion path (or equivalent handling in `CodeMethod`) would keep the `/auth` UX consistent for both authorization variants.</violation>
</file>
<file name="packages/opencode/src/altimate/plugin/altimate.ts">
<violation number="1" location="packages/opencode/src/altimate/plugin/altimate.ts:60">
P2: Two simultaneous Altimate sign-ins cannot be correlated independently even though this flow creates per-state entries. `ProviderAuth` keeps only one pending OAuth result per provider ID, so the second authorize call overwrites the first; serializing concurrent authorizations or propagating a flow identifier through the outer auth state would prevent cross-wiring.</violation>
<violation number="2" location="packages/opencode/src/altimate/plugin/altimate.ts:85">
P2: The loopback listener can remain bound to port 7317 after an auth flow ends without `callback()` being invoked. Cleanup should stop the server whenever the pending map becomes empty, including timeout and callback-handler paths.</violation>
</file>
<file name="packages/tui/src/app.tsx">
<violation number="1" location="packages/tui/src/app.tsx:561">
P2: On a fresh launch, `/connect` can open before provider data is loaded, and selecting Gateway or a BYOK provider then does nothing because the welcome picker resolves against the still-empty provider catalog. Waiting for plugin readiness alone is insufficient; include the sync bootstrap state before setting `firstRunPickerHandled`.</violation>
<violation number="2" location="packages/tui/src/app.tsx:1097">
P2: If an update event arrives during first-run onboarding, the handler returns and never replays the confirmation after onboarding becomes ready; users only get the footer indicator despite the comment promising to prompt once ready. Queue the version or trigger the prompt from a readiness effect.</violation>
</file>
<file name="packages/opencode/src/altimate/api/client.ts">
<violation number="1" location="packages/opencode/src/altimate/api/client.ts:210">
P2: Gateway sign-in can still hang indefinitely when the exchange endpoint sends headers and then stalls its body. `fetch()` resolves before `res.text()`/`res.json()` consumes that body, so this `finally` clears the abort timer too early; keep the timer active around the complete fetch and response-body handling.</violation>
</file>
<file name="packages/opencode/src/plugin/tui/altimate/provider-credentials.tsx">
<violation number="1" location="packages/opencode/src/plugin/tui/altimate/provider-credentials.tsx:87">
P2: If api.client.instance.dispose() throws after clearCredentials() succeeds, the error toast is misleading — credentials were already cleared. Restructure so dispose errors are non-fatal: clear credentials first, then dispose the client independently, and only show an error if clearing actually fails.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| // altimate_change start — first-run gate without dead chat / error copy: a normal | ||
| // message with no provider ready opens the welcome picker (the message is | ||
| // discarded) with a friendly line, rather than erroring. | ||
| if (!ready()) { |
There was a problem hiding this comment.
P2: First-run model gating also intercepts slash commands: /auth and /logout cannot run when no model is ready because this branch executes before command dispatch and clears the input. Restrict the welcome gate to non-command normal prompts, and allow model-independent onboarding commands to dispatch before requiring selectedModel.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tui/src/component/prompt/index.tsx, line 1050:
<comment>First-run model gating also intercepts slash commands: `/auth` and `/logout` cannot run when no model is ready because this branch executes before command dispatch and clears the input. Restrict the welcome gate to non-command normal prompts, and allow model-independent onboarding commands to dispatch before requiring `selectedModel`.</comment>
<file context>
@@ -1036,6 +1044,19 @@ export function Prompt(props: PromptProps) {
+ // altimate_change start — first-run gate without dead chat / error copy: a normal
+ // message with no provider ready opens the welcome picker (the message is
+ // discarded) with a friendly line, rather than erroring.
+ if (!ready()) {
+ dialog.replace(() => (
+ <DialogModelWelcome intro="First, let's connect your AI model — then I'll get right on that." />
</file context>
| const res = await fetch(url, { | ||
| method: "POST", | ||
| headers: { | ||
| "x-tenant": instance, | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ token }), | ||
| signal: controller.signal, | ||
| }).finally(() => clearTimeout(timeout)) | ||
| if (!res.ok) { | ||
| const body = await res.text().catch(() => "") | ||
| throw new Error(`Social token exchange failed (${res.status} ${res.statusText}) ${body}`) | ||
| } | ||
| const data = (await res.json()) as { auth_token?: string } | ||
| if (!data.auth_token) throw new Error("Social token exchange did not return an auth_token") | ||
| return data.auth_token |
There was a problem hiding this comment.
P2: Gateway sign-in can still hang indefinitely when the exchange endpoint sends headers and then stalls its body. fetch() resolves before res.text()/res.json() consumes that body, so this finally clears the abort timer too early; keep the timer active around the complete fetch and response-body handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/api/client.ts, line 210:
<comment>Gateway sign-in can still hang indefinitely when the exchange endpoint sends headers and then stalls its body. `fetch()` resolves before `res.text()`/`res.json()` consumes that body, so this `finally` clears the abort timer too early; keep the timer active around the complete fetch and response-body handling.</comment>
<file context>
@@ -185,6 +196,35 @@ export namespace AltimateApi {
+ // upstream_fix parity: bound the request so a network stall can't hang the callback.
+ const controller = new AbortController()
+ const timeout = setTimeout(() => controller.abort(), 15_000)
+ const res = await fetch(url, {
+ method: "POST",
+ headers: {
</file context>
| const res = await fetch(url, { | |
| method: "POST", | |
| headers: { | |
| "x-tenant": instance, | |
| "Content-Type": "application/json", | |
| }, | |
| body: JSON.stringify({ token }), | |
| signal: controller.signal, | |
| }).finally(() => clearTimeout(timeout)) | |
| if (!res.ok) { | |
| const body = await res.text().catch(() => "") | |
| throw new Error(`Social token exchange failed (${res.status} ${res.statusText}) ${body}`) | |
| } | |
| const data = (await res.json()) as { auth_token?: string } | |
| if (!data.auth_token) throw new Error("Social token exchange did not return an auth_token") | |
| return data.auth_token | |
| try { | |
| const res = await fetch(url, { | |
| method: "POST", | |
| headers: { | |
| "x-tenant": instance, | |
| "Content-Type": "application/json", | |
| }, | |
| body: JSON.stringify({ token }), | |
| signal: controller.signal, | |
| }) | |
| if (!res.ok) { | |
| const body = await res.text().catch(() => "") | |
| throw new Error(`Social token exchange failed (${res.status} ${res.statusText}) ${body}`) | |
| } | |
| const data = (await res.json()) as { auth_token?: string } | |
| if (!data.auth_token) throw new Error("Social token exchange did not return an auth_token") | |
| return data.auth_token | |
| } finally { | |
| clearTimeout(timeout) | |
| } |
| <text fg={theme().textMuted} wrapMode="word" width="100%"> | ||
| Ideas or issues? Join the community. | ||
| </text> | ||
| <text |
There was a problem hiding this comment.
P2: The community and docs links reinvent the Link component inline — use the existing <Link href="..."> from src/ui/link.tsx instead, so click behavior stays in one place.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tui/src/feature-plugins/sidebar/footer.tsx, line 39:
<comment>The community and docs links reinvent the Link component inline — use the existing `<Link href="...">` from `src/ui/link.tsx` instead, so click behavior stays in one place.</comment>
<file context>
@@ -1,84 +1,55 @@
+ <text fg={theme().textMuted} wrapMode="word" width="100%">
+ Ideas or issues? Join the community.
+ </text>
+ <text
+ wrapMode="word"
+ width="100%"
</file context>
| const isFav = favorites.some((f) => f.providerID === provider.id && f.modelID === modelID) | ||
| return { | ||
| value: { providerID: provider.id, modelID } as { providerID: string; modelID: string } | string, | ||
| title: info.name ?? modelID, |
There was a problem hiding this comment.
P2: OpenCode *-nano models are now selectable even though the picker previously blocked them, allowing users to choose models that the existing UI intentionally treated as unusable. Preserving the OpenCode nano guard keeps these entries visible but non-selectable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tui/src/component/dialog-model.tsx, line 63:
<comment>OpenCode `*-nano` models are now selectable even though the picker previously blocked them, allowing users to choose models that the existing UI intentionally treated as unusable. Preserving the OpenCode nano guard keeps these entries visible but non-selectable.</comment>
<file context>
@@ -18,112 +27,98 @@ export function DialogModel(props: { providerID?: string }) {
+ const isFav = favorites.some((f) => f.providerID === provider.id && f.modelID === modelID)
+ return {
+ value: { providerID: provider.id, modelID } as { providerID: string; modelID: string } | string,
+ title: info.name ?? modelID,
+ description: warn ? `${provider.name} · ${warn}` : provider.name,
+ category: "READY",
</file context>
| let firstRunPickerHandled = false | ||
| createEffect(() => { | ||
| if (firstRunPickerHandled) return | ||
| if (!ready()) return // wait until providers are loaded before deciding |
There was a problem hiding this comment.
P2: On a fresh launch, /connect can open before provider data is loaded, and selecting Gateway or a BYOK provider then does nothing because the welcome picker resolves against the still-empty provider catalog. Waiting for plugin readiness alone is insufficient; include the sync bootstrap state before setting firstRunPickerHandled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tui/src/app.tsx, line 561:
<comment>On a fresh launch, `/connect` can open before provider data is loaded, and selecting Gateway or a BYOK provider then does nothing because the welcome picker resolves against the still-empty provider catalog. Waiting for plugin readiness alone is insufficient; include the sync bootstrap state before setting `firstRunPickerHandled`.</comment>
<file context>
@@ -535,18 +541,59 @@ function App(props: { onSnapshot?: () => Promise<string[]>; pluginHost: TuiPlugi
+ let firstRunPickerHandled = false
+ createEffect(() => {
+ if (firstRunPickerHandled) return
+ if (!ready()) return // wait until providers are loaded before deciding
+ firstRunPickerHandled = true
+ if (onboardingReady()) return // already set up — no gate
</file context>
| if (!ready()) return // wait until providers are loaded before deciding | |
| if (!ready() || sync.status === "loading") return // wait until providers are loaded before deciding |
| // altimate_change start — /logout: clear the stored gateway credential. Self-contained | ||
| // (dispatched from the packages/tui slash command in app.tsx) since AltimateApi is | ||
| // opencode-side and unreachable from packages/tui. | ||
| async function logout(api: TuiPluginApi) { |
There was a problem hiding this comment.
P2: If api.client.instance.dispose() throws after clearCredentials() succeeds, the error toast is misleading — credentials were already cleared. Restructure so dispose errors are non-fatal: clear credentials first, then dispose the client independently, and only show an error if clearing actually fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/plugin/tui/altimate/provider-credentials.tsx, line 87:
<comment>If api.client.instance.dispose() throws after clearCredentials() succeeds, the error toast is misleading — credentials were already cleared. Restructure so dispose errors are non-fatal: clear credentials first, then dispose the client independently, and only show an error if clearing actually fails.</comment>
<file context>
@@ -81,6 +81,20 @@ function show(api: TuiPluginApi) {
+// altimate_change start — /logout: clear the stored gateway credential. Self-contained
+// (dispatched from the packages/tui slash command in app.tsx) since AltimateApi is
+// opencode-side and unreachable from packages/tui.
+async function logout(api: TuiPluginApi) {
+ try {
+ await AltimateApi.clearCredentials()
</file context>
| if (id === "opencode") return Object.values(p.models).some((m) => m.cost?.input != null && m.cost.input !== 0) | ||
| return Object.keys(p.models).length > 0 |
There was a problem hiding this comment.
P2: A provider with only deprecated models is treated as ready but produces no selectable model, so it disappears from both READY and NEEDS-SETUP. Readiness should use the same non-deprecated model predicate as the READY list, including in the OpenCode cost check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tui/src/component/dialog-model.tsx, line 36:
<comment>A provider with only deprecated models is treated as ready but produces no selectable model, so it disappears from both READY and NEEDS-SETUP. Readiness should use the same non-deprecated model predicate as the READY list, including in the OpenCode cost check.</comment>
<file context>
@@ -18,112 +27,98 @@ export function DialogModel(props: { providerID?: string }) {
+ function providerReady(id: string) {
+ const p = sync.data.provider.find((x) => x.id === id)
+ if (!p) return false
+ if (id === "opencode") return Object.values(p.models).some((m) => m.cost?.input != null && m.cost.input !== 0)
+ return Object.keys(p.models).length > 0
+ }
</file context>
| if (id === "opencode") return Object.values(p.models).some((m) => m.cost?.input != null && m.cost.input !== 0) | |
| return Object.keys(p.models).length > 0 | |
| + if (id === "opencode") | |
| + return Object.values(p.models).some( | |
| + (m) => m.status !== "deprecated" && m.cost?.input != null && m.cost.input !== 0, | |
| + ) | |
| + return Object.values(p.models).some((m) => m.status !== "deprecated") |
| // up; and flag known-bad tool-callers with a persistent "⚠ unreliable model" chip in | ||
| // the prompt meta row (same WARNLIST the model picker warns with). | ||
| const ready = useReady() | ||
| const unreliableModel = createMemo(() => Boolean(WARNLIST[local.model.parsed().model])) |
There was a problem hiding this comment.
P3: The unreliable-model chip is missing for models with display names because it looks up WARNLIST by parsed().model, not the model ID used as the warning-map key. Use local.model.current()?.modelID for this lookup so qwen-plus remains warned regardless of its display name.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tui/src/component/prompt/index.tsx, line 255:
<comment>The unreliable-model chip is missing for models with display names because it looks up `WARNLIST` by `parsed().model`, not the model ID used as the warning-map key. Use `local.model.current()?.modelID` for this lookup so `qwen-plus` remains warned regardless of its display name.</comment>
<file context>
@@ -246,6 +248,12 @@ export function Prompt(props: PromptProps) {
+ // up; and flag known-bad tool-callers with a persistent "⚠ unreliable model" chip in
+ // the prompt meta row (same WARNLIST the model picker warns with).
+ const ready = useReady()
+ const unreliableModel = createMemo(() => Boolean(WARNLIST[local.model.parsed().model]))
+ // altimate_change end
</file context>
| const unreliableModel = createMemo(() => Boolean(WARNLIST[local.model.parsed().model])) | |
| const unreliableModel = createMemo(() => Boolean(WARNLIST[local.model.current()?.modelID ?? ""])) |
| if (zenIdx === -1) list.push(bigPickle) | ||
| else list.splice(zenIdx, 0, bigPickle) |
There was a problem hiding this comment.
P3: When a user already has an OpenCode/Zen model ready, Big Pickle is appended to the end of NEEDS SETUP instead of appearing before the lower-priority providers. In a long provider list this breaks the promised ordering and makes the fallback harder to find.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tui/src/component/dialog-model.tsx, line 109:
<comment>When a user already has an OpenCode/Zen model ready, Big Pickle is appended to the end of NEEDS SETUP instead of appearing before the lower-priority providers. In a long provider list this breaks the promised ordering and makes the fallback harder to find.</comment>
<file context>
@@ -18,112 +27,98 @@ export function DialogModel(props: { providerID?: string }) {
+ }
+ // Big Pickle sits at priority 4 — just above OpenCode Zen (priority 5).
+ const zenIdx = list.findIndex((o) => o.value === "opencode")
+ if (zenIdx === -1) list.push(bigPickle)
+ else list.splice(zenIdx, 0, bigPickle)
+ return list
</file context>
| if (zenIdx === -1) list.push(bigPickle) | |
| else list.splice(zenIdx, 0, bigPickle) | |
| const insertAt = | |
| zenIdx === -1 | |
| ? list.findIndex( | |
| (o) => typeof o.value === "string" && (PROVIDER_PRIORITY[o.value] ?? 99) > 4, | |
| ) | |
| : zenIdx | |
| if (insertAt === -1) list.push(bigPickle) | |
| else list.splice(insertAt, 0, bigPickle) |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
| // Exchange the short-lived, one-time login_token for the gateway | ||
| // auth_token server-side — the raw api_key never rides in the URL. | ||
| const authToken = await AltimateApi.exchangeSocialToken(creds.api_url, creds.instance, creds.token) |
There was a problem hiding this comment.
CRITICAL: creds.api_url is not validated before the one-time login_token is POSTed to it
api_url comes straight from the browser callback (url.searchParams.get("url"), line 96) and is passed unmodified to exchangeSocialToken, which POSTs the short-lived login_token (+ tenant) to ${apiUrl}/auth/social/exchange. Unlike the manual paste path (parseAltimateKey, which at least requires http(s)://), the OAuth path validates neither scheme nor host. A malicious browser extension or an open-redirect in the web app can swap the url param to exfiltrate the one-time token and exchange it for the gateway auth_token. Require https:// and allowlist known Altimate hosts before exchanging (except in an explicit dev mode).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in b9332a88bd — added isTrustedApiUrl: the one-time token is only exchanged against HTTPS Altimate hosts (*.myaltimate.com / *.altimate.ai), an explicitly configured ALTIMATE_API_URL, or loopback; the callback is rejected otherwise.
| ), | ||
| on(onboardingReady, (isReady, prev) => { | ||
| if (scanGateShown) return | ||
| if (isReady && prev === false) { |
There was a problem hiding this comment.
WARNING: The scan gate re-fires for returning connected users on every launch
onboardingReady() starts false at launch because providers aren't loaded until async sync bootstrap, so on()'s first run sees prev === undefined (no gate). The later false -> true transition when bootstrap loads a returning user's connected provider then satisfies prev === false, showing DialogScanGate — contradicting the comment's "a returning user never sees it." The sibling first-run effect (lines 559-565) confirms providers load async by gating on ready(); this effect should too (or latch the post-bootstrap value) so the gate only fires on a genuine in-session setup.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in b9332a88bd — the scan gate is armed only when the launch starts un-onboarded (same signal as the first-run picker), so a returning user whose onboardingReady flips false→true after sync never sees it.
| } | ||
|
|
||
| useKeyboard((evt) => { | ||
| if (evt.name === "up" || (evt.ctrl && evt.name === "p")) return move(-1) |
There was a problem hiding this comment.
SUGGESTION: Arrow keys aren't preventDefault-ed in DialogModelWelcome
The up/down branches return move(-1) / return move(1) skip evt.preventDefault(), unlike the return branch here and both sibling dialogs in this PR (DialogBigPickleConfirm below, DialogScanGate). Without it the key event can bubble to a parent/global handler, risking double-handling. Mirror the evt.preventDefault() the other two dialogs use.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| (runtime status stays up top in the scroll area). JTBD first, then | ||
| community/docs. Replaces the stale "Getting started" box. */} | ||
| {/* white divider marking the start of the help section */} | ||
| <text fg={theme().text}>┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄</text> |
There was a problem hiding this comment.
SUGGESTION: The inline divider duplicates the Dotted helper
This divider repeats the exact glyph string already factored into Dotted (line 14), differing only by color — and the "white divider" comment above is inaccurate (it renders theme().text, not white). Parameterize Dotted by color and reuse it for both dividers.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 5 Issues Found | Recommendation: Merge (non-blocking) Incremental re-review of head Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (10 files)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit b9332a8)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit b9332a8)Status: 2 Issues Found | Recommendation: Merge (non-blocking suggestions only) Incremental re-review of head
No new issues were introduced by this commit. Two pre-existing minor suggestions remain (unchanged code, re-verified against HEAD). Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit cd60d7a)Status: 4 Issues Found | Recommendation: Address before merge Overview
Incremental re-review of the latest head ( Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (22 files)
Reviewed by glm-5.2 · Input: 111.3K · Output: 48.9K · Cached: 1.3M Review guidance: REVIEW.md from base branch |
…e for returning users Two review findings: - CRITICAL (kilo-code / cubic): the one-time login_token was POSTed to the callback-supplied `url` without validation, so a crafted callback could exfiltrate the token. Add `isTrustedApiUrl` — allow only HTTPS Altimate-owned hosts (`*.myaltimate.com` / `*.altimate.ai`), an explicitly configured `ALTIMATE_API_URL`, or loopback (http ok) — and reject the callback otherwise. - MAJOR (cubic conf-10 / kilo / coderabbit): the Part 2 scan gate re-fired for returning connected users on every launch, because `onboardingReady` flips false→true once sync loads providers. Arm the gate only when this launch starts un-onboarded (the same signal that opens the first-run picker); a returning user never sees it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 2 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/tui/src/app.tsx">
<violation number="1" location="packages/tui/src/app.tsx:568">
P2: Returning users can still see the Step 2 scan dialog when plugin startup completes before provider bootstrap: this line arms the gate before `sync.ready` confirms that the initial provider state is known. Deferring the first-run decision until sync bootstrap has settled would prevent the later provider-load transition from being mistaken for newly completed onboarding.</violation>
</file>
<file name="packages/opencode/src/altimate/plugin/altimate.ts">
<violation number="1" location="packages/opencode/src/altimate/plugin/altimate.ts:33">
P2: IPv6 loopback API bases are rejected even though this branch is intended to allow local HTTP endpoints: `ALTIMATE_API_URL=http://[::1]:...` leaves `isLoopback` false and fails the protocol check before token exchange. Compare against the bracketed hostname (or use a loopback-address check).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (!ready()) return // wait until providers are loaded before deciding | ||
| firstRunPickerHandled = true | ||
| if (onboardingReady()) return // already set up — no gate | ||
| armScanGate = true |
There was a problem hiding this comment.
P2: Returning users can still see the Step 2 scan dialog when plugin startup completes before provider bootstrap: this line arms the gate before sync.ready confirms that the initial provider state is known. Deferring the first-run decision until sync bootstrap has settled would prevent the later provider-load transition from being mistaken for newly completed onboarding.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tui/src/app.tsx, line 568:
<comment>Returning users can still see the Step 2 scan dialog when plugin startup completes before provider bootstrap: this line arms the gate before `sync.ready` confirms that the initial provider state is known. Deferring the first-run decision until sync bootstrap has settled would prevent the later provider-load transition from being mistaken for newly completed onboarding.</comment>
<file context>
@@ -556,25 +556,30 @@ function App(props: { onSnapshot?: () => Promise<string[]>; pluginHost: TuiPlugi
if (!ready()) return // wait until providers are loaded before deciding
firstRunPickerHandled = true
if (onboardingReady()) return // already set up — no gate
+ armScanGate = true
dialog.replace(() => <DialogModelWelcome />)
})
</file context>
| return false | ||
| } | ||
| const host = u.hostname | ||
| const isLoopback = host === "localhost" || host === "127.0.0.1" || host === "::1" |
There was a problem hiding this comment.
P2: IPv6 loopback API bases are rejected even though this branch is intended to allow local HTTP endpoints: ALTIMATE_API_URL=http://[::1]:... leaves isLoopback false and fails the protocol check before token exchange. Compare against the bracketed hostname (or use a loopback-address check).
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 33:
<comment>IPv6 loopback API bases are rejected even though this branch is intended to allow local HTTP endpoints: `ALTIMATE_API_URL=http://[::1]:...` leaves `isLoopback` false and fails the protocol check before token exchange. Compare against the bracketed hostname (or use a loopback-address check).</comment>
<file context>
@@ -18,6 +18,32 @@ const DEFAULT_WEB_URL = "https://app.myaltimate.com"
+ return false
+ }
+ const host = u.hostname
+ const isLoopback = host === "localhost" || host === "127.0.0.1" || host === "::1"
+ if (u.protocol !== "https:" && !(u.protocol === "http:" && isLoopback)) return false
+ const configured = process.env.ALTIMATE_API_URL
</file context>
| const isLoopback = host === "localhost" || host === "127.0.0.1" || host === "::1" | |
| const isLoopback = host === "localhost" || host === "127.0.0.1" || host === "::1" || host === "[::1]" |
* feat(onboarding): ship jaffle-shop DuckDB starter sample
New shipping asset at `packages/opencode/sample-projects/jaffle-shop-duckdb/`.
Ships alongside the wrapper npm package (publish.ts wiring lands in the
next commit); a runtime resolver in `sample-source-resolver.ts` finds it
across dev / test / dist install layouts. A `/starter` slash command (Phase 4)
materializes a copy into a user-owned directory so a fresh user can walk a
real dbt project without connecting a datasource.
**Shape:**
- `dbt_project.yml` / `profiles.yml` — DuckDB profile with project-relative
path, so no host paths bake into the shipped source.
- 4 models (2 staging, 2 marts) + 2 seed CSVs — jaffle-shop layout,
forked from the dbt-tools test fixture and given its own life so a
test-fixture edit can't accidentally change the shipped sample.
- `models/staging/schema.yml` + `models/marts/schema.yml` — per-column
descriptions + `unique` / `not_null` / `relationships` tests, so the
static reviewer surfaces (/discover, /review) have real material to
work with.
- `sample-manifest.json` — version-stamped project metadata used by
the marker-based conflict-detection when the sample is materialized to
the user's filesystem (Phase 4).
- `target/manifest.json` — pre-compiled dbt manifest committed alongside
the source. Ships so that static workflows work with ZERO external
tools installed (no dbt-core, no dbt-duckdb needed for /discover or
/review). Sanitized to strip host paths (replaced with
`{{SAMPLE_ROOT}}` sentinels) and to zero timestamps + invocation_id so
regenerations are deterministic.
- `regenerate.sh` — maintainer script that re-runs `dbt compile` +
sanitizes + stages the manifest. Run after editing sample source; the
freshness test (Phase 5) will fail if source changes without a matching
manifest refresh.
**Not shipped:** `target/graph.gpickle` (Python pickle, no JS consumer),
`target/catalog.json` (warehouse introspection with env-specific
metadata), `target/run_results.json` (run-specific, no static value).
* feat(onboarding): resolve + ship starter sample from wrapper package
Wire the shipping side of the starter sample so the wrapper npm package
carries it and runtime code can find it across install layouts.
**publish.ts::copyAssets** — copies `packages/opencode/sample-projects/`
into the wrapper package alongside the existing `bin/` and `skills/`
copies. Only the shippable subset of `target/` (manifest.json) is copied
— the rest is excluded so a stale `dbt build` on a materialized user
copy can't contaminate the shipped source.
**sample-source-resolver.ts** — runtime lookup for the shipped sample.
Hunts across four candidate layouts so a single code path works in dev
(bun run src/index.ts), test (bun test), production (compiled bun exe
under `<wrapper>/bin/altimate-code`), and less-common install layouts
(pnpm content-addressable, npx cache) where the exe sits two hops from
the wrapper root. `ALTIMATE_STARTER_SAMPLE_DIR` env override is honored
first — used by tests to point at a fixture and by users pointing at a
hand-curated fork of the sample.
Also exports `loadShippedManifest()` — reads the pre-compiled
`target/manifest.json` and rehydrates the `{{SAMPLE_ROOT}}` sentinels
with the user's materialized target path. This is what the static
review-pipeline consumers (/discover, /review) call to walk the sample
DAG without dbt on PATH.
* fix(onboarding): sample-source-resolver + regenerate script — codex-review fixes
Applied four gaps from the Phase 3 codex adversarial review:
1. **Resolver honors symlinked `process.execPath`.** npm global installs
symlink the binary from `/usr/local/bin/altimate-code` to the actual
location under `lib/node_modules/`, Homebrew uses `libexec`, pnpm uses
`.bin` shims. The previous resolver walked from the shim's dirname and
would miss `../sample-projects/`. Now `fs.realpathSync(process.execPath)`
first, then hunt from the real location.
2. **`loadShippedManifest` JSON-safe rehydration.** The prior text-level
substitution of `{{SAMPLE_ROOT}}` sentinels corrupted JSON if the
materialized target path contained JSON-significant characters. A path
like `/tmp/a"b` broke the JSON with an unescaped double-quote; a
Windows path like `C:\Users\name\sample` produced invalid `\U` escape
sequences. Now: parse the manifest first, walk the tree with a new
exported `rehydrateSentinels()`, substitute ONLY inside string leaf
values, preserve object keys / numbers / booleans / nulls. Same fix
applied in `regenerate.sh` so a maintainer's home directory can't
accidentally sentinelize legitimate content (a model description or
compiled SQL literal that happens to contain the maintainer's absolute
path).
3. **`generated_at` pinned to a plausible past date instead of the epoch.**
Zero-epoch (`1970-01-01`) is a trap for downstream freshness-check /
staleness-detection tooling that may treat it as pathological. Fixed
to `2026-07-24T00:00:00Z` — bumped only when the maintainer wants to
signal a manifest-shape refresh, not on every regenerate.
4. **Longest-sentinel-first replace order.** In `rehydrateSentinels` the
`{{SAMPLE_ROOT_PARENT}}` sentinel is substituted before
`{{SAMPLE_ROOT}}` so the shorter one can't match inside the longer
one's expansion window.
Windows `cp`/`mkdir` portability in `publish.ts::copyAssets` (codex point
#3) was DEFERRED: matches the existing shell-based pattern used elsewhere
in the file, and release CI runs on Ubuntu only. Track as a follow-up
when the release moves off Ubuntu.
* feat(onboarding): first-run activation — core logic (detection, marker, materialize, tools, KV)
Phase 4a — the non-UI logic behind the activation prompt and /starter
slash command. Adds five modules under
`packages/opencode/src/altimate/onboarding/`:
- **kv-keys.ts** — four KV keys (`onboarding.activation.dismissed_at`,
`.completed_choice`, `onboarding.sample_project.path`,
`.version`) plus the `ActivationChoice` enum. Keys deliberately split
so a support engineer can inspect each state independently and so KV /
on-disk marker divergence stays reasonable to debug.
- **tool-detection.ts** — `detectDbtRuntime()` probes `dbt --version`
once per process and parses its plugin list for the "duckdb" adapter
line. Cached for the process lifetime; force-refresh available for
tests. Post-materialize UX filters "run" options (dbt build, live
query) when `hasDbtDuckdb` is false, so shipped commands can't
silently fail on users without the Python side installed.
- **marker.ts** — `.altimate-sample.json` marker semantics.
`classifyTarget(dir, version)` returns one of `empty` /
`our-sample-current` / `our-sample-different-version` / `unknown-dir`.
`findSafeTarget()` walks the `<name>`, `<name>-2`, `<name>-3` sequence
until a non-unknown-dir slot appears — never overwrites an unknown
directory. Marker is the AUTHORITATIVE source of truth for filesystem
safety; the KV path is merely a convenience index (per codex feedback:
"marker wins on divergence").
- **materialize.ts** — copies the shipped sample source into the user's
chosen target. Whitelist-driven (no wholesale recursive copy) so
future contributor scratch files don't accidentally leak into user
installs. Enforces the marker-based conflict policy from marker.ts.
`rejectUnsafeHome()` refuses to materialize into `/root` (sudo mistake),
`/tmp/*` (ephemeral runner), or `/` (misconfigured container) with an
actionable error the caller surfaces verbatim to the user.
- **detection.ts** — `detectUsableSetup(cwd)` returns
`"usable" | "detected-not-usable" | "nothing"` for ordering the three
options in the activation dialog. Wraps the existing
`detectDbtProject()` primitive with a targeted `profiles.yml`
regex scan that respects dbt's precedence (project-local →
`$DBT_PROFILES_DIR` → `~/.dbt/`). Doesn't validate credentials — that
would require a warehouse handshake we're not spending on activation.
Phase 4b (next commit) wires the TUI dialog, the slash commands, and
the app.tsx / onboard-connect.txt integration points that call into this
logic.
* fix(onboarding): Phase 4a codex-review refinements
Four adjustments from the Phase 4a codex adversarial pass:
1. **detection.ts — broadened profile-key regex.** The old regex
`^<name>:\s*$` only matched an unquoted, non-commented top-level key
with nothing after the colon — false-negatived quoted keys
(`"jaffle-shop":`), inline mappings (`jaffle_shop: {target: dev}`),
and trailing comments (`jaffle_shop: # local`). New pattern accepts
optional matching single/double quotes and any trailing content.
Known Jinja-wrapped / anchor-referenced false-negatives are
documented as accepted for v1 — verdict impact is only option
ordering (dialog still shows every choice), so erring on the
sample-side is the safer bias.
2. **marker.ts — attemptLimit 10 → 100 + randomized fallback.** A user
with `altimate-sample-dbt-{1..10}` (retries, benchmark runs, support
copies) would previously get a hard `Error: No safe target found`
at activation time. Now the loop tries 100 numeric slots; if all
are held by unrelated content, one final randomized `-<6hex>` slot
is attempted. Only after both fall through does it throw — the
collision odds on the randomized slot alone are ~1-in-16.7M, so the
only realistic path to failure is genuine environmental hostility.
3. **marker.ts + materialize.ts — parent-writable pre-check.** New
`checkParentWritable()` in marker.ts is called in materialize.ts
BEFORE candidate hunting. Turns a raw `EACCES` from `mkdirSync`
deep in the copy step into a clear "Target parent directory X is
not writable: <reason>" error the caller can surface. Handles
read-only enterprise homes, NFS glitches, container mounts.
4. **materialize.ts — README.md added to MATERIALIZE_ENTRIES.** New
`sample-projects/jaffle-shop-duckdb/README.md` documents what's
inside the sample and what to try (works-with-zero-tools vs
needs-dbt-duckdb). Codex flagged that shipping a sample directory
with no accompanying "what next" reading material was a
context-loss risk. README is also copied into the wrapper package
via publish.ts.
5. **tool-detection.ts — call-site guidance for cache staleness.**
Docstring on `detectDbtRuntime` now explicitly documents WHEN
callers must pass `{ force: true }` (after materialization, before
any run-workflow invocation). This is Phase 4b's problem to obey,
but calling it out here makes the intent visible in the module.
Refactor: `MaterializeResult.suffixIndex` → `suffix` (renamed for the
new `number | string` shape now that randomized fallback is a possible
value).
* feat(onboarding): activation dialog + /starter + /activation wiring
Phase 4b — the customer-facing surfaces that consume Phase 4a's core
logic. Adds three touch-points:
**1. `DialogActivation` (packages/tui/src/component/dialog-activation.tsx)**
The 3-option picker that fires when the scan-gate "No" path resolves and
also from the `/activation` slash command. Custom `<box>` layout matching
the visual style of `DialogScanGate` and `DialogModelWelcome` (part 1 +
part 2a of onboarding). Keyboard: 1/2/3 shortcuts, up/down + Enter,
Escape → dismissed. On any selection: persists both
`onboarding.activation.completed_choice` and
`onboarding.activation.dismissed_at` in KV so the dialog does not
auto-fire on future launches, then calls the injected `onChoose`
callback.
Options ordered by an async `detectUsableSetup(cwd)` probe: a
`dbt_project.yml` + resolvable `profiles.yml` verdict leads with
"Connect data"; otherwise "Open sample project" is first.
`packages/tui/src/altimate/onboarding/` — TUI-side `kv-keys.ts` +
`detection.ts` colocated here because the TUI package cannot import
from `packages/opencode` (workspace dep is `@opencode-ai/core` only).
Detection is a small self-contained fs walk mirroring
`detectDbtProject()` from opencode-side project-scan.
**2. `app.tsx` scan-gate "No" → open DialogActivation**
The scan-gate at app.tsx:585 previously hardcoded `if (arg === "skip") return`
— dropped the user into an empty chat with no continuation. Now the
"skip" branch calls `dialog.replace(() => <DialogActivation ... />)` with
a shared `dispatchActivationChoice` handler that routes each of the
four possible choices to the right follow-up: `connect_data` runs
`/onboard-connect scan`, `sample_project` runs `/starter`,
`describe_use_case` prefills the prompt buffer with a starter hint
(does NOT auto-submit — user finishes the sentence), `dismissed` is a
no-op with an already-persisted KV timestamp.
**3. Slash commands: `/starter` + `/activation`**
- `starter.txt` — LLM template that invokes the (Phase 4a) materialize
logic, then reports the result with one of three branch outputs:
reused, fresh, or fresh-with-suffix. Post-materialize UX suggestions
are strictly static-workflow only (dbt-duckdb install caveat goes at
the end for users who want live queries).
- `activation.txt` — one-line placeholder. The real work happens in
`appCommands` where a slash-name registration intercepts `/activation`
BEFORE it reaches the LLM and directly reopens the dialog. Escape
hatch for users who dismissed the dialog too early — addresses the
design consult's #1 concern ("if 'skip all' hides every useful
recovery affordance, users are stranded").
Both commands are registered in `packages/opencode/src/command/index.ts`
under `Default.STARTER` and `Default.ACTIVATION`, joining the existing
`/onboard-connect` follow-up family.
**4. `onboard-connect.txt` branch 4 → mentions `/starter`**
The "genuinely nothing yet" branch of the scan template previously
advertised "scaffold a project" as an aspirational offer with nothing
behind it. Now it explicitly points at `/starter` alongside "cd into
your project and run /discover" and "paste SQL / describe your use
case" — the same three routes the activation dialog offers, so both
entry points (scan-gate "No" AND scan-found-nothing) converge on the
same next-action set.
* fix(onboarding): Phase 4b codex-review refinements
Four fixes from the Phase 4b codex adversarial pass:
1. **Rename TUI detection → `tui-detection.ts`** to make the ownership
split explicit: this module is DISPLAY ORDER ONLY, not an
authoritative "usable setup" verdict. Opencode-side consumers
(agents, tools, /discover flows) must NOT import from here — they
own their own detection surface. Rewrote the file header to spell
this out so a future contributor doesn't wire this into a slash
command by accident.
2. **Gate keyboard input on detection completion** in `DialogActivation`.
`selected` starts at -1 while `detectUsableSetup()` is in flight; only
Escape works during that window. This closes a race where a user
with a valid dbt setup could Enter on the sample-first fallback
ordering (because verdict was undefined) and end up on /starter
when "Connect data" was the intended default. Detection is ~100ms
so the "Checking local project…" label is one-frame territory. Also
documented the `process.cwd()` assumption + wrapper-installer edge
case for the Phase 5 e2e test to cover.
3. **Prefill wording** for the "Describe your own use case" path
changed from the sentence fragment `"I'd like to "` to
`"Describe what you're trying to do: "`. If a user accidentally
hits Enter on the preamble, the LLM still receives a coherent
question rather than a broken fragment. Comment now also flags the
`parts: []` clear as a low-risk-but-non-zero draft-loss for future
recovery-flow work.
4. **`/activation` template becomes a real non-TUI fallback.**
Previous version claimed the dialog was already open even when
invoked in headless / ACP / `--print` mode where the TUI intercept
doesn't fire — the model would then advertise a picker that
didn't exist. Now the template presents the three choices as a
plain-text list and asks the user to pick by name. The TUI palette
intercept still short-circuits this template entirely for
interactive sessions, so the fallback only renders where it's
needed.
* test(onboarding): Phase 5a — marker, materialize, tool-detection, sample-source-resolver
Four unit test files landing 47 tests total, plus an off-by-one fix in
the resolver that the first test-run flushed out.
**marker.test.ts** — 22 tests covering `readMarker`/`writeMarker`
round-trip, the four `classifyTarget` decision-table branches (empty /
our-current / our-different-version / unknown-dir), and
`findSafeTarget`'s numeric-suffix loop + randomized-hex fallback when
all numbered slots are held by unrelated content. Plus the codex-flagged
`checkParentWritable` pre-check with writable and nonexistent parents.
**materialize.test.ts** — 8 tests. Fresh copy verifies all whitelisted
files land + profiles.yml is intact + marker is correct. Second-call
reuse-detection asserts no re-copy + no marker rewrite (materialization
timestamps preserved). Preferred-collision → `-2` suffix without
touching user's original file. Version-bump paths cover both the
"prompt before upgrading" (allowInPlaceUpgrade=false) and
"upgrade-in-place" (allowInPlaceUpgrade=true) branches. Unsafe HOME
scenarios exercised via `rejectUnsafeHome` (undefined, `/`, `/tmp/*`,
`/root` under non-root uid).
**sample-source-resolver.test.ts** — 12 tests. Env override path,
default dev-source-tree path, and the JSON-safe `rehydrateSentinels`
tree walk with adversarial inputs codex flagged in Phase 3 review:
strings with double quotes (`/tmp/a"b`), Windows backslash paths
(`C:\Users\...`), object keys that happen to match the sentinel
literally (must be preserved), and the parent-then-root replace order
(so the shorter sentinel can't shadow the longer one). Plus
end-to-end `loadShippedManifest` against the real committed manifest
— asserts no dangling sentinels after substitution AND the target path
appears where expected.
**tool-detection.test.ts** — 5 tests pinning the `dbt --version`
parser regex against representative dbt 1.x outputs. Covers the codex-
flagged "dbt present, no dbt-duckdb" scenario, the "'duckdb' as a
substring in an upgrade hint (not a plugin line)" false-positive
guard, and the strict-formatting requirement so bare-word "duckdb"
without a colon never counts.
**Resolver off-by-one fix**: the `dev-source-tree` candidate went 4
hops up from `packages/opencode/src/altimate/onboarding/` — which
lands at `packages/` — but sample-projects lives at
`packages/opencode/sample-projects/`. Fixed to 3 hops. The mistake was
invisible to Phase 3's smoke path (it happened to fall through to a
different candidate); Phase 5's tests forced a real assertion and
exposed it.
47 pass, 0 fail. Full altimate suite: 3773 pass, 640 skip, 0 fail.
* docs(onboarding): VHS tape + helper script for /starter demo
`starter-sample.tape` — VHS script that drives the demo. Renders
`docs/media/starter-sample.gif` when run with `vhs docs/media/starter-sample.tape`.
The rendered GIF is git-ignored (regenerable, ~700KB binary blob not
worth committing to a public repo); reviewers render locally or view
attachments on the PR.
`starter-sample-demo.sh` — helper shell script that the tape shells
out to. VHS's `Type` command can't reliably escape long nested-quote
shell one-liners (bun -e with an inline JS string that imports
materializeSample), so the tape delegates each demo step to a named
action on this script. Also lets a reader `bash` this file directly
for a non-recorded reproduction.
**What the recorded demo shows** (in order):
1. `materialize` — fresh copy to `$HOME/altimate-sample-demo/`
2. `ls` — expected files (README, dbt_project.yml, profiles.yml,
.altimate-sample.json marker, models/, seeds/, target/)
3. `find` — full tree layout
4. `cat .altimate-sample.json` — the conflict-detection marker
5. `wc -l target/manifest.json` — pre-compiled manifest present
6. README head — what-to-try guidance
7. Second `materialize` — reports `reused: true`, no re-copy
**NOT in this recording** — the interactive TUI activation dialog
itself. That path gates on `useReady()` which needs a live provider or
the setupComplete signal from finishing OAuth; scripting it in VHS
needs an auth-mock harness we don't have yet. Tracked as a follow-up.
This recording proves the LEAF flow (sample materialization) works
end-to-end against the shipped asset resolver + marker + copy logic
committed in Phases 3 & 4a.
Also gitignores `docs/media/*.gif` so future renders don't
accidentally get staged.
* feat(onboarding): starter_materialize tool — LLM-invoked wrapper around materializeSample
Fills the vaporware gap in the `/starter` slash-command flow: the
template at `packages/opencode/src/command/template/starter.txt`
already asks the LLM to *"Call the `starter_materialize` tool exactly
once, with no arguments"* — but I had never actually registered such a
tool. Users typing `/starter` would have hit an "unknown tool" error
and the whole materialization flow would silently fail.
**`packages/opencode/src/altimate/tools/starter-materialize.ts`** —
new `Tool.define("starter_materialize", ...)` modeled directly on
`feedback-submit.ts` (the canonical altimate-side pattern for a
slash-command tool that DOES something and returns structured metadata
for the template to branch on):
- Zod schema with 3 OPTIONAL parameters (preferred_target_name,
target_parent, allow_in_place_upgrade) — none required, so the
LLM's "no arguments" invocation works.
- Reads `sample-manifest.json`'s `version` field via the shipped
sample-source-resolver, so bumping the sample version auto-flows to
the marker without a code change here.
- Delegates to `materializeSample()` from Phase 4a; wraps its return
into the `{title, metadata: {targetPath, reused, suffix, note},
output}` shape the template's three branches consume.
- On failure (unsafe HOME, unwritable parent, missing source),
returns `{title, metadata: {error}, output: <actionable message>}`
— output text is passed through verbatim by the template.
**Registered** in `packages/opencode/src/tool/registry.ts` inside the
existing altimate_change block, right next to `FeedbackSubmitTool`.
Import + array entry.
**Test** at `packages/opencode/test/altimate/tools/starter-materialize.test.ts`
(5 tests, all pass). Covers each of the three success branches
(reused / fresh / suffixed), the version-mismatch prompt-hint branch,
and the failure-message passthrough. Uses the existing `initTool()`
fixture in `test/altimate/tool-fixture.ts` to unwrap the Effect-based
Tool.define into a plain `execute(args, ctx)` for assertion.
All 3778 altimate suite tests pass, typecheck clean.
This is the fix for the biggest gap I flagged in the last doubt list:
the /starter flow now actually works end-to-end. Precedent confirmed
via Sarav's `/onboard-connect` implementation (same pattern:
LLM-driven slash template + registered tool with structured return)
and via the `feedback-submit.ts` shape — both established before this
work.
* refactor(onboarding): rename starter_materialize tool → sample_setup
The onboarding template we route to (packages/opencode/src/command/template/
onboard-connect.txt) refers to the sample bootstrap tool as sample_setup;
align the tool's registered name so the LLM's tool call resolves. No
behavior change — same materialization logic, same schema, same tests.
* feat(onboarding): activation menu as agent-appended text; drop the modal dialog
The activation-menu design lands as agent-emitted text at the end of every
/onboard-connect branch, matching the ticket mockup: JTBD-worded options
(see downstream / review a SQL PR / try the sample project / describe
your own), rendered inline in the chat rather than a modal overlay.
Rationale: the modal DialogActivation approach we prototyped had two
practical blockers — auth-arrival timing meant the false→true transition
never fired for users with pre-loaded creds, and the fixed option-set
couldn't personalize per scan verdict. Emitting the menu from the
onboard-connect template dodges both, keeps warehouse-connected
personalization ("you've got 12 dbt models and a Snowflake connection…"),
and reuses existing skill routing (dbt-analyze, sql-review, cost-report,
sample_setup, dbt build, sql_execute) instead of standing up parallel
dispatch machinery.
Also handles the 'Build & query it' branch when dbt-duckdb isn't
installed: bash-probes for the adapter first and, if missing, surfaces
an actionable install instruction with two paths (paste an existing
dbt binary path, or run 'pip install dbt-duckdb'). Once available,
runs dbt build, then explicitly wires dbt-profiles → warehouse_add →
sql_execute so the DuckDB connection is registered before any query.
Scan-gate 'No' now dispatches /onboard-connect skip again (previously
close-only) so the template's skip branch runs and the menu emerges
naturally in the chat.
Removes: /starter and /activation slash commands + their templates,
the DialogActivation TUI component, KV keys + detection modules that
supported it, and the /starter VHS demo tape (no longer meaningful).
Also drops the stale KV_SAMPLE_PROJECT_PATH reference in marker.ts'
docstring.
* fix(onboarding): harden sample_setup against path traversal + atomic materialize
Codex review flagged two safety gaps in the LLM-facing sample_setup tool.
**Path traversal (P0)**
preferred_target_name was documented as a directory name but accepted
any trimmed string, then path.join'd directly to targetParent. A
prompt-injected model turn could pass '../foo' and escape the intended
parent.
- Regex allowlist on the Zod schema (tool boundary — the LLM sees the
contract): /^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$/. Single segment,
no path separators, no leading dot.
- Same regex enforced in materializeSample as defense in depth (the
tool schema catches the LLM path, this catches other callers).
- Post-path.resolve containment check: resolvedTarget must equal
resolvedParent or start with resolvedParent + sep. Belt-and-suspenders
for any future findSafeTarget change.
- 19 adversarial tests: ../escape, absolute paths, hidden dirs,
whitespace, shell metachars, empty string — all rejected before any
fs write. Plus 7 explicit accepted-name tests for the happy shape.
**Interrupted materialize strands a broken dir (P1)**
Old flow: copySampleTree(target) → writeMarker(target). Crash between
steps leaves a markerless dir. Next run classifies it as unknown-dir,
suffixes into <name>-2, leaves the broken <name> in place forever.
- Atomic materialize: write to .<name>.tmp-<hex> staging, write marker
there, then fs.renameSync → final target. On POSIX rename is atomic;
a crash mid-copy leaves an orphan tmp dir (different name) instead
of a half-written target.
- sweepOrphanStaging at the start of each materialize removes any
prior orphans matching .<preferredName>.tmp-*. Scoped to the current
preferredName so a different sample's staging isn't touched.
- For allow_in_place_upgrade: remove the outdated old target only
AFTER the staging tree is fully written, right before the rename.
- 2 new tests cover the crash-cleanup path (pre-seeded orphan gets
swept, fresh materialize lands at <name>/ not <name>-2/) and the
scoping (other-sample orphans are left alone).
* fix(onboarding): template branches for sample_setup states + de-vacate tests
Codex review flagged three more items on this branch.
**Template return-state branching (P1)**
sample_setup returns five distinct states (fresh, plain-reuse, version-
conflict-with-Caller-must-prompt, error, suffix-collision) but the
template only handled the happy path. LLM would present a stale sample
or a hard error as usable.
Rewrote the routing block so the LLM branches on metadata explicitly:
(a) metadata.error → surface the actionable message verbatim, no menu;
(b) reused=true + note contains 'Caller must prompt' → ask user to
reset/keep/install-alongside before calling with allow_in_place_upgrade;
(c) reused=true → 'already set up at <path>'; then menu;
(d) reused=false + suffix>0 → 'materialized alongside at <suffix path>';
then menu;
(e) reused=false + suffix=0 → 'created at <path>'; then menu.
Only branches c/d/e reach the SAMPLE menu.
**Skip-branch instruction conflict (P2)**
Skip branch said 'Respond with exactly \<opener\>' then 'Then act on
their answer', while the global rule at the bottom said the activation
menu is 'ALWAYS' appended after every branch. Ambiguous for the LLM
and the user's next reply.
Reworded: the opener AND the menu go out together in the same reply
(the opener frames it; the menu is the concrete next action). If the
user then answers free-text ('Snowflake', 'a dbt project', 'just
exploring'), handle it directly; if they pick a menu number, run the
routing.
**Vacuous tests (P2)**
- tool-detection.test.ts previously duplicated HAS_DBT_DUCKDB_RE +
VERSION_RE into the test file and asserted against those local
copies — the real detectDbtRuntime() was never invoked, so impl
drift wouldn't be caught. Rewrote to invoke detectDbtRuntime() for
real via a PATH-override + bash-script dbt stub. 8 tests covering
duckdb-present/absent, stderr-vs-stdout, non-zero exit, missing
binary, non-executable file, and cache honored-vs-forced.
- sample-source-resolver.test.ts's 'loads shipped manifest' test
returned early if resolveSampleSource() failed — passed trivially
under any resolver breakage. Replaced early-return with
expect(location).toBeDefined() so a broken resolver fails loud.
* fix(onboarding): consensus-review Round 1–3 — safety, correctness, packaging
Round 1 (blocking): tool contract + safety + packaging
- sample_setup: put a `status: ok|error` prefix in output so the model
(which only reads output, never metadata) can branch reliably; add
`install_alongside` and thread through materialize; drop
`target_parent` from the LLM-facing schema (was a bypass of the
rejectUnsafeHome guard).
- materialize: allowlist preferred name (regex) + post-resolve
containment check; atomic staging + rename; sentinel rehydration for
target/manifest.json baked to the FINAL target path (not staging);
Flock-serialized selection + write; re-classify before rmSync;
age-guarded orphan sweep.
- sample-source-resolver: add ALTIMATE_BIN_DIR candidate so
Windows / --ignore-scripts / ALTIMATE_CODE_BIN_PATH installs still find
the shipped sample.
- publish: ship .gitignore for the sample project (materializer expects it);
keep-in-sync note with MATERIALIZE_ENTRIES.
- regenerate: sanitize identity / wall-clock fields (user_id, project_id,
invocation_id, all created_at, generated_at, etc.) so the shipped
manifest doesn't leak maintainer telemetry.
- onboard-connect.txt: branch on status prefix (a/b/c/d/e); route
install-alongside; drop shell-out in favour of the tool's `dbt:` line.
Round 2 (correctness / concurrency / telemetry)
- Tool metadata sets `success: false` on all failure paths so
`core_failure` telemetry actually fires.
- Template covers hex-string suffix branch (was numeric-only).
- Template branch offers Reset / Keep / Install-alongside on version
conflict; install-alongside plumbs through findSafeTarget with
skipVersionMismatch.
- detectDbtRuntime is called from sample_setup and its result surfaces
as the `dbt:` line the template reads.
Round 3 (smaller stuff)
- classifyTarget: lstatSync so symlinked targets are classified
unknown-dir instead of being followed (can't reuse-through-link or
silently unlink).
- rejectUnsafeHome: reject os.tmpdir() and Windows system dirs
(SYSTEMROOT, PROGRAMFILES), not just /tmp/*.
- findSafeTarget: bail out to hex fallback after N consecutive
unknown-dir hits instead of burning ~100 stat syscalls.
- profiles.yml: reword comment to say `path:` resolves against process
CWD, not the project dir.
- sample .gitignore: `target/*` (not `target/`) so the
`!target/manifest.json` re-include is not inert.
- sample_setup: resolve source once and thread through materialize
(was doing the candidate hunt twice per call).
* test(onboarding): add coverage for consensus-review test gaps (27–31)
- materialize.test: split traversal `toThrow` alternation so the regex
layer's message is asserted per name (was hiding whether the containment
check ever fired).
- materialize.test: default-`targetParent` test — pins os.homedir(),
omits targetParent, asserts the sample lands under the pinned home.
- materialize.test: symlinked preferred slot classified unknown-dir
→ suffixed to -2, symlink untouched (proves lstatSync branch).
- materialize.test: 11 consecutive unknown-dir slots → bail-early to hex
fallback (proves findSafeTarget's CONSECUTIVE_UNKNOWN_LIMIT).
- materialize.test: existing tmp-parent tests now pass
`allowUnsafeParent: true` — required because the tightened
rejectUnsafeHome now refuses os.tmpdir() prefixes.
- sample-setup.test: install_alongside branch — seeded 0.5.0 marker in
slot 0, alongside call lands in slot -2, old slot untouched.
- sample-setup.test: `dbt:` line exists in output and matches the
documented shape (present / missing with adapter status) — protects
the template's Build & query it branch from a silent regression.
- sample-setup.test: makeTmpHome carves out a UUID-suffixed subdir of
the real HOME (the tool has no allowUnsafeParent escape hatch on its
LLM-facing schema).
- verify-freshness.test.ts (in sample dir): freshness guard — every
sha256-checksummed node in the shipped manifest matches its source
file's dbt hash (rstrip-one-\n convention); identity + wall-clock
fields scrubbed.
- publish-parity.test.ts: assert publish.ts's copy list covers every
MATERIALIZE_ENTRIES path so a future runtime whitelist addition
can't ship in dev but be missing in prod.
* fix(onboarding): codex sweep — canonicalize HOME + lstat orphan sweep
Follow-up to the consensus-review Round 1–3 commits. A codex sweep of the
same four failure classes (path bypass, platform guards, fs.rm sites,
resource cleanup) surfaced two real gaps not in the panel's list:
- NEW-1/NEW-4 — `rejectUnsafeHome` was comparing raw strings before
realpath. A caller passing `/private/tmp/foo` on macOS bypassed both
`startsWith('/tmp/')` (because `/tmp` is a symlink to `/private/tmp`,
so the literal prefixes don't share bytes) and the `os.tmpdir()`
check (macOS reports `/var/folders/…` but its realpath is
`/private/var/folders/…`). Now canonicalizes both sides — matches
against `/tmp`, `realpath('/tmp')`, `os.tmpdir()`, and
`realpath(os.tmpdir())`. Test asserts a `/private/tmp/…` HOME is
refused on macOS.
- NEW-5 — `sweepOrphanStaging` used `fs.statSync` on discovered
entries. A symlinked `.starter.tmp-<hex>` would get the target's
mtime for age classification (not the link's own), which could
either misfire the age guard or cause the sweep to rm the symlink
over live content. Now uses `lstatSync` and skips symlinks entirely
— same class as finding 21 (which we already applied to
`classifyTarget`). Test seeds a backdated symlinked orphan and
asserts the sweep leaves it alone.
* fix(onboarding): dbt runtime probe falls back through cmd.exe on Windows
Node's `execFile("dbt")` on Windows uses CreateProcess, which honours
PATHEXT for `.exe` and `.com` but NOT `.cmd`/`.bat` — those need a shell.
Some Windows dbt install layouts (older `pip install --user` script
dirs, certain corporate distributions, some WSL-bridge shims) drop a
`dbt.cmd` wrapper on PATH rather than `dbt.exe`. Without a fallback,
those users would see `dbt: missing` on the template's "Build & query it"
branch even when dbt is installed and on PATH.
Fix: on Windows only, if the direct `execFile("dbt", ...)` misses, retry
through `cmd.exe /c dbt --version`. cmd's own resolver honours the full
PATHEXT so it finds any wrapper shape. Args are constant strings so
there's no injection surface. macOS/Linux keep the single shell-less
probe.
Codex flagged this in the class-B sweep of `execFile` call sites. No
Windows CI covers this branch yet, so verify manually if you touch it.
* fix(onboarding): kilo-code-bot findings — alongside branch wording + hoist refs
- `onboard-connect.txt` — branch (b)'s "Install alongside" branch used
to instruct the model to follow branch (d) or (e) after the alongside
re-invocation. That was wrong for branch (d): its "your existing
directory wasn't ours, so I put it alongside" wording assumes the old
directory held unrelated content, but on the alongside path the old
directory IS ours (an older-version sample) and the user explicitly
kept it. Give the alongside path its own follow-up wording that names
both the new and old paths and does NOT reuse branch (d)'s message.
- `materialize.ts` — the tmp-ref canonicalization inside
`rejectUnsafeHome` was recomputing `realpathSync("/tmp")` and
`realpathSync(os.tmpdir())` on every `check()` call (up to twice per
invocation) even though neither depends on the candidate. Hoist the
`refs` array out of the closure so those two syscalls run once.
* fix(onboarding): cubic P1s + codex-sweep NEW-10 — marker identity, template shell/path safety, manifest asset-set guard
Bot review (cubic-dev-ai + a class-scoped codex re-sweep) surfaced three
real bugs on this branch and one test-coverage gap:
- cubic P1-1: `classifyTarget` was returning `our-sample-current` on any
marker whose VERSION matched, ignoring `sampleName`. A future
second bundled sample writing a marker with the same version would
be silently reused/upgraded through as this sample. Add
`expectedSampleName` gate in `classifyTarget`; threading through
`findSafeTarget` and the reverify-before-rmSync call. Fallback slot
and materialize reverify now both consult sample identity. Tests
added for wrong-sampleName → unknown-dir + symlink and
unreadable-dir branches (which were previously untested at the
marker.test.ts level even though materialize.test.ts covered symlink
end-to-end).
- cubic P1-2: template's "user pastes a dbt path" branch interpolated
the pasted string directly into a bash pipeline. A paste like
`; rm -rf ~` would execute. Rewrite the branch to (a) refuse paths
containing shell metacharacters, (b) refuse non-executable files,
(c) verify via a single-quoted `'<path>' --version` invocation,
(d) require single-quoting on every subsequent use of the path.
- cubic P1-3: DuckDB profile advertises `target/jaffle.duckdb` — a
RELATIVE path. If passed as-is to `warehouse_add`, `sql_execute`
resolves it against its own cwd and connects to (or creates) an
empty database file wherever the CLI is running — not the one dbt
just built. Template now instructs the model to join the sample
path with the profile's `path:` and pass the ABSOLUTE result.
- cubic P2 #4: `copySampleTree` silently skipped ANY missing entry
("`.gitignore` is optional; skip quietly"), even required ones. A
broken package with missing `models/` would materialize an empty
dir + write a marker, then reuse-forever on subsequent runs.
Split into required vs optional entries; missing required entry
throws with a reinstall pointer instead of writing a marker.
- codex NEW-10: `verify-freshness.test.ts` iterated only over
checksummed manifest nodes. A maintainer adding a new .sql model
without re-running `regenerate.sh` would pass the per-node hash
check (their new model simply had no manifest node to compare
against). Added a set-membership test: walk `models/*.sql` and
`seeds/*.csv` on disk and assert each has a manifest node.
* fix(onboarding): refuse ANY single-quote in pasted dbt path (kilo followup)
Kilo-code-bot flagged that the previous fix refused only UNMATCHED single-
quotes in the pasted dbt path. A path with a matched pair like `a'b'c`
survives the denylist, but wrapping it as `'a'b'c'` for the shell makes
bash see three tokens (`a`, unquoted `b`, `c`) — the middle segment
escapes single-quoting silently and we'd run against a different binary
than the user pasted. Metacharacter denylist already blocks command
injection; this is the remaining correctness gap.
Refuse ANY single-quote instead of trying to count matched pairs.
Single-quoting the resulting path is then provably path-preserving and
the model doesn't have to reason about quote parity.
* refactor(onboarding): dedupe against existing monorepo utilities (5 of 10)
Codex sweep + a dbt-team reviewer flagged 10 duplications where new
onboarding code reinvented utilities that already existed. Fixing 5 in
this commit; the other 5 stay as-is with documented rationale.
Fixed (impact from worst to nicest):
- dbt detection: `tool-detection.ts::detectDbtRuntime` now delegates to
`dbt-tools/src/dbt-resolve.ts::{resolveDbt, validateDbt, buildDbtEnv}`.
The old `execFile("dbt", "--version")` (with a `dbt.cmd` Windows fallback)
was PATH-only, so users with dbt in a venv / uv / conda / pyenv / pipx /
poetry etc. saw `dbt: missing` on the template's "Build & query it"
branch. resolveDbt handles 16 Python env managers + Windows Scripts/ +
`.exe`. Reviewer's original ask.
- containment: `materialize.ts` now uses `Filesystem.containsReal` (symlink-
aware realpath + `..`-segment rejection). The old lexical
`startsWith(resolvedParent + path.sep)` was bypassable via a symlinked
`targetParent`, which is exactly the class of attack that helper was
built to catch.
- atomic JSON writes: extracted `Filesystem.writeJsonAtomic` in
`util/filesystem.ts`. `marker.ts::writeMarker` now uses it. Prior
in-file tmp-file + rename dance was duplicated in three other places
(persistence.ts, memory/store.ts, tracing.ts) — those can migrate at
their own pace; extraction is the enabling step.
- MATERIALIZE_ENTRIES single-sourcing: the shipped-file list previously
lived in three places (materialize.ts const, publish.ts cp block,
publish-parity.test.ts hardcoded copy). Now lives once in
`sample-manifest.json` as an `assets` array. `readSampleAssets()` reads
it in both materialize (runtime copy) and publish (release copy). The
parity test collapses from "three lists in sync" to "manifest matches
disk". Adding a new sample file is a one-line manifest edit.
- Glob.scan in freshness test: replaced the local recursive `walk()` with
`Glob.scan("models/**/*.sql")` + `Glob.scan("seeds/**/*.csv")` from the
shared core util.
Deferred (kept, with rationale — see PR discussion for full triage):
- manifest read/parse (5): `loadRawManifest` in altimate/native/dbt caches
and returns mutable shared refs; partial merge later.
- sha256 file hash (6): `Hash.sha256` exists; dbtFileHash's rstrip-`\n`
semantics is the value-add — keep as a thin wrapper (later).
- suffix hunt (8): our findSafeTarget has richer semantics (marker
awareness, hex fallback, bail-early) than core/src/project/copy.ts.
- hasSampleShape (9): keep, later rename to also check sample-manifest.json.
- regenerate.sh (10): maintainer script; explicit "dbt on PATH" prereq.
* fix(onboarding): cubic P1 traversal + P2 event-loop + P2 Windows dbt.cmd (post-dedupe review)
Post-dedupe re-review from cubic + kilo flagged three regressions I
introduced in the dedupe commit (cb53255). All three fixed:
- P1 (cubic, materialize.ts) — `readSampleAssets` accepted any string in
the `from` field. A malicious `sample-manifest.json` (custom bundle
or ALTIMATE_STARTER_SAMPLE_DIR override pointing at attacker content)
with `from: "../victim"` would slip past shape validation and get
joined to `writeTo` — escaping the staging dir and letting the copy
overwrite files outside it. Same class as the `preferredTargetName`
regex on the LLM-facing surface, applied here to the manifest-facing
surface. Rejects absolute paths, empty strings, and `..` segments.
Added a parameterized traversal test covering `../victim`,
`/etc/passwd`, `models/../../../etc/passwd`, and empty string.
- P2 (cubic + kilo, tool-detection.ts) — `validateDbt` from dbt-tools
is synchronous (`execFileSync` with 10s timeout). Calling it inline
in the async `probe()` blocked the TUI event loop for 1-10s per
cache miss on a slow/hung dbt install. Regression vs the previous
async `execFile` impl. Fix: keep `resolveDbt` for candidate lookup
(its own execs are cheap discovery-only), skip `validateDbt`
entirely, and derive `hasDbt` + version + adapter from a single
async `execFile` invocation of the resolved path. Also collapses the
duplicate `--version` fork (validateDbt + captureVersionOutput both
ran it) that kilo flagged.
- P2 (cubic, tool-detection.ts) — the previous dedupe commit dropped
the `cmd.exe /c dbt --version` Windows fallback. `resolveDbt` only
tries `.exe` binaries; users on Windows with `.cmd`/`.bat` wrappers
(older `pip install --user`, corporate distributions, WSL-bridge
shims) get `dbt: missing` and lose the Build & query it option in
the template. Fallback restored: after the direct probe fails,
on Windows we retry via `cmd.exe /c dbt --version` which uses
PATHEXT resolution.
---------
Co-authored-by: Haider <haider@altimate.ai>
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (9)
packages/opencode/test/altimate/onboarding/sample-source-resolver.test.ts (1)
123-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded
/separators make this assertion Windows-hostile.Compose the expected fragment with
path.join("packages", "opencode", "sample-projects", DEFAULT_SAMPLE_NAME)so the check holds onwin32.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/altimate/onboarding/sample-source-resolver.test.ts` at line 123, Update the path assertion in the sample-source resolver test to build the expected fragment with path.join("packages", "opencode", "sample-projects", DEFAULT_SAMPLE_NAME) instead of hardcoded slash separators, preserving the existing containment check across platforms.packages/opencode/src/altimate/onboarding/marker.ts (1)
168-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
findSafeTarget's JSDoc is detached and now stale.The block at Lines 168-184 is separated from the function by
CONSECUTIVE_UNKNOWN_LIMITand its own doc comment, so it no longer attaches as JSDoc. It also states the hex fallback triggers only when all numbered slots are held, which the 10-consecutive-skip short-circuit contradicts. Move it directly abovefindSafeTargetand reword to mention the early bail.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/onboarding/marker.ts` around lines 168 - 192, Move the introductory JSDoc block so it directly precedes findSafeTarget, keeping CONSECUTIVE_UNKNOWN_LIMIT separate. Update the description of numbered-slot scanning to state that the randomized fallback is used after CONSECUTIVE_UNKNOWN_LIMIT consecutive unrelated-content hits, rather than only after all numbered slots are occupied, while preserving the remaining fallback behavior.packages/opencode/test/altimate/onboarding/marker.test.ts (1)
174-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent
returnhides the skip.Use
test.skipIf(process.getuid?.() === 0)(...)so the run reports the test as skipped rather than passing vacuously.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/altimate/onboarding/marker.test.ts` around lines 174 - 190, Update the unreadable-directory test declaration to use test.skipIf(process.getuid?.() === 0), so root users receive an explicit skipped result instead of an early return. Remove the in-test root guard while preserving the existing test body and cleanup behavior.packages/opencode/sample-projects/regenerate.sh (2)
117-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the repo root from git instead of
../../...
"$SAMPLE_DIR/../../.."hardcodes the current depth;git -C "$SAMPLE_DIR" rev-parse --show-toplevel(or justgit -C "$SAMPLE_DIR" add -f ..., since-Conly needs to be inside the worktree) is depth-independent.🤖 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/sample-projects/regenerate.sh` at line 117, Update the git add command in regenerate.sh to derive the repository context from SAMPLE_DIR using git -C, replacing the hardcoded "$SAMPLE_DIR/../../.." traversal while preserving the forced addition of target/manifest.json.
19-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlso pre-check
python3like you dodbt.The script fails with a bare
command not foundifpython3is absent, unlike the friendly dbt check above.♻️ Proposed guard
if ! command -v dbt >/dev/null 2>&1; then echo "ERROR: dbt is not on PATH. Install with: pip install dbt-duckdb" >&2 exit 127 fi + +if ! command -v python3 >/dev/null 2>&1; then + echo "ERROR: python3 is required to sanitize the manifest." >&2 + exit 127 +fiAlso applies to: 55-57
🤖 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/sample-projects/regenerate.sh` around lines 19 - 22, Add a `python3` availability guard alongside the existing `dbt` check in `regenerate.sh`, using `command -v python3`, a clear installation-oriented error message, and a nonzero exit before any Python invocation. Apply the same validation to the other referenced Python execution path.packages/opencode/src/altimate/onboarding/materialize.ts (2)
82-87: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
fromguard misses Windows drive-absolute paths on POSIX.
path.isAbsolute("C:\\x")isfalseunderpath.posix, so a manifest entry likeC:\Windows\xslips through the check and is only neutralized bypath.joinsemantics. Cheap to close explicitly.🛡️ Proposed fix
- if (!from || from.startsWith("/") || path.isAbsolute(from) || from.split(/[/\\]/).includes("..")) { + if ( + !from || + from.startsWith("/") || + from.startsWith("\\") || + /^[A-Za-z]:/.test(from) || + path.isAbsolute(from) || + path.win32.isAbsolute(from) || + from.split(/[/\\]/).includes("..") + ) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/onboarding/materialize.ts` around lines 82 - 87, Update the `from` validation guard in the materialization flow to explicitly reject Windows drive-absolute paths such as `C:\...`, regardless of the host platform, while preserving the existing non-empty, POSIX-absolute, and `..` segment checks.
489-504: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAvoid hard-coding manifest rehydration to
target/manifest.json.
copySampleTree()assumes this asset is always namedtarget/manifest.json, so renaming it or adding an unrelatedkind: "dir"asset namedtarget/manifest.jsonwould bypass the sentinel rehydration and ship the manifest verbatim. Tie rehydration to the manifest asset directly, e.g. withrehydrate: trueon thesample-manifest.jsonentry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/onboarding/materialize.ts` around lines 489 - 504, Update copySampleTree to identify the manifest asset via an explicit rehydrate flag on the sample-manifest.json entry instead of checking entry.from === "target/manifest.json". Ensure the rehydration path still calls loadShippedManifest with finalTarget and preserves directory creation and JSON writing, while unrelated directory assets are copied normally.packages/opencode/script/publish.ts (1)
93-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
readSampleAssetshere instead of re-parsing the manifest.
src/altimate/onboarding/materialize.ts::readSampleAssetsalready parses the same file and validates entry shape plus rejects absolute/..frompaths. Re-parsing with a bare cast leaves publish as a second (unvalidated) consumer of the format the comment above claims is single-sourced — a malformed entry silently producescpagainst a bogus path instead of a clear error.♻️ Proposed refactor
- const sampleManifest = JSON.parse(fs.readFileSync(`${sampleSource}/sample-manifest.json`, "utf8")) as { - assets: Array<{ from: string; kind: "file" | "dir"; required: boolean }> - } + const sampleAssets = readSampleAssets(sampleSource) await $`mkdir -p ${sampleDest}` - for (const asset of sampleManifest.assets) { + for (const asset of sampleAssets) {Add at the top of the file:
import { readSampleAssets } from "../src/altimate/onboarding/materialize"🤖 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/script/publish.ts` around lines 93 - 107, Update the publish flow around sampleManifest to import and call readSampleAssets instead of reading and JSON-parsing sample-manifest.json with a cast. Use its validated asset entries for the existing asset-copy loop, preserving the current required-asset handling and destination creation behavior.packages/opencode/test/altimate/onboarding/materialize.test.ts (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAll four new onboarding test files create unscoped
os.tmpdir()scratch dirs. The shared root cause is not using the documentedtmpdir()fixture, so temp trees (including full materialized samples) are left behind after each run.
packages/opencode/test/altimate/onboarding/materialize.test.ts#L22-L24: replacemakeTmpParentwithawait using tmp = await tmpdir()per test.packages/opencode/test/altimate/onboarding/marker.test.ts#L27-L29: replacemakeTmpwith the same per-test fixture scoping.packages/opencode/test/altimate/onboarding/sample-source-resolver.test.ts#L37-L42: use the fixture for the wrapper/env staging dirs (also at Lines 63-71 and 94-97).packages/opencode/test/altimate/onboarding/publish-parity.test.ts#L63-L63: use the fixture instead ofrequire("os").tmpdir()(also at Line 82), which additionally removes therequirein an ESM module.Based on learnings: for brand-new test files under
packages/opencode/test/altimate/, importtmpdirfromfixture/fixture.tsand useawait using tmp = await tmpdir()with per-test scoping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/altimate/onboarding/materialize.test.ts` around lines 22 - 24, Replace unscoped temporary-directory creation with the documented per-test tmpdir fixture across all four onboarding tests: in packages/opencode/test/altimate/onboarding/materialize.test.ts:22-24 replace makeTmpParent; in marker.test.ts:27-29 replace makeTmp; in sample-source-resolver.test.ts:37-42, 63-71, and 94-97 use the fixture for wrapper and environment staging directories; and in publish-parity.test.ts:63 and 82 replace require("os").tmpdir(). Import tmpdir from fixture/fixture.ts, scope each test with await using tmp = await tmpdir(), and remove the now-unneeded ESM require.Source: Learnings
🤖 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/models/marts/schema.yml`:
- Around line 21-32: Add a relationships data test to orders.customer_id in
packages/opencode/sample-projects/jaffle-shop-duckdb/models/marts/schema.yml:21-32,
targeting customers.customer_id while retaining the existing not_null test. In
packages/opencode/sample-projects/jaffle-shop-duckdb/README.md:8-29, keep the
“tests + relationships” claim unchanged once the schema test is added; no direct
README change is required.
In `@packages/opencode/sample-projects/jaffle-shop-duckdb/README.md`:
- Line 8: Update the fenced code block in the README to include the text
language identifier, changing the opening fence to use ```text while preserving
its contents.
In `@packages/opencode/sample-projects/jaffle-shop-duckdb/sample-manifest.json`:
- Line 13: Update the DuckDB target path note in the sample manifest to state
that the path resolves relative to dbt’s invoking working directory, matching
the behavior documented in profiles.yml, rather than claiming it is
project-relative.
In `@packages/opencode/sample-projects/regenerate.sh`:
- Around line 90-112: Update the manifest read and write calls around json.load
and json.dump to explicitly use UTF-8 encoding instead of the locale default.
Preserve the existing replacement, scrubbing, metadata normalization, and JSON
formatting behavior.
In `@packages/opencode/src/altimate/onboarding/tool-detection.ts`:
- Around line 113-118: Normalize the combined dbt output by stripping ANSI
escape sequences before evaluating either the DuckDB plugin or version regex in
the surrounding detection function. Apply the cleaned value to both matches
while preserving existing parsing behavior, and add a test covering colorized
output that still detects the installed DuckDB version and onboarding option.
In `@packages/opencode/src/altimate/tools/sample-setup.ts`:
- Around line 96-115: Separate the error handling for resolveSampleSource and
readSampleVersionAt in the sample setup flow. Keep the existing missing-source
response, including sample_source_missing and reinstall guidance, only for
resolution failures; handle readSampleVersionAt errors with a distinct reason
and message indicating the sample manifest is invalid or unreadable, while
preserving the underlying error details in metadata and output.
In `@packages/opencode/src/command/template/onboard-connect.txt`:
- Around line 77-83: Update the onboarding flow around hasWarehouse so the
personalized warehouse menu and its downstream/cost-analysis options are shown
only after warehouse_add and warehouse_test successfully establish a verified
connection. Preserve the discovery menu for profiles, Docker, and
environment-variable findings, or introduce a distinct connected-warehouse state
rather than treating discovery as connectivity.
- Around line 182-212: Update the dbt executable guidance in the onboarding flow
to avoid Bash/POSIX-only validation and invocation. Reuse the platform-specific
executable detected by sample_setup and its tool runner for validating the
user-provided path and running dbt build, supporting native Windows paths and
executables while preserving safe argument handling and truthful build results.
- Around line 210-225: Update the onboarding flow after the dbt build command to
gate all subsequent profile discovery, warehouse setup, and sql_execute queries
on a successful build. If the build fails, stop before calling dbt-profiles or
querying, and clearly surface the failure with remediation guidance; preserve
the existing truthful PASS/FAIL reporting.
In `@packages/opencode/src/util/filesystem.ts`:
- Around line 289-293: Update writeJsonAtomic to wrap the writeFileSync and
renameSync operations in a try/finally block, and remove the generated tmpPath
in finally so failures do not leave temporary files behind. Preserve directory
creation and atomic rename behavior.
In `@packages/opencode/test/altimate/onboarding/sample-source-resolver.test.ts`:
- Around line 206-217: Update the test containing loadShippedManifest to
snapshot the current ALTIMATE_STARTER_SAMPLE_DIR value before deleting it and
restore that exact value in teardown, including removing the variable when it
was initially unset. Keep the existing resolver assertions and manifest-loading
behavior unchanged, and use the file’s established snapshot/restore pattern to
isolate process state for parallel tests.
In `@packages/opencode/test/altimate/onboarding/tool-detection.test.ts`:
- Around line 13-27: Update the tests around detectDbtRuntime to use per-test
`await using tmp = await tmpdir()` so temporary directories and `os.tmpdir()`
stubs are cleaned up automatically. Isolate the process.env-mutating probe and
module-global cache usage—via dependency injection or a subprocess—so parallel
Bun tests cannot observe shared state; retain `_resetDbtRuntimeCacheForTests`
only where still necessary.
---
Nitpick comments:
In `@packages/opencode/sample-projects/regenerate.sh`:
- Line 117: Update the git add command in regenerate.sh to derive the repository
context from SAMPLE_DIR using git -C, replacing the hardcoded
"$SAMPLE_DIR/../../.." traversal while preserving the forced addition of
target/manifest.json.
- Around line 19-22: Add a `python3` availability guard alongside the existing
`dbt` check in `regenerate.sh`, using `command -v python3`, a clear
installation-oriented error message, and a nonzero exit before any Python
invocation. Apply the same validation to the other referenced Python execution
path.
In `@packages/opencode/script/publish.ts`:
- Around line 93-107: Update the publish flow around sampleManifest to import
and call readSampleAssets instead of reading and JSON-parsing
sample-manifest.json with a cast. Use its validated asset entries for the
existing asset-copy loop, preserving the current required-asset handling and
destination creation behavior.
In `@packages/opencode/src/altimate/onboarding/marker.ts`:
- Around line 168-192: Move the introductory JSDoc block so it directly precedes
findSafeTarget, keeping CONSECUTIVE_UNKNOWN_LIMIT separate. Update the
description of numbered-slot scanning to state that the randomized fallback is
used after CONSECUTIVE_UNKNOWN_LIMIT consecutive unrelated-content hits, rather
than only after all numbered slots are occupied, while preserving the remaining
fallback behavior.
In `@packages/opencode/src/altimate/onboarding/materialize.ts`:
- Around line 82-87: Update the `from` validation guard in the materialization
flow to explicitly reject Windows drive-absolute paths such as `C:\...`,
regardless of the host platform, while preserving the existing non-empty,
POSIX-absolute, and `..` segment checks.
- Around line 489-504: Update copySampleTree to identify the manifest asset via
an explicit rehydrate flag on the sample-manifest.json entry instead of checking
entry.from === "target/manifest.json". Ensure the rehydration path still calls
loadShippedManifest with finalTarget and preserves directory creation and JSON
writing, while unrelated directory assets are copied normally.
In `@packages/opencode/test/altimate/onboarding/marker.test.ts`:
- Around line 174-190: Update the unreadable-directory test declaration to use
test.skipIf(process.getuid?.() === 0), so root users receive an explicit skipped
result instead of an early return. Remove the in-test root guard while
preserving the existing test body and cleanup behavior.
In `@packages/opencode/test/altimate/onboarding/materialize.test.ts`:
- Around line 22-24: Replace unscoped temporary-directory creation with the
documented per-test tmpdir fixture across all four onboarding tests: in
packages/opencode/test/altimate/onboarding/materialize.test.ts:22-24 replace
makeTmpParent; in marker.test.ts:27-29 replace makeTmp; in
sample-source-resolver.test.ts:37-42, 63-71, and 94-97 use the fixture for
wrapper and environment staging directories; and in publish-parity.test.ts:63
and 82 replace require("os").tmpdir(). Import tmpdir from fixture/fixture.ts,
scope each test with await using tmp = await tmpdir(), and remove the
now-unneeded ESM require.
In `@packages/opencode/test/altimate/onboarding/sample-source-resolver.test.ts`:
- Line 123: Update the path assertion in the sample-source resolver test to
build the expected fragment with path.join("packages", "opencode",
"sample-projects", DEFAULT_SAMPLE_NAME) instead of hardcoded slash separators,
preserving the existing containment check across platforms.
🪄 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: 65253767-440a-48db-9da2-cf5f10d1325b
⛔ Files ignored due to path filters (2)
packages/opencode/sample-projects/jaffle-shop-duckdb/seeds/raw_customers.csvis excluded by!**/*.csvpackages/opencode/sample-projects/jaffle-shop-duckdb/seeds/raw_orders.csvis excluded by!**/*.csv
📒 Files selected for processing (31)
.gitignorepackages/opencode/sample-projects/jaffle-shop-duckdb/.gitignorepackages/opencode/sample-projects/jaffle-shop-duckdb/README.mdpackages/opencode/sample-projects/jaffle-shop-duckdb/dbt_project.ymlpackages/opencode/sample-projects/jaffle-shop-duckdb/models/marts/customers.sqlpackages/opencode/sample-projects/jaffle-shop-duckdb/models/marts/orders.sqlpackages/opencode/sample-projects/jaffle-shop-duckdb/models/marts/schema.ymlpackages/opencode/sample-projects/jaffle-shop-duckdb/models/staging/schema.ymlpackages/opencode/sample-projects/jaffle-shop-duckdb/models/staging/stg_customers.sqlpackages/opencode/sample-projects/jaffle-shop-duckdb/models/staging/stg_orders.sqlpackages/opencode/sample-projects/jaffle-shop-duckdb/profiles.ymlpackages/opencode/sample-projects/jaffle-shop-duckdb/sample-manifest.jsonpackages/opencode/sample-projects/jaffle-shop-duckdb/target/manifest.jsonpackages/opencode/sample-projects/jaffle-shop-duckdb/verify-freshness.test.tspackages/opencode/sample-projects/regenerate.shpackages/opencode/script/publish.tspackages/opencode/src/altimate/onboarding/marker.tspackages/opencode/src/altimate/onboarding/materialize.tspackages/opencode/src/altimate/onboarding/sample-source-resolver.tspackages/opencode/src/altimate/onboarding/tool-detection.tspackages/opencode/src/altimate/tools/sample-setup.tspackages/opencode/src/command/template/onboard-connect.txtpackages/opencode/src/tool/registry.tspackages/opencode/src/util/filesystem.tspackages/opencode/test/altimate/onboarding/marker.test.tspackages/opencode/test/altimate/onboarding/materialize.test.tspackages/opencode/test/altimate/onboarding/publish-parity.test.tspackages/opencode/test/altimate/onboarding/sample-source-resolver.test.tspackages/opencode/test/altimate/onboarding/tool-detection.test.tspackages/opencode/test/altimate/tools/sample-setup.test.tspackages/tui/src/app.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tui/src/app.tsx
| - name: orders | ||
| description: One row per order with a joined customer name. | ||
| columns: | ||
| - name: order_id | ||
| description: Primary key. | ||
| data_tests: | ||
| - unique | ||
| - not_null | ||
| - name: customer_id | ||
| description: Foreign key to `stg_customers`. | ||
| data_tests: | ||
| - not_null |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Missing relationships test on the FK columns, and README overstates what's tested. orders.customer_id is documented as a foreign key but only has a not_null test — no relationships test exists anywhere in the marts schema, even though orders.sql relies on that FK via an inner join and the README claims relationship tests are present.
packages/opencode/sample-projects/jaffle-shop-duckdb/models/marts/schema.yml#L21-L32: add arelationshipsdata test onorders.customer_id(tocustomers.customer_id) so a future data or join change is caught instead of silently dropping rows.packages/opencode/sample-projects/jaffle-shop-duckdb/README.md#L8-L29: once the test above exists, this doc claim is accurate as written; otherwise, adjust the "tests + relationships" line to reflect actual coverage.
📍 Affects 2 files
packages/opencode/sample-projects/jaffle-shop-duckdb/models/marts/schema.yml#L21-L32(this comment)packages/opencode/sample-projects/jaffle-shop-duckdb/README.md#L8-L29
🤖 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/sample-projects/jaffle-shop-duckdb/models/marts/schema.yml`
around lines 21 - 32, Add a relationships data test to orders.customer_id in
packages/opencode/sample-projects/jaffle-shop-duckdb/models/marts/schema.yml:21-32,
targeting customers.customer_id while retaining the existing not_null test. In
packages/opencode/sample-projects/jaffle-shop-duckdb/README.md:8-29, keep the
“tests + relationships” claim unchanged once the schema test is added; no direct
README change is required.
|
|
||
| ## What's in here | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the fenced code block.
markdownlint (MD040) flags this fence for missing a language identifier.
📝 Proposed fix
-```
+```text
dbt_project.yml dbt project config📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/sample-projects/jaffle-shop-duckdb/README.md` at line 8,
Update the fenced code block in the README to include the text language
identifier, changing the opening fence to use ```text while preserving its
contents.
Source: Linters/SAST tools
| }, | ||
| "notes": [ | ||
| "Renamed profile from the original test fixture: `test_jaffle_shop` → `jaffle_shop`.", | ||
| "DuckDB target file resolves project-relative at `target/jaffle.duckdb`; no host paths bake into the profile.", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the DuckDB path note.
This says the target resolves project-relative, while profiles.yml documents that it resolves from dbt’s invoking working directory. The shipped metadata will mislead users running dbt outside the sample directory.
🤖 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/sample-projects/jaffle-shop-duckdb/sample-manifest.json` at
line 13, Update the DuckDB target path note in the sample manifest to state that
the path resolves relative to dbt’s invoking working directory, matching the
behavior documented in profiles.yml, rather than claiming it is
project-relative.
| with open(manifest_path) as f: | ||
| obj = json.load(f) | ||
| obj = replace_paths(obj) | ||
| scrub_created_at(obj) | ||
| if isinstance(obj.get("metadata"), dict): | ||
| md = obj["metadata"] | ||
| # Identity — persistent UUIDs from the compiler's ~/.dbt/.user.yml | ||
| # and dbt-internal project fingerprint. Neither should ship. | ||
| md["user_id"] = ZERO_UUID | ||
| md["project_id"] = ZERO_UUID | ||
| md["invocation_id"] = ZERO_UUID | ||
| # Every wall-clock timestamp in metadata. | ||
| md["generated_at"] = FIXED_ISO | ||
| md["invocation_started_at"] = FIXED_ISO | ||
| md["run_started_at"] = FIXED_ISO | ||
| # Do not steer the installer's dbt toward "yes I accepted telemetry" — | ||
| # the compilation author's preference is not the installer's preference. | ||
| md["send_anonymous_usage_stats"] = False | ||
| # env can carry USER, PWD, HOME — strip it entirely. | ||
| md.pop("env", None) | ||
| with open(manifest_path, "w") as f: | ||
| json.dump(obj, f, indent=2, sort_keys=True) | ||
| f.write("\n") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Pin UTF-8 encoding on the manifest read/write.
open() uses the locale encoding, so regenerating under LC_ALL=C/POSIX raises UnicodeDecodeError on any non-ASCII byte in a dbt description or compiled SQL. Force UTF-8 explicitly.
🛡️ Proposed fix
-with open(manifest_path) as f:
+with open(manifest_path, encoding="utf-8") as f:
obj = json.load(f)
@@
-with open(manifest_path, "w") as f:
+with open(manifest_path, "w", encoding="utf-8") as f:
json.dump(obj, f, indent=2, sort_keys=True)As per coding guidelines: "force a UTF-8 locale where required" for shell scripts.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with open(manifest_path) as f: | |
| obj = json.load(f) | |
| obj = replace_paths(obj) | |
| scrub_created_at(obj) | |
| if isinstance(obj.get("metadata"), dict): | |
| md = obj["metadata"] | |
| # Identity — persistent UUIDs from the compiler's ~/.dbt/.user.yml | |
| # and dbt-internal project fingerprint. Neither should ship. | |
| md["user_id"] = ZERO_UUID | |
| md["project_id"] = ZERO_UUID | |
| md["invocation_id"] = ZERO_UUID | |
| # Every wall-clock timestamp in metadata. | |
| md["generated_at"] = FIXED_ISO | |
| md["invocation_started_at"] = FIXED_ISO | |
| md["run_started_at"] = FIXED_ISO | |
| # Do not steer the installer's dbt toward "yes I accepted telemetry" — | |
| # the compilation author's preference is not the installer's preference. | |
| md["send_anonymous_usage_stats"] = False | |
| # env can carry USER, PWD, HOME — strip it entirely. | |
| md.pop("env", None) | |
| with open(manifest_path, "w") as f: | |
| json.dump(obj, f, indent=2, sort_keys=True) | |
| f.write("\n") | |
| with open(manifest_path, encoding="utf-8") as f: | |
| obj = json.load(f) | |
| obj = replace_paths(obj) | |
| scrub_created_at(obj) | |
| if isinstance(obj.get("metadata"), dict): | |
| md = obj["metadata"] | |
| # Identity — persistent UUIDs from the compiler's ~/.dbt/.user.yml | |
| # and dbt-internal project fingerprint. Neither should ship. | |
| md["user_id"] = ZERO_UUID | |
| md["project_id"] = ZERO_UUID | |
| md["invocation_id"] = ZERO_UUID | |
| # Every wall-clock timestamp in metadata. | |
| md["generated_at"] = FIXED_ISO | |
| md["invocation_started_at"] = FIXED_ISO | |
| md["run_started_at"] = FIXED_ISO | |
| # Do not steer the installer's dbt toward "yes I accepted telemetry" — | |
| # the compilation author's preference is not the installer's preference. | |
| md["send_anonymous_usage_stats"] = False | |
| # env can carry USER, PWD, HOME — strip it entirely. | |
| md.pop("env", None) | |
| with open(manifest_path, "w", encoding="utf-8") as f: | |
| json.dump(obj, f, indent=2, sort_keys=True) | |
| f.write("\n") |
🤖 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/sample-projects/regenerate.sh` around lines 90 - 112,
Update the manifest read and write calls around json.load and json.dump to
explicitly use UTF-8 encoding instead of the locale default. Preserve the
existing replacement, scrubbing, metadata normalization, and JSON formatting
behavior.
Source: Coding guidelines
| const combined = `${out.stdout}\n${out.stderr}` | ||
| const hasDbtDuckdb = /^\s*-\s*duckdb:/m.test(combined) | ||
|
|
||
| const versionMatch = | ||
| combined.match(/-\s*installed:\s*([0-9]+\.[0-9]+\.[0-9]+\S*)/) ?? | ||
| combined.match(/core=([0-9]+\.[0-9]+\.[0-9]+\S*)/) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Strip ANSI sequences before parsing dbt output.
The comments explicitly support colorized output, but these regexes parse it raw. Escape codes before the plugin or version can report a usable DuckDB installation as missing and hide runnable onboarding options. Normalize ANSI control sequences before both matches and add a colored-output test.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/src/altimate/onboarding/tool-detection.ts` around lines 113
- 118, Normalize the combined dbt output by stripping ANSI escape sequences
before evaluating either the DuckDB plugin or version regex in the surrounding
detection function. Apply the cleaned value to both matches while preserving
existing parsing behavior, and add a test covering colorized output that still
detects the installed DuckDB version and onboarding option.
| Building the sample needs the dbt CLI + the DuckDB adapter. Two options: | ||
| 1. If you already have dbt installed somewhere, paste the path to the | ||
| `dbt` binary (e.g. `/Users/you/venvs/dbt/bin/dbt`) and I'll use it | ||
| from there — no reinstall needed. | ||
| 2. Install fresh: run `pip install dbt-duckdb` (grabs both dbt-core and | ||
| the DuckDB adapter), then say "ready" and I'll continue. | ||
|
|
||
| If the user pastes a path (option 1), do NOT interpolate the pasted | ||
| string into a shell pipeline verbatim — a paste like `; rm -rf ~` would | ||
| execute. Validate first, then always run through a single-quoted argv: | ||
| 1. Refuse if the path contains any of `; & | > < $ ` ( ) { } newline` | ||
| or any single-quote. Ask the user to paste again. (Matched-pair | ||
| single quotes still break the single-quote wrap below — `'a'b'c'` | ||
| leaves `b` unquoted — so ANY single-quote is refused, not just | ||
| unmatched ones.) | ||
| 2. Refuse if the file at that path is not executable | ||
| (`test -x '<path>'` — quote the path). | ||
| 3. Verify it's a dbt-duckdb binary by running the SINGLE-QUOTED | ||
| command `'<path>' --version 2>&1` via bash and checking that its | ||
| stdout contains a line starting with `- duckdb:`. Do NOT chain | ||
| with `grep -q` on user-controlled strings. | ||
| 4. If verified, remember the path and use `'<path>' build` (single- | ||
| quoted) for the build step below. Never pass the path unquoted. | ||
| If they install fresh (option 2) and say "ready", call `sample_setup` | ||
| again — the fresh call re-probes with `{ force: true }` and its `dbt:` | ||
| line will now say `present`. Only shell out if you have to; the tool's | ||
| probe is the source of truth. | ||
|
|
||
| Once dbt is available, run `dbt build` (or the validated `'<path>' build` | ||
| from above, with the path single-quoted) in the sample project dir via | ||
| bash and report the PASS/FAIL counts truthfully. Before |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Avoid Bash/POSIX-only dbt instructions.
This flow requires /Users/... paths, test -x, Bash, and POSIX shell quoting despite the project’s platform-specific dbt detection. Native Windows paths and executables will fail or be misinterpreted. Reuse the detected executable/tool runner and provide platform-specific validation and invocation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/src/command/template/onboard-connect.txt` around lines 182
- 212, Update the dbt executable guidance in the onboarding flow to avoid
Bash/POSIX-only validation and invocation. Reuse the platform-specific
executable detected by sample_setup and its tool runner for validating the
user-provided path and running dbt build, supporting native Windows paths and
executables while preserving safe argument handling and truthful build results.
| Once dbt is available, run `dbt build` (or the validated `'<path>' build` | ||
| from above, with the path single-quoted) in the sample project dir via | ||
| bash and report the PASS/FAIL counts truthfully. Before | ||
| running any queries, call the `dbt-profiles` tool with `projectDir` | ||
| pointed at the sample dir to discover the DuckDB profile. The profile | ||
| reports the DuckDB path as `target/jaffle.duckdb` — a RELATIVE path | ||
| (dbt-duckdb resolves it against `dbt build`'s working directory, so it | ||
| landed at `<sample-path>/target/jaffle.duckdb`). If you pass that | ||
| relative string straight to `warehouse_add`, `sql_execute` will resolve | ||
| it against ITS own cwd and open (or create) an empty duckdb file | ||
| wherever the CLI happens to be running from — which is exactly the | ||
| bug that surfaced. Absolutize first: join the sample path from the | ||
| earlier `sample_setup` output with the profile's `path:` value, then | ||
| pass the ABSOLUTE path (`<sample-path>/target/jaffle.duckdb`) to | ||
| `warehouse_add` so `sql_execute` connects to the database dbt just | ||
| built. Then offer a first query and run it with `sql_execute`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stop before querying after a failed dbt build.
The flow reports build PASS/FAIL, then proceeds to profile discovery and sql_execute without requiring success. A failed build can leave stale or empty DuckDB data, producing misleading query results. Only continue to profiles and queries after a successful build; otherwise surface the failure and remediation.
🧰 Tools
🪛 LanguageTool
[style] ~220-~220: Consider an alternative for the overused word “exactly”.
Context: ...I happens to be running from — which is exactly the bug that surfaced. Absolutize fir...
(EXACTLY_PRECISELY)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/src/command/template/onboard-connect.txt` around lines 210
- 225, Update the onboarding flow after the dbt build command to gate all
subsequent profile discovery, warehouse setup, and sql_execute queries on a
successful build. If the build fails, stop before calling dbt-profiles or
querying, and clearly surface the failure with remediation guidance; preserve
the existing truthful PASS/FAIL reporting.
| export function writeJsonAtomic(targetPath: string, value: unknown): void { | ||
| const tmpPath = `${targetPath}.tmp-${randomBytes(6).toString("hex")}` | ||
| mkdirSync(dirname(targetPath), { recursive: true }) | ||
| writeFileSync(tmpPath, JSON.stringify(value, null, 2) + "\n") | ||
| renameSync(tmpPath, targetPath) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching filesystem.ts:\n'
fd -a 'filesystem\.ts$' . | sed 's#^\./##'
printf '\nRelevant function and nearby imports/types:\n'
file="$(fd 'filesystem\.ts$' . | grep 'packages/opencode/src/util/filesystem.ts' | head -n 1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,330p' "$file" | nl -ba | sed -n '220,330p'
fi
printf '\nSearch for cleanup patterns and atomic temp writes:\n'
rg -n "writeJsonAtomic|tmp-\b|unlinkSync|removeSync|finally|renameSync|writeFileSync" packages/opencode/src/util/filesystem.ts packages/opencode/src || true
printf '\nProgrammatic evidence of Node synchronous fs APIs referenced in file:\n'
node - <<'JS'
const keys = ['writeFileSync', 'renameSync', 'unlinkSync'];
for (const fn of keys) {
console.log(`${fn} is a ${typeof require('fs')[fn]}`);
}
const { cp } = require('child_process');
cp.spawnSync(process.execPath, ['-e', 'require("child_process");console.log("node fs exists")'], { encoding:'utf8' });
JSRepository: AltimateAI/altimate-code
Length of output: 612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/opencode/src/util/filesystem.ts"
printf 'Line count:\n'
wc -l "$file"
printf '\nRelevant imports and writeJsonAtomic implementation:\n'
sed -n '1,20p' "$file"
sed -n '280,300p' "$file"
printf '\nSynchronous fs and cleanup references:\n'
rg -n "writeJsonAtomic|tmp-|unlinkSync|removeSync|rm\(|finally|renameSync|writeFileSync|write" "$file" || true
printf '\nNode API availability and semantics probe:\n'
node - <<'JS'
const fs = require('fs');
const path = require('path');
const base = `/tmp/opencode-verify-${process.pid}`;
const dir = `${base}/dir`;
const target = `${dir}/value.json`;
const tmp = `${target}.tmp-${Math.random().toString(36).slice(2, 8)}`;
try {
fs.mkdirSync(dir, { recursive: true });
fs.writeFileSync(tmp, 'ok');
fs.renameSync(tmp, target);
console.log('renameSync succeeded when source exists');
console.log('target exists:', fs.existsSync(target));
console.log('tmp exists after rename:', fs.existsSync(tmp));
fs.unlinkSync(target);
try {
fs.renameSync(tmp, target);
console.log('renameSync succeeded when source missing');
} catch (e) {
console.log('renameSync failed correctly when source missing');
console.log(e.code);
}
try {
fs.writeFileSync(tmp, 'ok');
fs.writeFileSync(path.join(dir, new Error()), '');
} catch (e) {
console.log('writeFileSync failed on invalid output path');
console.log('tmp still exists after failed write:', fs.existsSync(tmp));
}
} finally {
try { fs.rmSync(base, { recursive: true, force: true }); } catch {}
}
JSRepository: AltimateAI/altimate-code
Length of output: 4417
Clean up the temporary file when writeJsonAtomic fails.
writeFileSync or renameSync can throw, leaving <targetPath>.tmp-* behind. Wrap the write/rename sequence in try/finally and remove the temp path in finally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/src/util/filesystem.ts` around lines 289 - 293, Update
writeJsonAtomic to wrap the writeFileSync and renameSync operations in a
try/finally block, and remove the generated tmpPath in finally so failures do
not leave temporary files behind. Preserve directory creation and atomic rename
behavior.
Source: Coding guidelines
| describe("loadShippedManifest — end-to-end against the real shipped manifest", () => { | ||
| test("loading the shipped sample's manifest.json with a target substitution yields no dangling sentinels", () => { | ||
| delete process.env["ALTIMATE_STARTER_SAMPLE_DIR"] | ||
| const location = resolveSampleSource() | ||
| // Every branch of this test suite runs from within the worktree checkout | ||
| // where the shipped sample tree lives at | ||
| // packages/opencode/sample-projects/jaffle-shop-duckdb/. If the resolver | ||
| // returns undefined here, the fallback candidate list is broken — that | ||
| // IS the failure mode this test exists to catch. Do not silently skip. | ||
| expect(location, "resolveSampleSource() returned undefined — the resolver's candidate-path list can no longer find the shipped sample tree in a dev checkout").toBeDefined() | ||
| const materializedTarget = "/tmp/materialized-target" | ||
| const manifest = loadShippedManifest(location!.path, materializedTarget) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore ALTIMATE_STARTER_SAMPLE_DIR after deleting it.
This test deletes the env var with no save/restore, so it permanently mutates process state for every test that runs after it in the same worker — the other tests in this file deliberately snapshot/restore.
🛡️ Proposed fix
- delete process.env["ALTIMATE_STARTER_SAMPLE_DIR"]
- const location = resolveSampleSource()
+ const origEnv = process.env["ALTIMATE_STARTER_SAMPLE_DIR"]
+ delete process.env["ALTIMATE_STARTER_SAMPLE_DIR"]
+ try {
+ const location = resolveSampleSource()
+ ...
+ } finally {
+ if (origEnv !== undefined) process.env["ALTIMATE_STARTER_SAMPLE_DIR"] = origEnv
+ }As per coding guidelines: tests using shared state "must provide teardown and isolation safe for parallel bun test execution."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("loadShippedManifest — end-to-end against the real shipped manifest", () => { | |
| test("loading the shipped sample's manifest.json with a target substitution yields no dangling sentinels", () => { | |
| delete process.env["ALTIMATE_STARTER_SAMPLE_DIR"] | |
| const location = resolveSampleSource() | |
| // Every branch of this test suite runs from within the worktree checkout | |
| // where the shipped sample tree lives at | |
| // packages/opencode/sample-projects/jaffle-shop-duckdb/. If the resolver | |
| // returns undefined here, the fallback candidate list is broken — that | |
| // IS the failure mode this test exists to catch. Do not silently skip. | |
| expect(location, "resolveSampleSource() returned undefined — the resolver's candidate-path list can no longer find the shipped sample tree in a dev checkout").toBeDefined() | |
| const materializedTarget = "/tmp/materialized-target" | |
| const manifest = loadShippedManifest(location!.path, materializedTarget) | |
| describe("loadShippedManifest — end-to-end against the real shipped manifest", () => { | |
| test("loading the shipped sample's manifest.json with a target substitution yields no dangling sentinels", () => { | |
| const origEnv = process.env["ALTIMATE_STARTER_SAMPLE_DIR"] | |
| delete process.env["ALTIMATE_STARTER_SAMPLE_DIR"] | |
| try { | |
| const location = resolveSampleSource() | |
| // Every branch of this test suite runs from within the worktree checkout | |
| // where the shipped sample tree lives at | |
| // packages/opencode/sample-projects/jaffle-shop-duckdb/. If the resolver | |
| // returns undefined here, the fallback candidate list is broken — that | |
| // IS the failure mode this test exists to catch. Do not silently skip. | |
| expect(location, "resolveSampleSource() returned undefined — the resolver's candidate-path list can no longer find the shipped sample tree in a dev checkout").toBeDefined() | |
| const materializedTarget = "/tmp/materialized-target" | |
| const manifest = loadShippedManifest(location!.path, materializedTarget) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/test/altimate/onboarding/sample-source-resolver.test.ts`
around lines 206 - 217, Update the test containing loadShippedManifest to
snapshot the current ALTIMATE_STARTER_SAMPLE_DIR value before deleting it and
restore that exact value in teardown, including removing the variable when it
was initially unset. Keep the existing resolver assertions and manifest-loading
behavior unchanged, and use the file’s established snapshot/restore pattern to
isolate process state for parallel tests.
Source: Coding guidelines
| import { afterEach, describe, expect, test } from "bun:test" | ||
| import fs from "node:fs" | ||
| import os from "node:os" | ||
| import path from "node:path" | ||
| import { _resetDbtRuntimeCacheForTests, detectDbtRuntime } from "../../../src/altimate/onboarding/tool-detection" | ||
|
|
||
| const ORIG_PATH = process.env.PATH ?? "" | ||
| const ORIG_ALTIMATE_DBT_PATH = process.env.ALTIMATE_DBT_PATH | ||
|
|
||
| afterEach(() => { | ||
| process.env.PATH = ORIG_PATH | ||
| if (ORIG_ALTIMATE_DBT_PATH === undefined) delete process.env.ALTIMATE_DBT_PATH | ||
| else process.env.ALTIMATE_DBT_PATH = ORIG_ALTIMATE_DBT_PATH | ||
| _resetDbtRuntimeCacheForTests() | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Isolate process state and clean up stub directories.
This suite mutates process.env and a module-global cache, while its os.tmpdir() stubs are never removed. Use per-test await using tmp = await tmpdir() and isolate the env-mutating probe (for example, dependency injection or a subprocess) so parallel Bun tests cannot observe each other’s state.
Based on learnings, new tests under packages/opencode/test/altimate/ should use await using tmp = await tmpdir(). As per coding guidelines, tests sharing global state must be isolated for parallel bun test execution.
Also applies to: 42-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/test/altimate/onboarding/tool-detection.test.ts` around
lines 13 - 27, Update the tests around detectDbtRuntime to use per-test `await
using tmp = await tmpdir()` so temporary directories and `os.tmpdir()` stubs are
cleaned up automatically. Isolate the process.env-mutating probe and
module-global cache usage—via dependency injection or a subprocess—so parallel
Bun tests cannot observe shared state; retain `_resetDbtRuntimeCacheForTests`
only where still necessary.
Sources: Coding guidelines, Learnings
| // resolveDbt: multi-manager search (venv/uv/pyenv/conda/pipx/poetry/… | ||
| // + PATH + explicit override via ALTIMATE_DBT_PATH). Returns the first | ||
| // candidate that exists. | ||
| const resolved = resolveDbt() |
There was a problem hiding this comment.
WARNING: resolveDbt() blocks the event loop, undercutting this file's async rationale
resolveDbt() (dbt-tools/src/dbt-resolve.ts:130/147/163) shells out via execFileSync — which dbt runs on every cache miss, plus pyenv which / asdf which (5s timeout each) when those shims exist. execFileSync blocks the Node main thread, so although probe() is async, those calls freeze the TUI for their full duration — the exact pattern lines 8-13 and 90-93 say they avoid by not using validateDbt. It is usually a few ms, but on a slow / NFS / automount PATH or a hung shim it stalls the UI for up to ~5-15s. Consider running the resolver off the event loop (async execFile, or a worker) so the probe is genuinely non-blocking.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // class as finding 21 / codex NEW-5. | ||
| const stat = fs.lstatSync(entryPath) | ||
| if (stat.isSymbolicLink()) continue | ||
| const ageMs = now - Math.max(stat.mtimeMs, stat.birthtimeMs || 0) |
There was a problem hiding this comment.
SUGGESTION: birthtimeMs term is dead weight and can break the orphan sweep
A directory's creation time (birthtimeMs) is always <= its mtimeMs, so Math.max(stat.mtimeMs, stat.birthtimeMs || 0) always reduces to stat.mtimeMs — the term never changes the result. It also backfires on birthtime-capable filesystems (APFS / NTFS): the orphan test backdates mtime via utimesSync (materialize.test.ts:490-491) but cannot backdate birthtime, so max picks the recent birth time, the orphan reads as young, and the both orphans gone assertion (test:508) fails there.
| const ageMs = now - Math.max(stat.mtimeMs, stat.birthtimeMs || 0) | |
| const ageMs = now - stat.mtimeMs |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| function hasSampleShape(dir: string): boolean { | ||
| try { | ||
| return fs.existsSync(path.join(dir, "dbt_project.yml")) |
There was a problem hiding this comment.
SUGGESTION: try/catch around fs.existsSync is dead code
fs.existsSync is documented to never throw — it swallows errors (including permission and broken-symlink cases) and returns false itself, so this catch branch can never run. The wrapper can be dropped to a bare return fs.existsSync(path.join(dir, 'dbt_project.yml')).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (state.kind === "our-sample-different-version" && !opts.allowInPlaceUpgrade) { | ||
| return { | ||
| targetPath, | ||
| reused: true, |
There was a problem hiding this comment.
SUGGESTION: reused: true for a version-mismatched target contradicts the field's docstring
The reused docstring (lines 135-137) says it is true only when the target already held our sample at the requested version, but this our-sample-different-version branch returns reused: true for an older version. The behavior is intentional and correct — sample_setup relies on reused: true plus a Caller must prompt note to drive its version-conflict prompt (sample-setup.ts:20-22) — but the narrow docstring is a trap: a maintainer who fixes the contradiction by flipping this to reused: false would silently break that prompt. Worth updating the docstring to cover the version-mismatch case.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
| fs.rmSync(targetPath, { recursive: true, force: true }) | ||
| } | ||
| fs.renameSync(stagingPath, targetPath) |
There was a problem hiding this comment.
SUGGESTION: rmSync then renameSync can leave the target deleted on failure
On the in-place upgrade path, rmSync(targetPath) (line 396) deletes the existing sample dir before renameSync(stagingPath, targetPath) moves the new one in. If the rename throws after the rm succeeded (e.g. the staging dir was removed externally -> ENOENT, or an EIO), the user's previous sample directory is gone and nothing replaces it; the catch only mentions staging left at .... A rename-aside sequence (rename the old dir to a sibling backup name, rename the new one in, then rm the backup) keeps targetPath populated on both success and failure.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
23 issues found across 33 files (changes from recent commits).
Not reviewed (too large): packages/opencode/sample-projects/jaffle-shop-duckdb/target/manifest.json (~16,839 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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/onboarding/sample-source-resolver.ts">
<violation number="1" location="packages/opencode/src/altimate/onboarding/sample-source-resolver.ts:145">
P3: A syntactically valid but non-object `target/manifest.json` is accepted and copied, leaving `/discover` and `/review` with an invalid manifest shape instead of the documented actionable failure. Validate that the parsed root is a non-null, non-array object before rehydrating it.</violation>
<violation number="2" location="packages/opencode/src/altimate/onboarding/sample-source-resolver.ts:162">
P2: Materializing under a path containing `{{SAMPLE_ROOT}}` corrupts manifest paths because the second split also replaces sentinel text introduced by `sampleRoot`/`sampleRootParent`. A single-pass replacement over the original string would preserve such user paths.</violation>
</file>
<file name="packages/opencode/src/util/filesystem.ts">
<violation number="1" location="packages/opencode/src/util/filesystem.ts:289">
P2: Passing a non-JSON-serializable value silently replaces the target with invalid JSON (`undefined\n`) instead of failing before the atomic replacement. Checking for an undefined serialization result before writing would preserve the helper's JSON contract.</violation>
<violation number="2" location="packages/opencode/src/util/filesystem.ts:293">
P3: Failed writes or renames leave `<targetPath>.tmp-*` behind, so disk-full or permission errors accumulate orphan files in every directory using this helper. Matching the cleanup behavior in `packages/tui/src/util/persistence.ts` would keep the atomic helper self-contained.</violation>
</file>
<file name="packages/opencode/script/publish.ts">
<violation number="1" location="packages/opencode/script/publish.ts:92">
P2: Standalone release users installing through the curl installer, Homebrew, or AUR will not receive the bundled sample, so the new sample onboarding flow cannot locate its source and `sample_setup` fails for those distributions. Packaging the sample into standalone archives as well, or explicitly disabling this onboarding path for those builds, would keep distribution behavior consistent.</violation>
</file>
<file name="packages/opencode/sample-projects/jaffle-shop-duckdb/sample-manifest.json">
<violation number="1" location="packages/opencode/sample-projects/jaffle-shop-duckdb/sample-manifest.json:2">
P3: `$comment` references `./regenerate.sh` but the script lives one directory up at `../regenerate.sh`. A maintainer following the instruction literally from the `jaffle-shop-duckdb/` directory will get a command-not-found error.</violation>
</file>
<file name="packages/opencode/src/altimate/onboarding/materialize.ts">
<violation number="1" location="packages/opencode/src/altimate/onboarding/materialize.ts:41">
P2: Release packaging does not call `readSampleAssets`; it reparses `sample-manifest.json` and copies each `asset.from` independently. The new validation and advertised single source of truth can therefore diverge between publishing and runtime, so sharing this parser with `publish.ts` would keep the security checks and asset behavior aligned.</violation>
<violation number="2" location="packages/opencode/src/altimate/onboarding/materialize.ts:292">
P2: Concurrent materializations through symlink aliases of the same parent are not serialized because `path.resolve` does not dereference symlinks; both calls can observe slot 0 as empty and race during removal/rename. Key the lock with the real parent path so the lock matches the physical target directory.</violation>
<violation number="3" location="packages/opencode/src/altimate/onboarding/materialize.ts:455">
P1: A user-owned directory named `.<preferredName>.tmp-*` can be recursively deleted after one hour because cleanup identifies candidates only by prefix and age, with no ownership marker. Restrict cleanup to staging entries carrying an internal marker, or avoid deleting arbitrary matching directories.</violation>
</file>
<file name="packages/opencode/sample-projects/regenerate.sh">
<violation number="1" location="packages/opencode/sample-projects/regenerate.sh:34">
P2: Regenerating unchanged sources with different installed dbt releases can change the committed manifest, contradicting the claim that diffs only change with source edits. Pin or validate the dbt-core/dbt-duckdb versions used for regeneration.</violation>
</file>
<file name="packages/opencode/src/altimate/onboarding/marker.ts">
<violation number="1" location="packages/opencode/src/altimate/onboarding/marker.ts:50">
P1: A symlinked marker can make an unrelated directory look like an Altimate sample, allowing the upgrade path to delete its contents. Reject marker symlinks and non-regular files before reading the marker.</violation>
<violation number="2" location="packages/opencode/src/altimate/onboarding/marker.ts:54">
P2: An `lstatSync` error is not evidence that the target is absent; only `ENOENT` should return `empty`. Other errors should return `unknown-dir` with the inspection failure so the no-overwrite policy remains fail-closed.</violation>
</file>
<file name="packages/opencode/src/command/template/onboard-connect.txt">
<violation number="1" location="packages/opencode/src/command/template/onboard-connect.txt:77">
P2: When the scan only finds a dbt profile, Docker database, or environment signal, this menu can offer cost analysis and other real-warehouse jobs before the connection is added and tested. Restrict the real-warehouse menu to already configured or successfully added/tested connections so options match what the environment can actually do.</violation>
<violation number="2" location="packages/opencode/src/command/template/onboard-connect.txt:129">
P3: Choosing Reset can leave the onboarding response after the upgrade tool call without the SAMPLE menu or a success/failure explanation. Handling the second call's result and presenting the menu on success keeps all version-conflict choices actionable.</violation>
<violation number="3" location="packages/opencode/src/command/template/onboard-connect.txt:201">
P3: The pasted dbt executable verification can reject valid dbt output because dbt indents its plugin lines. Matching after trimming leading whitespace, consistent with the existing runtime probe, avoids blocking users with a working adapter.</violation>
<violation number="4" location="packages/opencode/src/command/template/onboard-connect.txt:210">
P2: If `dbt build` fails, the flow still proceeds toward warehouse registration and a first query, which can present partial or stale data as the sample result. The query branch should continue only after a successful build, while reporting the failure and offering remediation otherwise.</violation>
<violation number="5" location="packages/opencode/src/command/template/onboard-connect.txt:210">
P2: A user can receive `dbt: present` even though dbt is only available through the resolver and is not on the shell's `PATH`; the instructed bare `dbt build` then fails with command-not-found. The flow should expose/use the resolved executable and environment, or explicitly validate a path before building.</violation>
<violation number="6" location="packages/opencode/src/command/template/onboard-connect.txt:213">
P2: The build-and-query path asks for a `dbt-profiles` tool, but this repository registers `dbt_profiles`; the tool call will fail instead of discovering the DuckDB profile. Using the registered underscore name keeps the onboarding path callable.</violation>
</file>
<file name="packages/opencode/test/altimate/onboarding/sample-source-resolver.test.ts">
<violation number="1" location="packages/opencode/test/altimate/onboarding/sample-source-resolver.test.ts:45">
P2: Gap in test isolation: the "no override + shipped sample" test doesn't guard against ALTIMATE_BIN_DIR leaking from the environment. If a developer runs this specific test with ALTIMATE_BIN_DIR set to a path containing a valid sample tree, the resolver returns `wrapper-bin-dir` instead of `dev-source-tree`.</violation>
</file>
<file name="packages/opencode/src/altimate/onboarding/tool-detection.ts">
<violation number="1" location="packages/opencode/src/altimate/onboarding/tool-detection.ts:87">
P2: Users with dbt in an unactivated project-local `.venv`/uv environment are reported as missing, so run workflows stay hidden even though the shared resolver supports those directories. Passing the current project root to `resolveDbt` would include this common installation layout.</violation>
<violation number="2" location="packages/opencode/src/altimate/onboarding/tool-detection.ts:87">
P2: The dbt probe can freeze the TUI for up to roughly 15 seconds before the async version subprocess starts. Moving only `validateDbt` off the path does not make `probe()` non-blocking; resolver discovery needs an async or worker-thread path.</violation>
<violation number="3" location="packages/opencode/src/altimate/onboarding/tool-detection.ts:102">
P2: A Windows user whose `ALTIMATE_DBT_PATH` points to a `.cmd`/`.bat` outside PATH is incorrectly reported as having no dbt. The fallback should invoke the resolved override through `cmd.exe /c` (handling paths with spaces) or add its directory to the fallback environment.</violation>
<violation number="4" location="packages/opencode/src/altimate/onboarding/tool-detection.ts:114">
P2: A colorized `dbt --version` can report `hasDbt=true` while incorrectly setting `hasDbtDuckdb=false. The parser should strip ANSI control sequences before matching the plugin and version lines.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| if (stat.isSymbolicLink()) continue | ||
| const ageMs = now - Math.max(stat.mtimeMs, stat.birthtimeMs || 0) | ||
| if (ageMs < ORPHAN_MAX_AGE_MS) continue // young — could be a live sibling | ||
| fs.rmSync(entryPath, { recursive: true, force: true }) |
There was a problem hiding this comment.
P1: A user-owned directory named .<preferredName>.tmp-* can be recursively deleted after one hour because cleanup identifies candidates only by prefix and age, with no ownership marker. Restrict cleanup to staging entries carrying an internal marker, or avoid deleting arbitrary matching directories.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/onboarding/materialize.ts, line 455:
<comment>A user-owned directory named `.<preferredName>.tmp-*` can be recursively deleted after one hour because cleanup identifies candidates only by prefix and age, with no ownership marker. Restrict cleanup to staging entries carrying an internal marker, or avoid deleting arbitrary matching directories.</comment>
<file context>
@@ -0,0 +1,521 @@
+ if (stat.isSymbolicLink()) continue
+ const ageMs = now - Math.max(stat.mtimeMs, stat.birthtimeMs || 0)
+ if (ageMs < ORPHAN_MAX_AGE_MS) continue // young — could be a live sibling
+ fs.rmSync(entryPath, { recursive: true, force: true })
+ } catch {
+ // orphan we can't stat/remove — skip, don't fail the fresh materialize
</file context>
| export function readMarker(dir: string): SampleMarker | undefined { | ||
| const markerPath = path.join(dir, MARKER_FILE_NAME) | ||
| try { | ||
| const raw = fs.readFileSync(markerPath, "utf8") |
There was a problem hiding this comment.
P1: A symlinked marker can make an unrelated directory look like an Altimate sample, allowing the upgrade path to delete its contents. Reject marker symlinks and non-regular files before reading the marker.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/onboarding/marker.ts, line 50:
<comment>A symlinked marker can make an unrelated directory look like an Altimate sample, allowing the upgrade path to delete its contents. Reject marker symlinks and non-regular files before reading the marker.</comment>
<file context>
@@ -0,0 +1,250 @@
+export function readMarker(dir: string): SampleMarker | undefined {
+ const markerPath = path.join(dir, MARKER_FILE_NAME)
+ try {
+ const raw = fs.readFileSync(markerPath, "utf8")
+ const parsed = JSON.parse(raw) as unknown
+ if (!isSampleMarker(parsed)) return undefined
</file context>
| } | ||
| // Replace both sentinels, longest-first so SAMPLE_ROOT never matches | ||
| // inside a preceding SAMPLE_ROOT_PARENT hit. | ||
| return value.split(SAMPLE_ROOT_PARENT_SENTINEL).join(sampleRootParent).split(SAMPLE_ROOT_SENTINEL).join(sampleRoot) |
There was a problem hiding this comment.
P2: Materializing under a path containing {{SAMPLE_ROOT}} corrupts manifest paths because the second split also replaces sentinel text introduced by sampleRoot/sampleRootParent. A single-pass replacement over the original string would preserve such user paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/onboarding/sample-source-resolver.ts, line 162:
<comment>Materializing under a path containing `{{SAMPLE_ROOT}}` corrupts manifest paths because the second split also replaces sentinel text introduced by `sampleRoot`/`sampleRootParent`. A single-pass replacement over the original string would preserve such user paths.</comment>
<file context>
@@ -0,0 +1,175 @@
+ }
+ // Replace both sentinels, longest-first so SAMPLE_ROOT never matches
+ // inside a preceding SAMPLE_ROOT_PARENT hit.
+ return value.split(SAMPLE_ROOT_PARENT_SENTINEL).join(sampleRootParent).split(SAMPLE_ROOT_SENTINEL).join(sampleRoot)
+ }
+ if (Array.isArray(value)) {
</file context>
| * @param targetPath Destination path. Parent directory is created if missing. | ||
| * @param value Value to serialize as JSON. | ||
| */ | ||
| export function writeJsonAtomic(targetPath: string, value: unknown): void { |
There was a problem hiding this comment.
P2: Passing a non-JSON-serializable value silently replaces the target with invalid JSON (undefined\n) instead of failing before the atomic replacement. Checking for an undefined serialization result before writing would preserve the helper's JSON contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/util/filesystem.ts, line 289:
<comment>Passing a non-JSON-serializable value silently replaces the target with invalid JSON (`undefined\n`) instead of failing before the atomic replacement. Checking for an undefined serialization result before writing would preserve the helper's JSON contract.</comment>
<file context>
@@ -271,6 +272,27 @@ export namespace Filesystem {
+ * @param targetPath Destination path. Parent directory is created if missing.
+ * @param value Value to serialize as JSON.
+ */
+ export function writeJsonAtomic(targetPath: string, value: unknown): void {
+ const tmpPath = `${targetPath}.tmp-${randomBytes(6).toString("hex")}`
+ mkdirSync(dirname(targetPath), { recursive: true })
</file context>
| // publish-parity.test.ts asserts every asset exists in the shipped | ||
| // package. Adding a new sample file? Edit sample-manifest.json. | ||
| const sampleSource = "./sample-projects/jaffle-shop-duckdb" | ||
| const sampleDest = `${targetDir}/sample-projects/jaffle-shop-duckdb` |
There was a problem hiding this comment.
P2: Standalone release users installing through the curl installer, Homebrew, or AUR will not receive the bundled sample, so the new sample onboarding flow cannot locate its source and sample_setup fails for those distributions. Packaging the sample into standalone archives as well, or explicitly disabling this onboarding path for those builds, would keep distribution behavior consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/script/publish.ts, line 92:
<comment>Standalone release users installing through the curl installer, Homebrew, or AUR will not receive the bundled sample, so the new sample onboarding flow cannot locate its source and `sample_setup` fails for those distributions. Packaging the sample into standalone archives as well, or explicitly disabling this onboarding path for those builds, would keep distribution behavior consistent.</comment>
<file context>
@@ -78,6 +78,40 @@ async function copyAssets(targetDir: string) {
+ // publish-parity.test.ts asserts every asset exists in the shipped
+ // package. Adding a new sample file? Edit sample-manifest.json.
+ const sampleSource = "./sample-projects/jaffle-shop-duckdb"
+ const sampleDest = `${targetDir}/sample-projects/jaffle-shop-duckdb`
+ const sampleManifest = JSON.parse(fs.readFileSync(`${sampleSource}/sample-manifest.json`, "utf8")) as {
+ assets: Array<{ from: string; kind: "file" | "dir"; required: boolean }>
</file context>
| ): Record<string, unknown> { | ||
| const manifestPath = path.join(sampleSource, "target", "manifest.json") | ||
| const raw = fs.readFileSync(manifestPath, "utf8") | ||
| const parsed = JSON.parse(raw) as unknown |
There was a problem hiding this comment.
P3: A syntactically valid but non-object target/manifest.json is accepted and copied, leaving /discover and /review with an invalid manifest shape instead of the documented actionable failure. Validate that the parsed root is a non-null, non-array object before rehydrating it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/onboarding/sample-source-resolver.ts, line 145:
<comment>A syntactically valid but non-object `target/manifest.json` is accepted and copied, leaving `/discover` and `/review` with an invalid manifest shape instead of the documented actionable failure. Validate that the parsed root is a non-null, non-array object before rehydrating it.</comment>
<file context>
@@ -0,0 +1,175 @@
+): Record<string, unknown> {
+ const manifestPath = path.join(sampleSource, "target", "manifest.json")
+ const raw = fs.readFileSync(manifestPath, "utf8")
+ const parsed = JSON.parse(raw) as unknown
+ const parentTarget = path.dirname(materializedTarget)
+ return rehydrateSentinels(parsed, materializedTarget, parentTarget) as Record<string, unknown>
</file context>
| const parsed = JSON.parse(raw) as unknown | |
| const parsed = JSON.parse(raw) as unknown | |
| if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) { | |
| throw new Error(`${manifestPath} must contain a JSON object`) | |
| } |
| const tmpPath = `${targetPath}.tmp-${randomBytes(6).toString("hex")}` | ||
| mkdirSync(dirname(targetPath), { recursive: true }) | ||
| writeFileSync(tmpPath, JSON.stringify(value, null, 2) + "\n") | ||
| renameSync(tmpPath, targetPath) |
There was a problem hiding this comment.
P3: Failed writes or renames leave <targetPath>.tmp-* behind, so disk-full or permission errors accumulate orphan files in every directory using this helper. Matching the cleanup behavior in packages/tui/src/util/persistence.ts would keep the atomic helper self-contained.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/util/filesystem.ts, line 293:
<comment>Failed writes or renames leave `<targetPath>.tmp-*` behind, so disk-full or permission errors accumulate orphan files in every directory using this helper. Matching the cleanup behavior in `packages/tui/src/util/persistence.ts` would keep the atomic helper self-contained.</comment>
<file context>
@@ -271,6 +272,27 @@ export namespace Filesystem {
+ const tmpPath = `${targetPath}.tmp-${randomBytes(6).toString("hex")}`
+ mkdirSync(dirname(targetPath), { recursive: true })
+ writeFileSync(tmpPath, JSON.stringify(value, null, 2) + "\n")
+ renameSync(tmpPath, targetPath)
+ }
+
</file context>
| @@ -0,0 +1,27 @@ | |||
| { | |||
| "$comment": "Metadata about this sample project. Distinct from dbt's target/manifest.json — this is our own version stamp for conflict detection on the materialized copy at ~/altimate-sample-dbt/. If you edit any source file below, run ./regenerate.sh and commit the refreshed target/manifest.json alongside your change.", | |||
There was a problem hiding this comment.
P3: $comment references ./regenerate.sh but the script lives one directory up at ../regenerate.sh. A maintainer following the instruction literally from the jaffle-shop-duckdb/ directory will get a command-not-found error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/sample-projects/jaffle-shop-duckdb/sample-manifest.json, line 2:
<comment>`$comment` references `./regenerate.sh` but the script lives one directory up at `../regenerate.sh`. A maintainer following the instruction literally from the `jaffle-shop-duckdb/` directory will get a command-not-found error.</comment>
<file context>
@@ -0,0 +1,27 @@
+{
+ "$comment": "Metadata about this sample project. Distinct from dbt's target/manifest.json — this is our own version stamp for conflict detection on the materialized copy at ~/altimate-sample-dbt/. If you edit any source file below, run ./regenerate.sh and commit the refreshed target/manifest.json alongside your change.",
+ "name": "jaffle-shop-duckdb",
+ "version": "1.0.0",
</file context>
| (`test -x '<path>'` — quote the path). | ||
| 3. Verify it's a dbt-duckdb binary by running the SINGLE-QUOTED | ||
| command `'<path>' --version 2>&1` via bash and checking that its | ||
| stdout contains a line starting with `- duckdb:`. Do NOT chain |
There was a problem hiding this comment.
P3: The pasted dbt executable verification can reject valid dbt output because dbt indents its plugin lines. Matching after trimming leading whitespace, consistent with the existing runtime probe, avoids blocking users with a working adapter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/command/template/onboard-connect.txt, line 201:
<comment>The pasted dbt executable verification can reject valid dbt output because dbt indents its plugin lines. Matching after trimming leading whitespace, consistent with the existing runtime probe, avoids blocking users with a working adapter.</comment>
<file context>
@@ -45,5 +69,161 @@ Then take the FIRST matching branch:
+ (`test -x '<path>'` — quote the path).
+ 3. Verify it's a dbt-duckdb binary by running the SINGLE-QUOTED
+ command `'<path>' --version 2>&1` via bash and checking that its
+ stdout contains a line starting with `- duckdb:`. Do NOT chain
+ with `grep -q` on user-controlled strings.
+ 4. If verified, remember the path and use `'<path>' build` (single-
</file context>
| stdout contains a line starting with `- duckdb:`. Do NOT chain | |
| stdout contains a line which, after trimming leading whitespace, starts with `- duckdb:`. Do NOT chain |
| version. Reset it in place (any local edits lost), keep it as-is, | ||
| or install the new version alongside (your old copy stays where | ||
| it is)?" Wait for a clear answer. | ||
| - Reset → call `sample_setup` again with |
There was a problem hiding this comment.
P3: Choosing Reset can leave the onboarding response after the upgrade tool call without the SAMPLE menu or a success/failure explanation. Handling the second call's result and presenting the menu on success keeps all version-conflict choices actionable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/command/template/onboard-connect.txt, line 129:
<comment>Choosing Reset can leave the onboarding response after the upgrade tool call without the SAMPLE menu or a success/failure explanation. Handling the second call's result and presenting the menu on success keeps all version-conflict choices actionable.</comment>
<file context>
@@ -45,5 +69,161 @@ Then take the FIRST matching branch:
+ version. Reset it in place (any local edits lost), keep it as-is,
+ or install the new version alongside (your old copy stays where
+ it is)?" Wait for a clear answer.
+ - Reset → call `sample_setup` again with
+ `allow_in_place_upgrade: true`
+ - Keep as-is → do NOT call sample_setup again; present the
</file context>
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Consensus re-review (round 3) — PR #1001
Multi-model consensus re-review (Claude + GPT 5.4 Codex, Kimi K2.5, MiniMax M2.7, Qwen 3.6, MiMo V2.5 Pro, GLM-5.1) of the latest head, verifying the previous follow-up and reviewing the expanded onboarding/materialization subsystem.
Thanks — the prior blocker and all six NEW items are addressed. The untrusted-api_url exfiltration blocker is fixed via isTrustedApiUrl (HTTPS + Altimate-host allowlist + loopback + ALTIMATE_API_URL); the 127.0.0.1 redirect (NEW-2), /connect try/catch (NEW-5), and neutral "Authorization received" copy (NEW-6) all landed; all 16 earlier findings hold. The new materialize.ts/marker.ts subsystem is notably defensive (path-traversal rejection, rejectUnsafeHome, symlink-aware containment, atomic staging, Flock, good tests), and the committed target/manifest.json is intentional and clean.
Keeping this at request changes for one correctness regression the panel verified in the code:
- Blocking — first-run gate uses the wrong readiness signal (
app.tsx:565): keyed off plugin-hostready()rather thansync.status, so a returning user can be re-shown the welcome picker + scan gate on a normal relaunch — the regressionb9332a8meant to fix. Inline below. - Recommended before merge — surface the auth failure reason (NEW-4): failures are currently opaque in the TUI.
The remaining inline comments (dismiss cleanup, scan-gate null-ref, cross-workspace import) are fast-follows. Nothing security/data/correctness beyond the gate regression. Full write-up available on request.
| let armScanGate = false | ||
| createEffect(() => { | ||
| if (firstRunPickerHandled) return | ||
| if (!ready()) return // wait until providers are loaded before deciding |
There was a problem hiding this comment.
Returning users can get re-shown the first-run picker + scan gate
MAJOR — The first-run gate keys off the wrong readiness signal. ready() is set in pluginHost.start().finally(() => setReady(true)) (app.tsx:411-424) — it tracks plugin-host startup only, not sync/provider load. But this guard's comment says "wait until providers are loaded," which isn't what ready() means. onboardingReady() = connected() || setupComplete(), and connected() depends on sync.data.provider, which loads asynchronously from the server.
If the plugin host settles before sync populates providers (plausible — provider data is a server round-trip), a returning user with valid credentials is transiently seen as onboardingReady() === false → armScanGate = true and DialogModelWelcome opens; when sync then completes, onboardingReady flips false→true and the scan-gate effect (:581) fires. Net: a welcome-picker flash + unwanted scan gate on a normal relaunch — the exact regression b9332a8 intended to prevent. setupComplete is in-memory only, so it can't compensate at startup, and no test covers this arming/transition.
Fix: also gate on sync readiness — if (!ready() || sync.status !== "complete") return (sync.status is already used at :508/:533). Add an App-level test with delayed sync asserting a pre-connected provider never opens the picker/scan gate.
| // Log the reason (CSRF / timeout / invalid instance / …). Runs in the | ||
| // server process, so this goes to the log, not the TUI display. | ||
| console.error("[altimate] gateway sign-in failed:", err instanceof Error ? err.message : err) | ||
| return { type: "failed" } |
There was a problem hiding this comment.
When sign-in fails, the user can't tell why
MINOR — Auth/callback failure reason isn't surfaced to the TUI. The plugin logs the real reason via console.error but returns bare { type: "failed" } here; ProviderAuth converts that to OauthCallbackFailed({}) with no payload (provider/auth.ts:265), and the TUI shows only a generic "OAuth authorization failed. Try /connect again." (dialog-provider.tsx:391-392). So timeout / untrusted-URL / invalid-instance / exchange-401 are all indistinguishable to the user — poor onboarding + debug UX for the exact flow this PR is about.
Fix: extend the failed result / OauthCallbackFailed schema with a sanitized message and render it in AutoMethod. (Low effort — Qwen called it "a one-line wiring fix.")
| return { type: "failed" } | ||
| } finally { | ||
| // Keep the shared server up while another flow is still waiting. | ||
| if (pending.size === 0) stopCallbackServer() |
There was a problem hiding this comment.
A dismissed sign-in can leave port 7317 bound for the process lifetime
MINOR — Dismissing the auth dialog doesn't cancel the server-side flow. void result.catch(() => {}) (:207) correctly removes the unhandled-rejection risk. But stopCallbackServer() only runs from callback()'s finally (here). If the TUI dialog is dismissed and callback() is never awaited to completion, the pending entry clears at the 5-minute timeout yet the loopback server can stay bound past it. Resource leak, not a security hole.
Fix: on dismiss, reject/cancel the pending flow by state and stop the server when pending.size === 0; also stop the server on the timeout path, not only in callback().finally.
| // asks what the user is working on and offers the activation | ||
| // menu (sample dbt, downstream impact, SQL PR, or free chat). | ||
| // Template lives at packages/opencode/src/command/template/onboard-connect.txt. | ||
| const ref = promptRef.current |
There was a problem hiding this comment.
The scan gate can silently discard the user's Yes/No choice
MINOR — Silent no-op if the prompt ref isn't mounted. onChoose does const ref = promptRef.current; if (!ref) return after the dialog has already cleared itself, so in any route/timing state where the prompt ref is unavailable the user's scan/skip choice is dropped with no feedback. This is largely a symptom of the same wrong-signal gating as the first-run finding (the gate can fire before the home prompt is ready).
Fix: toast/retry/navigate fallback when ref is null instead of returning silently.
| // this after the initial dedupe. We keep resolveDbt (its exec calls are | ||
| // cheap discovery-only) but do the actual version probe via async | ||
| // execFile below. | ||
| import { resolveDbt, buildDbtEnv } from "../../../../dbt-tools/src/dbt-resolve" |
There was a problem hiding this comment.
dbt detection reaches into another package's internal source file
MINOR — Cross-workspace internal-path coupling. This imports ../../../../dbt-tools/src/dbt-resolve — 5 levels up into another workspace package's internal source (to dodge its side-effectful src/index.ts; the package has no exports map). It couples opencode to dbt-tools' internal file layout, not a published API: a rename/move of dbt-resolve.ts breaks typecheck + runtime with no dependency contract surfacing it.
Fix: add a side-effect-free exports-mapped subpath to @altimateai/dbt-tools (e.g. @altimateai/dbt-tools/dbt-resolve) and import through it.
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Approving with changes requested, to avoid blocking timely merge.
2 participants
Summary
CLI onboarding for the Altimate LLM Gateway + a welcoming first-run experience, plus quick sign-in/out commands.
Gateway sign-in
:7317) + browser → web sign-up page → credential delivered back to the CLI; saved to~/.altimate/altimate.json(overridable viaALTIMATE_WEB_URL).First-run experience
WelcomePanelboot box on the home screen with readiness-aware tips (/connect→/discover); update toast suppressed until a model is ready.useReady/markSetupCompletegate the first-run chat lock (unlocks after connecting, choosing Big Pickle, or picking a ready model).Provider picker
/connectopens the curatedDialogModelWelcome— top 5 providers (Altimate Gateway first) + "Search all providers…", a Big Pickle fallback, and a bright-green ✓ "selected" marker on the currently-active provider/model./modeldialog is restructured into READY / NEEDS-SETUP sections.New TUI commands
/auth— start the Altimate LLM Gateway sign-in directly (DialogAltimateAuth)./logout— clear the saved gateway credential (AltimateApi.clearCredentials), disconnect, and reload.Rebase safety
component/altimate-onboarding.tsx;dialog-model.tsxtrimmed to pristineuseConnected+ the markedDialogModelrestructure, so future upstream (opencode) merges have a minimal, clearly-marked conflict surface. No behavior change.Test plan
/connect→ Altimate LLM Gateway → browser → provisioned + connected/agents/v1/models→200)/authlaunches gateway sign-in;/logoutclears creds + disconnectsbun run typecheckclean🤖 Generated with Claude Code
Summary by CodeRabbit
replace.sh).