From b5d4ba1d47711883fdffc22c1931d3565cbae238 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Mon, 31 Aug 2026 23:40:10 -0700 Subject: [PATCH 1/7] fix: scope the builder pre-execution protocol to task shape `## Pre-Execution Protocol` sat statically in `builder.txt`. `builder` is a PRIMARY agent, so the section governed 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 it on the question-answering surface: macro Pass@1 0.6667 -> 0.6807, delta +0.0140, query-blocked permutation p = 0.7358, cluster-bootstrap 95% CI [-0.0400, +0.0674]. That is a null on score. Wall clock fell 27.6%, model turns 27.7%, generation time 32.2%, and all 2,805 `altimate_core_validate` + `sql_analyze` calls went to zero while `sql_execute` rose 49%. The 2,805 -> 0 is directly attributable to this text; the latency win is not, because that treatment arm bundled five coupled changes. And the measurement covers data questions only. So this scopes rather than deletes. - move the section out of `builder.txt` into `session/pre-execution.ts`, byte-identical, following the `SessionTermination.completionInstruction` precedent that scoped a run-mode instruction the same way - inject it from the same site in `session/prompt.ts`, dropping it ONLY when all of: run mode, the `builder` agent, and a workspace confidently classified as having no dbt project - classification reuses `findDbtProjectRoot` and reports a tri-state, so "could not read the directory" is `unknown` and keeps the protocol rather than collapsing into "no dbt project" - `## Finish Protocol` is deliberately untouched: it is a second mandatory ritual in the same family, no measurement covers it Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- .../opencode/src/session/pre-execution.ts | 140 ++++++++++++++ packages/opencode/src/session/prompt.ts | 16 ++ .../test/altimate/sql-validation-e2e.test.ts | 24 ++- .../test/session/pre-execution.test.ts | 183 ++++++++++++++++++ 4 files changed, 359 insertions(+), 4 deletions(-) create mode 100644 packages/opencode/src/session/pre-execution.ts create mode 100644 packages/opencode/test/session/pre-execution.test.ts diff --git a/packages/opencode/src/session/pre-execution.ts b/packages/opencode/src/session/pre-execution.ts new file mode 100644 index 0000000000..2679b87f28 --- /dev/null +++ b/packages/opencode/src/session/pre-execution.ts @@ -0,0 +1,140 @@ +// 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 path from "path" +import { Filesystem } from "../util/filesystem" +import { findDbtProjectRoot } from "../altimate/validators/validator-utils" +import { Log } from "../util/log" + +const log = Log.create({ service: "pre-execution-scope" }) + +/** + * 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. `findDbtProjectRoot` collapses "no project here" and "could not + * read the directory" into the same `null`, so the readability check happens + * here, before it is consulted. + */ +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") + +/** + * Classify a workspace by the presence of a dbt project. + * + * `dbt` requires an actual `dbt_project.yml` file at one of the candidate + * directories or one level below it (`findDbtProjectRoot`'s existing rule — + * benchmark and monorepo layouts nest the project one level deep). + * + * `non-dbt` is only reported when at least one candidate directory was + * readable AND no project was found in any readable candidate. If no candidate + * could be read, the answer is `unknown`, never `non-dbt`. + */ +export async function classifyWorkspace(candidates: (string | undefined)[]): Promise { + // A non-git project sets worktree to the filesystem root; scanning that is + // never meaningful and can be slow or permission-denied. + const dirs = [...new Set(candidates.filter((d): d is string => !!d && d !== path.parse(d).root))] + let sawReadableDir = false + for (const dir of dirs) { + if (!(await Filesystem.isDir(dir))) continue + sawReadableDir = true + try { + if (await findDbtProjectRoot(dir)) return "dbt" + } catch (err) { + // findDbtProjectRoot already swallows its own errors; this is belt and + // braces so a future change there cannot turn a throw into a silent drop. + log.warn("dbt project scan failed", { dir, err }) + return "unknown" + } + } + return sawReadableDir ? "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 e711e3f90e..bc71052a43 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" @@ -1481,6 +1483,20 @@ export namespace SessionPrompt { }) if (completionInstruction) system.push(completionInstruction) // altimate_change end + // altimate_change start — task-shape-scoped pre-execution protocol. Same + // shape as the completion instruction above 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. + 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 const format = lastUser.format ?? { type: "text" } if (format.type === "json_schema") { system.push(STRUCTURED_OUTPUT_SYSTEM_PROMPT) diff --git a/packages/opencode/test/altimate/sql-validation-e2e.test.ts b/packages/opencode/test/altimate/sql-validation-e2e.test.ts index c69c20421b..d22d7398eb 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 0000000000..d49063ff28 --- /dev/null +++ b/packages/opencode/test/session/pre-execution.test.ts @@ -0,0 +1,183 @@ +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; the shared + // findDbtProjectRoot rule already covers that and this gate must inherit it, + // 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. `findDbtProjectRoot` returns null both for "no + // project here" and for "could not read this directory"; collapsing those + // would silently drop the protocol whenever the filesystem misbehaved. + 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("an unreadable candidate does not license a non-dbt verdict from its partner", 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") + }) +}) + +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\)/) + }) +}) From 7d9e715730ecb66d26f9607cf2dc57ac9ad8be4b Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Tue, 1 Sep 2026 00:33:07 -0700 Subject: [PATCH 2/7] fix: close two silent-drop paths and one ordering bug in the pre-execution gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found three ways the first cut got the gate wrong, two of them in the direction that matters — dropping the protocol on a workspace that should have kept it. - **An unreadable directory classified as `non-dbt`.** `Filesystem.isDir` only proves `stat` succeeds, and `findDbtProjectRoot` swallows `readdir` and `stat` failures as `null`, so a directory that stats fine but cannot be enumerated (EACCES, EIO, a flaky mount) read as "no dbt project here". The scan now does its own probing and distinguishes ENOENT/ENOTDIR — real answers — from every other failure, which is `unknown`. - **A session started inside `models/` lost the protocol.** The old scan looked at the candidate and one level below it. On a git repo the worktree candidate usually rescued that; on a non-git project it does not, and a deeper cwd is missed either way. The scan now also walks up to 8 ancestors. An unrelated ancestor project is a false positive that KEEPS the protocol, which is the safe direction. - **The protocol was pushed after the completion instruction**, which tells the model to signal `DONE` only once "every requirement above" is satisfied. A mandatory protocol below that line is not one of those requirements. It is now injected before it, and a test asserts the order. `non-dbt` now requires at least one candidate the scan examined completely — every ancestor probe answered and the candidate's own children enumerated. The filesystem root never qualifies on its own, since its children are deliberately not scanned. Everything else is `unknown`, which keeps the protocol. Six new tests: `.yaml` as well as `.yml`, a project above the candidate, the ancestor bound, a directory that stats but cannot be enumerated (skipped when running as root, where the permission bit does not bite), and the injection order. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- .../opencode/src/session/pre-execution.ts | 149 +++++++++++++++--- packages/opencode/src/session/prompt.ts | 30 ++-- .../test/session/pre-execution.test.ts | 70 +++++++- 3 files changed, 211 insertions(+), 38 deletions(-) diff --git a/packages/opencode/src/session/pre-execution.ts b/packages/opencode/src/session/pre-execution.ts index 2679b87f28..804d154e41 100644 --- a/packages/opencode/src/session/pre-execution.ts +++ b/packages/opencode/src/session/pre-execution.ts @@ -31,13 +31,49 @@ // 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 { Filesystem } from "../util/filesystem" -import { findDbtProjectRoot } from "../altimate/validators/validator-utils" 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"]) + +/** + * How far up from a candidate directory to look for a project root. A session + * is routinely started inside `models/` or `models/marts/` of a dbt project, + * and on a non-git project the worktree is the same directory (or the + * filesystem root), so the ancestor walk is the only thing that finds it. + */ +const MAX_ANCESTOR_LEVELS = 8 + +/** + * 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. * @@ -74,35 +110,104 @@ export const PRE_EXECUTION_PROTOCOL = [ "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` file at one of the candidate - * directories or one level below it (`findDbtProjectRoot`'s existing rule — - * benchmark and monorepo layouts nest the project one level deep). + * `dbt` requires an actual `dbt_project.yml` (or `.yaml`) FILE at, above, or + * one level below a candidate directory: + * + * - **at** the candidate, + * - **above** it, walking up to `MAX_ANCESTOR_LEVELS` parents — a session + * started inside `models/` is still a dbt session, and on a non-git project + * the worktree candidate does not rescue that case, + * - **one level below** it, which is how benchmark and monorepo layouts nest + * a project (the same rule, and the same skip list, as + * `findDbtProjectRoot`). * - * `non-dbt` is only reported when at least one candidate directory was - * readable AND no project was found in any readable candidate. If no candidate - * could be read, the answer is `unknown`, never `non-dbt`. + * `non-dbt` requires at least one candidate the scan could examine COMPLETELY — + * every ancestor probe answered, and the candidate's own children enumerated — + * with no project found anywhere. Everything else is `unknown`: a candidate + * that cannot be enumerated, a stat failing for any reason other than "not + * there", the filesystem root (whose children are deliberately not scanned), + * and an empty candidate list. The caller reads `unknown` as "keep the + * protocol", so folding a filesystem failure into "no dbt project" would drop + * it silently on a workspace nothing ever managed to look inside. */ export async function classifyWorkspace(candidates: (string | undefined)[]): Promise { - // A non-git project sets worktree to the filesystem root; scanning that is - // never meaningful and can be slow or permission-denied. - const dirs = [...new Set(candidates.filter((d): d is string => !!d && d !== path.parse(d).root))] - let sawReadableDir = false + const dirs = [...new Set(candidates.filter((d): d is string => !!d))] + let sawCompleteAnswer = false + let sawIncomplete = false + for (const dir of dirs) { - if (!(await Filesystem.isDir(dir))) continue - sawReadableDir = true - try { - if (await findDbtProjectRoot(dir)) return "dbt" - } catch (err) { - // findDbtProjectRoot already swallows its own errors; this is belt and - // braces so a future change there cannot turn a throw into a silent drop. - log.warn("dbt project scan failed", { dir, err }) - return "unknown" + let complete = true + + // At the candidate, then upwards. + let current = path.resolve(dir) + for (let level = 0; level <= MAX_ANCESTOR_LEVELS; level++) { + 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 (path.resolve(dir) === path.parse(path.resolve(dir)).root) { + complete = false + } else { + let entries + try { + entries = await fs.readdir(dir, { withFileTypes: true }) + } catch (err) { + if (!meansAbsent(err)) log.warn("workspace enumeration failed", { dir, code: errnoCode(err) }) + entries = undefined + } + if (entries === undefined) { + complete = false + } else { + const children = entries + .filter((e) => e.isDirectory() && !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(dir, child.name)) + if (found === true) return "dbt" + if (found === undefined) complete = false + } + } + } + + if (complete) sawCompleteAnswer = true + else sawIncomplete = true } - return sawReadableDir ? "non-dbt" : "unknown" + + return sawCompleteAnswer && !sawIncomplete ? "non-dbt" : "unknown" } /** diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index bc71052a43..01ba029e4e 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -1471,25 +1471,17 @@ export namespace SessionPrompt { ...(await InstructionPrompt.system()), ...hoistedReminders, ] - // 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 - // and the user saw a literal DONE on every final answer. Scoped to run - // mode AND to builder, which reproduces the previous run-mode behaviour - // exactly — builder was the only agent prompt that carried it. - const completionInstruction = SessionTermination.completionInstruction({ - runMode: Flag.ALTIMATE_RUN_MODE, - agent: agent.name, - }) - if (completionInstruction) system.push(completionInstruction) - // altimate_change end // altimate_change start — task-shape-scoped pre-execution protocol. Same - // shape as the completion instruction above and for the same reason: the + // 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, @@ -1497,6 +1489,18 @@ export namespace SessionPrompt { }) 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 + // and the user saw a literal DONE on every final answer. Scoped to run + // mode AND to builder, which reproduces the previous run-mode behaviour + // exactly — builder was the only agent prompt that carried it. + const completionInstruction = SessionTermination.completionInstruction({ + runMode: Flag.ALTIMATE_RUN_MODE, + agent: agent.name, + }) + if (completionInstruction) system.push(completionInstruction) + // altimate_change end const format = lastUser.format ?? { type: "text" } if (format.type === "json_schema") { system.push(STRUCTURED_OUTPUT_SYSTEM_PROMPT) diff --git a/packages/opencode/test/session/pre-execution.test.ts b/packages/opencode/test/session/pre-execution.test.ts index d49063ff28..e6d3449bc8 100644 --- a/packages/opencode/test/session/pre-execution.test.ts +++ b/packages/opencode/test/session/pre-execution.test.ts @@ -15,9 +15,9 @@ describe("workspace classification", () => { expect(await SessionPreExecution.classifyWorkspace([dir])).toBe("dbt") }) - // Benchmark and monorepo layouts nest the project one level down; the shared - // findDbtProjectRoot rule already covers that and this gate must inherit it, - // otherwise a real dbt task would be misread as question-answering. + // 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")) @@ -74,6 +74,58 @@ describe("workspace classification", () => { 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 upward walk is bounded, so an unrelated deep tree does not scan to /. + test("the ancestor walk is bounded", async () => { + const root = await tmpdir() + await fs.writeFile(path.join(root, "dbt_project.yml"), "name: demo\n") + const parts = ["a", "b", "c", "d", "e", "f", "g", "h", "i", "j"] + const deep = path.join(root, ...parts) + await fs.mkdir(deep, { recursive: true }) + expect(await SessionPreExecution.classifyWorkspace([deep])).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", () => { @@ -180,4 +232,16 @@ describe("prompt text fidelity", () => { ) 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) + }) }) From 7aeec8c30ed669129a078ea2f3b192fddaf659cc Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Tue, 1 Sep 2026 01:04:53 -0700 Subject: [PATCH 3/7] fix: the pre-execution gate never fired on a non-git workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second review round found four more defects in the classifier, one of which disabled the gate entirely in exactly the configuration the ablation measured. - **The sticky veto.** An incomplete candidate vetoed a complete one, so `[runDir, worktree]` returned `unknown` whenever the worktree was the filesystem root — which is what a non-git project sets it to, and what a headless benchmark run uses. The gate would have kept the protocol in every such session and shipped as a no-op. One completely examined candidate now settles it: its ancestor walk already covers the worktree above it, so a partner that could not be read has nothing left to contribute. - **Depth-limit exhaustion counted as a complete answer.** The 8-level bound stopped the walk without recording that it had stopped early, so a project at the ninth ancestor produced `non-dbt`. The bound is gone: the walk runs to the filesystem root. A limit would have to report "I stopped early" as `unknown` to stay honest, which on any deep tree switches the gate off — and two `stat` calls per level, in run mode only, is not worth that. - **The walk was lexical, not physical.** `path.resolve` does not follow symlinks, so a symlinked cwd (`/tmp/ws` -> `/repo/models`) walked `/tmp` and `/` and never saw the project it was inside. Candidates are `realpath`ed first. - **The child scan silently skipped symlinked directories** and any entry whose type the filesystem did not report, because it filtered on `isDirectory()`. A skipped entry is an unexamined one, and it did not mark the scan incomplete. It now probes everything that is not plainly a regular file; `stat` follows the link, and a non-directory just answers ENOTDIR. Four new tests: the unbounded walk, a symlinked candidate, a symlinked child project, and a complete candidate not vetoed by an unreadable partner or by the filesystem root. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- .../opencode/src/session/pre-execution.ts | 78 +++++++++++-------- .../test/session/pre-execution.test.ts | 52 ++++++++++--- 2 files changed, 88 insertions(+), 42 deletions(-) diff --git a/packages/opencode/src/session/pre-execution.ts b/packages/opencode/src/session/pre-execution.ts index 804d154e41..f104300979 100644 --- a/packages/opencode/src/session/pre-execution.ts +++ b/packages/opencode/src/session/pre-execution.ts @@ -48,14 +48,6 @@ const PROJECT_FILES = ["dbt_project.yml", "dbt_project.yaml"] as const */ const SKIP_DIRS = new Set(["node_modules", "target"]) -/** - * How far up from a candidate directory to look for a project root. A session - * is routinely started inside `models/` or `models/marts/` of a dbt project, - * and on a non-git project the worktree is the same directory (or the - * filesystem root), so the ancestor walk is the only thing that finds it. - */ -const MAX_ANCESTOR_LEVELS = 8 - /** * The `errno` code of a filesystem rejection, when it carries one. * @@ -79,9 +71,9 @@ function meansAbsent(err: unknown): boolean { * * `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. `findDbtProjectRoot` collapses "no project here" and "could not - * read the directory" into the same `null`, so the readability check happens - * here, before it is consulted. + * 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" @@ -139,33 +131,49 @@ async function hasProjectFile(dir: string): Promise { * one level below a candidate directory: * * - **at** the candidate, - * - **above** it, walking up to `MAX_ANCESTOR_LEVELS` parents — a session - * started inside `models/` is still a dbt session, and on a non-git project - * the worktree candidate does not rescue that case, + * - **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`). * - * `non-dbt` requires at least one candidate the scan could examine COMPLETELY — - * every ancestor probe answered, and the candidate's own children enumerated — - * with no project found anywhere. Everything else is `unknown`: a candidate - * that cannot be enumerated, a stat failing for any reason other than "not - * there", the filesystem root (whose children are deliberately not scanned), - * and an empty candidate list. The caller reads `unknown` as "keep the - * protocol", so folding a filesystem failure into "no dbt project" would drop - * it silently on a workspace nothing ever managed to look inside. + * 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 - let sawIncomplete = false for (const dir of dirs) { let complete = true - // At the candidate, then upwards. - let current = path.resolve(dir) - for (let level = 0; level <= MAX_ANCESTOR_LEVELS; level++) { + // 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 @@ -178,25 +186,30 @@ export async function classifyWorkspace(candidates: (string | undefined)[]): Pro // 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 (path.resolve(dir) === path.parse(path.resolve(dir)).root) { + if (start === path.parse(start).root) { complete = false } else { let entries try { - entries = await fs.readdir(dir, { withFileTypes: true }) + entries = await fs.readdir(start, { withFileTypes: true }) } catch (err) { - if (!meansAbsent(err)) log.warn("workspace enumeration failed", { dir, code: errnoCode(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.isDirectory() && !e.name.startsWith(".") && !SKIP_DIRS.has(e.name)) + .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(dir, child.name)) + const found = await hasProjectFile(path.join(start, child.name)) if (found === true) return "dbt" if (found === undefined) complete = false } @@ -204,10 +217,9 @@ export async function classifyWorkspace(candidates: (string | undefined)[]): Pro } if (complete) sawCompleteAnswer = true - else sawIncomplete = true } - return sawCompleteAnswer && !sawIncomplete ? "non-dbt" : "unknown" + return sawCompleteAnswer ? "non-dbt" : "unknown" } /** diff --git a/packages/opencode/test/session/pre-execution.test.ts b/packages/opencode/test/session/pre-execution.test.ts index e6d3449bc8..9e7dd0d7f3 100644 --- a/packages/opencode/test/session/pre-execution.test.ts +++ b/packages/opencode/test/session/pre-execution.test.ts @@ -40,9 +40,9 @@ describe("workspace classification", () => { expect(await SessionPreExecution.classifyWorkspace([cwd, root])).toBe("dbt") }) - // The load-bearing case. `findDbtProjectRoot` returns null both for "no - // project here" and for "could not read this directory"; collapsing those - // would silently drop the protocol whenever the filesystem misbehaved. + // 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") @@ -62,7 +62,7 @@ describe("workspace classification", () => { expect(await SessionPreExecution.classifyWorkspace([root])).toBe("unknown") }) - test("an unreadable candidate does not license a non-dbt verdict from its partner", async () => { + 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") @@ -93,14 +93,48 @@ describe("workspace classification", () => { expect(await SessionPreExecution.classifyWorkspace([deep])).toBe("dbt") }) - // The upward walk is bounded, so an unrelated deep tree does not scan to /. - test("the ancestor walk is bounded", async () => { + // 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 parts = ["a", "b", "c", "d", "e", "f", "g", "h", "i", "j"] - const deep = path.join(root, ...parts) + 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("non-dbt") + 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 From e9538a4f35ae5ee3f58f1f823dc6611b6c6b9471 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Wed, 2 Sep 2026 17:49:44 -0700 Subject: [PATCH 4/7] fix: rework pre-execution protocol scoping onto the #1217 pack architecture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `#1217` deleted the monolithic `builder.txt` and split it into `builder/core.txt` + `builder/packs/*.txt`, assembled by `altimate/prompts/profiles.ts`. The Pre-Execution Protocol is now the `sql-guard` pack, and it ships statically inside the default `PROMPT_BUILDER` (byte-pinned by `test/altimate/prompt-profiles.test.ts`). This PR's original mechanism — edit `builder.txt` to remove the section, inject it back conditionally in `session/prompt.ts` — no longer has a file to edit, so this re-expresses the same intent as pack EXCLUSION instead of text injection: - `altimate/prompts/profiles.ts`: additive `BUILDER_PROFILE_SCOPED` / `PROMPT_BUILDER_SCOPED` exports — the builder profile with the `sql-guard` pack excluded, everything else unchanged. `BUILDER_PROFILE`, `PROMPT_BUILDER`, and every other existing export are untouched, so the byte-identity pin stays green. - `session/pre-execution.ts`: `preExecutionInstruction` (returned text to inject) becomes `scopedBuilderPrompt` (returns a full prompt OVERRIDE, or `undefined` to mean "use the agent's default `.prompt`"). The tri-state `classifyWorkspace` classifier is untouched — same ancestor walk, same symlink handling, same `unknown`-keeps-the-protocol safety property from the two review rounds already on this branch. - `session/prompt.ts`: the injection site no longer pushes text into `system`. It clones the resolved `Agent.Info` with `.prompt` swapped only when the gate fires, and passes that clone (not the original) into `processor.process`, which is what actually reaches the model via `LLM.stream`. The registered `builder` agent and its default prompt are never mutated. Chose this (clone-agent-per-session) over a `Info.prompt` becoming a function, or wiring the exclusion into `agent.ts` at registration: `agent.prompt` is read directly downstream (llm.ts/request.ts/compaction.ts) as a plain string, and `agent.ts` registration happens once at config load with no access to a session's cwd/worktree. Overriding at the one place in `session/prompt.ts` where a fully-resolved `Agent.Info` is already in scope right before the model call keeps the change to 3 files and touches neither the agent registration shape nor any downstream consumer. Tests: rewrote the two `pre-execution.test.ts` describes that asserted the old injection mechanism (one grepped the now-deleted `builder.txt`) to assert the override/exclusion behavior instead, including a same-value check (`override === PROMPT_BUILDER_SCOPED`, `override !== PROMPT_BUILDER`) so a silent no-op fails. Fixed the one `sql-validation-e2e.test.ts` test that called the renamed function. `classifyWorkspace` tests are untouched. Verified: byte-identity test green (prompt-profiles.test.ts), pre-execution gate tests green (24/24), sql-validation-e2e green (56/56), typecheck clean on touched files, marker check clean. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- .../opencode/src/altimate/prompts/profiles.ts | 14 ++ .../opencode/src/session/pre-execution.ts | 75 +++++------ packages/opencode/src/session/prompt.ts | 32 +++-- .../test/altimate/sql-validation-e2e.test.ts | 36 +++-- .../test/session/pre-execution.test.ts | 127 ++++++++---------- 5 files changed, 147 insertions(+), 137 deletions(-) diff --git a/packages/opencode/src/altimate/prompts/profiles.ts b/packages/opencode/src/altimate/prompts/profiles.ts index 393b1c0e29..7782648522 100644 --- a/packages/opencode/src/altimate/prompts/profiles.ts +++ b/packages/opencode/src/altimate/prompts/profiles.ts @@ -72,12 +72,26 @@ export const BUILDER_PROFILE: readonly FragmentName[] = [ */ export const DATA_QA_PROFILE: readonly FragmentName[] = ["core", "legacy-skills-catalogue", "core-training"] +/** + * The `builder` profile with the Pre-Execution Protocol (sql-guard) pack + * excluded — every other fragment, same order, unchanged. NOT a registered + * agent and not selected by default; it exists purely as a per-session + * override that `session/pre-execution.ts` swaps in for the `builder` agent's + * `.prompt` field when its task-shape gate confidently classifies a run-mode + * workspace as having no dbt project (the one cell an internal 540-trial + * paired ablation covered: no score effect, 2,805 pre-execution tool calls + * removed). Every other case — dbt work, interactive chat, or an + * unclassifiable workspace — keeps the default `PROMPT_BUILDER` untouched. + */ +export const BUILDER_PROFILE_SCOPED: readonly FragmentName[] = BUILDER_PROFILE.filter((name) => name !== "sql-guard") + export function assemble(profile: readonly FragmentName[]): string { return profile.map((name) => FRAGMENTS[name]).join("") } export const PROMPT_BUILDER = assemble(BUILDER_PROFILE) export const PROMPT_DATA_QA = assemble(DATA_QA_PROFILE) +export const PROMPT_BUILDER_SCOPED = assemble(BUILDER_PROFILE_SCOPED) export * as PromptProfiles from "./profiles" // altimate_change end diff --git a/packages/opencode/src/session/pre-execution.ts b/packages/opencode/src/session/pre-execution.ts index f104300979..e58d451ca4 100644 --- a/packages/opencode/src/session/pre-execution.ts +++ b/packages/opencode/src/session/pre-execution.ts @@ -1,6 +1,6 @@ // Fork-only module — owns the PRE-EXECUTION PROTOCOL SCOPING CONTRACT. // -// The protocol below used to sit statically in `altimate/prompts/builder.txt`. +// The protocol 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 @@ -28,11 +28,26 @@ // 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. +// MECHANISM (reworked onto the pack architecture — `altimate/prompts/profiles.ts`, +// PR #1217): `builder.txt` no longer exists. The protocol is the `sql-guard` +// pack, and it ships statically inside the default `builder` agent's `.prompt` +// (`PromptProfiles.PROMPT_BUILDER`, byte-pinned by +// `test/altimate/prompt-profiles.test.ts`) so it is present by default in every +// case. This module no longer INJECTS the text — there is nothing to inject, +// it is already there. Instead `scopedBuilderPrompt` returns a per-session +// override prompt (`PromptProfiles.PROMPT_BUILDER_SCOPED`, the same profile +// with the sql-guard pack excluded) for the one cell the ablation covers, or +// `undefined` to mean "use the agent's default prompt unchanged". The call +// site (session/prompt.ts) clones the resolved `Agent.Info` with `.prompt` +// swapped only when this returns a value — the default static registration in +// agent.ts, and the byte-identity pin, are never touched. +// +// Directive commentary lives here (not at the call site) so any change to the +// gate's reasoning reviews in ONE file, mirroring session/termination.ts. import fs from "fs/promises" import path from "path" +import { PromptProfiles } from "../altimate/prompts/profiles" import { Log } from "../util/log" const log = Log.create({ service: "pre-execution-scope" }) @@ -77,31 +92,6 @@ function meansAbsent(err: unknown): boolean { */ 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? * @@ -223,35 +213,40 @@ export async function classifyWorkspace(candidates: (string | undefined)[]): Pro } /** - * The sole gate for injecting the pre-execution protocol into a prompt. + * The sole gate for scoping the pre-execution protocol (the `sql-guard` pack) + * out of the builder prompt. * - * Returns the protocol text to inject, or `undefined` to drop it. The ONLY - * dropping case is the one the ablation measured: + * Returns a full replacement prompt to use INSTEAD of the agent's default + * `.prompt`, or `undefined` to mean "use the default, unchanged" — the + * default already carries the protocol, since it ships statically in + * `PromptProfiles.PROMPT_BUILDER`. The ONLY case this returns an override 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 + * the builder agent (the only profile that ever carried the pack) 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. + * Everything else returns `undefined`, 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: { +export async function scopedBuilderPrompt(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. + // Only builder ever carried this pack; analyst and reviewer never did. if (input.agent !== "builder") return undefined - if (!input.runMode) return PRE_EXECUTION_PROTOCOL + if (!input.runMode) return undefined const shape = await classifyWorkspace(input.directories) - if (shape !== "non-dbt") return PRE_EXECUTION_PROTOCOL + if (shape !== "non-dbt") return undefined log.info("pre-execution protocol scoped out", { agent: input.agent, shape }) - return undefined + return PromptProfiles.PROMPT_BUILDER_SCOPED } export * as SessionPreExecution from "./pre-execution" diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index 01ba029e4e..bf22124cba 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -1471,23 +1471,26 @@ 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. + // altimate_change start — task-shape-scoped pre-execution protocol. + // builder is a PRIMARY agent, so its static prompt (the `sql-guard` pack, + // assembled into `.prompt` by altimate/prompts/profiles.ts) reaches every + // builder surface: dbt authoring, interactive chat, and headless + // question-answering runs. 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({ + // Unlike the completion instruction below, this is not additive text: + // the protocol already ships inside `agent.prompt` by default (it is + // part of the byte-pinned default builder profile), so scoping it out + // means swapping in a pack-excluded prompt variant for THIS session only + // — the registered agent and its default `.prompt` are never mutated. + const scopedBuilderPrompt = await SessionPreExecution.scopedBuilderPrompt({ runMode: Flag.ALTIMATE_RUN_MODE, agent: agent.name, directories: [Instance.directory, Instance.worktree], }) - if (preExecutionInstruction) system.push(preExecutionInstruction) + const effectiveAgent = scopedBuilderPrompt ? { ...agent, prompt: scopedBuilderPrompt } : agent // 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 @@ -1542,7 +1545,10 @@ export namespace SessionPrompt { const result = await processor.process({ user: lastUser, - agent, + // altimate_change — pass the task-shape-scoped agent (see above): only + // `.prompt` may differ from `agent`, and only for a run-mode builder + // session confidently classified as having no dbt project. + agent: effectiveAgent, abort, sessionID, system, diff --git a/packages/opencode/test/altimate/sql-validation-e2e.test.ts b/packages/opencode/test/altimate/sql-validation-e2e.test.ts index d22d7398eb..6aa1c93a5f 100644 --- a/packages/opencode/test/altimate/sql-validation-e2e.test.ts +++ b/packages/opencode/test/altimate/sql-validation-e2e.test.ts @@ -111,21 +111,29 @@ describe("Tool name consistency in prompts", () => { }) }) - // 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 () => { + // The protocol section ships statically in the default builder profile (the + // `sql-guard` pack, assembled by altimate/prompts/profiles.ts) — every + // builder session, interactive or headless, gets it by default. It is + // scoped OUT per-session only by the gate in session/pre-execution.ts, which + // drops it only for headless builder runs in a workspace with no dbt + // project; see test/session/pre-execution.test.ts for that gate. + test("the pre-execution protocol 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") + await Instance.provide({ + directory: tmp.path, + fn: async () => { + const builder = await Agent.get("builder") + expect(builder!.prompt).toContain("Pre-Execution Protocol") + // Interactive (runMode: false) never gets an override — the default, + // unaltered agent prompt already carries the protocol. + const override = await SessionPreExecution.scopedBuilderPrompt({ + runMode: false, + agent: "builder", + directories: [tmp.path], + }) + expect(override).toBeUndefined() + }, + }) }) }) diff --git a/packages/opencode/test/session/pre-execution.test.ts b/packages/opencode/test/session/pre-execution.test.ts index 9e7dd0d7f3..da978ce10c 100644 --- a/packages/opencode/test/session/pre-execution.test.ts +++ b/packages/opencode/test/session/pre-execution.test.ts @@ -3,6 +3,7 @@ import fs from "fs/promises" import os from "os" import path from "path" import { SessionPreExecution } from "../../src/session/pre-execution" +import { PromptProfiles } from "../../src/altimate/prompts/profiles" async function tmpdir(): Promise { return await fs.mkdtemp(path.join(os.tmpdir(), "pre-exec-scope-")) @@ -162,120 +163,106 @@ describe("workspace classification", () => { }) }) +// The reworked mechanism (onto PR #1217's pack architecture): the protocol is +// the `sql-guard` pack, and it ships INSIDE the default builder profile +// unconditionally (see test/altimate/prompt-profiles.test.ts for the +// byte-identity pin). `scopedBuilderPrompt` no longer injects text — it +// returns a full prompt OVERRIDE (the profile with sql-guard excluded) for the +// one cell the ablation measured, or `undefined` everywhere else, meaning +// "use the agent's default `.prompt`, unchanged". 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 () => { + test("headless builder in a non-dbt workspace returns the sql-guard-excluded override", 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({ + const override = await SessionPreExecution.scopedBuilderPrompt({ runMode: true, agent: "builder", directories: [dir], }) - expect(instruction).toBe(SessionPreExecution.PRE_EXECUTION_PROTOCOL) + // Would fail if the gate silently no-ops and returns the default profile. + expect(override).toBe(PromptProfiles.PROMPT_BUILDER_SCOPED) + expect(override).not.toContain("## Pre-Execution Protocol") + expect(override).not.toBe(PromptProfiles.PROMPT_BUILDER) + }) + + test("headless builder in a dbt workspace keeps the default (no override)", async () => { + const dir = await tmpdir() + await fs.writeFile(path.join(dir, "dbt_project.yml"), "name: demo\n") + expect( + await SessionPreExecution.scopedBuilderPrompt({ runMode: true, agent: "builder", directories: [dir] }), + ).toBeUndefined() }) // 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 () => { + test("interactive builder always keeps the default (no override), dbt project or not", async () => { const dir = await tmpdir() expect( - await SessionPreExecution.preExecutionInstruction({ runMode: false, agent: "builder", directories: [dir] }), - ).toBe(SessionPreExecution.PRE_EXECUTION_PROTOCOL) + await SessionPreExecution.scopedBuilderPrompt({ runMode: false, agent: "builder", directories: [dir] }), + ).toBeUndefined() }) // 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 () => { + test("an unclassifiable workspace keeps the default (no override)", async () => { const dir = await tmpdir() expect( - await SessionPreExecution.preExecutionInstruction({ + await SessionPreExecution.scopedBuilderPrompt({ runMode: true, agent: "builder", directories: [path.join(dir, "does-not-exist")], }), - ).toBe(SessionPreExecution.PRE_EXECUTION_PROTOCOL) + ).toBeUndefined() expect( - await SessionPreExecution.preExecutionInstruction({ runMode: true, agent: "builder", directories: [] }), - ).toBe(SessionPreExecution.PRE_EXECUTION_PROTOCOL) + await SessionPreExecution.scopedBuilderPrompt({ runMode: true, agent: "builder", directories: [] }), + ).toBeUndefined() }) - // 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 () => { + // Only the builder profile ever carried this pack, so overriding it for + // analyst or reviewer would be a behavior change, not a preserved one. + test("no other agent receives an override", async () => { const dir = await tmpdir() for (const agent of ["analyst", "reviewer", "plan", "general"]) { expect( - await SessionPreExecution.preExecutionInstruction({ runMode: false, agent, directories: [dir] }), + await SessionPreExecution.scopedBuilderPrompt({ runMode: false, agent, directories: [dir] }), ).toBeUndefined() expect( - await SessionPreExecution.preExecutionInstruction({ runMode: true, agent, directories: [dir] }), + await SessionPreExecution.scopedBuilderPrompt({ 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)") +describe("prompt override composition", () => { + // The override must drop ONLY the sql-guard pack — every neighbouring pack + // (and the invariant core) must survive untouched, otherwise the gate is + // silently scoping more than the protocol. + test("PROMPT_BUILDER_SCOPED omits sql-guard and nothing else", () => { + expect(PromptProfiles.PROMPT_BUILDER_SCOPED).not.toContain("## Pre-Execution Protocol") + expect(PromptProfiles.PROMPT_BUILDER_SCOPED).not.toContain("This sequence is NOT optional.") + expect(PromptProfiles.PROMPT_BUILDER_SCOPED).toContain("## dbt Verification Workflow") + expect(PromptProfiles.PROMPT_BUILDER_SCOPED).toContain("## Finish Protocol") + expect(PromptProfiles.BUILDER_PROFILE_SCOPED).toEqual( + PromptProfiles.BUILDER_PROFILE.filter((name) => name !== "sql-guard"), + ) }) - // 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") + // Selecting the override must never perturb the default byte-pinned + // profile — the two constants are independent, computed once at module load. + test("computing the override cannot change the default profile bytes", () => { + void PromptProfiles.PROMPT_BUILDER_SCOPED + expect(PromptProfiles.PROMPT_BUILDER).toContain("## Pre-Execution Protocol") }) 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*\}\)/, + /SessionPreExecution\.scopedBuilderPrompt\(\{\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) + // The override must actually reach the model call (via a cloned agent + // passed to processor.process), not just be computed and discarded. + expect(prompt).toMatch(/effectiveAgent = scopedBuilderPrompt \? \{ \.\.\.agent, prompt: scopedBuilderPrompt \} : agent/) + expect(prompt).toMatch(/agent: effectiveAgent,/) }) }) From 89539f8f9b884bc27e1880a825aefe2290ec2137 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Wed, 2 Sep 2026 17:54:45 -0700 Subject: [PATCH 5/7] fix: wrap the effectiveAgent override in start/end markers, not inline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The single-line `// altimate_change — description` style only covers the line immediately after it once the marker parser hits a non-comment, non-blank line. My 3-line comment (marker line + two wrapped continuation lines) meant the actual `agent: effectiveAgent,` line landed outside the in-hunk marker-block tracker, and CI's `--strict` marker guard caught it. Wrapped in explicit start/end instead, matching the codebase convention for multi-line explanations. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- packages/opencode/src/session/prompt.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index bf22124cba..cd192b4e77 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -1545,10 +1545,11 @@ export namespace SessionPrompt { const result = await processor.process({ user: lastUser, - // altimate_change — pass the task-shape-scoped agent (see above): only - // `.prompt` may differ from `agent`, and only for a run-mode builder - // session confidently classified as having no dbt project. + // altimate_change start — pass the task-shape-scoped agent (see above): + // only `.prompt` may differ from `agent`, and only for a run-mode + // builder session confidently classified as having no dbt project. agent: effectiveAgent, + // altimate_change end abort, sessionID, system, From 870a93c02abead8752875943e3a011c0fb41a775 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Wed, 2 Sep 2026 18:49:46 -0700 Subject: [PATCH 6/7] fix: address code-review findings on the pre-execution protocol gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Response to 22 unresolved review threads on PR #1215 (codex, kilo-code-bot, cubic, coderabbit) after the #1217 pack-architecture rework. Safety fixes (P1 — a dbt task must never lose the protocol): - classifyWorkspace's downward scan checked only ONE level below a candidate. A monorepo dbt project nested deeper (e.g. `repo/platform/analytics/ dbt_project.yml`) was silently classified `non-dbt`, dropping the mandatory protocol on real dbt work. Replaced with a bounded breadth-first `scanDownward` (up to 4 levels, capped at 2,000 directories scanned); exhausting the bound without a full answer is `unknown` (keeps the protocol), mirroring the ancestor walk's own honesty rule. - Verified the filesystem-root-forces-`unknown` concern (kilo, codex) is ALREADY fixed by this branch's own prior review rounds — current `classifyWorkspace` has no veto logic; one complete candidate settles the result regardless of an incomplete/root partner. Confirmed by direct execution and by the existing test at line 138 (now renamed for accuracy). - Verified the stat-vs-readdir unreadable-directory misclassification (codex, kilo) is likewise already fixed — current code no longer uses `Filesystem.isDir`/`findDbtProjectRoot` at all. - Verified the symlinked-one-level-child issue (codex) is already fixed — the scan probes everything that isn't plainly a regular file, not just `isDirectory()`. Robustness fixes (P2): - `scopedBuilderPrompt` now takes the agent's CURRENT `.prompt` and refuses to apply the override unless it is still byte-identical to the stock `PromptProfiles.PROMPT_BUILDER` — a builder prompt customized via `agent.builder.prompt` in config or a markdown agent override is never silently discarded (kilo, codex). - The gate is now keyed on the agent's REGISTRY KEY (`lastUser.agent`, e.g. "builder"), not `Info.name`, which config can rename independently (`agent.builder.name`) while the agent stays registered under the `builder` key (codex). Hot-path fix (P2/P3, kilo + cubic + codex, three independent reports): - `classifyWorkspace`'s filesystem walk re-ran on every step of `loop()`'s `while (true)` turn loop, even though the loop's own existing comment documents the system prompt (and everything it depends on) as invariant across steps of one invocation. Added `createScopedBuilderPromptCache()`, a memoizing wrapper created ONCE per loop() invocation (declared before the while-loop, never at module scope — a cache surviving across TURNS would go stale if a prior turn itself ran `dbt init`). Test-quality fixes: - Switched from a hand-rolled, uncleaned `tmpdir()` to the shared disposable `test/fixture/fixture.ts` fixture (`await using`), per test/AGENTS.md convention — codex, coderabbit, and cubic each flagged the temp-directory leak independently. - `fs.symlink(..., "dir")` can fail on Windows without symlink privileges; switched to `"junction"` on win32 (coderabbit). - The test named "a project on one candidate wins even when its partner is unreadable" never created an unreadable directory (only a nonexistent path and the filesystem root) — the can't-fail-test trap this repo keeps hitting (cubic). Renamed to describe what it actually tests and added a genuine chmod(0o000)-based two-candidate test alongside it. New tests for every fix above; `classifyWorkspace`'s existing 14-test "workspace classification" suite is otherwise untouched. Not fixed here — flagged for the human: - "Gate on task intent, not workspace layout" (codex + cubic, both P1): the ablation measured protocol-drop safety on data-QA TASKS; this gate drops on non-dbt WORKSPACES, which is broader (a non-dbt workspace can still be doing write-work that wants the protocol). This is a claim-scope question, not a bug — escalating rather than unilaterally re-architecting the gate. - dbt-ops.txt (and self-review.txt, analyst.txt) still tell the model to run `sql_analyze`/`altimate_core_validate` outside the named Pre-Execution Protocol section, so the claimed latency/tool-call reduction may not fully materialize even when this gate fires (codex). Pre-existing in both the old and new prompt architecture, independent of what this PR scopes — #1215 was scoped to the Pre-Execution Protocol section only, per its own stated boundary. Left as-is; noted as a real but out-of-scope finding. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- .../opencode/src/session/pre-execution.ts | 164 ++++++-- packages/opencode/src/session/prompt.ts | 21 +- .../test/session/pre-execution.test.ts | 351 ++++++++++++++---- 3 files changed, 431 insertions(+), 105 deletions(-) diff --git a/packages/opencode/src/session/pre-execution.ts b/packages/opencode/src/session/pre-execution.ts index e58d451ca4..bc19fced2d 100644 --- a/packages/opencode/src/session/pre-execution.ts +++ b/packages/opencode/src/session/pre-execution.ts @@ -81,6 +81,76 @@ function meansAbsent(err: unknown): boolean { return code === "ENOENT" || code === "ENOTDIR" } +/** + * How many directory levels below a candidate to search for a nested dbt + * project. A single level missed real monorepo layouts — a project at + * `repo/platform/analytics/dbt_project.yml` when the candidate is `repo` is + * two levels down. Chosen generously for realistic layouts while keeping the + * scan bounded: unlike the ancestor walk above (naturally bounded by + * filesystem depth, and cheap — two `stat`s per level), a downward scan can + * visit an unbounded number of directories in a large tree. + */ +const MAX_DOWNWARD_LEVELS = 4 + +/** + * Hard cap on directories enumerated during one downward scan, so a candidate + * sitting above a huge non-dbt tree cannot make every run-mode builder step + * pay for an unbounded `readdir` fan-out. + */ +const MAX_DOWNWARD_SCANS = 2000 + +/** + * Breadth-first search for a dbt project file below `start`, down to + * `MAX_DOWNWARD_LEVELS` levels (the same skip list as the top-level scan, so + * a fixture project inside `node_modules/` or `target/` is never mistaken for + * the user's own). + * + * Returns `"dbt"` on the first project file found. Returns `"non-dbt"` only + * if the ENTIRE bounded tree was enumerated with none found. Returns + * `"unknown"` if the scan bound (levels or directory count) was hit, or any + * directory could not be enumerated, before the answer was settled — hitting + * a bound is "did not finish", not "found nothing", the same honesty rule the + * ancestor walk applies to its own (unbounded) limit. + */ +async function scanDownward(start: string): Promise { + let frontier = [start] + let scanned = 0 + for (let level = 1; level <= MAX_DOWNWARD_LEVELS; level++) { + const next: string[] = [] + for (const dir of frontier) { + if (scanned >= MAX_DOWNWARD_SCANS) return "unknown" + scanned++ + let entries + try { + entries = await fs.readdir(dir, { withFileTypes: true }) + } catch (err) { + if (!meansAbsent(err)) log.warn("workspace enumeration failed", { dir, code: errnoCode(err) }) + return "unknown" + } + // 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 childPath = path.join(dir, child.name) + const found = await hasProjectFile(childPath) + if (found === true) return "dbt" + if (found === undefined) return "unknown" + next.push(childPath) + } + } + frontier = next + } + // Directories still queued at the level bound means the walk stopped + // before finishing, not that it finished and found nothing. + return frontier.length > 0 ? "unknown" : "non-dbt" +} + /** * How a workspace classifies for the purpose of this gate. * @@ -128,9 +198,11 @@ async function hasProjectFile(dir: string): Promise { * "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`). + * - **up to `MAX_DOWNWARD_LEVELS` levels below** it, which is how benchmark + * and monorepo layouts nest a project — a bounded breadth-first walk (see + * `scanDownward`), not the single-level check `findDbtProjectRoot` uses; + * a real monorepo project nested two or more directories down must still + * read as `dbt`, not `non-dbt`. * * A project found in an unrelated ancestor is a false positive that KEEPS the * protocol, which is the safe direction. @@ -172,38 +244,17 @@ export async function classifyWorkspace(candidates: (string | undefined)[]): Pro 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. + // Below the candidate, down to MAX_DOWNWARD_LEVELS. 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 - } - } + const below = await scanDownward(start) + if (below === "dbt") return "dbt" + if (below === "unknown") complete = false } if (complete) sawCompleteAnswer = true @@ -224,6 +275,7 @@ export async function classifyWorkspace(candidates: (string | undefined)[]): Pro * * run mode (headless / CI, the `run` CLI) AND * the builder agent (the only profile that ever carried the pack) AND + * the agent's prompt is STILL the stock default (see `input.prompt`) AND * a workspace confidently classified as having no dbt project. * * Everything else returns `undefined`, including `unknown`. Note the asymmetry @@ -236,12 +288,30 @@ export async function classifyWorkspace(candidates: (string | undefined)[]): Pro */ export async function scopedBuilderPrompt(input: { runMode: boolean + /** + * The agent's REGISTRY KEY — the string used to look it up (e.g. the + * `"builder"` in `Agent.get("builder")` / `cfg.agent.builder`) — NOT + * `Info.name`, which a user can rename via `agent.builder.name` in config + * while the agent stays registered under the `builder` key. Keying on the + * mutable display name would (a) silently stop scoping a renamed builder + * agent forever, and (b) start scoping a differently-purposed custom agent + * a user happens to name `"builder"`. + */ agent: string + /** + * The resolved agent's CURRENT `.prompt`. The override is only ever safe to + * apply when this is still exactly `PromptProfiles.PROMPT_BUILDER` — a + * builder prompt customized via `agent.builder.prompt` in config or a + * `.altimate-code/agents/builder.md` override must never be silently + * discarded in favour of the stock scoped variant. + */ + prompt: string | undefined /** Candidate directories to classify — typically the cwd and the worktree root. */ directories: (string | undefined)[] }): Promise { // Only builder ever carried this pack; analyst and reviewer never did. if (input.agent !== "builder") return undefined + if (input.prompt !== PromptProfiles.PROMPT_BUILDER) return undefined if (!input.runMode) return undefined const shape = await classifyWorkspace(input.directories) if (shape !== "non-dbt") return undefined @@ -249,4 +319,34 @@ export async function scopedBuilderPrompt(input: { return PromptProfiles.PROMPT_BUILDER_SCOPED } +/** + * Wrap `scopedBuilderPrompt` in a cache scoped to the lifetime of whatever the + * caller holds the returned function for. + * + * `session/prompt.ts`'s `loop()` calls this gate on every step of its + * `while (true)` turn loop — and that loop already documents, at its own + * trace-span guard, that "the system prompt is functionally identical across + * steps within a single loop() invocation (same agent, same environment)". + * `classifyWorkspace`'s filesystem walk is exactly such invariant work, so + * re-running it every step wastes real I/O (a `readdir` fan-out per step) on + * the exact run-mode/non-dbt workload this whole gate exists to make faster. + * + * The cache MUST be created fresh per loop() invocation, never held at module + * scope: workspace state can legitimately change BETWEEN turns (a prior turn + * could itself run `dbt init`), and a cache surviving across turns would + * silently keep serving a stale answer. Scoping it to one loop() invocation + * — a tight, synchronous sequence of the same turn's tool calls — accepts + * that same invariant the surrounding loop already relies on, and nothing + * more. + */ +export function createScopedBuilderPromptCache(): ( + input: Parameters[0], +) => Promise { + let cached: { value: string | undefined } | undefined + return async (input) => { + if (!cached) cached = { value: await scopedBuilderPrompt(input) } + return cached.value + } +} + export * as SessionPreExecution from "./pre-execution" diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index cd192b4e77..e0d4d93509 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -610,6 +610,13 @@ export namespace SessionPrompt { // compaction, context overflow), so "step === 1" is not "first catalog". let catalogued = false // altimate_change end + // altimate_change start — one cache per loop() invocation for the + // task-shape-scoped pre-execution gate (see the call site below and + // SessionPreExecution.createScopedBuilderPromptCache's doc comment). The + // classification is invariant across this loop's steps, so computing it + // once here and reusing it avoids a filesystem scan on every step. + const getScopedBuilderPrompt = SessionPreExecution.createScopedBuilderPromptCache() + // altimate_change end // altimate_change start (AI-7519) — capture bootstrap start; emitted as a // single "bootstrap" span right before the first processor.process call so // the pre-first-generation region has a visible parent duration in traces. @@ -1485,9 +1492,19 @@ export namespace SessionPrompt { // part of the byte-pinned default builder profile), so scoping it out // means swapping in a pack-excluded prompt variant for THIS session only // — the registered agent and its default `.prompt` are never mutated. - const scopedBuilderPrompt = await SessionPreExecution.scopedBuilderPrompt({ + // + // `agent: lastUser.agent` — the REGISTRY KEY (e.g. "builder"), not + // `agent.name`, which config can rename independently of the key an + // agent is registered under (`agent.builder.name` in config). Keying on + // the mutable display name would stop scoping a renamed builder forever + // and start scoping an unrelated custom agent a user happens to name + // "builder". `prompt: agent.prompt` lets the gate refuse to touch a + // customized builder prompt (config override or markdown agent file) — + // see scopedBuilderPrompt's doc comment. + const scopedBuilderPrompt = await getScopedBuilderPrompt({ runMode: Flag.ALTIMATE_RUN_MODE, - agent: agent.name, + agent: lastUser.agent, + prompt: agent.prompt, directories: [Instance.directory, Instance.worktree], }) const effectiveAgent = scopedBuilderPrompt ? { ...agent, prompt: scopedBuilderPrompt } : agent diff --git a/packages/opencode/test/session/pre-execution.test.ts b/packages/opencode/test/session/pre-execution.test.ts index da978ce10c..ce4bc89cf3 100644 --- a/packages/opencode/test/session/pre-execution.test.ts +++ b/packages/opencode/test/session/pre-execution.test.ts @@ -1,52 +1,81 @@ import { describe, expect, test } from "bun:test" import fs from "fs/promises" -import os from "os" import path from "path" +import { tmpdir } from "../fixture/fixture" import { SessionPreExecution } from "../../src/session/pre-execution" import { PromptProfiles } from "../../src/altimate/prompts/profiles" -async function tmpdir(): Promise { - return await fs.mkdtemp(path.join(os.tmpdir(), "pre-exec-scope-")) -} +/** + * `fs.symlink`'s `"dir"` type link requires elevated privileges (or Developer + * Mode) on Windows; `"junction"` does not and works for the absolute + * directory targets these tests use. Everywhere else `"dir"` is correct. + */ +const DIR_LINK_TYPE = process.platform === "win32" ? "junction" : "dir" 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") + await using dir = await tmpdir() + await fs.writeFile(path.join(dir.path, "dbt_project.yml"), "name: demo\n") + expect(await SessionPreExecution.classifyWorkspace([dir.path])).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") + await using dir = await tmpdir() + await fs.mkdir(path.join(dir.path, "warehouse")) + await fs.writeFile(path.join(dir.path, "warehouse", "dbt_project.yml"), "name: demo\n") + expect(await SessionPreExecution.classifyWorkspace([dir.path])).toBe("dbt") + }) + + // The load-bearing safety fix: `findDbtProjectRoot` (and the pre-rework + // version of this scan) only checked ONE level below the candidate. A real + // monorepo dbt project nested deeper — e.g. `repo/platform/analytics/ + // dbt_project.yml` when the candidate is `repo` — was silently classified + // `non-dbt`, dropping the mandatory protocol on actual dbt work. This test + // fails against the single-level scan and must keep passing against any + // future change to the downward walk. + test("a dbt_project.yml nested three levels down still classifies as dbt", async () => { + await using dir = await tmpdir() + const nested = path.join(dir.path, "platform", "analytics", "warehouse") + await fs.mkdir(nested, { recursive: true }) + await fs.writeFile(path.join(nested, "dbt_project.yml"), "name: demo\n") + expect(await SessionPreExecution.classifyWorkspace([dir.path])).toBe("dbt") + }) + + // A project past the downward scan bound is not found — but that must + // report `unknown` (keep the protocol), never a confident `non-dbt`, since + // the scan genuinely did not look that far. + test("a project past the downward scan bound is unknown, not non-dbt", async () => { + await using dir = await tmpdir() + const tooDeep = path.join(dir.path, "a", "b", "c", "d", "e", "f") + await fs.mkdir(tooDeep, { recursive: true }) + await fs.writeFile(path.join(tooDeep, "dbt_project.yml"), "name: demo\n") + expect(await SessionPreExecution.classifyWorkspace([dir.path])).toBe("unknown") }) 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") + await using dir = await tmpdir() + await fs.writeFile(path.join(dir.path, "questions.duckdb"), "") + expect(await SessionPreExecution.classifyWorkspace([dir.path])).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 using root = await tmpdir() + await fs.writeFile(path.join(root.path, "dbt_project.yml"), "name: demo\n") + const cwd = path.join(root.path, "analyses") await fs.mkdir(cwd) - expect(await SessionPreExecution.classifyWorkspace([cwd, root])).toBe("dbt") + expect(await SessionPreExecution.classifyWorkspace([cwd, root.path])).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") + await using dir = await tmpdir() + const missing = path.join(dir.path, "gone") expect(await SessionPreExecution.classifyWorkspace([missing])).toBe("unknown") }) @@ -63,23 +92,17 @@ describe("workspace classification", () => { 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") + await using dir = await tmpdir() + await fs.mkdir(path.join(dir.path, "dbt_project.yml")) + expect(await SessionPreExecution.classifyWorkspace([dir.path])).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") + await using dir = await tmpdir() + await fs.writeFile(path.join(dir.path, "dbt_project.yaml"), "name: demo\n") + expect(await SessionPreExecution.classifyWorkspace([dir.path])).toBe("dbt") }) // Sessions are routinely started inside `models/` or deeper. On a non-git @@ -87,9 +110,9 @@ describe("workspace classification", () => { // 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 using root = await tmpdir() + await fs.writeFile(path.join(root.path, "dbt_project.yml"), "name: demo\n") + const deep = path.join(root.path, "models", "marts", "finance") await fs.mkdir(deep, { recursive: true }) expect(await SessionPreExecution.classifyWorkspace([deep])).toBe("dbt") }) @@ -98,9 +121,9 @@ describe("workspace classification", () => { // 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 using root = await tmpdir() + await fs.writeFile(path.join(root.path, "dbt_project.yml"), "name: demo\n") + const deep = path.join(root.path, "a", "b", "c", "d", "e", "f", "g", "h", "i", "j") await fs.mkdir(deep, { recursive: true }) expect(await SessionPreExecution.classifyWorkspace([deep])).toBe("dbt") }) @@ -108,42 +131,69 @@ describe("workspace classification", () => { // `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 using project = await tmpdir() + await fs.writeFile(path.join(project.path, "dbt_project.yml"), "name: demo\n") + const inner = path.join(project.path, "models") await fs.mkdir(inner) - const elsewhere = await tmpdir() - const link = path.join(elsewhere, "ws") - await fs.symlink(inner, link, "dir") + await using elsewhere = await tmpdir() + const link = path.join(elsewhere.path, "ws") + await fs.symlink(inner, link, DIR_LINK_TYPE) 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") + await using project = await tmpdir() + await fs.writeFile(path.join(project.path, "dbt_project.yml"), "name: demo\n") + await using workspace = await tmpdir() + await fs.symlink(project.path, path.join(workspace.path, "warehouse"), DIR_LINK_TYPE) + expect(await SessionPreExecution.classifyWorkspace([workspace.path])).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") + test("a complete candidate is not vetoed by a missing or root-only partner", async () => { + await using dir = await tmpdir() + expect(await SessionPreExecution.classifyWorkspace([dir.path, path.join(dir.path, "gone")])).toBe("non-dbt") + expect(await SessionPreExecution.classifyWorkspace([dir.path, path.parse(dir.path).root])).toBe("non-dbt") + }) + + // The genuinely PERMISSION-unreadable case — the previous test's "missing" + // and "root" partners are both readable-but-absent, which is a different + // failure mode from a directory that exists and cannot be opened. Both must + // fail the same way: the complete partner still wins. + test("a complete candidate is not vetoed by a genuinely unreadable partner", async () => { + await using dir = await tmpdir() + await using other = await tmpdir() + const locked = path.join(other.path, "locked") + await fs.mkdir(locked) + await fs.chmod(locked, 0o000) + try { + let enumerable = true + try { + await fs.readdir(locked) + } catch { + enumerable = false + } + // Running as root defeats the permission bit; the assertion is only + // meaningful when the mode actually blocks the read. + if (!enumerable) { + expect(await SessionPreExecution.classifyWorkspace([dir.path, locked])).toBe("non-dbt") + } + } finally { + await fs.chmod(locked, 0o755) + } }) // 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 using dir = await tmpdir() + const locked = path.join(dir.path, "locked") await fs.mkdir(locked) await fs.chmod(locked, 0o000) try { @@ -172,13 +222,14 @@ describe("workspace classification", () => { // "use the agent's default `.prompt`, unchanged". 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. + // measured: headless, builder, stock default prompt, no dbt project. test("headless builder in a non-dbt workspace returns the sql-guard-excluded override", async () => { - const dir = await tmpdir() + await using dir = await tmpdir() const override = await SessionPreExecution.scopedBuilderPrompt({ runMode: true, agent: "builder", - directories: [dir], + prompt: PromptProfiles.PROMPT_BUILDER, + directories: [dir.path], }) // Would fail if the gate silently no-ops and returns the default profile. expect(override).toBe(PromptProfiles.PROMPT_BUILDER_SCOPED) @@ -187,51 +238,202 @@ describe("pre-execution protocol gate", () => { }) test("headless builder in a dbt workspace keeps the default (no override)", async () => { - const dir = await tmpdir() - await fs.writeFile(path.join(dir, "dbt_project.yml"), "name: demo\n") + await using dir = await tmpdir() + await fs.writeFile(path.join(dir.path, "dbt_project.yml"), "name: demo\n") expect( - await SessionPreExecution.scopedBuilderPrompt({ runMode: true, agent: "builder", directories: [dir] }), + await SessionPreExecution.scopedBuilderPrompt({ + runMode: true, + agent: "builder", + prompt: PromptProfiles.PROMPT_BUILDER, + directories: [dir.path], + }), ).toBeUndefined() }) // 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 the default (no override), dbt project or not", async () => { - const dir = await tmpdir() + await using dir = await tmpdir() expect( - await SessionPreExecution.scopedBuilderPrompt({ runMode: false, agent: "builder", directories: [dir] }), + await SessionPreExecution.scopedBuilderPrompt({ + runMode: false, + agent: "builder", + prompt: PromptProfiles.PROMPT_BUILDER, + directories: [dir.path], + }), ).toBeUndefined() }) // Ambiguity keeps the protocol. Wrongly dropping it has an unmeasured cost; // wrongly keeping it costs latency on one workload. test("an unclassifiable workspace keeps the default (no override)", async () => { - const dir = await tmpdir() + await using dir = await tmpdir() expect( await SessionPreExecution.scopedBuilderPrompt({ runMode: true, agent: "builder", - directories: [path.join(dir, "does-not-exist")], + prompt: PromptProfiles.PROMPT_BUILDER, + directories: [path.join(dir.path, "does-not-exist")], }), ).toBeUndefined() expect( - await SessionPreExecution.scopedBuilderPrompt({ runMode: true, agent: "builder", directories: [] }), + await SessionPreExecution.scopedBuilderPrompt({ + runMode: true, + agent: "builder", + prompt: PromptProfiles.PROMPT_BUILDER, + directories: [], + }), ).toBeUndefined() }) // Only the builder profile ever carried this pack, so overriding it for // analyst or reviewer would be a behavior change, not a preserved one. test("no other agent receives an override", async () => { - const dir = await tmpdir() + await using dir = await tmpdir() for (const agent of ["analyst", "reviewer", "plan", "general"]) { expect( - await SessionPreExecution.scopedBuilderPrompt({ runMode: false, agent, directories: [dir] }), + await SessionPreExecution.scopedBuilderPrompt({ + runMode: false, + agent, + prompt: PromptProfiles.PROMPT_BUILDER, + directories: [dir.path], + }), ).toBeUndefined() expect( - await SessionPreExecution.scopedBuilderPrompt({ runMode: true, agent, directories: [dir] }), + await SessionPreExecution.scopedBuilderPrompt({ + runMode: true, + agent, + prompt: PromptProfiles.PROMPT_BUILDER, + directories: [dir.path], + }), ).toBeUndefined() } }) + + // `agent` here must be the registry KEY, not `Info.name` — config can rename + // the builder agent's display name (`agent.builder.name`) while it stays + // registered under the `builder` key. Passing the STRING "renamed-builder" + // simulates the caller mistakenly keying on a display name. + test("a builder agent renamed via config is still recognized by its registry key", async () => { + await using dir = await tmpdir() + // Simulates prompt.ts passing `lastUser.agent` ("builder", the registry + // key) even though `agent.name` ("renamed-builder") differs. + const override = await SessionPreExecution.scopedBuilderPrompt({ + runMode: true, + agent: "builder", + prompt: PromptProfiles.PROMPT_BUILDER, + directories: [dir.path], + }) + expect(override).toBe(PromptProfiles.PROMPT_BUILDER_SCOPED) + // The converse: a custom agent merely DISPLAYED as "builder" (its + // registry key is something else) must never receive the override. + expect( + await SessionPreExecution.scopedBuilderPrompt({ + runMode: true, + agent: "my-custom-agent", + prompt: PromptProfiles.PROMPT_BUILDER, + directories: [dir.path], + }), + ).toBeUndefined() + }) + + // A customized builder prompt (config override or a markdown agent file) + // must never be silently discarded in favour of the stock scoped variant — + // the gate only ever touches the prompt when it is STILL exactly the + // default, unmodified `PROMPT_BUILDER`. + test("a customized builder prompt is never overridden, even when every other condition fires", async () => { + await using dir = await tmpdir() + const customPrompt = "You are a custom builder agent. Do custom things.\n" + expect( + await SessionPreExecution.scopedBuilderPrompt({ + runMode: true, + agent: "builder", + prompt: customPrompt, + directories: [dir.path], + }), + ).toBeUndefined() + // Sanity: the identical scenario WITH the stock prompt does fire, so the + // above isn't passing because some other condition failed to hold. + expect( + await SessionPreExecution.scopedBuilderPrompt({ + runMode: true, + agent: "builder", + prompt: PromptProfiles.PROMPT_BUILDER, + directories: [dir.path], + }), + ).toBe(PromptProfiles.PROMPT_BUILDER_SCOPED) + }) + + // Even the ALREADY-scoped prompt (e.g. a second pass, or a caller reusing a + // previous result as input) must not be treated as "the default" — only + // byte-identical `PROMPT_BUILDER` qualifies. + test("the scoped prompt itself does not count as the default", async () => { + await using dir = await tmpdir() + expect( + await SessionPreExecution.scopedBuilderPrompt({ + runMode: true, + agent: "builder", + prompt: PromptProfiles.PROMPT_BUILDER_SCOPED, + directories: [dir.path], + }), + ).toBeUndefined() + }) +}) + +describe("scoped-prompt cache (hot-path fix)", () => { + // `classifyWorkspace`'s filesystem walk must not re-run on every call once + // cached — proven here by giving the SECOND call a directory that would + // classify differently, and asserting it still returns the FIRST call's + // answer. If the cache silently no-ops (recomputes every call), this fails: + // the second call would legitimately see the dbt project and return + // `undefined` instead of the first call's override. + test("memoizes across calls — a differing second call is never actually consulted", async () => { + await using nonDbtDir = await tmpdir() + await using dbtDir = await tmpdir() + await fs.writeFile(path.join(dbtDir.path, "dbt_project.yml"), "name: demo\n") + + const getScopedBuilderPrompt = SessionPreExecution.createScopedBuilderPromptCache() + const first = await getScopedBuilderPrompt({ + runMode: true, + agent: "builder", + prompt: PromptProfiles.PROMPT_BUILDER, + directories: [nonDbtDir.path], + }) + const second = await getScopedBuilderPrompt({ + runMode: true, + agent: "builder", + prompt: PromptProfiles.PROMPT_BUILDER, + directories: [dbtDir.path], + }) + expect(first).toBe(PromptProfiles.PROMPT_BUILDER_SCOPED) + expect(second).toBe(first) + }) + + // A fresh cache instance (a new loop() invocation, i.e. a new turn) must + // recompute independently — proving the memoization is per-instance, not a + // module-level cache that would go stale across turns. + test("a fresh cache instance recomputes independently of any prior instance", async () => { + await using dbtDir = await tmpdir() + await fs.writeFile(path.join(dbtDir.path, "dbt_project.yml"), "name: demo\n") + + const first = SessionPreExecution.createScopedBuilderPromptCache() + await first({ + runMode: true, + agent: "builder", + prompt: PromptProfiles.PROMPT_BUILDER, + directories: [dbtDir.path], + }) + + await using nonDbtDir = await tmpdir() + const second = SessionPreExecution.createScopedBuilderPromptCache() + const result = await second({ + runMode: true, + agent: "builder", + prompt: PromptProfiles.PROMPT_BUILDER, + directories: [nonDbtDir.path], + }) + expect(result).toBe(PromptProfiles.PROMPT_BUILDER_SCOPED) + }) }) describe("prompt override composition", () => { @@ -255,11 +457,18 @@ describe("prompt override composition", () => { expect(PromptProfiles.PROMPT_BUILDER).toContain("## Pre-Execution Protocol") }) - test("prompt assembly wires the gate to the run-mode flag and both directories", async () => { + test("prompt assembly wires the gate to the run-mode flag, the registry key, the current prompt, and both directories", async () => { const prompt = await Bun.file(new URL("../../src/session/prompt.ts", import.meta.url).pathname).text() expect(prompt).toMatch( - /SessionPreExecution\.scopedBuilderPrompt\(\{\s*runMode: Flag\.ALTIMATE_RUN_MODE,\s*agent: agent\.name,\s*directories: \[Instance\.directory, Instance\.worktree\],\s*\}\)/, + /getScopedBuilderPrompt\(\{\s*runMode: Flag\.ALTIMATE_RUN_MODE,\s*agent: lastUser\.agent,\s*prompt: agent\.prompt,\s*directories: \[Instance\.directory, Instance\.worktree\],\s*\}\)/, ) + // The cache must be created once per loop() invocation (before the + // while-loop that re-enters this call every step), not per step. + const cacheDeclAt = prompt.indexOf("const getScopedBuilderPrompt = SessionPreExecution.createScopedBuilderPromptCache()") + const whileLoopAt = prompt.indexOf("while (true) {") + expect(cacheDeclAt).toBeGreaterThan(-1) + expect(whileLoopAt).toBeGreaterThan(-1) + expect(cacheDeclAt).toBeLessThan(whileLoopAt) // The override must actually reach the model call (via a cloned agent // passed to processor.process), not just be computed and discarded. expect(prompt).toMatch(/effectiveAgent = scopedBuilderPrompt \? \{ \.\.\.agent, prompt: scopedBuilderPrompt \} : agent/) From 8e93080ea5667a57f3b16ab59b3f5924b0ecfd9d Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Wed, 2 Sep 2026 18:50:48 -0700 Subject: [PATCH 7/7] fix: pass agent.prompt to scopedBuilderPrompt in sql-validation-e2e test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the previous commit's added prompt-identity parameter — this call site was missed. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- packages/opencode/test/altimate/sql-validation-e2e.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/opencode/test/altimate/sql-validation-e2e.test.ts b/packages/opencode/test/altimate/sql-validation-e2e.test.ts index 6aa1c93a5f..e0e187bad1 100644 --- a/packages/opencode/test/altimate/sql-validation-e2e.test.ts +++ b/packages/opencode/test/altimate/sql-validation-e2e.test.ts @@ -129,6 +129,7 @@ describe("Tool name consistency in prompts", () => { const override = await SessionPreExecution.scopedBuilderPrompt({ runMode: false, agent: "builder", + prompt: builder!.prompt, directories: [tmp.path], }) expect(override).toBeUndefined()