feat: first-run activation menu + bundled dbt sample - #1046
Open
sahrizvi wants to merge 19 commits into
Open
Conversation
added 12 commits
July 24, 2026 19:33
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).
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.
…eview 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.
…r, 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.
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).
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.
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.
…ple-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.
`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.
…nd 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.
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.
…dal 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.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
added 2 commits
July 28, 2026 11:29
…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).
…e 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.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
added 4 commits
July 28, 2026 14:39
…ckaging 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).
- 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.
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.
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.
Contributor
Author
Review status — consensus + codex sweepReady for bot review. Local branch is at Fixed on this branch23 findings from the multi-model consensus review (safety, correctness, telemetry, tests) — see commits Codex sweep of the 4 rejected-claim classes — commits Deferred — on this branch's surface, chose not to fix nowBots may still want to flag these; posting here so the intent is transparent.
Pre-existing — not on this branch's surfaceCodex flagged these while auditing the rejection classes end-to-end. They live in
Follow-up test gap the PR doesn't cover
|
sahrizvi
marked this pull request as ready for review
July 28, 2026 11:22
![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.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| than `0` (a number like `1` or a hex string like `a1b2c3`) → The | ||
| preferred name was taken by unrelated content; the sample landed | ||
| at the suffixed path (see `path:`). Say: "Materialized the sample | ||
| at <path> (your existing directory wasn't ours, so I put it |
There was a problem hiding this comment.
[SUGGESTION]: Branch (d) wording is incorrect for the install-alongside path
Branch (b) routes an explicit "Install alongside" choice to sample_setup with install_alongside: true, then tells the agent to "follow branch (d) or (e)". That path materializes the new version into <name>-2 and returns reused: false + a non-zero suffix, which lands in branch (d). But the verbatim message here claims "your existing directory wasn't ours" — in the alongside case that directory IS our older-version sample (the user just chose to keep it), not unrelated content. After an explicit alongside choice this is misleading. Consider an alongside-specific follow-up (e.g. "Installed the new sample alongside your existing copy at ; your old version is untouched") rather than reusing branch (d)'s unrelated-content wording.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // "/tmp") can't be realpathed as a directory on all systems so we | ||
| // include them raw; the canonical versions catch the /private/tmp | ||
| // and /private/var/folders/... bypasses. | ||
| const refs: string[] = ["/tmp", canonicalize("/tmp"), os.tmpdir(), canonicalize(os.tmpdir())] |
There was a problem hiding this comment.
[SUGGESTION]: refs recomputes realpathSync on each check() call
check() rebuilds refs, calling canonicalize("/tmp") and canonicalize(os.tmpdir()) (each a fs.realpathSync syscall) on every invocation. check is called up to twice here (once for canonicalHome, once for the raw home), and neither canonical reference depends on the candidate, so those two realpath calls run redundantly. Hoist refs (or just the two canonical values) out of check so they are computed once.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Incremental NotesNew commit
No new issues introduced in the incremental diff. Previous Review Summary (commit ae31698)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit ae31698)Status: 2 Suggestions | Recommendation: Merge (non-blocking polish) Overview
Issue DetailsSUGGESTION
NotesSecurity-critical paths are well-hardened: Files Reviewed (10 source files)
Reviewed by glm-5.2 · Input: 28.2K · Output: 4K · Cached: 307.1K Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
15 issues found across 32 files
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/sample-projects/jaffle-shop-duckdb/sample-manifest.json">
<violation number="1" location="packages/opencode/sample-projects/jaffle-shop-duckdb/sample-manifest.json:7">
P2: `requires` field declares dbt version constraints that are never enforced at runtime — the manifest is the source of truth for sample version (`version`), but the `requires.dbt-core` and `requires.dbt-duckdb` constraints have no consumers. If someone has dbt-core 1.6.x or dbt-duckdb 2.x there's no early validation, just a late failure when the dbt commands themselves break. Consider either wiring the constraints into the tool-detection probe or documenting in a comment that they're informational-only.</violation>
</file>
<file name="packages/opencode/src/altimate/onboarding/marker.ts">
<violation number="1" location="packages/opencode/src/altimate/onboarding/marker.ts:121">
P1: A different bundled sample using the same target name and version is treated as this sample. Compare the marker's `sampleName` with the requested sample before returning either `our-sample-*` state; otherwise setup can reuse the wrong project, or an approved upgrade can replace it.</violation>
</file>
<file name="packages/opencode/test/altimate/onboarding/publish-parity.test.ts">
<violation number="1" location="packages/opencode/test/altimate/onboarding/publish-parity.test.ts:28">
P2: The hardcoded MATERIALIZE_ENTRIES list in this test is a manual copy from materialize.ts, creating a one-directional sync risk. The test verifies publish.ts against its own list, but it cannot detect when materialize.ts adds entries that this test list, publish.ts, or both miss. A maintainer adding to materialize.ts has no automated signal if they forget to update this file. Consider adding a cross-check that reads materialize.ts's source, extracts the `from` values (e.g., via a regex or AST parse), and asserts they match the hardcoded list here — that enforces sync in both directions while keeping the publish.ts parity check independent of import-chain shadowing.</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:46">
P2: Global state leak: `delete process.env["ALTIMATE_STARTER_SAMPLE_DIR"]` at line 206 mutates shared environment without restoration. Other test files running in parallel (bun's default) may read a missing var, causing flaky failures. The other tests in this file wrap env mutations in try/finally; this one should follow the same pattern.</violation>
</file>
<file name="packages/opencode/sample-projects/regenerate.sh">
<violation number="1" location="packages/opencode/sample-projects/regenerate.sh:9">
P2: Schema YAML edits can merge with a stale precompiled manifest: the advertised freshness guard only hashes SQL and seed files. Extend the guard to fingerprint `dbt_project.yml`, `profiles.yml`, and schema YAML (or remove the claim that every source edit is caught).</violation>
<violation number="2" location="packages/opencode/sample-projects/regenerate.sh:63">
P3: The sanitized manifest records node/macro `created_at` timestamps on 2026-07-25 while metadata timestamps are pinned to 2026-07-24, so consumers comparing these fields see an internally inconsistent generated-at timeline. Using the epoch corresponding to `FIXED_ISO` keeps the promised release-day timestamp consistent.</violation>
</file>
<file name="packages/opencode/sample-projects/jaffle-shop-duckdb/README.md">
<violation number="1" location="packages/opencode/sample-projects/jaffle-shop-duckdb/README.md:8">
P3: The file tree in the 'What's in here' section omits `.gitignore`, which is one of the shipped files (confirmed via `MATERIALIZE_ENTRIES` in `materialize.ts`). Users who materialize the sample will get a `.gitignore` without seeing it documented. Add it to the tree so the listing stays accurate.</violation>
</file>
<file name="packages/opencode/sample-projects/jaffle-shop-duckdb/models/marts/schema.yml">
<violation number="1" location="packages/opencode/sample-projects/jaffle-shop-duckdb/models/marts/schema.yml:21">
P2: orders model schema omits `customer_name`, `order_date`, and `amount` columns that the SQL model produces. Same discoverability gap as the customers model — these columns won't surface in dbt docs or the review UI.</violation>
</file>
<file name="packages/opencode/src/altimate/onboarding/materialize.ts">
<violation number="1" location="packages/opencode/src/altimate/onboarding/materialize.ts:402">
P2: An unrelated old file or directory named like `.<preferredTargetName>.tmp-*` is recursively deleted during setup. Restrict cleanup to staging directories that can be positively identified as materializer-owned, or leave ambiguous entries untouched.</violation>
<violation number="2" location="packages/opencode/src/altimate/onboarding/materialize.ts:423">
P2: Incomplete shipped assets are reported as a successful materialization, then permanently reused, so offline discover/review fails later without setup surfacing the packaging error. Skip only optional `.gitignore`; fail for every required entry.</violation>
</file>
<file name="packages/opencode/src/command/template/onboard-connect.txt">
<violation number="1" location="packages/opencode/src/command/template/onboard-connect.txt:183">
P1: A pasted dbt path can execute extra shell commands because it is interpolated directly into a bash pipeline. Pass it as a separately escaped argument (or use a shell-less runner) before checking `--version` and building.</violation>
<violation number="2" location="packages/opencode/src/command/template/onboard-connect.txt:194">
P1: The sample query flow can connect to a new/empty DuckDB file instead of the database just built, because its profile path is relative and `warehouse_add` receives it unchanged. Convert the discovered DuckDB path to an absolute path under the sample directory before registering the connection.</violation>
</file>
<file name="packages/opencode/test/altimate/onboarding/marker.test.ts">
<violation number="1" location="packages/opencode/test/altimate/onboarding/marker.test.ts:82">
P2: classifyTarget has two security-relevant branches (symlink target → unknown-dir and unreadable directory → unknown-dir) that are not covered by any test, despite the source's explicit lstatSync-based symlink detection and EACCES handling. Consider adding tests for these paths to maintain parity with the source's safety contract.</violation>
</file>
<file name="packages/opencode/sample-projects/jaffle-shop-duckdb/verify-freshness.test.ts">
<violation number="1" location="packages/opencode/sample-projects/jaffle-shop-duckdb/verify-freshness.test.ts:16">
P2: The hash algorithm is described with two contradictory contracts: the block docstring says `rstrip("\n")` (ALL trailing newlines), the function JSDoc says only one trailing newline. The implementation strips one `\n`, matching the JSDoc but not the block comment. Pick one contract and align all three — or if single-strip is intentional, update the block docstring so future maintainers aren't misled about which algorithm the test actually implements.</violation>
</file>
<file name="packages/opencode/sample-projects/jaffle-shop-duckdb/profiles.yml">
<violation number="1" location="packages/opencode/sample-projects/jaffle-shop-duckdb/profiles.yml:3">
P3: Comment incorrectly states that dbt-duckdb resolves the `path` against the process working directory. Per the official dbt-duckdb documentation, the `path` is resolved relative to the profiles.yml file location by default. Since this profiles.yml lives at the project root, the practical behavior is the same (the database lands at `<project>/target/jaffle.duckdb` when running from the project directory), but the comment's explanation is wrong and will mislead anyone trying to understand or debug the path resolution if they run dbt from a different directory while pointing at this profiles.yml.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| reason: "directory not empty and has no altimate-code marker (would clobber unrelated content)", | ||
| } | ||
| } | ||
| if (marker.version === expectedVersion) { |
There was a problem hiding this comment.
P1: A different bundled sample using the same target name and version is treated as this sample. Compare the marker's sampleName with the requested sample before returning either our-sample-* state; otherwise setup can reuse the wrong project, or an approved upgrade can replace 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/marker.ts, line 121:
<comment>A different bundled sample using the same target name and version is treated as this sample. Compare the marker's `sampleName` with the requested sample before returning either `our-sample-*` state; otherwise setup can reuse the wrong project, or an approved upgrade can replace it.</comment>
<file context>
@@ -0,0 +1,221 @@
+ reason: "directory not empty and has no altimate-code marker (would clobber unrelated content)",
+ }
+ }
+ if (marker.version === expectedVersion) {
+ return { kind: "our-sample-current", marker, path: dir }
+ }
</file context>
| 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, then register | ||
| it as a warehouse connection via `warehouse_add` — otherwise `sql_execute` |
There was a problem hiding this comment.
P1: The sample query flow can connect to a new/empty DuckDB file instead of the database just built, because its profile path is relative and warehouse_add receives it unchanged. Convert the discovered DuckDB path to an absolute path under the sample directory before registering the connection.
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 194:
<comment>The sample query flow can connect to a new/empty DuckDB file instead of the database just built, because its profile path is relative and `warehouse_add` receives it unchanged. Convert the discovered DuckDB path to an absolute path under the sample directory before registering the connection.</comment>
<file context>
@@ -45,5 +69,132 @@ Then take the FIRST matching branch:
+ 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, then register
+ it as a warehouse connection via `warehouse_add` — otherwise `sql_execute`
+ has nothing to connect to. Then offer a first query and run it with
+ `sql_execute`.
</file context>
| the DuckDB adapter), then say "ready" and I'll continue. | ||
|
|
||
| If the user pastes a path (option 1), verify it works with | ||
| `<path> --version 2>&1 | grep -q "duckdb:"` via bash and use that binary |
There was a problem hiding this comment.
P1: A pasted dbt path can execute extra shell commands because it is interpolated directly into a bash pipeline. Pass it as a separately escaped argument (or use a shell-less runner) before checking --version and building.
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 183:
<comment>A pasted dbt path can execute extra shell commands because it is interpolated directly into a bash pipeline. Pass it as a separately escaped argument (or use a shell-less runner) before checking `--version` and building.</comment>
<file context>
@@ -45,5 +69,132 @@ Then take the FIRST matching branch:
+ the DuckDB adapter), then say "ready" and I'll continue.
+
+ If the user pastes a path (option 1), verify it works with
+ `<path> --version 2>&1 | grep -q "duckdb:"` via bash and use that binary
+ explicitly for the build (`<path> build` instead of `dbt build`). If they
+ install fresh (option 2) and say "ready", call `sample_setup` again — the
</file context>
| "version": "1.0.0", | ||
| "kind": "altimate-starter-sample", | ||
| "source": "packages/opencode/sample-projects/jaffle-shop-duckdb", | ||
| "requires": { |
There was a problem hiding this comment.
P2: requires field declares dbt version constraints that are never enforced at runtime — the manifest is the source of truth for sample version (version), but the requires.dbt-core and requires.dbt-duckdb constraints have no consumers. If someone has dbt-core 1.6.x or dbt-duckdb 2.x there's no early validation, just a late failure when the dbt commands themselves break. Consider either wiring the constraints into the tool-detection probe or documenting in a comment that they're informational-only.
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 7:
<comment>`requires` field declares dbt version constraints that are never enforced at runtime — the manifest is the source of truth for sample version (`version`), but the `requires.dbt-core` and `requires.dbt-duckdb` constraints have no consumers. If someone has dbt-core 1.6.x or dbt-duckdb 2.x there's no early validation, just a late failure when the dbt commands themselves break. Consider either wiring the constraints into the tool-detection probe or documenting in a comment that they're informational-only.</comment>
<file context>
@@ -0,0 +1,16 @@
+ "version": "1.0.0",
+ "kind": "altimate-starter-sample",
+ "source": "packages/opencode/sample-projects/jaffle-shop-duckdb",
+ "requires": {
+ "dbt-core": ">=1.7 <2.0",
+ "dbt-duckdb": ">=1.7 <2.0"
</file context>
| // import chain re-exported it — a re-export shadow that always agrees | ||
| // with itself is not a real cross-check. The lint is against the shape | ||
| // publish.ts actually writes on disk. | ||
| const MATERIALIZE_ENTRIES = [ |
There was a problem hiding this comment.
P2: The hardcoded MATERIALIZE_ENTRIES list in this test is a manual copy from materialize.ts, creating a one-directional sync risk. The test verifies publish.ts against its own list, but it cannot detect when materialize.ts adds entries that this test list, publish.ts, or both miss. A maintainer adding to materialize.ts has no automated signal if they forget to update this file. Consider adding a cross-check that reads materialize.ts's source, extracts the from values (e.g., via a regex or AST parse), and asserts they match the hardcoded list here — that enforces sync in both directions while keeping the publish.ts parity check independent of import-chain shadowing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/altimate/onboarding/publish-parity.test.ts, line 28:
<comment>The hardcoded MATERIALIZE_ENTRIES list in this test is a manual copy from materialize.ts, creating a one-directional sync risk. The test verifies publish.ts against its own list, but it cannot detect when materialize.ts adds entries that this test list, publish.ts, or both miss. A maintainer adding to materialize.ts has no automated signal if they forget to update this file. Consider adding a cross-check that reads materialize.ts's source, extracts the `from` values (e.g., via a regex or AST parse), and asserts they match the hardcoded list here — that enforces sync in both directions while keeping the publish.ts parity check independent of import-chain shadowing.</comment>
<file context>
@@ -0,0 +1,70 @@
+// import chain re-exported it — a re-export shadow that always agrees
+// with itself is not a real cross-check. The lint is against the shape
+// publish.ts actually writes on disk.
+const MATERIALIZE_ENTRIES = [
+ "README.md",
+ "dbt_project.yml",
</file context>
|
|
||
| ## What's in here | ||
|
|
||
| ``` |
There was a problem hiding this comment.
P3: The file tree in the 'What's in here' section omits .gitignore, which is one of the shipped files (confirmed via MATERIALIZE_ENTRIES in materialize.ts). Users who materialize the sample will get a .gitignore without seeing it documented. Add it to the tree so the listing stays accurate.
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/README.md, line 8:
<comment>The file tree in the 'What's in here' section omits `.gitignore`, which is one of the shipped files (confirmed via `MATERIALIZE_ENTRIES` in `materialize.ts`). Users who materialize the sample will get a `.gitignore` without seeing it documented. Add it to the tree so the listing stays accurate.</comment>
<file context>
@@ -0,0 +1,55 @@
+
+## What's in here
+
+```
+dbt_project.yml dbt project config
+profiles.yml DuckDB profile — path is project-relative
</file context>
| SENTINEL_ROOT = "{{SAMPLE_ROOT}}" | ||
| SENTINEL_PARENT = "{{SAMPLE_ROOT_PARENT}}" | ||
| FIXED_ISO = "2026-07-24T00:00:00Z" | ||
| FIXED_EPOCH = 1785000000.0 # 2026-07-24 near midnight UTC; matches FIXED_ISO closely enough |
There was a problem hiding this comment.
P3: The sanitized manifest records node/macro created_at timestamps on 2026-07-25 while metadata timestamps are pinned to 2026-07-24, so consumers comparing these fields see an internally inconsistent generated-at timeline. Using the epoch corresponding to FIXED_ISO keeps the promised release-day timestamp consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/sample-projects/regenerate.sh, line 63:
<comment>The sanitized manifest records node/macro `created_at` timestamps on 2026-07-25 while metadata timestamps are pinned to 2026-07-24, so consumers comparing these fields see an internally inconsistent generated-at timeline. Using the epoch corresponding to `FIXED_ISO` keeps the promised release-day timestamp consistent.</comment>
<file context>
@@ -0,0 +1,121 @@
+SENTINEL_ROOT = "{{SAMPLE_ROOT}}"
+SENTINEL_PARENT = "{{SAMPLE_ROOT_PARENT}}"
+FIXED_ISO = "2026-07-24T00:00:00Z"
+FIXED_EPOCH = 1785000000.0 # 2026-07-24 near midnight UTC; matches FIXED_ISO closely enough
+ZERO_UUID = "00000000-0000-0000-0000-000000000000"
+
</file context>
Comment on lines
+3
to
+8
| # `path:` is unqualified, so dbt-duckdb resolves it against the PROCESS | ||
| # working directory at build time — NOT the project directory. Run | ||
| # `dbt build` from the materialized sample dir (`cd <sample-path>`) and | ||
| # the database lands at `<sample-path>/target/jaffle.duckdb`. | ||
| # Run it from anywhere else and dbt writes to `$PWD/target/jaffle.duckdb`. | ||
| jaffle_shop: |
There was a problem hiding this comment.
P3: Comment incorrectly states that dbt-duckdb resolves the path against the process working directory. Per the official dbt-duckdb documentation, the path is resolved relative to the profiles.yml file location by default. Since this profiles.yml lives at the project root, the practical behavior is the same (the database lands at <project>/target/jaffle.duckdb when running from the project directory), but the comment's explanation is wrong and will mislead anyone trying to understand or debug the path resolution if they run dbt from a different directory while pointing at this profiles.yml.
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/profiles.yml, line 3:
<comment>Comment incorrectly states that dbt-duckdb resolves the `path` against the process working directory. Per the official dbt-duckdb documentation, the `path` is resolved relative to the profiles.yml file location by default. Since this profiles.yml lives at the project root, the practical behavior is the same (the database lands at `<project>/target/jaffle.duckdb` when running from the project directory), but the comment's explanation is wrong and will mislead anyone trying to understand or debug the path resolution if they run dbt from a different directory while pointing at this profiles.yml.</comment>
<file context>
@@ -0,0 +1,14 @@
+# DuckDB profile — everything runs locally against a single file at
+# `target/jaffle.duckdb` (created on first `dbt build`). No cloud credentials.
+# `path:` is unqualified, so dbt-duckdb resolves it against the PROCESS
+# working directory at build time — NOT the project directory. Run
+# `dbt build` from the materialized sample dir (`cd <sample-path>`) and
</file context>
Suggested change
| # `path:` is unqualified, so dbt-duckdb resolves it against the PROCESS | |
| # working directory at build time — NOT the project directory. Run | |
| # `dbt build` from the materialized sample dir (`cd <sample-path>`) and | |
| # the database lands at `<sample-path>/target/jaffle.duckdb`. | |
| # Run it from anywhere else and dbt writes to `$PWD/target/jaffle.duckdb`. | |
| jaffle_shop: | |
| # DuckDB profile — everything runs locally against a single file at | |
| # `target/jaffle.duckdb` (created on first `dbt build`). No cloud credentials. | |
| # `path:` is unqualified, so dbt-duckdb resolves it relative to this | |
| # `profiles.yml` location (the project directory). Run | |
| # `dbt build` from the materialized sample dir (`cd <sample-path>`) and | |
| # the database lands at `<sample-path>/target/jaffle.duckdb`. | |
| # Run it from anywhere else and behavior depends on where dbt finds this profile. |
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue for this PR
Closes #1045
Type of change
What does this PR do?
Two coupled features on top of the scan-gate work from the base branch:
/onboard-connectscan branch (all 4), the agent appends a JTBD-worded menu (see-downstream / review-SQL / try-sample / describe-your-own). Warehouse-connected users get a personalized variant with cost-analysis. Scan-gate "No" now dispatches/onboard-connect skipso the template's skip branch runs and the menu appears in the chat.packages/opencode/sample-projects/, materialized to the user's home by a newsample_setuptool. Idempotent, non-destructive (suffix variants on collision), HOME-safety-guarded (rejects/root,/tmp/*,/), versioned marker for a future upgrade flow. Ships with a pre-compiledtarget/manifest.jsonso/discoverand/reviewwork offline out of the box.Files touched:
packages/opencode/src/command/template/onboard-connect.txt— activation menu appended after every scan branch, with warehouse-connected + no-data variants, and a sample-mode follow-up menu withdbt build+sql_executerouting that handles missingdbt-duckdbgracefully.packages/opencode/src/altimate/tools/sample-setup.ts— LLM-facing tool.packages/opencode/src/altimate/onboarding/{marker,materialize,sample-source-resolver,tool-detection}.ts— supporting logic.packages/opencode/sample-projects/jaffle-shop-duckdb/— bundled sample (2 seeds, 4 models, schema tests, pre-compiled manifest).packages/opencode/script/publish.ts— copies the sample tree into the wrapper package at publish time.packages/tui/src/app.tsx— scan-gate "No" now dispatches/onboard-connect skip(reverts the later close-only change on the base branch).How did you verify your code works?
bun turbo typecheck→ 13/13 packages pass.sample_setupcontract — all pass.Screenshots / recordings
Local MP4 recordings captured for each branch (dialog navigation via a mocked auth path, plus real-gateway LLM output for skip / scan-empty / scan-detected / sample flow). Not committed to the repo — they're regeneratable from the VHS tape scripts in the reviewer's local environment.
Checklist
Codex review — addressed
sample_setup— regex allowlist on Zod schema + defense-in-depth check inmaterializeSample+ post-path.resolvecontainment assertion + 19 adversarial tests.github/meta/merge-commit.txtverified pre-existing on base branch (not our commit)sample_setupreturn-state branching — 5 explicit template branches for error / version-conflict / plain-reuse / suffix-collision / fresh, with prompt-user language for the "different version" case.tmp-<hex>staging +fs.renameSync+ orphan-sweep on next run; 2 new teststool-detection.test.tsrewritten to invokedetectDbtRuntimevia PATH-override with a bash-scriptdbtstub; 8 real subprocess testssample-source-resolver.test.tsno longer returns early on missing source (expect(location).toBeDefined()).gitignoremarker — verified as non-issue; repo convention doesn't wrap.gitignoreinaltimate_changemarkers (grep for markers in.gitignorereturns 0 across the tree)