feat(agent): add dbt-optimizer agent — 6-lane dbt project optimization with evals - #1092
feat(agent): add dbt-optimizer agent — 6-lane dbt project optimization with evals#1092anandgupta42 wants to merge 19 commits into
Conversation
…nd evals
- new native primary agent `dbt-optimizer` (alongside builder/analyst/reviewer/
plan): scan -> select candidates -> fix -> impact report -> PR loop. Deny-by-
default permission allowlist; `edit`/`bash` prompt for approval; the
`sql_execute_write` deny is re-applied after BOTH global and per-agent config
merges so no permissive config can turn the scan into a warehouse writer
- prompt (`prompts/dbt-optimizer.txt`) encodes the dbt-optimization taxonomy
research: 6 detection lanes (materialization/incremental with named
strategies, warehouse physical design gated on query-history evidence, SQL
anti-patterns, DAG economics, run-level orchestration, tests/docs/storage),
an evidence-attribution ladder, ROI-ranked triage, cost-honesty rules, and
an auto-fix vs propose-only boundary
- fix latent verification-tool bugs the agent's promises depend on:
- `altimate_core_rewrite` verify gate now requires `decidable !== false`
(engine can return `equivalent: true, decidable: false`);
`altimate_core_equivalence` reports UNDECIDABLE as its own verdict
- `sql_explain` blocks `analyze: true` for non-read-only statements —
EXPLAIN ANALYZE executes the query (incl. `SELECT ... INTO`)
- `sql_diff` wrapper rewritten to the native handler contract; it previously
read fields the handler never returns and reported everything "identical"
- evals: `optimizer-prompt-contract.test.ts` (prompt invariants + planted-
fixture evidence chain, whitespace-normalized assertions) and
`optimizer-agent-eval.test.ts` (opt-in live-binary eval gated on
`OPTIMIZER_LIVE_EVAL=1` + `OPENCODE_TEST_CLI`; 6-issue DuckDB fixture with
deterministic grading and a read-only tree-snapshot check)
- builder self-review gains a dbt-scoped "Optimization handoff" nudge —
primary agents are excluded from the task tool, so this is the only
build-time bridge to the optimizer
- docs: agent-catalog scaling + dbt-optimization taxonomy research reports in
`docs/internal/`
Closes #1091
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds a native Changesdbt Optimizer
Agent Catalog Research
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant dbtOptimizer
participant dbtProject
participant SqlExplainTool
participant SqlDiffTool
Developer->>dbtOptimizer: start optimization scan
dbtOptimizer->>dbtProject: inspect models and metadata
dbtOptimizer->>SqlExplainTool: validate and analyze read-only SQL
dbtOptimizer->>SqlDiffTool: assess rewrite equivalence
dbtOptimizer-->>Developer: report evidence, confidence, and impact
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/internal/2026-08-12-agent-catalog-scaling-research.md`:
- Around line 195-197: Update the external documentation references in the list
and the additional affected locations to use https:// instead of http://,
preserving the existing URLs and link text.
- Line 167: Update the observability guidance to require redacting audit
payloads before logging: define an allowlisted structured audit schema, redact
or hash sensitive fields, enforce retention limits and access controls, and
ensure raw model outputs are not logged by default.
- Around line 200-202: Update the two multiline reference links in the document,
including the entries around “Subagents | ChatGPT Learn” and the additionally
referenced lines, so each link label and URL use one-line Markdown link syntax
while preserving their destinations.
- Around line 96-101: Update the capability policy for dbt-optimizer so
warehouse/database writes use an explicit non-overridable deny state rather than
an approval-required tier. Ensure uploaded capability definitions and user or
administrator approvals cannot elevate this denial, and add coverage verifying
warehouse writes remain disabled after those requests and approvals.
- Around line 130-132: Define the catalog bundle’s signature envelope and
signing scope near the Distribution service and Local execution runtime
sections. Specify deterministic canonical serialization, exclude
provenance.signature from the signed bytes (or use a detached signature), and
identify the signing algorithm and key ID; add tests confirming tampering with
prompts, capabilities, or policy fields causes verification to fail.
- Around line 120-122: Clarify the promotion policy alongside the mode
definitions: state that only definitions with mode: both may be promoted, and
promotion must not rely solely on safety labels. If promotion may override mode,
require immutable-manifest and session-policy validation before allowing it;
otherwise explicitly prohibit such overrides.
- Around line 82-84: Update the hybrid package-channel design so cached bundles
are partitioned by the complete authorization scope and policy version,
preventing reuse across tenants, organizations, projects, users, repositories,
or policy revisions. Define explicit offline handling for authorization changes
and emergency revocation, including behavior for already-active sessions, and
add tests covering cache isolation, expiration, offline access, and revocation.
In `@docs/internal/2026-08-12-dbt-optimization-taxonomy-research.md`:
- Line 68: Replace citation [14] with [21] in the liquid-clustering statement at
docs/internal/2026-08-12-dbt-optimization-taxonomy-research.md:68 and the
corresponding statement at :81; make no other changes.
In `@packages/opencode/src/agent/agent.ts`:
- Around line 442-459: Enforce a single non-overridable warehouse-write
boundary: in packages/opencode/src/agent/agent.ts:442-459, either deny
warehouse-writing bash commands such as altimate-dbt build after all overrides
or explicitly allow only approved dev-target builds; make the matching choice in
packages/opencode/src/altimate/prompts/dbt-optimizer.txt:10-11 and 173-180, and
update packages/opencode/test/agent/agent.test.ts:134-173 to cover permissive
global and per-agent bash overrides for altimate-dbt build --model <name>.
In `@packages/opencode/src/altimate/tools/altimate-core-equivalence.ts`:
- Around line 50-57: Update the rewrite-approval gate in the equivalence result
handling to require both ed.equivalent === true and ed.decidable === true,
rejecting missing, false, or invalid decidable values while preserving the
existing failure behavior.
In `@packages/opencode/src/altimate/tools/sql-explain.ts`:
- Around line 84-107: Replace validateAnalyzeSafety’s regex-based check with
fail-closed, dialect-aware SQL tokenization that correctly handles string
literals and comments, rejects multiple executable statements, and permits only
a single genuinely read-only statement. Ensure analyze:true execution in the
surrounding flow uses a read-only warehouse role or transaction, and reject
write-capable SQL functions rather than relying on a SELECT prefix.
In `@packages/opencode/test/altimate/optimizer-agent-eval.test.ts`:
- Around line 94-95: Scope each test’s temporary worktree with the per-test
tmpdir() helper instead of manually calling fs.mkdtemp in the setup around
FIXTURE and workdir. Ensure cleanup is registered immediately and runs on
success, assertion or setup failure, and cancellation, while preserving the
existing fixture-copy and test behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a4684d0d-7279-4e38-963b-6d419f7eb8a3
📒 Files selected for processing (30)
docs/internal/2026-08-12-agent-catalog-scaling-research.mddocs/internal/2026-08-12-dbt-optimization-taxonomy-research.mdpackages/opencode/src/agent/agent.tspackages/opencode/src/altimate/prompts/builder.txtpackages/opencode/src/altimate/prompts/dbt-optimizer.txtpackages/opencode/src/altimate/tools/altimate-core-equivalence.tspackages/opencode/src/altimate/tools/altimate-core-rewrite.tspackages/opencode/src/altimate/tools/sql-diff.tspackages/opencode/src/altimate/tools/sql-explain.tspackages/opencode/test/agent/agent.test.tspackages/opencode/test/altimate/altimate-core-equivalence-formatters.test.tspackages/opencode/test/altimate/altimate-core-rewrite-verify.test.tspackages/opencode/test/altimate/carry-forward/agent-safety.test.tspackages/opencode/test/altimate/fixtures/optimizer-project-answer-key.mdpackages/opencode/test/altimate/fixtures/optimizer-project/dbt_project.ymlpackages/opencode/test/altimate/fixtures/optimizer-project/models/marts/dim_customers.sqlpackages/opencode/test/altimate/fixtures/optimizer-project/models/marts/fct_events_daily.sqlpackages/opencode/test/altimate/fixtures/optimizer-project/models/marts/legacy_events_backup.sqlpackages/opencode/test/altimate/fixtures/optimizer-project/models/marts/rpt_apac.sqlpackages/opencode/test/altimate/fixtures/optimizer-project/models/marts/rpt_eu.sqlpackages/opencode/test/altimate/fixtures/optimizer-project/models/marts/rpt_us.sqlpackages/opencode/test/altimate/fixtures/optimizer-project/models/schema.ymlpackages/opencode/test/altimate/fixtures/optimizer-project/models/staging/raw_customers.sqlpackages/opencode/test/altimate/fixtures/optimizer-project/models/staging/raw_events.sqlpackages/opencode/test/altimate/fixtures/optimizer-project/models/staging/stg_events.sqlpackages/opencode/test/altimate/fixtures/optimizer-project/profiles.ymlpackages/opencode/test/altimate/optimizer-agent-eval.test.tspackages/opencode/test/altimate/optimizer-prompt-contract.test.tspackages/opencode/test/altimate/tools/sql-diff.test.tspackages/opencode/test/altimate/tools/sql-explain.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19faea5144
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (7 files)
Incremental review of commit Fix these issues in Kilo Cloud Previous Review Summaries (10 snapshots, latest commit 03de86f)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 03de86f)Status: No Issues Found | Recommendation: Merge Incremental review of commit Files Reviewed (2 files)
Previous review (commit 74f6aae)Status: No Issues Found | Recommendation: Merge Incremental review of commit Files Reviewed (2 files)
Previous review (commit c94ae11)Status: 1 Suggestion Found | Recommendation: Merge (non-blocking) Incremental review of commits Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Previous review (commit ce69169)Status: No Issues Found | Recommendation: Merge Incremental review of commit Verified sound:
Files Reviewed (8 files)
Previous review (commit 9e95a1a)Status: No Issues Found | Recommendation: Merge Incremental review of commit Verified sound:
Files Reviewed (11 files)
Previous review (commit 6fac6c2)Status: No Issues Found | Recommendation: Merge Incremental review of commit The previous SUGGESTION (duplicated inline Verified sound:
Files Reviewed (15 files)
Previous review (commit 03f314a)Status: 1 Issue Found | Recommendation: Merge (1 optional cleanup) Overview
Incremental review (since The security-critical changes were verified sound:
Issue Details (click to expand)SUGGESTION
Files Reviewed (19 code files)
Previous review (commit 3298014)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files — incremental doc-only diff)
Clean documentation-only diff. Agent names, mode counts, and security-relevant permission claims (non-overridable Previous review (commit 0c3c316)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (7 code files + tests/fixtures/docs)
Fix these issues in Kilo Cloud Previous review (commit 19faea5)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (7 code files + tests/fixtures/docs)
Reviewed by glm-5.3 · Input: 69.2K · Output: 18.9K · Cached: 758.5K Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 30 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- `sql_explain` analyze-safety guard hardened: string literals are masked BEFORE comments (comments-first was bypassable via quoted `/*`..`*/` markers smuggling DML), multi-statement payloads rejected (non-trailing `;`), and dollar-quoted strings fail closed; adversarial tests added - rewrite verify gate tightened from `decidable !== false` to `decidable === true` — the field is required since altimate-core@0.5.1, so a missing value is a malformed response and fails closed - `sql_diff` now forwards `schema_context`/`dialect` to the native handler (equivalence never ran before — no schema ever reached it) and reports "not assessed" instead of "not proven" when no schema is supplied - warehouse-write boundary wording aligned: the agent description and prompt now state that the direct SQL write tool is denied non-overridably while dbt builds run only as user-approved shell commands against a dev target; a new test documents the boundary under permissive global + per-agent bash overrides (builds allowed, DDL and sql_execute_write still denied) - live eval: tmpdir cleanup via `await using tmpdir()` on all exit paths; nonzero CLI exit now fails the eval - docs: https links, one-line reference labels (MD039), Databricks liquid-clustering citations [14]->[21], and a status note on the catalog research doc recording the security review's four design requirements for the future SaaS distribution (not built in this PR) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
62dee14 to
0c3c316
Compare
full receipts (1 session)
orchestrator ·
|
|
Review comments addressed in 0c3c316 — disposition of each finding: Fixed
Skipped (with reasons)
🤖 Generated with Claude Code |
…n user docs - `data-engineering/agent-modes.md`: mode table 3 -> 5 (adds Reviewer with a pointer to the dbt PR Review page, and dbt-Optimizer); new dbt-Optimizer section covering the 4-phase loop, the 6 detection lanes, an example scan transcript, the permission model (edits/bash prompt; `sql_execute_write` denied non-overridably; dev-target builds only), and the builder handoff - `configure/agents.md`: built-in agent table + short Reviewer/dbt-Optimizer subsections linking to the canonical pages - `usage/tui.md`: data-engineering agent list updated to builder/analyst/reviewer/dbt-optimizer/plan - `llms.txt`: replace the stale "7 specialized agents (Builder, Analyst, Validator, Migrator, Researcher, Trainer, Executive)" line — those agents do not exist — with the real five and one-line access summaries Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/altimate/tools/sql-diff.ts`:
- Around line 46-52: Update the native handler’s schema-resolution result to
return equivalence_assessed and propagate the core result’s decidable field. In
the equivalenceLine logic, report “not assessed” when assessment did not run,
and only emit equivalent status and confidence metadata when both
result.equivalent === true and result.decidable === true; otherwise preserve
“not proven.”
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f3a339c-9a4d-4b4b-b4c6-d40e40fc812a
📒 Files selected for processing (13)
docs/internal/2026-08-12-agent-catalog-scaling-research.mddocs/internal/2026-08-12-dbt-optimization-taxonomy-research.mdpackages/opencode/src/agent/agent.tspackages/opencode/src/altimate/native/types.tspackages/opencode/src/altimate/prompts/dbt-optimizer.txtpackages/opencode/src/altimate/tools/altimate-core-rewrite.tspackages/opencode/src/altimate/tools/sql-diff.tspackages/opencode/src/altimate/tools/sql-explain.tspackages/opencode/test/agent/agent.test.tspackages/opencode/test/altimate/altimate-core-rewrite-verify.test.tspackages/opencode/test/altimate/optimizer-agent-eval.test.tspackages/opencode/test/altimate/tools/sql-diff.test.tspackages/opencode/test/altimate/tools/sql-explain.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/opencode/test/altimate/tools/sql-diff.test.ts
- packages/opencode/src/altimate/tools/altimate-core-rewrite.ts
- packages/opencode/test/altimate/tools/sql-explain.test.ts
- packages/opencode/src/altimate/tools/sql-explain.ts
- packages/opencode/test/agent/agent.test.ts
- packages/opencode/src/agent/agent.ts
- packages/opencode/src/altimate/prompts/dbt-optimizer.txt
- docs/internal/2026-08-12-dbt-optimization-taxonomy-research.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32980141a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/docs/data-engineering/agent-modes.md`:
- Around line 3-10: Resolve the mismatch in the mode count and table: update the
opening description and mode listing in agent-modes.md so they consistently
represent the intended set, either by adding the missing Plan mode with its
permissions and purpose or by changing “five” to “four” if Plan is intentionally
excluded.
- Line 213: Update the transcript example code fence at the affected
documentation block to specify a language, using text or console, so it complies
with Markdownlint MD040.
- Around line 241-244: Update the dbt build documentation in
docs/docs/data-engineering/agent-modes.md:241-244 and
docs/docs/configure/agents.md:38 to avoid claiming builds are restricted to
development targets. Either implement runtime target validation in the
altimate-dbt build flow before unsafeBuildModelImmediately and
unsafeBuildProjectImmediately, or revise both statements to describe approval
gating while recommending development targets.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 471303ed-1938-4fc2-b9e2-d3a5da65f0fa
📒 Files selected for processing (4)
docs/docs/configure/agents.mddocs/docs/data-engineering/agent-modes.mddocs/docs/llms.txtdocs/docs/usage/tui.md
There was a problem hiding this comment.
All reported issues were addressed across 17 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…cidability plumbing, permission hardening)
- `sql_explain` analyze guard rewritten as a single left-to-right LEXER that
masks 'single'/"double"/$tag$ strings and both comment forms in one pass —
regex masking is order-dependent and was bypassable in BOTH orders
(`SELECT '/*'; DELETE ...` beats comments-first; `SELECT /*'*/ ; DELETE ...
/*'*/` beats strings-first). Digit-bearing dollar tags handled; unterminated
constructs fail closed; `nextval`/`setval` blocked as sequence-mutating
functions; dual-use keywords (REPLACE/COPY/CALL/SET) exempt only in the
`kw(` function form so read-only queries are not blocked
- `sql.diff` native handler now returns `equivalence_assessed` (true only when
the schema RESOLVED and the check ran) and `decidable`; the wrapper reports
"not assessed" / UNDECIDABLE / equivalent / not proven correctly and its
metadata never carries `equivalent: true` without decidability. Same
metadata guarantee added to `altimate_core_equivalence`
- dbt reader tools (`dbt_manifest`, `dbt_lineage`, `dbt_unit_test_gen`) now
route out-of-project paths through the `external_directory` permission gate
like `read` does — no silent cross-project manifest/lineage extraction
- dbt-optimizer: `training_save`/`training_remove` moved from allow to ask —
training writes persistent memory outside the `edit` permission, so injected
instructions in scanned SQL must not silently poison future sessions
- equivalence + rewrite tools accept a `dialect` hint (forwarded to the
engine); optimizer prompt instructs passing the warehouse dialect during
verification, and its incremental preconditions are now history-conditional
(cost-blind candidates allowed at reduced confidence without query history)
- live eval: negation guard in signal scoring ("X is NOT an incremental
candidate" no longer scores as a hit)
- docs: analyze-guard claims aligned; agent-modes access-control table covers
all four SQL-capable modes; dev-target wording no longer implies runtime
enforcement; MD040 fence language
- agent description: "prompt for approval by default (explicit user config can
relax them)" — precise about what is enforced vs configurable
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 16 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db65b3e0e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…, analyze write-gate, undecidable semantics) - ALL path-taking analysis tools now gate through external_directory with relative paths resolved against the PROJECT directory (mirrors read.ts), never process.cwd(), and the gated resolved path is exactly what gets read: dbt_manifest / dbt_lineage / dbt_unit_test_gen, altimate_core_parse_dbt (project_dir, directory-kind), and the schema_path of validate / check / grade / rewrite / equivalence / column_lineage / testgen via a shared guardSchemaPath helper - EXPLAIN ANALYZE now requires the sql_execute_write permission: text-level lexing cannot prove a SELECT side-effect-free (dblink_exec, UDFs), so analyze:true asks for write approval — write-denied agents (analyst, reviewer, dbt-optimizer) cannot run it at all; the optimizer prompt now says estimated plans are its only plan evidence - sql_diff: decidable !== true now reads UNDECIDABLE regardless of the `equivalent` value — engine abstention (parse/plan failure) is not a refutation and must not render as "not proven" - dbt-optimizer: schema_index moved to "ask" — it crawls the warehouse and rewrites the persistent global schema cache, which the read-only scan must not do unprompted - live eval: subprocess runs with an isolated HOME/XDG and a minimal env (model API keys only) so it can never load the developer's real warehouse connections or permissive config overrides; negation heuristic is now clause-bounded (comma/sentence breaks the association) with a tighter window - tests: external-dir coverage for all three dbt tools on BOTH branches plus a cwd-independence case; analyze write-gate approve/deny execute tests; undecidable-refutation sql_diff case Not adopted (replied on-thread): training-tool ask is enforced by the generic per-tool permission gate in session/tools.ts (runner calls ctx.ask with the tool name before every execute), so no in-tool check is needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ard (P0) PG `E'\''` (and MySQL backslash escapes) end a string literal where the lexer's ''-pair rule would continue, letting `SELECT E'\''; DELETE FROM t; SELECT ''` mask real DML as string content and execute under EXPLAIN ANALYZE. Any backslash inside a single-quoted literal now fails closed (caller falls back to the estimated plan). Defense-in-depth: even before this fix, the round-3 sql_execute_write gate meant write-denied agents could not reach this path; this closes it for write-capable agents pre-approval too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…review) The sql_execute_write ask for EXPLAIN ANALYZE lacked the `always` array that the permission reply handler iterates for permanent approvals — an "always allow" reply could fail without recording the approval. Now `always: ["*"]`, matching sql_execute's write-path convention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/opencode/test/altimate/tools/dbt-external-dir.test.ts (1)
20-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a denied permission.
makeCtx().askalways resolves, so the suite proves only that the gate asks. Each tool calls the gate inside itstryblock, so a rejectedaskbecomes a generic tool ERROR result instead of a propagated denial. No test pins that behavior, and no test proves the dispatcher is never called after a denial.Add a context whose
askrejects. Assert that the tool returns an error result and that the mock handler did not run.Also applies to: 65-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/altimate/tools/dbt-external-dir.test.ts` around lines 20 - 35, Extend makeCtx and the dbt external-directory tool tests with a context whose ask method rejects to simulate denied permission. Assert each affected tool returns an error result and verify its mocked dispatcher or handler is not called after the denial, covering the gate invocation within the tool try blocks.packages/opencode/src/altimate/tools/dbt-manifest.ts (1)
22-23: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsolidate the repeated external-directory path guard.
Instance.containsPathalready usesFilesystem.containsReal; do not add separatefs.realpathhandling. Consolidate the four duplicated dbt path-resolution andassertExternalDirectoryLegacycalls into a shared helper that acceptskind, while retainingguardSchemaPathfor schema files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/tools/dbt-manifest.ts` around lines 22 - 23, Consolidate the repeated dbt path resolution and assertExternalDirectoryLegacy calls into one shared helper accepting the path and kind, reusing Instance.containsPath/Filesystem.containsReal without adding separate fs.realpath handling. Apply the helper in packages/opencode/src/altimate/tools/dbt-manifest.ts (lines 22-23), packages/opencode/src/altimate/tools/dbt-lineage.ts (lines 23-26), packages/opencode/src/altimate/tools/dbt-unit-test-gen.ts (lines 34-37), packages/opencode/src/altimate/tools/altimate-core-parse-dbt.ts (lines 18-21), packages/opencode/src/altimate/tools/altimate-core-column-lineage.ts (line 21), and packages/opencode/src/altimate/tools/altimate-core-rewrite.ts (line 32); retain guardSchemaPath for schema files.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/altimate/tools/sql-explain.ts`:
- Around line 263-285: The EXPLAIN ANALYZE permission request in the analyze
branch must include the required always patterns. Update the ctx.ask call to
provide always with the same SQL resource pattern as patterns, preserving the
existing permission, metadata, and denial handling.
In `@packages/opencode/test/altimate/optimizer-agent-eval.test.ts`:
- Line 72: Update the NEGATION regular expression’s trailing character class to
include ! and ? as sentence boundaries, so negation only matches within the
current sentence while preserving the existing word and length constraints.
- Around line 80-84: Update the signal-matching loop in the evaluation logic to
inspect every occurrence of each signal in window, rather than only the first
RegExp.exec result. Continue past negated matches and return true when any later
occurrence is non-negated, while preserving the existing preceding-context
check.
In `@packages/opencode/test/altimate/tools/dbt-external-dir.test.ts`:
- Around line 37-58: Add an afterEach teardown in the test file that calls
Dispatcher.reset() after every test, ensuring the handlers registered by
mockHandlers are removed while preserving the existing beforeEach setup.
---
Nitpick comments:
In `@packages/opencode/src/altimate/tools/dbt-manifest.ts`:
- Around line 22-23: Consolidate the repeated dbt path resolution and
assertExternalDirectoryLegacy calls into one shared helper accepting the path
and kind, reusing Instance.containsPath/Filesystem.containsReal without adding
separate fs.realpath handling. Apply the helper in
packages/opencode/src/altimate/tools/dbt-manifest.ts (lines 22-23),
packages/opencode/src/altimate/tools/dbt-lineage.ts (lines 23-26),
packages/opencode/src/altimate/tools/dbt-unit-test-gen.ts (lines 34-37),
packages/opencode/src/altimate/tools/altimate-core-parse-dbt.ts (lines 18-21),
packages/opencode/src/altimate/tools/altimate-core-column-lineage.ts (line 21),
and packages/opencode/src/altimate/tools/altimate-core-rewrite.ts (line 32);
retain guardSchemaPath for schema files.
In `@packages/opencode/test/altimate/tools/dbt-external-dir.test.ts`:
- Around line 20-35: Extend makeCtx and the dbt external-directory tool tests
with a context whose ask method rejects to simulate denied permission. Assert
each affected tool returns an error result and verify its mocked dispatcher or
handler is not called after the denial, covering the gate invocation within the
tool try blocks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 467cc3f7-75f1-4dc6-9f40-b700d54c9f81
📒 Files selected for processing (23)
docs/docs/data-engineering/agent-modes.mdpackages/opencode/src/agent/agent.tspackages/opencode/src/altimate/native/sql/register.tspackages/opencode/src/altimate/native/types.tspackages/opencode/src/altimate/prompts/dbt-optimizer.txtpackages/opencode/src/altimate/tools/altimate-core-check.tspackages/opencode/src/altimate/tools/altimate-core-column-lineage.tspackages/opencode/src/altimate/tools/altimate-core-equivalence.tspackages/opencode/src/altimate/tools/altimate-core-grade.tspackages/opencode/src/altimate/tools/altimate-core-parse-dbt.tspackages/opencode/src/altimate/tools/altimate-core-rewrite.tspackages/opencode/src/altimate/tools/altimate-core-testgen.tspackages/opencode/src/altimate/tools/altimate-core-validate.tspackages/opencode/src/altimate/tools/dbt-lineage.tspackages/opencode/src/altimate/tools/dbt-manifest.tspackages/opencode/src/altimate/tools/dbt-unit-test-gen.tspackages/opencode/src/altimate/tools/schema-path-guard.tspackages/opencode/src/altimate/tools/sql-diff.tspackages/opencode/src/altimate/tools/sql-explain.tspackages/opencode/test/altimate/optimizer-agent-eval.test.tspackages/opencode/test/altimate/tools/dbt-external-dir.test.tspackages/opencode/test/altimate/tools/sql-diff.test.tspackages/opencode/test/altimate/tools/sql-explain.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/opencode/test/altimate/tools/sql-explain.test.ts
- docs/docs/data-engineering/agent-modes.md
- packages/opencode/src/altimate/prompts/dbt-optimizer.txt
- packages/opencode/src/agent/agent.ts
- packages/opencode/src/altimate/tools/sql-diff.ts
- packages/opencode/src/altimate/tools/altimate-core-equivalence.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e7bbd20e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fac6c2cda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…on.disabled, masked classifier, CI fix
- registry exposure filter rewritten onto Permission.disabled — the previous
hand-rolled check was doubly wrong: defaults' leading "*": "allow" made
matching.some() always true (filter never dropped anything), and the missing
edit/write/apply_patch->edit remap would have dropped the write tool from
the optimizer entirely. Permission.disabled is last-match-wins with the
house remap; regression tests cover optimizer (denied mutators out, write
kept via edit remap) and analyst (pattern-scoped bash kept, write out)
- sql-classify side-effect detection now runs against lexer-MASKED SQL
(shared sql-text-mask module, extracted from sql-explain): a block comment
between function name and paren no longer bypasses (dblink_exec\/**\/(..)),
names in string literals\/comments no longer false-positive, unlexable SQL
fails closed as write
- parse-dbt uses guardExternalFile — fixes the TypeScript CI failure ("No
context found for instance" in the context-free error-propagation tests)
- training_save\/training_remove\/schema_index asks moved OUTSIDE try\/catch so
RejectedError\/CorrectedError\/DeniedError propagate with framework
blocked-call semantics (mirrors sql-execute)
- eval env: AWS_* deliberately dropped (doubles as warehouse credentials,
contradicting subprocess isolation)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
3 issues found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/tools/altimate-core-parse-dbt.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/altimate-core-parse-dbt.ts:17">
P2: When `project_dir` is empty, `guardExternalFile` returns `""` before resolving it, and the nullish fallback preserves that value. Pass `"."` for empty input or reject empty strings so the parser does not receive an invalid project path.</violation>
</file>
<file name="packages/opencode/src/altimate/tools/sql-text-mask.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/sql-text-mask.ts:50">
P1: When a SQL payload uses CR-only line endings, this branch treats the line comment as extending to EOF and hides later statements. Search for both `\r` and `\n` so side-effect detection and `EXPLAIN ANALYZE` safety cannot be bypassed.</violation>
<violation number="2" location="packages/opencode/src/altimate/tools/sql-text-mask.ts:60">
P2: PostgreSQL allows `$` inside unquoted identifiers, so `foo$bar$` is not necessarily a dollar-quote opener. Require a dollar-quote delimiter to start at a token boundary; otherwise valid reads are forced through write permission or rejected by analyze mode.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e95a1a91f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ique_id impact traversal, eval model pinning - sql-text-mask: line comments end at \r OR \n (CR-only payloads no longer extend a comment over later statements); dollar-quote openers require a token boundary (PostgreSQL identifiers may contain $) - sql-classify: CR\/CRLF normalized to LF before BOTH the AST engine and the fallback (the engine treats -- comments as LF-terminated too); fallback now masks via the shared lexer instead of ordered regexes (closes the napi-unavailable literal-comment bypass); lo_import\/lo_export\/lo_unlink\/ pg_reload_conf\/pg_rotate_logfile added to the side-effect list - impact_analysis findDownstream traverses by dbt unique_id (name-suffix only as fallback for id-less fixtures) — package-qualified models sharing a name are no longer conflated; entry model is pre-visited so a model is never its own downstream - parse-dbt: empty project_dir normalizes to "." before the guard - live eval pins the model (--model, OPTIMIZER_EVAL_MODEL override) so results measure a known model, not ambient credential enumeration order - docs: agent-modes SQL table no longer advertises direct SQL execution for the reviewer (it has no sql_execute\/sql_explain tools) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce69169d76
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…ths, LCS sql.diff - registry exposure filter exempts the internal `invalid` tool — session\/llm repairs unknown\/malformed tool calls by renaming them to it, so filtering it under deny-by-default agents turned repaired calls into turn-ending failures - impact_analysis: findDownstream is now a true multi-seed BFS (depth = the SHORTEST distance from any seed; DFS with shared visited could label direct dependents transitive depending on manifest order); results carry unique_id, and affected-test accounting uses those ids instead of a name-keyed map that dropped same-named package branches; column-level impact now states its best-effort limitation and points at dbt_lineage - sql.diff: LCS-based line diff (one top-of-file insertion is ONE changed line, not the whole file) with an O(n*m) cap falling back to index alignment; equivalence checking isolated in its own try\/catch so an engine failure downgrades to "not assessed" instead of erasing the text diff Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…1092 review) `SELECT "lo_import"(...)` is a valid PostgreSQL invocation, but the masker replaces double-quoted content with "" — hiding the name from the side-effect scan. A quoted-form regex on the raw SQL now escalates these to write; a match inside a string literal only costs a prompt (safe direction). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c94ae11530
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- OPTIMIZER_EVAL_AUTH: inject a model-provider auth.json into the isolated HOME (model plane only; warehouse isolation unchanged) - OPTIMIZER_EVAL_TRANSCRIPT: dump the full transcript for analysis - provider env pass-through corrected from real-run failures: google needs GEMINI_API_KEY/GOOGLE_GENERATIVE_AI_API_KEY (not GOOGLE_API_KEY); vertex detection reads GOOGLE_CLOUD_PROJECT/GOOGLE_CLOUD_LOCATION (NOT the GOOGLE_VERTEX_* names in the models.dev env list) + ADC path - spawnSync killSignal SIGKILL (SIGTERM left an errored CLI dangling ~2h) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Not reviewed (too large): packages/opencode/src/provider/models-snapshot.ts (~2 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74f6aae907
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The agent-catalog-scaling and dbt-optimization-taxonomy research reports are internal working documents, not customer-facing — removed from the repo; the canonical copies live in the internal knowledge base. The customer docs (docs/docs/*) are unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ntifiers, eval ADC isolation, diff context
- sql-text-mask: SQL Server [bracket] and MySQL `backtick` identifier states
(doubled-delimiter escapes; unterminated fails closed) — bracket content
like [--] can no longer open a comment state and swallow a following DELETE;
new preserveQuotedIdentifiers option keeps delimited-identifier CONTENT
- sql-classify: quoted side-effect calls detected on the identifier-preserving
mask, so a comment wedged between quote and paren ("lo_import"\/**\/(..))
collapses and the delimited name still matches; fallback classifier rejects
write keywords inside read-shaped WITH\/EXPLAIN\/SELECT statements (WITH x AS
(SELECT 1) DELETE .., EXPLAIN ANALYZE DELETE ..) with the function-form
exemption for REPLACE\/COPY\/CALL\/SET
- registry: the `invalid` exemption applies only to the builtin definition
(registrySource !== "external") — a plugin tool named `invalid` cannot ride
the exemption past a deny-by-default agent
- sql.diff: honors context_lines with real hunks (" " context, "..." gaps);
oversized fallback output is explicitly marked approximate
- impact_analysis: target matching computed once (not-found check and
affected-id set derive from the same list)
- live eval: GOOGLE_APPLICATION_CREDENTIALS\/GOOGLE_CLOUD_PROJECT removed from
the pass-through — ADC is a BigQuery WAREHOUSE credential per the repo's own
env detection, so forwarding it defeated the isolation the env block exists
to provide
- prompt: dead-model lane must read raw manifest exposures (dbt_manifest\/
impact_analysis do not surface them) before declaring a model dead
- docs: SHOW\/DESCRIBE\/EXPLAIN documented as ambiguous-classified (denied for
analyst\/optimizer; schema_inspect\/sql_explain are the read paths)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 225cb4d507
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…igured denies, guard rejections propagate, eval same-item scoring - permission (BOTH ask paths, next.ts + index.ts): a persisted "always" approval is consulted only when the configured ruleset says ask — never to flip a deny. Previously approvals were appended after the ruleset under last-match-wins, so a write approval granted in a builder session silently defeated the dbt-optimizer's non-overridable sql_execute_write deny after a session switch. Regression test pins deny-wins-over-persisted-approval - all path-guarded tools (dbt readers, impact_analysis, parse-dbt, the schema_path core wrappers, sql_analyze) rethrow permission-lifecycle errors (Rejected\/Corrected\/Denied) from their catch blocks via a shared isPermissionError helper — a user's rejection now reaches the session processor instead of degrading into a retryable error result - live eval scores model+signal within the SAME candidate item (numbered-list segmentation with paragraph fallback) — a ±400-char window could credit a claim made about the neighboring candidate Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 18 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03de86fe30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ermission ordering, diff hardening
- sql-text-mask: bracket spans are consumed as a unit (comment-state safety)
but their CONTENT is always preserved in the output — blanking hid
side-effecting Postgres array-subscript expressions (arr[nextval(..)]) from
both the classifier and the analyze guard; preserved text is never re-lexed
so it can only ADD scan-visible material (fail-safe)
- session permissions (legacy tools:{} request input) merge BEFORE the agent
ruleset at both ask sites — a request carrying tools:{sql_execute_write:
true} could previously outrank the optimizer's non-overridable deny under
last-match-wins; agent config is now authoritative over session grants
- sql.diff: context_lines floored to a nonnegative integer (fractional values
corrupted the keep-marking); difference-array marking keeps hunk building
O(ops) for any context size; approximate path notes context unavailability
- sql-classify fallback: statement-form write check anchored to statement
start (SELECT set FROM t no longer escalates)
- eval segmentation splits only on numbered candidates\/headings — bullet
splitting tore a candidate's model line from its evidence fields
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 81ec904. Configure here.
| // `tools: { sql_execute_write: true }` would flip dbt-optimizer's | ||
| // non-overridable deny. Agent config is authoritative; session rules | ||
| // may only fill gaps. | ||
| ruleset: Permission.merge(input.session.permission ?? [], input.agent.permission), |
There was a problem hiding this comment.
Session deny ceiling broken
High Severity
Swapping merge order to merge(session.permission, agent.permission) makes agent rules win last-match. That blocks a session allow from overriding an agent deny, but it also lets agent allow override session deny. Subagent sessions rely on parent session denies from deriveSubagentSessionPermission as hard ceilings; those ceilings no longer hold at runtime, contrary to plan-mode-subagent-bypass expectations.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 81ec904. Configure here.
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/session/prompt.ts">
<violation number="1" location="packages/opencode/src/session/prompt.ts:1720">
P1: When a session or inherited parent carries an explicit deny, this merge lets the selected agent re-enable that tool. Keep session denies as a runtime ceiling while still applying agent non-overridable denies after session grants.</violation>
</file>
<file name="packages/opencode/src/altimate/tools/sql-classify.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/sql-classify.ts:59">
P3: The new anchored WRITE_STATEMENT_FORM regex cannot change any classification. Statements are trimmed before the loop, and any statement starting with replace/copy/call/set already fails READ_PATTERN, so the `!READ_PATTERN.test(stmt)` disjunct marks it `write` regardless. The `\s*` anchor also duplicates the trim. Remove the regex and its `WRITE_STATEMENT_FORM.test(stmt)` disjunct; the guarded comment about `SELECT set FROM t` can live on the READ_PATTERN/loop check where the behavior is actually enforced.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ruleset: PermissionNext.merge(input.agent.permission, input.session.permission ?? []), | ||
| // altimate_change start — session rules must not outrank agent config | ||
| // (see session/tools.ts): merged FIRST so agent denies stay final. | ||
| ruleset: PermissionNext.merge(input.session.permission ?? [], input.agent.permission), |
There was a problem hiding this comment.
P1: When a session or inherited parent carries an explicit deny, this merge lets the selected agent re-enable that tool. Keep session denies as a runtime ceiling while still applying agent non-overridable denies after session grants.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/session/prompt.ts, line 1720:
<comment>When a session or inherited parent carries an explicit deny, this merge lets the selected agent re-enable that tool. Keep session denies as a runtime ceiling while still applying agent non-overridable denies after session grants.</comment>
<file context>
@@ -1715,7 +1715,10 @@ export namespace SessionPrompt {
- ruleset: PermissionNext.merge(input.agent.permission, input.session.permission ?? []),
+ // altimate_change start — session rules must not outrank agent config
+ // (see session/tools.ts): merged FIRST so agent denies stay final.
+ ruleset: PermissionNext.merge(input.session.permission ?? [], input.agent.permission),
+ // altimate_change end
} as Parameters<typeof PermissionNext.ask>[0])
</file context>
| // replace/copy/call/set double as read-only functions and column names; only | ||
| // the STATEMENT form (keyword at statement start) counts as a write — | ||
| // scanning the whole statement would escalate `SELECT set FROM t`. | ||
| const WRITE_STATEMENT_FORM = /^\s*(replace|copy|call|set)\b/i |
There was a problem hiding this comment.
P3: The new anchored WRITE_STATEMENT_FORM regex cannot change any classification. Statements are trimmed before the loop, and any statement starting with replace/copy/call/set already fails READ_PATTERN, so the !READ_PATTERN.test(stmt) disjunct marks it write regardless. The \s* anchor also duplicates the trim. Remove the regex and its WRITE_STATEMENT_FORM.test(stmt) disjunct; the guarded comment about SELECT set FROM t can live on the READ_PATTERN/loop check where the behavior is actually enforced.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/tools/sql-classify.ts, line 59:
<comment>The new anchored WRITE_STATEMENT_FORM regex cannot change any classification. Statements are trimmed before the loop, and any statement starting with replace/copy/call/set already fails READ_PATTERN, so the `!READ_PATTERN.test(stmt)` disjunct marks it `write` regardless. The `\s*` anchor also duplicates the trim. Remove the regex and its `WRITE_STATEMENT_FORM.test(stmt)` disjunct; the guarded comment about `SELECT set FROM t` can live on the READ_PATTERN/loop check where the behavior is actually enforced.</comment>
<file context>
@@ -53,9 +53,10 @@ function classifyFallback(sql: string): { queryType: "read" | "write"; blocked:
+ // replace/copy/call/set double as read-only functions and column names; only
+ // the STATEMENT form (keyword at statement start) counts as a write —
+ // scanning the whole statement would escalate `SELECT set FROM t`.
+ const WRITE_STATEMENT_FORM = /^\s*(replace|copy|call|set)\b/i
let queryType: "read" | "write" = "read"
let blocked = false
</file context>
| // triggers a prompt). | ||
| const start = i | ||
| let j = i + 1 | ||
| for (;;) { |
There was a problem hiding this comment.
SUGGESTION: The bracket-scanning loop duplicates consumeDelimited verbatim
Lines 82-91 re-implement the exact scanning logic of consumeDelimited (lines 38-49) — same ]] escape, same -1/null on unterminated — differing only in unconditionally preserving content. Extend consumeDelimited with a forcePreserve boolean (e.g. consumeDelimited("[", "]", true)) so the scanner exists once; the bracket-specific rationale comment can stay at the call site.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| else if (sql[j] === "]") break | ||
| else j++ | ||
| } | ||
| out += sql.slice(start, j + 1) |
There was a problem hiding this comment.
SUGGESTION: File header no longer matches the new always-preserve bracket behavior
The doc header (lines 18-22) still says preserveQuotedIdentifiers keeps the content of "double", [bracket], and backtick identifiers and that "the default blanks them". After this change, bracket content is preserved regardless of the option, so the header is now inaccurate for [...]. A caller relying on the documented default-blanking for bracket identifiers would be misled — update the header to state brackets are always preserved (with the subscript-visibility rationale) while " and ` still honor the option.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81ec904d23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ruleset: PermissionNext.merge(input.agent.permission, input.session.permission ?? []), | ||
| // altimate_change start — session rules must not outrank agent config | ||
| // (see session/tools.ts): merged FIRST so agent denies stay final. | ||
| ruleset: PermissionNext.merge(input.session.permission ?? [], input.agent.permission), |
There was a problem hiding this comment.
When a prompt request uses the backward-compatible tools: { read: false } or disables another capability, that setting becomes a session-level deny, but merging it before the agent rules makes the agent's later wildcard or tool-specific allow win under last-match-wins evaluation. Since registry exposure also considers only agent permissions, the disabled tool remains available and can execute silently—for example, every native agent starts with "*": "allow", so read: false is ineffective. The new merge-order fix therefore closes the optimizer-write override by breaking all request-level tool restrictions; preserve session denials while enforcing the optimizer's immutable SQL-write deny separately. The same reversed merge appears in session/tools.ts.
Useful? React with 👍 / 👎.
1 participant
Issue for this PR
Closes #1091
Type of change
What does this PR do?
Adds
dbt-optimizer, a 5th native primary agent that scans a dbt project for fixable issues and proposes targeted fixes with cost/impact reporting (the agent behind the planned Optimize workflow). Four parts:1. Agent registration (
agent.ts). Deny-by-default permission allowlist: read/analysis/finops tools allowed,editandbashprompt per action,sql_execute_writedenied. The deny is re-applied after the global user-config merge AND after the per-agent config merge — permission evaluation is last-match-wins, so without the second re-applicationagent."dbt-optimizer".permission.sql_execute_write: "allow"would have silently won (regression tests cover both paths). Nameddbt-optimizerrather thanoptimizerbecause more optimizer agents are planned; no alias shim needed since nothing shipped under the old name.2. Prompt (
prompts/dbt-optimizer.txt). Encodes the taxonomy fromdocs/internal/2026-08-12-dbt-optimization-taxonomy-research.md: 6 detection lanes, an evidence-attribution ladder (invocation-ID/query-tag down to lineage-match, with confidence labeling), ROI-ranked triage, cost-honesty rules ("not estimable" is a valid answer; never invent dollar figures), a 4-phase loop that stops after scan for candidate selection, and an auto-fix vs propose-only boundary. Builder's self-review gains a dbt-scoped "Optimization handoff" nudge — primary agents are excluded from the task tool (task.tsfiltersmode !== "primary"), so the nudge is the only build-time bridge and auto-delegation of cost-incurring scans is deliberately NOT wired.3. Verification-tool fixes. Three latent bugs in existing tools the agent's core promises depend on, found during review: the rewrite verify gate trusted
equivalent: trueeven when the engine saiddecidable: false(now UNDECIDABLE = unproven, everywhere it surfaces);sql_explain analyze:trueexecutes the statement on Postgres/MySQL/DuckDB/Trino and had no statement-class guard (now blocked for anything non-read-only, includingSELECT ... INTO); thesql_diffwrapper read response fields the native handler never returns, so every comparison reported "identical".4. Evals. Tier 1 (CI): 20 deterministic tests asserting the prompt's non-negotiable invariants (whitespace-normalized so reflow doesn't break them) plus an evidence-chain suite proving each planted fixture issue is genuinely detectable. Tier 2 (opt-in): a live eval that runs the compiled binary against a 7-model DuckDB fixture with 6 planted issues (incremental candidate, dead model, SELECT * propagation, ORDER BY, verbatim-duplicated CTE ×3, untested model) and grades deterministically — ≥4/6 recall with directional signal phrases, exit-code check, and a tree-snapshot proving the scan modified/removed/added nothing. Answer key lives outside the scanned directory.
How did you verify your code works?
tsgo --noEmitclean; upstream marker check clean (--markers --base main --strict); oxlint 0 errors on changed files.Screenshots / recordings
Not a UI change — the agent appears in the existing Tab ring/agent list.
Checklist
🤖 Generated with Claude Code
Summary by cubic
Adds the
dbt-optimizeragent for six‑lane dbt project scans with evidence‑backed fixes, and strengthens SQL, path, and permission safety. Previously, EXPLAIN ANALYZE and equivalence gates could execute or misclassify writes, built‑ins could bypass deny‑by‑default exposure, session/persisted approvals could override denies, and path‑taking tools could read outside the project; now analyze requires write approval with a safe fallback, UNDECIDABLE never counts as equivalent, denied tools are not exposed, approvals cannot flip a deny, and out‑of‑project reads are gated.Agent and permissions: registers
dbt-optimizerwith deny‑by‑default;edit/bashask;sql_execute_writeis non‑overridable and re‑applied after config merges. Built‑in tool exposure honors denies viaPermission.disabled(internalinvalidexempt). Persisted “always allow” approvals and session‑supplied rules can no longer override configured denies (session rules merge before agent at ask time).SQL/paths/analysis:
sql_explainanalyze mode requires write approval and uses a single‑pass masker for literals/comments/delimited identifiers (incl. bracket/dollar quoting and subscript cases); unsafe SQL falls back to estimated plans.sql_classifyshares the masker, normalizes CR/CRLF, detects quoted side‑effect calls (including identifier‑quoted), and rejects write keywords hidden inside read‑shaped WITH/EXPLAIN/SELECT; statement‑form checks anchor at start.sql_diffuses LCS line diffs withcontext_linesand robust hunking, forwardsschema_context/dialect, and reportsequivalence_assessed/decidable; undecidable results never count as proven equivalent. All dbt readers andschema_pathwrappers gate throughexternal_directory, resolve paths relative to the project, read the same resolved path, and propagate permission rejections. Impact analysis is a true multi‑seed BFS by dbtunique_id(shortest depths, no self‑loops); affected tests counted byunique_id. Prompt encodes six lanes and build safety with a builder handoff nudge; deterministic prompt/evidence tests ship, plus an opt‑in DuckDB live eval with same‑candidate scoring. Docs surfacereviewer/dbt-optimizeragents.Written for commit 81ec904. Summary will update on new commits.
Summary by CodeRabbit
New Features
dbt-optimizeragent for evidence-based analysis and approval-gated fixes.Bug Fixes
Documentation
Note
High Risk
Changes touch permission evaluation, warehouse SQL execution paths (EXPLAIN ANALYZE and write classification), and a new agent that can propose edits and approved shell/dbt builds—mistakes could allow unintended writes or bypass user denials.
Overview
Introduces the
dbt-optimizerprimary agent: a deny-by-default allowlist for scan/analysis/finops tools, approval-gatededit/bash, and a non-overridablesql_execute_writedeny (re-applied after global and per-agent config merges). A new prompt defines a four-phase loop (scan → fix → impact → PR) across six optimization lanes with cost-honesty rules; docs andbuilderself-review now nudge users to switch agents instead of silent out-of-scope fixes.SQL and verification behavior is tightened: a lexer-based
maskLiteralsAndCommentsbacks safer write classification (side-effect functions, read-shaped writes) andsql_explainblocks or permission-gatesanalyze:trueso executing plans cannot bypass warehouse-write controls.sql_diff/ equivalence tooling now match the native handler (LCS diffs,equivalence_assessed/decidable) and treat undecidable results as unproven across rewrite/equivalence surfaces.Path and session safety: dbt manifest/lineage/parse tools and
schema_patharguments go throughexternal_directorygating with project-relative resolution;schema_indexand training mutations prompt explicitly. Permission evaluation no longer lets stored “always allow” approvals override configured deny rules (protecting optimizer/reviewer write denials after agent switches).impact_analysisdownstream traversal is improved (multi-seed BFS byunique_id, clearer column-impact caveats).Reviewed by Cursor Bugbot for commit 81ec904. Bugbot is set up for automated code reviews on this repo. Configure here.