diff --git a/packages/opencode/src/altimate/prompts/builder.txt b/packages/opencode/src/altimate/prompts/builder.txt index 47ff6e588..26a48ef9a 100644 --- a/packages/opencode/src/altimate/prompts/builder.txt +++ b/packages/opencode/src/altimate/prompts/builder.txt @@ -53,23 +53,6 @@ When creating dbt models: - Update schema.yml files alongside model changes - Run `lineage_check` to verify column-level data flow - -## Pre-Execution Protocol - -Before executing ANY SQL via sql_execute, follow this mandatory sequence: - -1. **Analyze first**: Run `sql_analyze` on the query. Check for HIGH severity anti-patterns. - - If HIGH severity issues found (SELECT *, cartesian products, missing WHERE on DELETE/UPDATE, full table scans on large tables): FIX THEM before executing. Show the user what you found and the fixed query. - - If MEDIUM severity issues found: mention them and proceed unless the user asks to fix. - -2. **Validate syntax**: Run `altimate_core_validate` to catch syntax errors and schema issues BEFORE hitting the warehouse. - -3. **Execute**: Only after steps 1-2 pass, run `sql_execute`. - -This sequence is NOT optional. Skipping it means the user pays for avoidable mistakes. You are the customer's cost advocate — every credit saved is trust earned. If the user explicitly requests skipping the protocol, note the risk and proceed. - -For trivial queries (e.g., `SELECT 1`, `SHOW TABLES`), use judgment — skip the full sequence but still validate syntax. - ## dbt Verification Workflow After ANY dbt operation (build, run, test, model creation/modification): diff --git a/packages/opencode/src/session/pre-execution.ts b/packages/opencode/src/session/pre-execution.ts new file mode 100644 index 000000000..f10430097 --- /dev/null +++ b/packages/opencode/src/session/pre-execution.ts @@ -0,0 +1,257 @@ +// Fork-only module — owns the PRE-EXECUTION PROTOCOL SCOPING CONTRACT. +// +// The protocol below used to sit statically in `altimate/prompts/builder.txt`. +// builder is a PRIMARY agent, so a static section there governs every builder +// surface at once: dbt authoring, interactive chat, and headless +// question-answering runs. A pre-registered paired ablation (540 trials on a +// public data-question benchmark, one binary across both arms) measured what +// the section costs on the question-answering surface: +// +// macro Pass@1 control 0.6667 → treatment 0.6807 +// delta +0.0140, query-blocked permutation p = 0.7358, +// cluster-bootstrap 95% CI [-0.0400, +0.0674] → no score effect +// wall clock 440.9s → 319.4s (-27.6%) +// model turns -27.7%, generation time -32.2% +// `altimate_core_validate` + `sql_analyze` calls 2,805 → 0 +// `sql_execute` calls +49% (the freed budget went into real querying) +// +// The 2,805 → 0 is the one number directly attributable to this text: the +// ritual is prompt-ordered, and deleting the order deletes it completely. The +// latency win is NOT attributable to this section alone — that treatment arm +// bundled five coupled changes and the experiment declined to attribute. +// +// So this module SCOPES rather than deletes. The measurement covers exactly +// one cell — headless question-answering in a workspace with no dbt project — +// and that is the only cell where the protocol is dropped. dbt work and +// interactive chat, where a pre-execution discipline may genuinely earn its +// place, are unmeasured and keep it. Anything that cannot be classified +// confidently keeps it too: the cost of keeping it is latency on one workload, +// the cost of wrongly dropping it is unmeasured. +// +// Directive text lives here (not at the call site) so any wording-change review +// covers ONE file, mirroring session/termination.ts. + +import fs from "fs/promises" +import path from "path" +import { Log } from "../util/log" + +const log = Log.create({ service: "pre-execution-scope" }) + +/** Files that mark a directory as a dbt project root. */ +const PROJECT_FILES = ["dbt_project.yml", "dbt_project.yaml"] as const + +/** + * Subdirectories never considered candidates for a nested dbt project, mirroring + * `findDbtProjectRoot`'s skip list so a fixture project shipped inside + * `node_modules/foo/` or a compiled artifact in `target/` is not mistaken for + * the user's real project. + */ +const SKIP_DIRS = new Set(["node_modules", "target"]) + +/** + * The `errno` code of a filesystem rejection, when it carries one. + * + * `ENOENT` and `ENOTDIR` are real answers — nothing is there. Every other code, + * and an error carrying no code at all, means the question went unanswered. + */ +function errnoCode(err: unknown): string | undefined { + if (typeof err !== "object" || err === null || !("code" in err)) return undefined + const code = err.code + return typeof code === "string" ? code : undefined +} + +/** True when a rejection means "nothing is there", rather than "could not tell". */ +function meansAbsent(err: unknown): boolean { + const code = errnoCode(err) + return code === "ENOENT" || code === "ENOTDIR" +} + +/** + * How a workspace classifies for the purpose of this gate. + * + * `unknown` is a real, load-bearing state, not a placeholder: it is what we + * report when the filesystem could not answer the question, and it keeps the + * protocol. The existing `findDbtProjectRoot` helper cannot serve this gate + * directly because it collapses "no project here" and "could not look here" + * into the same `null`, and this gate turns a directive off on the difference. + */ +export type WorkspaceShape = "dbt" | "non-dbt" | "unknown" + +/** + * The mandatory pre-execution sequence, verbatim as it shipped in + * `builder.txt`. Prompt-visible text — changes need extra review. + * + * Kept byte-identical to the previous static section so that in every case + * where the gate injects it, the resolved prompt is unchanged from before. + */ +export const PRE_EXECUTION_PROTOCOL = [ + "## Pre-Execution Protocol", + "", + "Before executing ANY SQL via sql_execute, follow this mandatory sequence:", + "", + "1. **Analyze first**: Run `sql_analyze` on the query. Check for HIGH severity anti-patterns.", + " - If HIGH severity issues found (SELECT *, cartesian products, missing WHERE on DELETE/UPDATE, full table scans on large tables): FIX THEM before executing. Show the user what you found and the fixed query.", + " - If MEDIUM severity issues found: mention them and proceed unless the user asks to fix.", + "", + "2. **Validate syntax**: Run `altimate_core_validate` to catch syntax errors and schema issues BEFORE hitting the warehouse.", + "", + "3. **Execute**: Only after steps 1-2 pass, run `sql_execute`.", + "", + "This sequence is NOT optional. Skipping it means the user pays for avoidable mistakes. You are the customer's cost advocate — every credit saved is trust earned. If the user explicitly requests skipping the protocol, note the risk and proceed.", + "", + "For trivial queries (e.g., `SELECT 1`, `SHOW TABLES`), use judgment — skip the full sequence but still validate syntax.", +].join("\n") + +/** + * Does `dir` itself contain a dbt project file? + * + * Returns `undefined` — not `false` — when the filesystem could not answer. + * ENOENT and ENOTDIR are real answers ("nothing there"); anything else (EACCES, + * EIO, a transient network mount failure) is not, and must not be read as + * "no dbt project here". + */ +async function hasProjectFile(dir: string): Promise { + let sawUnknown = false + for (const name of PROJECT_FILES) { + try { + if ((await fs.stat(path.join(dir, name))).isFile()) return true + } catch (err) { + if (meansAbsent(err)) continue + log.warn("project-file probe failed", { dir, name, code: errnoCode(err) }) + sawUnknown = true + } + } + return sawUnknown ? undefined : false +} + +/** + * Classify a workspace by the presence of a dbt project. + * + * `dbt` requires an actual `dbt_project.yml` (or `.yaml`) FILE at, above, or + * one level below a candidate directory: + * + * - **at** the candidate, + * - **above** it, every ancestor up to the filesystem root. A session is + * routinely started inside `models/`, and on a non-git project the worktree + * candidate is the same directory, so nothing else would find the project. + * The walk is deliberately unbounded: a depth limit would have to report + * "I stopped early" as `unknown` to stay honest, which on any deep tree + * turns the gate off entirely. Two `stat` calls per level, in run mode + * only, is not worth that. + * - **one level below** it, which is how benchmark and monorepo layouts nest + * a project (the same rule, and the same skip list, as + * `findDbtProjectRoot`). + * + * A project found in an unrelated ancestor is a false positive that KEEPS the + * protocol, which is the safe direction. + * + * `non-dbt` requires at least one candidate the scan examined COMPLETELY — its + * symlinks resolved, every ancestor probe answered up to the root, and its own + * children enumerated and probed — with no project found. If no candidate + * managed that, the answer is `unknown`, which keeps the protocol. One complete + * answer is enough: the ancestor walk from that candidate already covers the + * worktree above it, so a partner candidate that could not be read has nothing + * left to contribute. + */ +export async function classifyWorkspace(candidates: (string | undefined)[]): Promise { + const dirs = [...new Set(candidates.filter((d): d is string => !!d))] + let sawCompleteAnswer = false + + for (const dir of dirs) { + let complete = true + + // Resolve symlinks first. `path.resolve` is lexical, so a symlinked cwd + // (`/tmp/ws` -> `/repo/models`) would walk `/tmp` and `/` and never see the + // project the session is actually inside. + let start: string + try { + start = await fs.realpath(dir) + } catch (err) { + if (!meansAbsent(err)) log.warn("candidate realpath failed", { dir, code: errnoCode(err) }) + continue + } + + // At the candidate, then upwards to the filesystem root. + let current = start + for (;;) { + const found = await hasProjectFile(current) + if (found === true) return "dbt" + if (found === undefined) complete = false + const parent = path.dirname(current) + if (parent === current) break + current = parent + } + + // One level below. The filesystem root is a legitimate stop rather than a + // directory to enumerate — a non-git project sets worktree to it, and + // scanning its children is meaningless and can be slow or permission-denied + // — so the root on its own never yields a complete answer. + if (start === path.parse(start).root) { + complete = false + } else { + let entries + try { + entries = await fs.readdir(start, { withFileTypes: true }) + } catch (err) { + if (!meansAbsent(err)) log.warn("workspace enumeration failed", { dir: start, code: errnoCode(err) }) + entries = undefined + } + if (entries === undefined) { + complete = false + } else { + // Probe everything that is not plainly a regular file. Filtering on + // `isDirectory()` would silently skip symlinked directories and any + // entry whose type the filesystem did not report, and a skipped entry + // is an unexamined one. `hasProjectFile` on a non-directory just gets + // ENOTDIR, which is a real "nothing there". + const children = entries + .filter((e) => !e.isFile() && !e.name.startsWith(".") && !SKIP_DIRS.has(e.name)) + // Deterministic order: fs.readdir's order varies across filesystems. + .sort((a, b) => a.name.localeCompare(b.name)) + for (const child of children) { + const found = await hasProjectFile(path.join(start, child.name)) + if (found === true) return "dbt" + if (found === undefined) complete = false + } + } + } + + if (complete) sawCompleteAnswer = true + } + + return sawCompleteAnswer ? "non-dbt" : "unknown" +} + +/** + * The sole gate for injecting the pre-execution protocol into a prompt. + * + * Returns the protocol text to inject, or `undefined` to drop it. The ONLY + * dropping case is the one the ablation measured: + * + * run mode (headless / CI, the `run` CLI) AND + * the builder agent (the only prompt that ever carried the section) AND + * a workspace confidently classified as having no dbt project. + * + * Everything else keeps it, including `unknown`. Note the asymmetry is + * deliberate: run mode is not itself a task-shape signal, it is the surface the + * evidence covers. Widening this to interactive chat needs its own measurement. + * + * Classification is only performed when the cheap conditions already hold, so + * an interactive session pays no filesystem cost for this gate. + */ +export async function preExecutionInstruction(input: { + runMode: boolean + agent: string + /** Candidate directories to classify — typically the cwd and the worktree root. */ + directories: (string | undefined)[] +}): Promise { + // Only builder ever carried this section; analyst and reviewer never did. + if (input.agent !== "builder") return undefined + if (!input.runMode) return PRE_EXECUTION_PROTOCOL + const shape = await classifyWorkspace(input.directories) + if (shape !== "non-dbt") return PRE_EXECUTION_PROTOCOL + log.info("pre-execution protocol scoped out", { agent: input.agent, shape }) + return undefined +} + +export * as SessionPreExecution from "./pre-execution" diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index 02d4eb18c..b081e0e4a 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -20,6 +20,8 @@ import { type Tool as AITool, tool, jsonSchema, type ToolCallOptions, asSchema } import { SessionCompaction } from "./compaction" import { NudgeArbiter } from "./nudge" import { SessionTermination } from "./termination" +// altimate_change — task-shape-scoped pre-execution protocol (see pre-execution.ts) +import { SessionPreExecution } from "./pre-execution" import { Instance } from "../project/instance" import { Bus } from "../bus" import { ProviderTransform } from "../provider/transform" @@ -1456,6 +1458,24 @@ export namespace SessionPrompt { ...(await InstructionPrompt.system()), ...hoistedReminders, ] + // altimate_change start — task-shape-scoped pre-execution protocol. Same + // shape as the completion instruction below and for the same reason: the + // text used to sit in builder.txt, and builder is a PRIMARY agent, so it + // reached every builder surface. A paired ablation measured it as pure + // overhead on headless question-answering (2,805 ritual tool calls → 0, no + // score effect) but says nothing about dbt work or interactive chat, so + // those keep it. See session/pre-execution.ts for the gate and the numbers. + // + // Injected BEFORE the completion instruction, which says to signal DONE + // only once "every requirement above" is satisfied. A mandatory protocol + // pushed after it would not be one of those requirements. + const preExecutionInstruction = await SessionPreExecution.preExecutionInstruction({ + runMode: Flag.ALTIMATE_RUN_MODE, + agent: agent.name, + directories: [Instance.directory, Instance.worktree], + }) + if (preExecutionInstruction) system.push(preExecutionInstruction) + // altimate_change end // altimate_change start — run-mode-only completion instruction. This text // used to sit in builder.txt, but builder is a PRIMARY agent, so it also // reached interactive chat, where nothing interprets or strips the token diff --git a/packages/opencode/test/altimate/sql-validation-e2e.test.ts b/packages/opencode/test/altimate/sql-validation-e2e.test.ts index c69c20421..d22d7398e 100644 --- a/packages/opencode/test/altimate/sql-validation-e2e.test.ts +++ b/packages/opencode/test/altimate/sql-validation-e2e.test.ts @@ -8,6 +8,7 @@ * 4. altimate_core_check composite pipeline works end-to-end * 5. sql.analyze composite pipeline works end-to-end * 6. Pre-execution protocol tools are callable (sql_analyze → altimate_core_validate → sql_execute) + * (the protocol TEXT itself is gated in session/pre-execution.ts, tested there) * 7. sql-classify correctly gates sql_execute * 8. Analyst and builder agent permissions are consistent with their prompts */ @@ -15,6 +16,7 @@ import { describe, expect, test, beforeAll, afterAll } from "bun:test" import fs from "fs" import path from "path" +import { SessionPreExecution } from "../../src/session/pre-execution" import * as Dispatcher from "../../src/altimate/native/dispatcher" import { registerAll } from "../../src/altimate/native/altimate-core" import { registerAllSql } from "../../src/altimate/native/sql/register" @@ -95,22 +97,36 @@ describe("Tool name consistency in prompts", () => { }) }) - test("builder prompt contains pre-execution protocol with correct tool names", async () => { + test("builder prompt names the pre-execution tools", async () => { await using tmp = await tmpdir() await Instance.provide({ directory: tmp.path, fn: async () => { const builder = await Agent.get("builder") expect(builder).toBeDefined() - // Pre-Execution Protocol references: expect(builder!.prompt).toContain("sql_analyze") expect(builder!.prompt).toContain("altimate_core_validate") expect(builder!.prompt).toContain("sql_execute") - // The protocol section itself - expect(builder!.prompt).toContain("Pre-Execution Protocol") }, }) }) + + // The protocol section itself is no longer static in builder.txt — it is + // injected per-session by the gate in session/pre-execution.ts, which drops it + // only for headless builder runs in a workspace with no dbt project. The + // wording is unchanged; see test/session/pre-execution.test.ts for the gate. + test("the pre-execution protocol still reaches an interactive builder", async () => { + await using tmp = await tmpdir() + const instruction = await SessionPreExecution.preExecutionInstruction({ + runMode: false, + agent: "builder", + directories: [tmp.path], + }) + expect(instruction).toContain("Pre-Execution Protocol") + expect(instruction).toContain("sql_analyze") + expect(instruction).toContain("altimate_core_validate") + expect(instruction).toContain("sql_execute") + }) }) // --------------------------------------------------------------------------- diff --git a/packages/opencode/test/session/pre-execution.test.ts b/packages/opencode/test/session/pre-execution.test.ts new file mode 100644 index 000000000..9e7dd0d7f --- /dev/null +++ b/packages/opencode/test/session/pre-execution.test.ts @@ -0,0 +1,281 @@ +import { describe, expect, test } from "bun:test" +import fs from "fs/promises" +import os from "os" +import path from "path" +import { SessionPreExecution } from "../../src/session/pre-execution" + +async function tmpdir(): Promise { + return await fs.mkdtemp(path.join(os.tmpdir(), "pre-exec-scope-")) +} + +describe("workspace classification", () => { + test("a dbt_project.yml at the root classifies as dbt", async () => { + const dir = await tmpdir() + await fs.writeFile(path.join(dir, "dbt_project.yml"), "name: demo\n") + expect(await SessionPreExecution.classifyWorkspace([dir])).toBe("dbt") + }) + + // Benchmark and monorepo layouts nest the project one level down; this gate + // inherits `findDbtProjectRoot`'s rule and skip list, otherwise a real dbt + // task would be misread as question-answering. + test("a dbt_project.yml one level down still classifies as dbt", async () => { + const dir = await tmpdir() + await fs.mkdir(path.join(dir, "warehouse")) + await fs.writeFile(path.join(dir, "warehouse", "dbt_project.yml"), "name: demo\n") + expect(await SessionPreExecution.classifyWorkspace([dir])).toBe("dbt") + }) + + test("a readable directory with no dbt project classifies as non-dbt", async () => { + const dir = await tmpdir() + await fs.writeFile(path.join(dir, "questions.duckdb"), "") + expect(await SessionPreExecution.classifyWorkspace([dir])).toBe("non-dbt") + }) + + // The two-directory case: the cwd may be a plain subfolder of a dbt project. + test("any readable candidate carrying a project wins", async () => { + const root = await tmpdir() + await fs.writeFile(path.join(root, "dbt_project.yml"), "name: demo\n") + const cwd = path.join(root, "analyses") + await fs.mkdir(cwd) + expect(await SessionPreExecution.classifyWorkspace([cwd, root])).toBe("dbt") + }) + + // The load-bearing case. A scan that collapses "no project here" and "could + // not look here" into one answer silently drops the protocol whenever the + // filesystem misbehaves. + test("a directory that does not exist is unknown, never non-dbt", async () => { + const dir = await tmpdir() + const missing = path.join(dir, "gone") + expect(await SessionPreExecution.classifyWorkspace([missing])).toBe("unknown") + }) + + test("no candidates at all is unknown", async () => { + expect(await SessionPreExecution.classifyWorkspace([])).toBe("unknown") + expect(await SessionPreExecution.classifyWorkspace([undefined, ""])).toBe("unknown") + }) + + // A non-git project sets worktree to the filesystem root. Scanning it is + // meaningless, and it must not count as the readable directory that licenses + // a non-dbt verdict on its own. + test("the filesystem root is not a candidate", async () => { + const root = path.parse(process.cwd()).root + expect(await SessionPreExecution.classifyWorkspace([root])).toBe("unknown") + }) + + test("a project on one candidate wins even when its partner is unreadable", async () => { + const dir = await tmpdir() + await fs.writeFile(path.join(dir, "dbt_project.yml"), "name: demo\n") + expect(await SessionPreExecution.classifyWorkspace([path.join(dir, "nope"), dir])).toBe("dbt") + }) + + // A directory named dbt_project.yml is not a dbt project. + test("a dbt_project.yml directory is not a project", async () => { + const dir = await tmpdir() + await fs.mkdir(path.join(dir, "dbt_project.yml")) + expect(await SessionPreExecution.classifyWorkspace([dir])).toBe("non-dbt") + }) + + test("dbt_project.yaml counts as well as .yml", async () => { + const dir = await tmpdir() + await fs.writeFile(path.join(dir, "dbt_project.yaml"), "name: demo\n") + expect(await SessionPreExecution.classifyWorkspace([dir])).toBe("dbt") + }) + + // Sessions are routinely started inside `models/` or deeper. On a non-git + // project the worktree candidate is the same directory, so the ancestor walk + // is the only thing that finds the project — without it a real dbt session + // classifies as non-dbt and loses the protocol. + test("a project above the candidate is found", async () => { + const root = await tmpdir() + await fs.writeFile(path.join(root, "dbt_project.yml"), "name: demo\n") + const deep = path.join(root, "models", "marts", "finance") + await fs.mkdir(deep, { recursive: true }) + expect(await SessionPreExecution.classifyWorkspace([deep])).toBe("dbt") + }) + + // The walk runs to the filesystem root rather than stopping at a depth + // limit. A limit would have to report "I stopped early" as unknown to stay + // honest, which on any deep tree switches the gate off entirely. + test("the ancestor walk is not depth-limited", async () => { + const root = await tmpdir() + await fs.writeFile(path.join(root, "dbt_project.yml"), "name: demo\n") + const deep = path.join(root, "a", "b", "c", "d", "e", "f", "g", "h", "i", "j") + await fs.mkdir(deep, { recursive: true }) + expect(await SessionPreExecution.classifyWorkspace([deep])).toBe("dbt") + }) + + // `path.resolve` is lexical. A symlinked cwd would walk the link's own + // parents and never see the project the session is actually inside. + test("a symlinked candidate is resolved before the walk", async () => { + const project = await tmpdir() + await fs.writeFile(path.join(project, "dbt_project.yml"), "name: demo\n") + const inner = path.join(project, "models") + await fs.mkdir(inner) + const elsewhere = await tmpdir() + const link = path.join(elsewhere, "ws") + await fs.symlink(inner, link, "dir") + expect(await SessionPreExecution.classifyWorkspace([link])).toBe("dbt") + }) + + // Filtering children on isDirectory() would skip symlinked directories, and + // a skipped entry is an unexamined one. + test("a symlinked child project is found", async () => { + const project = await tmpdir() + await fs.writeFile(path.join(project, "dbt_project.yml"), "name: demo\n") + const workspace = await tmpdir() + await fs.symlink(project, path.join(workspace, "warehouse"), "dir") + expect(await SessionPreExecution.classifyWorkspace([workspace])).toBe("dbt") + }) + + // One completely examined candidate settles it. Its ancestor walk already + // covers the worktree above it, so a partner that could not be read has + // nothing left to contribute — and vetoing on it would return unknown for + // every non-git project, where the worktree candidate is the filesystem root. + test("a complete candidate is not vetoed by an unreadable partner", async () => { + const dir = await tmpdir() + expect(await SessionPreExecution.classifyWorkspace([dir, path.join(dir, "gone")])).toBe("non-dbt") + expect(await SessionPreExecution.classifyWorkspace([dir, path.parse(dir).root])).toBe("non-dbt") + }) + + // The failure that matters most: a directory that stats fine but cannot be + // enumerated. Collapsing that into "no dbt project" would drop the protocol + // on a workspace nobody ever looked inside. + test("a directory that cannot be enumerated is unknown, not non-dbt", async () => { + const dir = await tmpdir() + const locked = path.join(dir, "locked") + await fs.mkdir(locked) + await fs.chmod(locked, 0o000) + try { + // Running as root defeats the permission bit; the assertion is only + // meaningful when the mode actually blocks the read. + let enumerable = true + try { + await fs.readdir(locked) + } catch { + enumerable = false + } + if (enumerable) return + expect(await SessionPreExecution.classifyWorkspace([locked])).toBe("unknown") + } finally { + await fs.chmod(locked, 0o755) + } + }) +}) + +describe("pre-execution protocol gate", () => { + // The ONLY combination that drops the protocol is the one the ablation + // measured: headless, builder, no dbt project in the workspace. + test("headless builder in a non-dbt workspace drops the protocol", async () => { + const dir = await tmpdir() + expect( + await SessionPreExecution.preExecutionInstruction({ runMode: true, agent: "builder", directories: [dir] }), + ).toBeUndefined() + }) + + test("headless builder in a dbt workspace keeps it", async () => { + const dir = await tmpdir() + await fs.writeFile(path.join(dir, "dbt_project.yml"), "name: demo\n") + const instruction = await SessionPreExecution.preExecutionInstruction({ + runMode: true, + agent: "builder", + directories: [dir], + }) + expect(instruction).toBe(SessionPreExecution.PRE_EXECUTION_PROTOCOL) + }) + + // Interactive chat is a builder surface the ablation never covered, so it is + // unchanged from before this PR regardless of what the workspace looks like. + test("interactive builder always keeps it, dbt project or not", async () => { + const dir = await tmpdir() + expect( + await SessionPreExecution.preExecutionInstruction({ runMode: false, agent: "builder", directories: [dir] }), + ).toBe(SessionPreExecution.PRE_EXECUTION_PROTOCOL) + }) + + // Ambiguity keeps the protocol. Wrongly dropping it has an unmeasured cost; + // wrongly keeping it costs latency on one workload. + test("an unclassifiable workspace keeps it", async () => { + const dir = await tmpdir() + expect( + await SessionPreExecution.preExecutionInstruction({ + runMode: true, + agent: "builder", + directories: [path.join(dir, "does-not-exist")], + }), + ).toBe(SessionPreExecution.PRE_EXECUTION_PROTOCOL) + expect( + await SessionPreExecution.preExecutionInstruction({ runMode: true, agent: "builder", directories: [] }), + ).toBe(SessionPreExecution.PRE_EXECUTION_PROTOCOL) + }) + + // Only builder.txt ever carried this section, so injecting it for analyst or + // reviewer would be a new instruction, not a preserved one. + test("no other agent receives the protocol", async () => { + const dir = await tmpdir() + for (const agent of ["analyst", "reviewer", "plan", "general"]) { + expect( + await SessionPreExecution.preExecutionInstruction({ runMode: false, agent, directories: [dir] }), + ).toBeUndefined() + expect( + await SessionPreExecution.preExecutionInstruction({ runMode: true, agent, directories: [dir] }), + ).toBeUndefined() + } + }) +}) + +describe("prompt text fidelity", () => { + // The injected text must be byte-identical to what builder.txt shipped, so + // that every kept case produces the same resolved prompt as before. + test("the injected protocol is verbatim the section that was removed", async () => { + const expected = [ + "## Pre-Execution Protocol", + "", + "Before executing ANY SQL via sql_execute, follow this mandatory sequence:", + ].join("\n") + expect(SessionPreExecution.PRE_EXECUTION_PROTOCOL.startsWith(expected)).toBe(true) + expect(SessionPreExecution.PRE_EXECUTION_PROTOCOL).toContain("This sequence is NOT optional.") + expect(SessionPreExecution.PRE_EXECUTION_PROTOCOL).toContain("altimate_core_validate") + expect(SessionPreExecution.PRE_EXECUTION_PROTOCOL).toContain("sql_analyze") + expect(SessionPreExecution.PRE_EXECUTION_PROTOCOL.endsWith("still validate syntax.")).toBe(true) + }) + + // If the section came back into the static prompt file the gate would be a + // no-op and every surface would carry it again. + test("the builder prompt file no longer carries the section", async () => { + const prompt = await Bun.file(new URL("../../src/altimate/prompts/builder.txt", import.meta.url).pathname).text() + expect(prompt).not.toContain("## Pre-Execution Protocol") + expect(prompt).not.toContain("This sequence is NOT optional.") + // The neighbouring sections must survive — only the one section moved. + expect(prompt).toContain("## dbt Verification Workflow") + expect(prompt).toContain("## Finish Protocol (mandatory before ending any build/fix task)") + }) + + // The Finish Protocol is a SECOND mandatory ritual in the same family, added + // after the binary the ablation measured was built. It is deliberately left + // alone: no measurement covers it, and this PR does not speak to it. + test("the Finish Protocol is untouched by this change", async () => { + const prompt = await Bun.file(new URL("../../src/altimate/prompts/builder.txt", import.meta.url).pathname).text() + expect(prompt).toContain("Re-read the task's literal requirements") + expect(prompt).toContain("Run the final build and tests") + }) + + test("prompt assembly wires the gate to the run-mode flag and both directories", async () => { + const prompt = await Bun.file(new URL("../../src/session/prompt.ts", import.meta.url).pathname).text() + expect(prompt).toMatch( + /SessionPreExecution\.preExecutionInstruction\(\{\s*runMode: Flag\.ALTIMATE_RUN_MODE,\s*agent: agent\.name,\s*directories: \[Instance\.directory, Instance\.worktree\],\s*\}\)/, + ) + expect(prompt).toMatch(/if \(preExecutionInstruction\) system\.push\(preExecutionInstruction\)/) + }) + + // The completion instruction tells the model to signal DONE only once "every + // requirement above" is satisfied. A mandatory protocol pushed after it would + // sit outside that scope, so ordering here is behavioural, not cosmetic. + test("the protocol is pushed before the completion instruction", async () => { + const prompt = await Bun.file(new URL("../../src/session/prompt.ts", import.meta.url).pathname).text() + const protocolAt = prompt.indexOf("if (preExecutionInstruction) system.push(preExecutionInstruction)") + const completionAt = prompt.indexOf("if (completionInstruction) system.push(completionInstruction)") + expect(protocolAt).toBeGreaterThan(-1) + expect(completionAt).toBeGreaterThan(-1) + expect(protocolAt).toBeLessThan(completionAt) + }) +})