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 23 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: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (11 files)
Fix these issues in Kilo Cloud Previous Review Summaries (13 snapshots, latest commit 81ec904)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 81ec904)This review did not run. Your provider API key hit its rate limit, so the Previous review (commit 81ec904)This review did not run. Your provider API key hit its rate limit, so the Previous review (commit 81ec904)Status: 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 (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
[Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by deepseek-v4-pro · Input: 89.4K · Output: 53.8K · Cached: 2.5M 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".
- 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.
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
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".
# Conflicts: # docs/docs/llms.txt # packages/opencode/src/altimate/tools/altimate-core-check.ts
💡 Codex Review Preserve specific allows when reapplying agent denies
Reappending every agent deny also appends deny-by-default agents' altimate-code/packages/opencode/src/session/tools.ts Lines 81 to 85 in e0d20a1 Enforce session tool denials before registry execution
Fresh evidence after the earlier request-denial finding is that the new merge only affects calls a handler makes through https://github.com/AltimateAI/altimate-code/blob/e0d20a1acd215c867bcc65b2c9972cba8a970de6/packages/opencode/src/provider/models-snapshot.ts#L2 This regenerated snapshot removes the entire  Skip write approval for plan-only ANALYZE backends
When ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…op redundant analyze statement-form check - impact_analysis targetMatches now also matches unique_id, so the unique_id entry path findDownstream already supports is reachable through the tool (package-collision scoping); previously a unique_id returned MODEL NOT FOUND - sql_explain analyze guard: removed the statementFormOnly check — a single statement (multi-statement already rejected) beginning with replace/copy/ call/set fails readOnlyStart, and one starting with a read keyword cannot also be their statement form, so !readOnlyStart already covers them. The whole-body scan wrongly blocked reads projecting a column named `set` - regression tests for both 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: 6442366608
ℹ️ 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 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…st, align edit-tool remaps, real unique_id tool test The round-12 deny-wins-both-ways fix had a critical flaw the reviewers caught: re-applying ALL agent deny rules after the session ruleset re-appended the deny-by-default catch-all (`"*": "deny"`), which then won last-match over the agent's own read/grep/... allows — the optimizer (and analyst/reviewer) scan could not read the project at runtime. - both ask sites now re-apply only PERMISSION-SPECIFIC agent denies (r.permission !== "*"): sql_execute_write and DDL bash patterns stay immutable over session grants, but the catch-all is not moved past the allowlist. Merge-level regression test asserts grep stays allow, a session read:deny holds as a ceiling, and sql_execute_write/DDL denies win over a session allow - PermissionNext.disabled EDIT_TOOLS now includes apply_patch (+ patch, multiedit) matching Permission.disabled, so GPT-backed optimizer sessions keep a file-editing tool through the fix phase instead of losing apply_patch to the wildcard deny; both lists cross-reference each other - impact_analysis: replaced the duplicate findDownstream unique_id test with a TOOL-level test that invokes ImpactAnalysisTool with a unique_id arg against a two-package mock manifest and asserts it is not MODEL NOT FOUND (guards the targetMatches fix a findDownstream-only test could not) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 6 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: bbd95b4560
ℹ️ 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".
… equivalence abstention, marker fix - session deny-reapply now re-applies only the agent's EFFECTIVE denies (Permission.evaluate against the agent ruleset === deny), so a DEFAULT deny the agent later overrode with its own allow (e.g. `question: allow`) is not resurrected past that allow at runtime. Fixes the optimizer's question permission being denied. Both ask sites; merge-regression test now asserts grep+question+sql_execute_write (the assertions that actually exercise the mechanism) - schema-path-guard canonicalizes symlinks (realpathSync, nearest-existing- ancestor fallback) BEFORE the external_directory gate and returns the real path, so an in-project symlink pointing at a sibling/private file is gated by its true location and the read hits the same target - altimate_core_equivalence: an engine abstention (decidable:false) that also carries validation_errors now yields UNDECIDABLE, not ERROR — only a transport/dispatch error (result.error) is an operational failure - live-eval snapshotTree: prepend `dir` only when Dirent parent is relative (absolute parentPath would duplicate the path) - revert an unnecessary index.ts edits[] change (patch/multiedit) to the upstream form so Marker Guard passes; the round-10 stored-approval fix in index.ts is preserved, and apply_patch was already mapped there upstream 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.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 54d2df6. Configure here.
| // diagnostics accompanying decidable:false must yield UNDECIDABLE so the | ||
| // optimizer can tell "engine could not decide" from "tool broke". | ||
| const undecidableAbstention = data.decidable === false | ||
| const isRealFailure = !!result.error || (!!error && !undecidableAbstention) |
There was a problem hiding this comment.
Equivalence abstention swallows data errors
Medium Severity
The new abstention gate treats any non-result.error diagnostic as non-failure whenever decidable is false, but error also folds in data.error. An engine payload with both decidable: false and data.error is marked success/UNDECIDABLE while formatEquivalence still short-circuits on data.error and prints an error line, so title and body disagree and a real engine error is reported as a clean abstention.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 54d2df6. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54d2df677d
ℹ️ 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".
| input.agent.permission.filter( | ||
| (r) => | ||
| r.action === "deny" && | ||
| r.permission !== "*" && |
There was a problem hiding this comment.
Fresh evidence beyond the prior permission-wildcard fix is that this still reapplies denials whose permission is specific but whose pattern is "*". For example, Analyst defines bash: { "*": "deny", "ls *": "allow", ... }, and Plan similarly defines edit: { "*": "deny", <plan paths>: "allow" }; evaluating the literal deny pattern keeps these denials in the filtered set, after which appending them last makes every actual ls command or permitted plan-file edit resolve to deny. Exclude broad pattern denials when later agent rules provide narrower allows, or compute the effective agent decision for the actual requested pattern instead; the duplicate merge in session/tools.ts needs the same correction.
Useful? React with 👍 / 👎.
| // diagnostics accompanying decidable:false must yield UNDECIDABLE so the | ||
| // optimizer can tell "engine could not decide" from "tool broke". | ||
| const undecidableAbstention = data.decidable === false | ||
| const isRealFailure = !!result.error || (!!error && !undecidableAbstention) |
There was a problem hiding this comment.
SUGGESTION: UNDECIDABLE results that also carry a top-level data.error still print an "Error" line, contradicting the non-failure verdict.
When result.error is absent and data.error is set alongside decidable === false, this line makes isRealFailure === false, so title becomes "Equivalence: UNDECIDABLE" and metadata.success is true. However output: formatEquivalence(isRealFailure ? { ...data, error } : data) (line 77) passes data unchanged, and formatEquivalence checks data.error first (line 102) and returns "Error: <msg>". The optimizer agent then reads a success/UNDECIDABLE verdict whose output text claims an error — undercutting the goal of telling "engine could not decide" from "tool broke" in the data.error case (the validation_errors path is unaffected since formatEquivalence doesn't read it). Consider skipping the data.error branch in formatEquivalence when decidable === false, or passing { ...data, error: undefined } in the undecidable case.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
5 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/altimate/tools/altimate-core-equivalence.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/altimate-core-equivalence.ts:50">
P2: When the engine returns `data.error` with `decidable:false`, this suppresses a real error and reports success while rendering error output. Keep `data.error` failure-worthy while exempting only validation diagnostics.</violation>
</file>
<file name="packages/opencode/test/agent/agent.test.ts">
<violation number="1" location="packages/opencode/test/agent/agent.test.ts:964">
P2: This test cannot detect a regression in the production deny-reapply logic because it duplicates that logic while constructing `runtime`. If `src/session/tools.ts` reverted to reapplying the overridden `question` deny, this test would still pass; exercise the actual tool/session permission path or extract and test a shared implementation.</violation>
</file>
<file name="packages/opencode/src/session/prompt.ts">
<violation number="1" location="packages/opencode/src/session/prompt.ts:1735">
P2: When an agent has a wildcard deny followed by a narrower allow, this check evaluates the deny against its literal wildcard pattern rather than an actual request. It therefore re-appends that deny after the allow and breaks the permission system's last-match-wins override; determine reapplication from the effective matched rules without overriding narrower allows.</violation>
</file>
<file name="packages/opencode/src/permission/index.ts">
<violation number="1">
P1: When a custom or plugin tool is named `patch` or `multiedit`, an `edit: deny` rule no longer filters it from the registry or legacy request tool set. Keep these aliases mapped to `edit` (and share the alias list with `PermissionNext.disabled`) so edit-deny permissions cannot be bypassed by those tool IDs.</violation>
</file>
<file name="packages/opencode/src/altimate/tools/schema-path-guard.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/schema-path-guard.ts:47">
P2: When a path under an in-project symlink has multiple nonexistent trailing components, this fallback leaves `resolved` lexical because it retries only the immediate parent. The permission metadata and loader path can therefore look project-local while the loader later follows the symlink outside the project; walk to the nearest existing ancestor and append every remaining component.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -91,7 +91,14 @@ export const layer = Layer.effect( | |||
| let needsAsk = false | |||
There was a problem hiding this comment.
P1: When a custom or plugin tool is named patch or multiedit, an edit: deny rule no longer filters it from the registry or legacy request tool set. Keep these aliases mapped to edit (and share the alias list with PermissionNext.disabled) so edit-deny permissions cannot be bypassed by those tool IDs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/permission/index.ts, line 233:
<comment>When a custom or plugin tool is named `patch` or `multiedit`, an `edit: deny` rule no longer filters it from the registry or legacy request tool set. Keep these aliases mapped to `edit` (and share the alias list with `PermissionNext.disabled`) so edit-deny permissions cannot be bypassed by those tool IDs.</comment>
<file context>
@@ -230,8 +230,7 @@ export function merge(...rulesets: PermissionV1.Ruleset[]): PermissionV1.Rule[]
export function disabled(tools: string[], ruleset: PermissionV1.Ruleset): Set<string> {
- // Keep in sync with PermissionNext.disabled (next.ts).
- const edits = ["edit", "write", "apply_patch", "patch", "multiedit"]
+ const edits = ["edit", "write", "apply_patch"]
return new Set(
tools.filter((tool) => {
</file context>
| let needsAsk = false | |
| const edits = ["edit", "write", "apply_patch", "patch", "multiedit"] |
| // diagnostics accompanying decidable:false must yield UNDECIDABLE so the | ||
| // optimizer can tell "engine could not decide" from "tool broke". | ||
| const undecidableAbstention = data.decidable === false | ||
| const isRealFailure = !!result.error || (!!error && !undecidableAbstention) |
There was a problem hiding this comment.
P2: When the engine returns data.error with decidable:false, this suppresses a real error and reports success while rendering error output. Keep data.error failure-worthy while exempting only validation diagnostics.
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/altimate-core-equivalence.ts, line 50:
<comment>When the engine returns `data.error` with `decidable:false`, this suppresses a real error and reports success while rendering error output. Keep `data.error` failure-worthy while exempting only validation diagnostics.</comment>
<file context>
@@ -40,9 +40,14 @@ export const AltimateCoreEquivalenceTool = Tool.define("altimate_core_equivalenc
+ // diagnostics accompanying decidable:false must yield UNDECIDABLE so the
+ // optimizer can tell "engine could not decide" from "tool broke".
+ const undecidableAbstention = data.decidable === false
+ const isRealFailure = !!result.error || (!!error && !undecidableAbstention)
// altimate_change start — sql quality findings for telemetry
const findings: Telemetry.Finding[] = []
</file context>
| const isRealFailure = !!result.error || (!!error && !undecidableAbstention) | |
| const isRealFailure = !!result.error || !!data.error || (!!extractEquivalenceErrors(data) && !undecidableAbstention) |
| const runtime = Permission.merge( | ||
| optimizer!.permission, | ||
| session, | ||
| optimizer!.permission.filter( |
There was a problem hiding this comment.
P2: This test cannot detect a regression in the production deny-reapply logic because it duplicates that logic while constructing runtime. If src/session/tools.ts reverted to reapplying the overridden question deny, this test would still pass; exercise the actual tool/session permission path or extract and test a shared implementation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/agent/agent.test.ts, line 964:
<comment>This test cannot detect a regression in the production deny-reapply logic because it duplicates that logic while constructing `runtime`. If `src/session/tools.ts` reverted to reapplying the overridden `question` deny, this test would still pass; exercise the actual tool/session permission path or extract and test a shared implementation.</comment>
<file context>
@@ -961,11 +961,23 @@ it.instance(
optimizer!.permission,
session,
- optimizer!.permission.filter((r) => r.action === "deny" && r.permission !== "*"),
+ optimizer!.permission.filter(
+ (r) =>
+ r.action === "deny" &&
</file context>
| // Re-apply a deny only if it is the agent's EFFECTIVE decision — | ||
| // a default deny the agent later overrode with its own allow | ||
| // (e.g. `question`) must not be resurrected past that allow. | ||
| PermissionNext.evaluate(r.permission, r.pattern, input.agent.permission).action === "deny", |
There was a problem hiding this comment.
P2: When an agent has a wildcard deny followed by a narrower allow, this check evaluates the deny against its literal wildcard pattern rather than an actual request. It therefore re-appends that deny after the allow and breaks the permission system's last-match-wins override; determine reapplication from the effective matched rules without overriding narrower allows.
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 1735:
<comment>When an agent has a wildcard deny followed by a narrower allow, this check evaluates the deny against its literal wildcard pattern rather than an actual request. It therefore re-appends that deny after the allow and breaks the permission system's last-match-wins override; determine reapplication from the effective matched rules without overriding narrower allows.</comment>
<file context>
@@ -1725,7 +1725,15 @@ export namespace SessionPrompt {
+ // Re-apply a deny only if it is the agent's EFFECTIVE decision —
+ // a default deny the agent later overrode with its own allow
+ // (e.g. `question`) must not be resurrected past that allow.
+ PermissionNext.evaluate(r.permission, r.pattern, input.agent.permission).action === "deny",
+ ),
),
</file context>
| resolved = fs.realpathSync(resolved) | ||
| } catch { | ||
| try { | ||
| resolved = path.join(fs.realpathSync(path.dirname(resolved)), path.basename(resolved)) |
There was a problem hiding this comment.
P2: When a path under an in-project symlink has multiple nonexistent trailing components, this fallback leaves resolved lexical because it retries only the immediate parent. The permission metadata and loader path can therefore look project-local while the loader later follows the symlink outside the project; walk to the nearest existing ancestor and append every remaining component.
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/schema-path-guard.ts, line 47:
<comment>When a path under an in-project symlink has multiple nonexistent trailing components, this fallback leaves `resolved` lexical because it retries only the immediate parent. The permission metadata and loader path can therefore look project-local while the loader later follows the symlink outside the project; walk to the nearest existing ancestor and append every remaining component.</comment>
<file context>
@@ -32,7 +33,22 @@ export async function guardExternalFile(
+ resolved = fs.realpathSync(resolved)
+ } catch {
+ try {
+ resolved = path.join(fs.realpathSync(path.dirname(resolved)), path.basename(resolved))
+ } catch {
+ /* neither leaf nor parent exists — gate the lexical path as-is */
</file context>
| resolved = path.join(fs.realpathSync(path.dirname(resolved)), path.basename(resolved)) | |
| resolved = (() => { | |
| let current = resolved | |
| const suffix: string[] = [] | |
| while (true) { | |
| try { | |
| return path.join(fs.realpathSync(current), ...suffix) | |
| } catch { | |
| const parent = path.dirname(current) | |
| if (parent === current) return resolved | |
| suffix.unshift(path.basename(current)) | |
| current = parent | |
| } | |
| } | |
| })() |
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 evidence-backed dbt project scans and tightens permission, SQL-safety, and path-gating so read-scoped scans cannot write or exfiltrate. Old: stored “always allow” approvals and tool exposure could bypass agent denies, EXPLAIN ANALYZE could execute writes, and out-of-project paths weren’t uniformly gated; New: non‑overridable denies hold across sessions, EXPLAIN ANALYZE requires write approval, and all analysis paths gate through a canonicalized external-directory check.dbt-optimizerwith deny-by-default;edit/bashask;sql_execute_writeis non‑overridable. Built-in tool exposure now honors denies viaPermission.disabled, with edit-tool remaps to keepapply_patch/patch/multiedit underedit.*catch‑all), so ceilings hold without clobbering allowlists.sql_explainblocks multi‑statement input and now requiressql_execute_writeapproval foranalyze:true, falling back to estimated plans on denial.sql_diff: LCS line diffs with hunk context; forwardsschema_context/dialect; returnsequivalence_assessed,decidable, and never treats UNDECIDABLE as equivalent. Engine failures no longer erase the text diff.parse-dbt, and allschema_pathwrappers resolve paths relative to the project, canonicalize symlinks before gating, pass the same real path to reads, and propagate permission rejections.unique_id; target matching acceptsunique_id; affected tests counted byunique_id.revieweranddbt-optimizer. Equivalence abstentions (decidable:false) report UNDECIDABLE, not ERROR, even with validation errors.Written for commit 54d2df6. 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
Touches permission evaluation (deny vs stored “always” approvals), SQL write classification, EXPLAIN ANALYZE execution, and external path gating—security-critical warehouse and filesystem surfaces.
Overview
Adds
dbt-optimizer, a fifth primary agent that scans a dbt project in four phases (read-only scan → user-selected fixes → impact report → optional PR) with a deny-by-default allowlist: analysis/finops tools allowed,edit/bashask, andsql_execute_writedenied non-overridably even after user/agent config merge. Builder’s self-review now hands off optimizer-shaped issues instead of fixing them out of scope.Hardens the permission and tool surface the agent depends on. Stored “always” approvals no longer override a configured deny. Path-taking dbt/core tools go through a shared
external_directorygate (symlink-canonicalized).schema_indexand training save/remove prompt before mutating persistent state.SQL safety: a lexer-based masker drives write classification (side-effect functions, comment/literal bypasses, CR line endings).
sql_explain analyze:trueis limited to a single read-only statement and requiressql_execute_write. Equivalence/rewrite/sql_diffnow distinguish UNDECIDABLE from proven equivalent, use LCS diffs, and no longer treat engine abstention as a false safety claim. Impact analysis traverses by dbtunique_idso package-colliding names are not dropped. Docs list Reviewer and dbt-Optimizer alongside the existing modes.Reviewed by Cursor Bugbot for commit 54d2df6. Bugbot is set up for automated code reviews on this repo. Configure here.