Skip to content

Commit 6c8b445

Browse files
sahrizvisahrizvi
committed
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.
1 parent 89028ff commit 6c8b445

5 files changed

Lines changed: 197 additions & 40 deletions

File tree

‎packages/opencode/sample-projects/jaffle-shop-duckdb/verify-freshness.test.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,46 @@ describe("verify-freshness — committed manifest matches source files", () => {
9494
expect(failures).toEqual([])
9595
})
9696

97+
test("shipped source files are all represented in the manifest (codex NEW-10 — set membership, not just per-node hashing)", () => {
98+
// Companion to the per-node hash test. That test iterates nodes and
99+
// hashes what's there; it says nothing about whether new source files
100+
// added under models/ or seeds/ actually made it into the manifest.
101+
// A maintainer who adds a new .sql model but forgets to re-run
102+
// regenerate.sh would sail past the per-node hash check (the model
103+
// simply has no manifest node to compare against). This test locks
104+
// the source SET so that gap is caught.
105+
const manifest = JSON.parse(fs.readFileSync(MANIFEST_PATH, "utf8")) as {
106+
nodes?: Record<string, DbtNode>
107+
}
108+
const nodes = manifest.nodes ?? {}
109+
const manifestPaths = new Set<string>()
110+
for (const node of Object.values(nodes)) {
111+
if (node.original_file_path) manifestPaths.add(node.original_file_path)
112+
}
113+
// Enumerate the actual source tree the maintainer curates. Only files
114+
// dbt itself would compile: .sql models, .csv seeds. Docs, YAML,
115+
// manifest metadata files are not per-file compiled — dbt notices
116+
// schema.yml through its own parser and doesn't emit a checksum'd
117+
// node for it.
118+
const expectedFiles: string[] = []
119+
const walk = (relDir: string, exts: RegExp) => {
120+
const absDir = path.join(SAMPLE_DIR, relDir)
121+
if (!fs.existsSync(absDir)) return
122+
for (const entry of fs.readdirSync(absDir, { withFileTypes: true })) {
123+
const relPath = path.join(relDir, entry.name)
124+
if (entry.isDirectory()) walk(relPath, exts)
125+
else if (exts.test(entry.name)) expectedFiles.push(relPath)
126+
}
127+
}
128+
walk("models", /\.sql$/)
129+
walk("seeds", /\.csv$/)
130+
const missing = expectedFiles.filter((f) => !manifestPaths.has(f))
131+
expect(
132+
missing,
133+
`source files exist under models/ or seeds/ but have no manifest node — re-run sample-projects/regenerate.sh: ${missing.join(", ")}`,
134+
).toEqual([])
135+
})
136+
97137
test("manifest identity fields are scrubbed (no maintainer UUID or wall-clock times leaked to installers)", () => {
98138
// Companion to regenerate.sh's sanitizer: identity + wall-clock fields
99139
// must be zeroed/pinned. If a maintainer runs `dbt compile` by hand

‎packages/opencode/src/altimate/onboarding/marker.ts‎

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,14 +72,30 @@ export function writeMarker(dir: string, marker: SampleMarker): void {
7272
* - No such dir → empty (safe to create + materialize)
7373
* - Empty dir → empty (safe to materialize into)
7474
* - Has our marker,
75+
* sampleName differs → unknown-dir (marker is ours but belongs to
76+
* a DIFFERENT sample — never reuse or upgrade
77+
* through it; treat as a foreign directory)
78+
* - Has our marker,
79+
* sampleName matches,
7580
* version matches → our-sample-current (reuse — nothing to do)
7681
* - Has our marker,
82+
* sampleName matches,
7783
* version differs → our-sample-different-version (offer upgrade)
7884
* - Non-empty dir,
7985
* no marker (or bad kind) → unknown-dir (NEVER overwrite; caller must
8086
* suffix `-2`, `-3` etc. or refuse)
87+
*
88+
* `expectedSampleName` gates the marker's `sampleName` field so an existing
89+
* sample-A marker never satisfies a sample-B request. Currently the shipped
90+
* CLI only carries jaffle-shop-duckdb, so this branch is unreachable in
91+
* practice — but a future second sample must not silently reuse an existing
92+
* first-sample dir just because the version happens to match.
8193
*/
82-
export function classifyTarget(dir: string, expectedVersion: string): TargetState {
94+
export function classifyTarget(
95+
dir: string,
96+
expectedVersion: string,
97+
expectedSampleName: string,
98+
): TargetState {
8399
// lstatSync (not statSync) so a symlinked target is classified as
84100
// unknown-dir rather than by what it points at. If a user has
85101
// ~/altimate-sample-dbt -> /somewhere-else, we must not "reuse" the
@@ -118,6 +134,19 @@ export function classifyTarget(dir: string, expectedVersion: string): TargetStat
118134
reason: "directory not empty and has no altimate-code marker (would clobber unrelated content)",
119135
}
120136
}
137+
// Gate on sampleName BEFORE version. A marker whose sampleName differs
138+
// from what we're materializing is not "ours" for THIS request — even
139+
// though it was written by an altimate-code CLI. Treat it as a foreign
140+
// directory: never reuse-through it, never authorize an
141+
// allowInPlaceUpgrade against it. Falls into the same suffix escalation
142+
// path as an unrelated non-sample dir.
143+
if (marker.sampleName !== expectedSampleName) {
144+
return {
145+
kind: "unknown-dir",
146+
path: dir,
147+
reason: `marker belongs to sample '${marker.sampleName}', not '${expectedSampleName}' — refusing to reuse or overwrite a different sample`,
148+
}
149+
}
121150
if (marker.version === expectedVersion) {
122151
return { kind: "our-sample-current", marker, path: dir }
123152
}
@@ -167,6 +196,7 @@ export function findSafeTarget(
167196
parentDir: string,
168197
preferredName: string,
169198
expectedVersion: string,
199+
expectedSampleName: string,
170200
attemptLimit: number = 100,
171201
opts: {
172202
/** When true, treat `our-sample-different-version` the same as `unknown-dir`
@@ -186,7 +216,7 @@ export function findSafeTarget(
186216
for (let i = 0; i < attemptLimit; i++) {
187217
const name = i === 0 ? preferredName : `${preferredName}-${i + 1}`
188218
const candidate = path.join(parentDir, name)
189-
const state = classifyTarget(candidate, expectedVersion)
219+
const state = classifyTarget(candidate, expectedVersion, expectedSampleName)
190220
if (!skippable(state.kind)) return { path: candidate, state, suffix: i }
191221
consecutiveSkipped++
192222
// The parent has enough unrelated content that continuing the numeric
@@ -199,7 +229,7 @@ export function findSafeTarget(
199229
const randomTag = randomBytes(3).toString("hex")
200230
const randomName = `${preferredName}-${randomTag}`
201231
const randomCandidate = path.join(parentDir, randomName)
202-
const state = classifyTarget(randomCandidate, expectedVersion)
232+
const state = classifyTarget(randomCandidate, expectedVersion, expectedSampleName)
203233
if (!skippable(state.kind)) {
204234
return { path: randomCandidate, state, suffix: randomTag }
205235
}

‎packages/opencode/src/altimate/onboarding/materialize.ts‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,20 @@ import { DEFAULT_SAMPLE_NAME, loadShippedManifest, resolveSampleSource, type Sam
3131
/** Files/dirs relative to the sample source that get materialized to the
3232
* user's target dir. Explicitly enumerated (no glob) so future changes
3333
* to the sample layout are a deliberate opt-in edit here. */
34-
const MATERIALIZE_ENTRIES: ReadonlyArray<{ from: string; kind: "file" | "dir" }> = [
35-
{ from: "README.md", kind: "file" },
36-
{ from: "dbt_project.yml", kind: "file" },
37-
{ from: "profiles.yml", kind: "file" },
38-
{ from: "sample-manifest.json", kind: "file" },
39-
{ from: ".gitignore", kind: "file" },
40-
{ from: "models", kind: "dir" },
41-
{ from: "seeds", kind: "dir" },
42-
{ from: "target/manifest.json", kind: "file" },
34+
const MATERIALIZE_ENTRIES: ReadonlyArray<{ from: string; kind: "file" | "dir"; required: boolean }> = [
35+
{ from: "README.md", kind: "file", required: true },
36+
{ from: "dbt_project.yml", kind: "file", required: true },
37+
{ from: "profiles.yml", kind: "file", required: true },
38+
{ from: "sample-manifest.json", kind: "file", required: true },
39+
// .gitignore is the ONLY optional entry — dev checkouts may lack it and
40+
// materialize should not fail. Every other entry is load-bearing for
41+
// /discover, /review, or the "Build & query it" flow; missing any of
42+
// them means a broken package and we must fail loudly rather than mark
43+
// the incomplete copy as reused-forever.
44+
{ from: ".gitignore", kind: "file", required: false },
45+
{ from: "models", kind: "dir", required: true },
46+
{ from: "seeds", kind: "dir", required: true },
47+
{ from: "target/manifest.json", kind: "file", required: true },
4348
]
4449

4550
export interface MaterializeOptions {
@@ -258,6 +263,7 @@ async function materializeUnderLock(
258263
targetParent,
259264
preferredName,
260265
opts.sampleVersion,
266+
sampleName,
261267
100,
262268
{ skipVersionMismatch: opts.installAlongside === true },
263269
)
@@ -333,7 +339,7 @@ async function materializeUnderLock(
333339
// the lock, etc.); a stale classification must not authorize a
334340
// destructive remove.
335341
if (fs.existsSync(targetPath)) {
336-
const nowState = classifyTarget(targetPath, opts.sampleVersion)
342+
const nowState = classifyTarget(targetPath, opts.sampleVersion, sampleName)
337343
const stillEligible =
338344
(state.kind === "our-sample-different-version" && nowState.kind === "our-sample-different-version") ||
339345
(state.kind === "empty" && nowState.kind === "empty")
@@ -422,7 +428,19 @@ function copySampleTree(source: string, writeTo: string, finalTarget: string): v
422428
for (const entry of MATERIALIZE_ENTRIES) {
423429
const from = path.join(source, entry.from)
424430
const to = path.join(writeTo, entry.from)
425-
if (!fs.existsSync(from)) continue // .gitignore is optional; skip quietly
431+
if (!fs.existsSync(from)) {
432+
// Optional entries (currently just .gitignore) skip quietly. Required
433+
// entries mean the shipped package is broken — surface that as an
434+
// error before writing a marker that would falsely mark this
435+
// incomplete copy as reused-forever on subsequent runs.
436+
if (entry.required) {
437+
throw new Error(
438+
`Sample source at ${source} is missing required entry '${entry.from}' — ` +
439+
`the shipped package is incomplete. Reinstall with: npm i -g @altimateai/altimate-code@latest`,
440+
)
441+
}
442+
continue
443+
}
426444
if (entry.kind === "dir") {
427445
fs.cpSync(from, to, { recursive: true, force: true })
428446
} else if (entry.from === "target/manifest.json") {

‎packages/opencode/src/command/template/onboard-connect.txt‎

Lines changed: 33 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -186,21 +186,40 @@ moment, not another menu):
186186
2. Install fresh: run `pip install dbt-duckdb` (grabs both dbt-core and
187187
the DuckDB adapter), then say "ready" and I'll continue.
188188

189-
If the user pastes a path (option 1), verify it works with
190-
`<path> --version 2>&1 | grep -q "duckdb:"` via bash and use that binary
191-
explicitly for the build (`<path> build` instead of `dbt build`). If they
192-
install fresh (option 2) and say "ready", call `sample_setup` again — the
193-
fresh call re-probes with `{ force: true }` and its `dbt:` line will now
194-
say `present`. Only shell out if you have to; the tool's probe is the
195-
source of truth.
196-
197-
Once dbt is available, run `dbt build` (or `<path> build`) in the sample
198-
project dir via bash and report the PASS/FAIL counts truthfully. Before
189+
If the user pastes a path (option 1), do NOT interpolate the pasted
190+
string into a shell pipeline verbatim — a paste like `; rm -rf ~` would
191+
execute. Validate first, then always run through a single-quoted argv:
192+
1. Refuse if the path contains any of `; & | > < $ ` ( ) { } newline`
193+
or unmatched single-quote. Ask the user to paste again.
194+
2. Refuse if the file at that path is not executable
195+
(`test -x '<path>'` — quote the path).
196+
3. Verify it's a dbt-duckdb binary by running the SINGLE-QUOTED
197+
command `'<path>' --version 2>&1` via bash and checking that its
198+
stdout contains a line starting with `- duckdb:`. Do NOT chain
199+
with `grep -q` on user-controlled strings.
200+
4. If verified, remember the path and use `'<path>' build` (single-
201+
quoted) for the build step below. Never pass the path unquoted.
202+
If they install fresh (option 2) and say "ready", call `sample_setup`
203+
again — the fresh call re-probes with `{ force: true }` and its `dbt:`
204+
line will now say `present`. Only shell out if you have to; the tool's
205+
probe is the source of truth.
206+
207+
Once dbt is available, run `dbt build` (or the validated `'<path>' build`
208+
from above, with the path single-quoted) in the sample project dir via
209+
bash and report the PASS/FAIL counts truthfully. Before
199210
running any queries, call the `dbt-profiles` tool with `projectDir`
200-
pointed at the sample dir to discover the DuckDB profile, then register
201-
it as a warehouse connection via `warehouse_add` — otherwise `sql_execute`
202-
has nothing to connect to. Then offer a first query and run it with
203-
`sql_execute`.
211+
pointed at the sample dir to discover the DuckDB profile. The profile
212+
reports the DuckDB path as `target/jaffle.duckdb` — a RELATIVE path
213+
(dbt-duckdb resolves it against `dbt build`'s working directory, so it
214+
landed at `<sample-path>/target/jaffle.duckdb`). If you pass that
215+
relative string straight to `warehouse_add`, `sql_execute` will resolve
216+
it against ITS own cwd and open (or create) an empty duckdb file
217+
wherever the CLI happens to be running from — which is exactly the
218+
bug that surfaced. Absolutize first: join the sample path from the
219+
earlier `sample_setup` output with the profile's `path:` value, then
220+
pass the ABSOLUTE path (`<sample-path>/target/jaffle.duckdb`) to
221+
`warehouse_add` so `sql_execute` connects to the database dbt just
222+
built. Then offer a first query and run it with `sql_execute`.
204223
- "Something else — describe it" → just ask what they're working on; free chat.
205224

206225
Keep the tone calm and honest: the scan only reads local files the user already

0 commit comments

Comments
 (0)