feat(validators): dbt-tests-pass validator + dbt-schema-verify hardening#849
Merged
Conversation
…ments
Adds research/kimi-k26-ade-bench-2026-05-10/ with a blog-ready writeup of how
the Moonshot Kimi-K2.6 model behaves as a coding agent inside altimate-code's
agent loop, derived from 78 trial traces against ADE-Bench. Findings cover
tool-usage distribution, wall-clock anatomy (~89% model generation, ~5%
tools), prompt-cache amplification (85.8%), per-failure-class taxonomy, and
extended appendices (per-trial manifest, pass-rate by family, skill
invocation log, cost/runtime distribution, reproducibility command, glossary,
open questions).
Also extends two shipped skills with generic dbt-best-practice patterns
surfaced during the analysis (all benchmark-agnostic, applicable to any dbt
project):
- dbt-develop/SKILL.md
* stronger description with explicit invocation triggers
* new section on transformation-logic pitfalls: incremental high-water
marks (>= vs >), snapshot strategy selection, LEFT JOIN + COUNT(*)
phantom rows, type harmonization in COALESCE/CASE/UNION, date-spine
completeness, off-by-one window boundaries, uniqueness enforcement,
window-LIMIT tiebreakers
* deliverable-enumeration step in Validate phase + iron rule
* unit-test verification step + iron rule
- dbt-unit-tests/SKILL.md
* new iron rule requiring mock data to exercise every SQL construct's
failure mode (LEFT JOIN unmatched parents, NULLIF zero, CASE branches,
COALESCE all-null, window boundaries, date spines, etc.)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the source-code + scripts + 4 small patches needed to plug altimate-code into upstream ade-bench. Lets anyone reproduce the 81.3% pass rate described in research/kimi-k26-ade-bench-2026-05-10/ without trusting the pre-aggregated numbers. What's included: - benchmark/ade-bench/README.md — full reproduction guide (prereqs, Docker memory, env-var knobs, step-by-step commands, troubleshooting) - benchmark/ade-bench/altimate_code_agent/ — drop-in agent module (AltimateCodeAgent class, JSON event parser, log formatter, install script that runs inside the trial container, tarball builder) - benchmark/ade-bench/patches/ — 4 small patches against upstream dbt-labs/ade-bench (register AgentName.ALTIMATE_CODE, wire it into the AgentFactory, export from installed_agents/__init__.py, route the existing shared/config/AGENTS.md baseline file the same way Codex receives it — pure parity, no benchmark-specific content) Explicitly NOT in this folder: - Trace files / per-trial agent.log / results.json (regenerable) - The 130 MB built tarball (build-local-tarball.sh recreates it) - Seed DuckDB databases (downloaded from dbt-labs/ade-bench releases) - Per-task ground-truth seeds + test SQL (those live in upstream ade-bench and are never sent to the agent at run time) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…itfalls
Two related changes, both shipped to every altimate-code user.
(1) `feat(skill)`: add `alwaysApply: bool` and `applyPaths: string|string[]`
frontmatter to skill metadata, mirroring Cursor's "Always Apply" and
"Auto Attached" rule modes. When a skill is `alwaysApply: true` or has
`applyPaths` matching at least one file under the worktree, its body
is inlined into the system prompt at session start under an
`<auto_loaded_skill>` block — the model no longer needs to invoke the
Skill tool to access that guidance.
Motivation: benchmark traces show the agent invokes the `Skill` tool
in <1% of tool calls, even after the skill description is rewritten
to be imperative. Many failures occur on patterns the relevant skill
already documents but the agent never loads. Auto-loading puts the
body deterministically in context for projects where the skill
applies.
Files:
• packages/opencode/src/skill/skill.ts — Info schema + both load
paths (filesystem + binary-embedded) pluck the new fields
• packages/opencode/src/session/system.ts — auto-inline matched
skill bodies after the existing available_skills XML block
• .opencode/skills/dbt-develop/SKILL.md — frontmatter now declares
`applyPaths: [dbt_project.yml, **/dbt_project.yml]`, so dbt
projects auto-load this skill's body (~270 lines of dbt
best-practice patterns) at session start
The existing skill-tool-invocation path is unchanged; auto-load is
additive. Skills without `alwaysApply` / `applyPaths` continue to
require explicit invocation. Prompt caching amortizes the extra
tokens across the long agent loop.
(2) `docs(skill)`: three new generic dbt pitfall sections in
`dbt-develop/SKILL.md`, all benchmark-agnostic best practices
surfaced during failure-trace analysis:
• String concatenation with `NULL` operands — `||` / `CONCAT`
propagate `NULL`; wrap with `COALESCE` or use `CONCAT_WS`.
Catches an invisible row-dropper in surrogate-key generation and
derived columns.
• dbt model versioning (dbt 1.8+) — when introducing a v2 of an
existing model, use dbt's `versions:` block in `_models.yml` with
`defined_in:`, not a sibling `_v2.sql` file. Otherwise downstream
lineage and `{{ ref(model, v=2) }}` resolution break.
• Strengthened the existing window-rank + `LIMIT` section to call
out determinism explicitly, including the `QUALIFY ROW_NUMBER()
OVER (... ORDER BY metric, id)` form and the "if you can't think
of a tiebreaker, you don't have a unique key yet" framing.
All three patterns are documented in well-known dbt style guides
and would benefit any real altimate-code user — they are not
benchmark-targeted tweaks.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds reference for the new auto-load mechanism to docs/docs/configure/skills.md: - Lists the two new frontmatter fields in the Frontmatter Fields table - New "Auto-loading skills" section explaining the lazy-load default, how `alwaysApply` and `applyPaths` change it, a worked example, a "when to use" table, and an honest section on context-size implications + prompt-cache amortization Pure documentation update — no code change in this commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two changes informed by trace analysis of the benchmark run with the
initial auto-load mechanism. With the auto-loaded body present in the
system prompt, 6 of 8 sampled failing trials never referenced any of
its guidance keywords (date spine, tiebreaker, deliverable, etc.) —
the model was treating the auto-loaded section as background reference
rather than binding directive. These two changes address the framing.
(1) `feat(system-prompt)`: move auto-loaded skill bodies BEFORE the
lazy-loaded `<available_skills>` XML block in the skills section.
Previously the order was:
1. "Use the skill tool to load a skill..." preamble
2. <available_skills> XML (long, descriptions only)
3. <auto_loaded_skill> body (binding guidance)
Now:
1. <auto_loaded_skill> body (binding guidance — read FIRST)
2. "Skills provide specialized instructions..." preamble
3. <available_skills> XML (lazy-loaded skills the agent can opt into)
Framing the auto-loaded body as "rules of the road" at the start
rather than supplementary documentation at the end. Pure ordering
change in `SystemPrompt.skills()` parts array — no schema or API
change. Applies to any skill using `applyPaths` or `alwaysApply`.
File: packages/opencode/src/session/system.ts
(2) `docs(skill)`: add a "Pre-completion checklist" section (§5) to
dbt-develop that the agent is told to emit with `[x]/[ ]` marks
before declaring the task done.
Each item is a yes/no question against patterns the skill already
documents (LEFT JOIN cardinality, date-spine completeness,
window-rank tiebreaker, type harmonization in COALESCE/CASE/UNION,
string-concat NULL handling, uniqueness enforcement, incremental
high-water mark, snapshot strategy, dbt model versioning v2,
unit-test verification).
The forcing function: the agent must produce the checklist text in
its final message. Unchecked items without a stated "n/a" reason
mean the task is not done. Forces the model to slow down at the
end and verify the patterns against the SQL it just wrote, rather
than silently skip the verification phase.
All items are generic dbt patterns applicable to any project — no
benchmark-specific test names, no solution-seed values, no
grading-rubric hints.
File: .opencode/skills/dbt-develop/SKILL.md
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…result
The "emit a [x]/[ ] checklist before declaring done" addition to
dbt-develop (§5, shipped two commits ago) was measured negative on
the post-A+B benchmark re-run:
- Checklist appeared in 6 of 14 still-failing trial outputs.
- Zero of those 6 flipped to PASS.
- In multiple traces, the agent self-marked `[x] LEFT JOIN
cardinality correct` while the underlying SQL still had the
exact phantom-row bug the item warned against.
The framing trained the model to perform verification theater
rather than actually re-read its SQL. The two flips attributed
earlier to "A+B" (helixops_saas007, helixops_saas009) trace back
to the placement reorder (A) — the checklist (B) contributed
nothing measurable, and adds 50+ lines of system-prompt content
for no benefit.
This commit:
(1) Removes §5 from `.opencode/skills/dbt-develop/SKILL.md`.
The other sections (Plan → Discover → Write → Validate,
Common Pitfalls in Transformation Logic, Iron Rules) stay
intact. The placement reorder in `system.ts` and the
`applyPaths`/`alwaysApply` frontmatter mechanism stay.
(2) Adds a "What we tried that didn't work" section to
research/kimi-k26-ade-bench-2026-05-10/findings.md so the
negative result is preserved as institutional knowledge.
The broader principle — "soft self-verification (model
promises it checked X) is unreliable on this model class;
hard verification (compile/test failures) still works" — is
worth keeping around.
(3) Updates the findings TL;DR with both the original 81.3%
headline and the post-second-wave 85.3% best-of-runs number,
with the caveat that the body of the post analyzes the
first-wave traces.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `LEFT JOIN + COUNT(*)` pitfall example referenced `dim_listings LEFT JOIN fct_reviews`. Those names are the canonical airbnb dbt-tutorial models (from Maven Analytics / public dbt courses) and also happen to be ADE-Bench tasks, so even though the rule itself is fully generic, the example wording was needlessly close to benchmark content. Swap to abstract `dim_parent LEFT JOIN fct_child` — the rule is identical, the wording is unambiguous. No behavior change. Cosmetic only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…iff validate step Three new generalizable dbt patterns surfaced from systematic trace analysis of ADE-Bench failures: 1. Iron Rule 8 — Schema Fidelity: agent must match the spec's column tuple exactly (names, types, ORDER, no extras). Adding "helpful" columns or substituting synonyms (supplier_id vs supplier_company) breaks AUTO_*_equality tests against the spec contract. 2. CTE-to-model refactor row preservation: when extracting a CTE into a standalone intermediate model, build it FROM the parent table the CTE started from, not the child table. The extracted model otherwise becomes effectively an INNER JOIN and drops parent rows with no children. Includes dbt_utils.equal_rowcount and audit_helper verification patterns. 3. Diff-against-spec step in the validate phase: agent produces three lists (columns_extra, columns_missing, columns_reordered) and treats any non-empty list as "not done". Verification > in-prompt negative rules (per the Self-Verification Dilemma literature). All three pass the "When working on any dbt project, ..." self-test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… check
Adds a new `altimate-dbt schema-verify --model <name>` subcommand that
mechanically diffs a model's produced columns against the schema.yml
spec and returns a structured `{verdict, columns_extra, columns_missing,
columns_reordered, type_mismatches}` result.
Background: trace analysis of repeated benchmark failures showed that
in-prompt rules ("match the column spec exactly") were being read but
not applied — the agent agrees in principle, then adds extra columns
or reorders them anyway. The Self-Verification Dilemma literature
predicts this: negative rules without a mechanical check are weak.
Design follows the existing dbt-tools split: dbt parsing lives in
altimate-code (via dbt-integration's adapter), so the bridge belongs
here. Spec source: `adapter.parseManifest().nodeMetaMap.lookupByBaseName(model).columns`
(schema.yml entries compiled into manifest.json). Actual source:
`adapter.getColumnsOfModel(model)` (warehouse / catalog).
Case-insensitive name comparison (dbt convention). Type mismatches
are reported only when the spec actually declares `data_type` —
common to omit it, and treating omission as a mismatch would
produce noise.
Skill change: the validate phase's "diff column shape" section now
prescribes calling `altimate-dbt schema-verify` and treating any
`mismatch` verdict as "not done", instead of asking the agent to
self-diff column lists.
Iron Rule 8 also tightened to point at the mechanical check.
Tests: 13 covering the four diff categories, the no-spec skip,
case-insensitivity, type-mismatch precedence rules, error
propagation, and two regression-style cases mirroring real
ade-bench failure shapes (extra rank-breakdown columns, leading-
column reorder).
All four pass the "When working on any dbt project, …" self-test.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move the schema-verify procedure out of dbt-develop's body (where it was ~30 lines deep in a 450-line skill) into its own focused skill that auto-loads on dbt projects. Why: trace inspection of v5 runs showed the agent reads the schema-verify instruction inside dbt-develop, agrees with it in chain-of-thought, then doesn't actually run the command. Burying a procedural step inside a discursive skill is part of the problem; the agent gives the step less attention than skill-top imperative content. Design: short, procedural, imperative skill body. Auto-loads via applyPaths alongside dbt-develop. Iron rules state the contract explicitly. Includes a fallback when altimate-dbt isn't available (reads schema.yml + dbt show by hand). Cross-references the dbt-develop "CTE row-preservation" pattern for the related row-count case (which schema-verify does NOT cover). dbt-develop now points at dbt-schema-verify instead of embedding the full procedure. Iron Rule 8 similarly points at the dedicated skill. Honest caveat: this is still a prompt-level intervention. Trace inspection of v4 and v5 runs both showed that even mechanically- callable tools get ignored if the harness doesn't enforce the call. The structural fix is harness-level: a before_terminate hook with per-domain completion validators, of which dbt-schema-verify would be one. That work is a follow-up — this skill is the cleanest prompt-side fix in the meantime. Passes the "When working on any dbt project, ..." self-test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The agent has been ignoring skill-level instructions to call schema-verify before declaring done (v4 and v5 trace inspection showed the agent reads the rule, writes the intention in chain-of-thought, then doesn't run the command). Building a full harness-level validator framework that intercepts session termination is real engineering; this commit ships the smallest forcing mechanism that doesn't require touching the session loop: auto-trigger schema-verify inside the build command's own response. The agent now cannot see a successful `altimate-dbt build --model X` without also seeing the schema-verify verdict in the same tool result. The diff is in the JSON response under `schema_verify`, in-context where the agent's attention sits — much harder to ignore than a system-prompt skill rule. Behavior: - `build` without `--model` is unchanged (project-wide build, no per-model verify makes sense). - `build --model X` runs schema-verify on X after a successful build. The full structured result lives at `schema_verify`. - A verify failure does NOT mask the build's stdout — both are reported. Build status remains the success/error signal. - If verify itself errors (missing manifest, unbuilt table), the error is reported under `schema_verify.error` with a fix hint. Tests: - Updated existing build-test mocks to include parseManifest + getColumnsOfModel (no behavior change, just shape consistency). - New assertion: build --model X result now contains schema_verify. Skill: dbt-schema-verify body adds a note that the agent gets schema-verify "for free" inside build's response, so it doesn't need to call it twice for verification. This is a stepping stone — the full validator framework that intercepts session termination is the next iteration. This commit tests whether putting the diff inline with the build response is enough to break through the ignore-the-rule pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
v7 trace inspection revealed the agent uses `altimate-dbt build` (no --model) for project-wide builds and `dbt build --model X` (plain dbt) for per-model — never `altimate-dbt build --model X`. So the per-model auto-trigger added in 3924009 never fired in any of the 30 trials, even though it was wired correctly. The hook missed because of the agent's command choice, not because the agent ignored the result. This commit extends the auto-trigger to the project-wide path: after a successful `altimate-dbt build` (no --model), iterate every model in the parsed manifest that has columns declared in schema.yml, run schema-verify on each, and roll up the results into a single `schema_verify_summary` field on the response: { "stdout": "...", "schema_verify_summary": { "models_checked": N, "match": M, "mismatch": K, "no_spec": L, "errored": E, "mismatches": [ { model, verdict, columns_extra, ... } ] } } Only the mismatches are reported in full. Match and no-spec models are counted but not echoed (keeps the response compact for 49-model projects). Errored models include the per-model error string so the agent can investigate. The summary is the closest a CLI command can get to harness-level enforcement without intercepting session termination: every project-wide build now returns the full diff against schema.yml in the same tool result the agent receives for the build. The agent literally cannot see a green project-build without also seeing every schema mismatch in the project. Tests: new "project-wide build collects mismatches" test exercises the 3-model case (match + mismatch + no-spec) end to end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Six experiments (v3-v9) proved that every form of completion-discipline
enforcement living inside the agent's command surface — skill rule, tool
description, auto-trigger inside a wrapping CLI, even binary substitution
— gets read, agreed-with in chain-of-thought, then ignored. In v9 the
agent actively found a backup binary at `.orig` to bypass the wrapping
shim. The Self-Verification Dilemma literature predicts this. The only
remaining lever is enforcement the agent cannot see: the harness
inspecting the world after `finishReason === "stop"` and refusing to
terminate if a registered validator says the work isn't done.
This commit adds the framework but does not enable it. Behavior is opt-in
via ALTIMATE_VALIDATORS_ENABLED=1, with a separate retry budget knob
(ALTIMATE_VALIDATORS_MAX_RETRIES=3 default). Telemetry fires
unconditionally so we can measure baseline fire rate against historical
traffic even before the gate is enabled.
Files added (framework, domain-agnostic):
- session/validators/types.ts — Validator, ValidatorResult,
ValidatorContext interfaces with a load-bearing comment explaining
why this lives in the harness and not in skills/tools.
- session/validators/registry.ts — Map-keyed registry + runAll that
catches per-validator exceptions and converts them to soft-passes
(a buggy validator should never brick the agent loop).
Files added (altimate domain, first concrete validator):
- altimate/validators/dbt-schema-verify.ts — wraps the existing
`altimate-dbt schema-verify` CLI. appliesTo: dbt project detected
in worktree. check: scans models/ for .sql files mtime'd in this
session, runs schema-verify on each, returns mismatch with a
structured fixHint listing columns_extra/missing/reordered.
- altimate/validators/index.ts — side-effect registration on import.
Wiring in session/prompt.ts step loop:
- After processor.process() returns and the model declared finish:"stop"
with no error and no pending tool calls, runAll() is dispatched.
- Telemetry fires for every validator regardless of opt-in.
- If the gate is enabled AND any validator failed AND we're under the
retry budget: a synthetic user message is appended to the session
with the aggregated failure reasons + fix hints. The step loop's
top-of-iteration break check then sees the newer user message and
does NOT break — the model gets one more turn to address the gap.
- Retry budget exhaustion falls through to the natural break.
Architectural choice: the dispatch hook is in prompt.ts not in
processor.ts. processor.process() returns per-step semantics (stop /
continue / compact); prompt.ts owns the multi-step harness loop. The
validator gate is a harness concept, not a stream concept.
Generalisable: the framework is domain-agnostic. New validators register
via `ValidatorRegistry.register(...)` from any module's side-effect
import. Phase 2 candidates (already scoped, not in this commit):
dbt-rowcount-preservation, dbt-tests-pass, sql-compile, sql-equivalence,
pii-scan, column-lineage. Each is ~30-50 LOC on top of the framework.
Not in this commit (deferred):
- The skill diet (extracting process-discipline content out of
dbt-develop / dbt-schema-verify into validator fixHints). Lands once
we've measured validator fire rates with the gate enabled.
- Unit tests for the framework (lives in a follow-up dedicated test
PR since prompt.ts is already heavily integration-tested upstream).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bun --single may tree-shake side-effect imports. Switch to explicit registerAltimateValidators() call so the registration is unambiguously referenced. Also add an info log on every hook entry so we can confirm the code path is reached even when validators don't fire. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ixes Harness-side completion-gate validator framework, completing the 3-part series from PR #792 (registry) + PR #800 (dbt-schema-verify) + this PR. ### New: dbt-tests-pass validator - Fires after the agent declares done (finish === "stop") - Detects dbt model `.sql` files modified since session start via mtime - Runs `altimate-dbt test --model <name>` against each touched model - Parses `Done. PASS=N WARN=N ERROR=N ...` summary from dbt output - Extracts individual failing test names from per-line output - Injects synthetic user message with fix hints when tests fail - `extractLastJsonObject()` handles altimate-dbt's JSON envelope + log noise - Only activates in dbt projects (scans for `dbt_project.yml`) ### Enhanced: dbt-schema-verify hardening - `parseSchemaVerifyOutput()` — scans backwards for last balanced `{...}` block to handle dbt log noise (ANSI codes, parser warnings) emitted before the JSON verdict - Debug logging for spawn errors and close events, gated behind `ALTIMATE_VALIDATORS_DEBUG=1` so normal sessions stay quiet - Better error fallback: reports non-JSON stdout when stderr is empty ### prompt.ts: debug-gated diagnostics - `ALTIMATE_VALIDATORS_DEBUG=1` env var gates all stderr console.error calls — on by default in ade-bench harness, off everywhere else - Added `validatorsEnabled &&` guard on dispatch condition (was missing) - Debug logs for dispatch_enter, dispatch_result, dispatch_error events - `hasError` field added to validator_hook_reached diagnostic ### build-local-tarball.sh: altimate-dbt on PATH - Added `"altimate-dbt": "./dbt-tools/bin/altimate-dbt"` to bin entries - Ensures `altimate-dbt` is available via PATH in benchmark Docker containers (was missing; validators depend on it) ### system.ts: fix stray altimate_change marker placement - Moved `// altimate_change end` from inside `skills()` function body (before the closing `}`) to outside it — the function's closing brace was appearing outside any marker block, triggering Marker Guard CI Co-Authored-By: Claude Sonnet 4.6 <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 to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
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:
📝 Walkthrough<review_stack_artifact> </review_stack_artifact> 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 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 `@benchmark/ade-bench/altimate_code_agent/altimate_code_agent.py`:
- Around line 227-229: The code appends self._model_name directly into the shell
command string (building variable command) which is unsafe; modify the command
construction in altimate_code_agent.py where command is built so that the model
name is safely escaped (e.g., wrap self._model_name with shlex.quote OR
preferably stop using a single shell-interpolated string and build command as a
list and pass to subprocess without shell=True), replacing the direct f" --model
{self._model_name}" interpolation to an escaped or argument-list approach to
prevent shell injection.
In `@benchmark/ade-bench/altimate_code_agent/altimate-code-setup.sh`:
- Around line 27-30: The script altimate-code-setup.sh currently falls back to
installing `@altimateai/altimate-code`@latest when a local tarball isn't staged;
change this to avoid non-deterministic installs by failing fast instead (or
pinning a known version). Modify the else branch that echoes "Local tarball not
staged; falling back to latest published" to either exit with a non-zero status
(e.g., echo an error and run exit 1) so the benchmark run stops when the tarball
is missing, or replace the npm install target with a fixed, documented version
string if you must auto-install; ensure the branch updates the message to
reflect the new behavior.
In `@benchmark/ade-bench/altimate_code_agent/build-local-tarball.sh`:
- Around line 11-14: REPO_ROOT is computed by climbing too many directories
("$(cd "$SCRIPT_DIR/../../../../../.." && pwd)") which can resolve outside the
repo; change the traversal to the correct depth (e.g. "$(cd
"$SCRIPT_DIR/../../.." && pwd)") so REPO_ROOT points to the repository root and
update dependent vars PKG_DIR, DBT_TOOLS_DIR and SKILLS_DIR remain based on
REPO_ROOT; locate and replace the REPO_ROOT assignment in build-local-tarball.sh
to use the corrected relative path.
In `@benchmark/ade-bench/README.md`:
- Around line 9-22: The README.md's fenced directory-tree code block is missing
a language tag (MD040); update the opening fence for the tree block in README.md
to include a language identifier (e.g., change ``` to ```text) so the snippet is
properly tagged, leaving the content and closing fence unchanged.
In `@docs/docs/configure/skills.md`:
- Around line 68-72: Add a language identifier to the fenced code block that
contains the <auto_loaded_skill> snippet so markdownlint rule MD040 is
satisfied; update the triple-backtick fence that wraps the <auto_loaded_skill
name="<skill-name>"> ... </auto_loaded_skill> block to use a language such as
"xml" (e.g., ```xml) so the code block is properly annotated.
- Around line 65-67: The docs currently say auto-loaded skills are appended
after <available_skills>, but runtime prepends matched skill bodies into the
skills section; update the wording around session start / the skills section
(lines mentioning "<available_skills>") to state that every matched skill body
is prepended to the system prompt in the skills section (before the
<available_skills> listing) so the documentation matches the implementation.
In `@packages/opencode/src/altimate/validators/dbt-schema-verify.ts`:
- Around line 36-57: isDbtProject currently only returns a boolean so nested
dbt_project.yml detections don’t yield the actual project path; change
isDbtProject to return the resolved project root path (string) or null (or add a
new findDbtProjectRoot that does this) and update appliesTo and check to use
that resolved root instead of ctx.workingDirectory when scanning directories or
spawning altimate-dbt; locate references in the same module (isDbtProject /
appliesTo / check) and ensure all file system operations and child_process
spawns use the discovered root path so nested projects run in the correct cwd.
- Around line 17-227: Refactor isDbtProject, modelsModifiedSince and
runSchemaVerify to use Effect-based services: replace direct fs/promises calls
with the FileSystem.FileSystem service (use its methods inside Effect.gen or
Effect.fn tracers) and replace child_process.spawn usage in runSchemaVerify with
ChildProcessSpawner.ChildProcessSpawner to spawn processes and ChildProcess.make
to manage the child handle; keep parseSchemaVerifyOutput as-is but call it from
the new Effect-based runSchemaVerify. Specifically, convert top-level async
functions to Effect.gen(function* { ... }) (or Effect.fn("Domain.method", ...)
wrappers) so file ops (stat, readdir) use FileSystem API and process exec uses
ChildProcessSpawner.spawn/make, propagate debug logging through the traced
Effect.fn calls, and ensure return types remain Promise-like by returning
Effect.toPromise where the callsites expect a Promise.
- Around line 268-291: The code currently drops null returns from
runSchemaVerify and returns ok: true whenever mismatches.length === 0, which
lets failures slip through; change the loop that calls runSchemaVerify (using
touched, modelNameFromPath, runSchemaVerify, ctx.workingDirectory) so that null
or errored runs are recorded in results (e.g., push a SchemaVerifyOutput entry
marking error/verdict appropriately instead of skipping), then update the final
check so ok: true is returned only when mismatches.length === 0 AND errored ===
0 AND results.length === touched.length (or otherwise ensure every touched model
was verified); keep using the existing symbols (results, mismatches, noSpec,
matches, errored) when computing the response.
- Around line 84-85: The code currently splits paths using "/" which fails on
Windows; update the checks to be OS-independent by using Node's path utilities:
normalize paths and split on path.sep (replace occurrences like full.split("/")
and p.split("/") with full = path.normalize(full) then full.split(path.sep), and
inside modelNameFromPath normalize p and split on path.sep or better use
path.parse/path.dirname/path.basename to extract the model directory/name),
ensuring the logic that detects "models" (where found.push(full) happens) and
the modelNameFromPath function work correctly across platforms.
In `@packages/opencode/src/altimate/validators/dbt-tests-pass.ts`:
- Around line 155-166: runDbtTest currently resolves null on spawn() errors
which lets the validator treat missing/failed altimate-dbt startup as a pass;
change runDbtTest so the child "error" handler produces a failing TestRunOutput
(include error.message/text) instead of resolving null, and adjust the caller
that currently drops null results (the validation logic handling runDbtTest
results) to treat that TestRunOutput as a failure (ok:false, checked:0 or
appropriate error metadata) so startup failures fail the validation rather than
being ignored.
- Around line 94-95: The code uses full.split("/") which fails on Windows;
update the checks in packages/opencode/src/altimate/validators/dbt-tests-pass.ts
to use path helpers: import path from "path", normalize the path
(path.normalize(full)) and split by path.sep (or use path.parse/path.basename or
path.relative and then split by path.sep) when checking for the "models" segment
and when deriving the model name in modelNameFromPath; replace all occurrences
of full.split("/") (including the other block around lines 109-111) so the logic
works cross-platform.
- Around line 49-64: The validator currently uses isDbtProject(cwd) which only
returns a boolean and treats nested dbt_project.yml files as "supported" while
later scans and altimate-dbt test invocations still run from the parent cwd;
change the behavior so the actual project directory is propagated: refactor
isDbtProject into a finder that returns the path of the dbt project directory
(e.g., return the matched subdirectory path or cwd when dbt_project.yml is
directly present) or add a new findDbtProjectDir(cwd): string | null helper,
then update appliesTo(...) and any places that call isDbtProject (and the code
that invokes altimate-dbt test) to use that returned projectDir when running
scans/commands instead of always using the parent cwd (ensure calls to
altimate-dbt test use the resolved projectDir).
In `@packages/opencode/src/session/prompt.ts`:
- Around line 1118-1124: The validator context vCtx is using process.cwd() which
can point outside the project; update vCtx to pass the session's project path
(use the session instance's directory/worktree property used elsewhere in the
loop, e.g. Instance.directory or Instance.worktree) instead of process.cwd() so
validators scan the correct tree; locate the vCtx construction in
session/prompt.ts and replace workingDirectory: process.cwd() with the
appropriate session instance property referenced in the surrounding code so
validators receive the project directory.
In `@packages/opencode/src/session/system.ts`:
- Around line 147-149: The code currently logs a warning in the catch inside
applyPaths (autoLoadLog.warn("applyPaths glob scan failed", { skill: skill.name,
err })) but a later catch (the one that converts scan errors to []) swallows the
error so that warning never runs; update the catch that currently converts
errors to [] (inside applyPaths or its helper) to log the error using
autoLoadLog.warn with skill.name and err (or rethrow after logging) instead of
silently returning [] so the warning path runs and the error is not hidden.
Ensure you reference and update the catch handling in applyPaths (and any helper
that returns [] on error) to either log via autoLoadLog.warn(...) or rethrow the
error.
- Line 109: The code injects skill.name directly into an XML attribute in the
parts.push call that builds `<auto_loaded_skill name="...">`; escape XML
attribute characters instead to prevent breaking the prompt (convert &, <, >, "
and ' to entities). Add or reuse a helper like escapeXmlAttr(name: string) and
call it where the code currently uses skill.name so parts.push uses the escaped
value (e.g., parts.push(`<auto_loaded_skill
name="${escapeXmlAttr(skill.name)}">`)). Ensure the helper is applied
consistently for any other XML attribute insertions in the same module.
In `@packages/opencode/src/session/validators/registry.ts`:
- Around line 14-71: ValidatorRegistry.runAll uses raw async/Promise calls and
must be converted to return an Effect so tracing via Effect.gen/Effect.fn is
preserved; update the Validator interface so appliesTo and check return
Effect<...> instead of Promise, change runAll (and its signature) to build and
return an Effect that iterates validators.values() inside an Effect.gen,
invoking v.appliesTo and v.check via
Effect.fn("Validator.appliesTo")/Effect.fn("Validator.check") (or wrap calls
with Effect.attempt or Effect.try) and translate thrown errors into the same
skipped/soft-pass results inside the Effect; finally, update callers (e.g., the
caller in packages/opencode/src/session/prompt.ts) to execute/run the returned
Effect rather than awaiting Promises so existing behavior and logging of
validator errors is preserved.
In `@research/kimi-k26-ade-bench-2026-05-10/findings.md`:
- Around line 511-513: The summary line "Distinct trials that invoked any skill:
**18 / 81**" is using the wrong denominator; update that fraction to use the
consistent 78 graded trials used elsewhere (i.e., change "**18 / 81**" to "**18
/ 78**" in the findings.md summary) and scan nearby summary lines like "Total
skill invocations: **19**" to ensure all denominators match the canonical 78.
- Around line 87-92: The fenced code blocks in findings.md are missing language
identifiers (e.g., the triple-backtick lines are just "```"); update each
untyped fenced block (the block around the checklist shown and the other block
at lines referenced in the review) to include a language tag such as ```text or
```md (or ```bash if it's shell commands) so they comply with MD040 and render
consistently; locate the untyped triple-backtick fences in the file and replace
them with language-specified fences for both occurrences.
🪄 Autofix (Beta)
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
Run ID: 30137a2f-09d3-4752-8379-35d41acc6a61
📒 Files selected for processing (28)
.opencode/skills/dbt-develop/SKILL.md.opencode/skills/dbt-schema-verify/SKILL.md.opencode/skills/dbt-unit-tests/SKILL.mdbenchmark/ade-bench/README.mdbenchmark/ade-bench/altimate_code_agent/__init__.pybenchmark/ade-bench/altimate_code_agent/altimate-code-setup.shbenchmark/ade-bench/altimate_code_agent/altimate_code_agent.pybenchmark/ade-bench/altimate_code_agent/build-local-tarball.shbenchmark/ade-bench/patches/01-agent_name.py.patchbenchmark/ade-bench/patches/02-agent_factory.py.patchbenchmark/ade-bench/patches/03-installed_agents_init.py.patchbenchmark/ade-bench/patches/04-agent_setup.py.patchdocs/docs/configure/skills.mdpackages/dbt-tools/src/commands/build.tspackages/dbt-tools/src/commands/schema-verify.tspackages/dbt-tools/src/index.tspackages/dbt-tools/test/build.test.tspackages/dbt-tools/test/schema-verify.test.tspackages/opencode/src/altimate/validators/dbt-schema-verify.tspackages/opencode/src/altimate/validators/dbt-tests-pass.tspackages/opencode/src/altimate/validators/index.tspackages/opencode/src/session/prompt.tspackages/opencode/src/session/system.tspackages/opencode/src/session/validators/registry.tspackages/opencode/src/session/validators/types.tspackages/opencode/src/skill/skill.tsresearch/kimi-k26-ade-bench-2026-05-10/README.mdresearch/kimi-k26-ade-bench-2026-05-10/findings.md
Comment on lines
+14
to
+71
| export const ValidatorRegistry = { | ||
| /** Register a validator. Overwrites any prior registration with the same name. */ | ||
| register(v: Validator): void { | ||
| validators.set(v.name, v) | ||
| }, | ||
|
|
||
| /** Remove a validator. Mostly useful for tests. */ | ||
| unregister(name: string): void { | ||
| validators.delete(name) | ||
| }, | ||
|
|
||
| /** Snapshot the current list of registered validators. */ | ||
| list(): readonly Validator[] { | ||
| return Array.from(validators.values()) | ||
| }, | ||
|
|
||
| /** Reset all registrations. Tests only. */ | ||
| clear(): void { | ||
| validators.clear() | ||
| }, | ||
|
|
||
| /** | ||
| * Run all validators that apply to the given context. Returns one entry | ||
| * per validator that was relevant (skipped validators are NOT in the | ||
| * result list). Validators that themselves throw are caught and logged; | ||
| * the framework converts them to a {ok: true} skip so a buggy validator | ||
| * cannot brick the agent loop. | ||
| */ | ||
| async runAll(ctx: ValidatorContext): Promise<Array<{ validator: Validator; result: ValidatorResult }>> { | ||
| const out: Array<{ validator: Validator; result: ValidatorResult }> = [] | ||
| for (const v of validators.values()) { | ||
| let applies = false | ||
| try { | ||
| applies = await v.appliesTo(ctx) | ||
| } catch { | ||
| // appliesTo() throwing is a validator bug; skip rather than block agent. | ||
| continue | ||
| } | ||
| if (!applies) continue | ||
| try { | ||
| const result = await v.check(ctx) | ||
| out.push({ validator: v, result }) | ||
| } catch (e) { | ||
| // check() throwing is also a validator bug; record as a soft pass so | ||
| // the agent isn't stuck behind a broken validator. The thrown error | ||
| // is logged by the caller. | ||
| out.push({ | ||
| validator: v, | ||
| result: { | ||
| ok: true, | ||
| details: { error: e instanceof Error ? e.message : String(e), skipped_due_to_validator_error: true }, | ||
| }, | ||
| }) | ||
| } | ||
| } | ||
| return out | ||
| }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the reviewed file
FILE="packages/opencode/src/session/validators/registry.ts"
echo "=== File: $FILE ==="
wc -l "$FILE"
sed -n '1,220p' "$FILE" | nl -ba
# Check for Effect usage and any Promise-based exports in the file
echo
echo "=== Search within file for Effect / Promise ==="
rg -n "Effect\.|Promise<|async " "$FILE" || true
# Find usages of ValidatorRegistry.runAll (and register/unregister/list) across the repo
echo
echo "=== Usages of ValidatorRegistry.runAll ==="
rg -n "ValidatorRegistry\.runAll" -S . || true
echo
echo "=== Usages of ValidatorRegistry.register/unregister/list/clear ==="
rg -n "ValidatorRegistry\.(register|unregister|list|clear)" -S . || true
# Find the Validator interface/type to see expected return types for appliesTo/check
echo
echo "=== Find Validator / ValidatorContext / ValidatorResult definitions ==="
rg -n "type Validator|interface Validator|ValidatorContext|ValidatorResult" packages/opencode/src/session/validators -S || trueRepository: AltimateAI/altimate-code
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/opencode/src/session/validators/registry.ts"
echo "=== File: $FILE ==="
wc -l "$FILE"
echo "--- content (with line numbers) ---"
awk '{printf "%5d\t%s\n", NR, $0}' "$FILE" | sed -n '1,200p'
echo
echo "=== Search within file for Effect / Promise ==="
rg -n "Effect\.|Promise<|async " "$FILE" || true
echo
echo "=== Usages of ValidatorRegistry.runAll ==="
rg -n "ValidatorRegistry\.runAll" -S packages/opencode || rg -n "ValidatorRegistry\.runAll" -S . || true
echo
echo "=== Usages of ValidatorRegistry.register/unregister/list/clear ==="
rg -n "ValidatorRegistry\.(register|unregister|list|clear)" -S packages/opencode || rg -n "ValidatorRegistry\.(register|unregister|list|clear)" -S . || true
echo
echo "=== Find Validator / ValidatorContext / ValidatorResult definitions ==="
rg -n "interface Validator|type Validator|ValidatorContext|ValidatorResult" packages/opencode/src/session/validators -S || trueRepository: AltimateAI/altimate-code
Length of output: 4941
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/opencode/src/session/prompt.ts"
echo "=== File: $FILE ==="
wc -l "$FILE"
echo "--- snippet around ValidatorRegistry.runAll usage ---"
# Use sed to avoid huge output
sed -n '1060,1160p' "$FILE" | awk '{printf "%5d\t%s\n", NR+1060, $0}'
echo
echo "=== Search for surrounding Effect usage in prompt.ts ---"
rg -n "Effect\.gen|Effect\.fn|yield\*|runAll\(vCtx\)|ValidatorRegistry\.runAll" "$FILE" || trueRepository: AltimateAI/altimate-code
Length of output: 5417
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/opencode/src/session/prompt.ts"
# Show a bit more context above and below the call site
sed -n '980,1165p' "$FILE" | awk '{print NR+980-1 "\t" $0}' | sed -n '1,220p'
echo
echo "=== Quick scan for Effect usage in prompt.ts ==="
rg -n "Effect\.gen|Effect\.fnUntraced|Effect\.fn\(|yield\*|runPromise|runFork" "$FILE" || true
echo
echo "=== Scan for async/Promise-only patterns near the call site ==="
sed -n '1040,1160p' "$FILE" | rg -n "async |await |Promise<" || trueRepository: AltimateAI/altimate-code
Length of output: 9636
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the altimate validators index
IDX="packages/opencode/src/altimate/validators/index.ts"
echo "=== $IDX ==="
wc -l "$IDX"
sed -n '1,200p' "$IDX" | nl -ba 2>/dev/null || sed -n '1,200p' "$IDX"
echo
echo "=== Find validator implementation files referenced in that index ==="
rg -n "register\\(([^)]+)\\)" "$IDX" || true
echo
echo "=== Search for implementations of appliesTo/check ==="
rg -n "appliesTo\\(|check\\(" packages/opencode/src/altimate/validators -S || true
echo
echo "=== Show the registered validator(s) files (best-effort) ==="
# Extract identifiers used in register(...) and locate their definition
IDS=$(rg -n "ValidatorRegistry\\.register\\(([^)]+)\\)" "$IDX" -o --replace '$1' | tr -d ' ' | tr '\n' ' ' | sed 's/;*$//')
echo "Identifiers: $IDS"
# Use fd to find candidate TS files containing each identifier
for id in $(echo "$IDS" | tr ' ' '\n' | sed '/^$/d'); do
echo
echo "--- searching for $id ---"
fd -e ts -e tsx -t f ".*" packages/opencode/src/altimate/validators >/dev/null 2>&1 || true
rg -n "\\b$id\\b" packages/opencode/src/altimate/validators -S --hidden || true
done
# Finally, inspect the specific types
TYPES="packages/opencode/src/session/validators/types.ts"
echo
echo "=== $TYPES (relevant parts) ==="
wc -l "$TYPES"
sed -n '1,220p' "$TYPES" | awk 'NR>=35 && NR<=160 {print NR "\t" $0}'Repository: AltimateAI/altimate-code
Length of output: 5592
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect prompt.ts imports / top-level for Effect usage
PROMPT="packages/opencode/src/session/prompt.ts"
echo "=== Top of $PROMPT ==="
sed -n '1,120p' "$PROMPT" | awk '{print NR "\t" $0}'
echo
echo "=== Effect usage scan in prompt.ts ==="
rg -n "from \"effect|Effect\\.|effect/" "$PROMPT" || true
rg -n "\\bEffect\\." "$PROMPT" || true
# 2) Inspect validator implementations around appliesTo/check
for V in \
packages/opencode/src/altimate/validators/dbt-schema-verify.ts \
packages/opencode/src/altimate/validators/dbt-tests-pass.ts
do
echo
echo "=== $V (appliesTo/check sections) ==="
wc -l "$V"
# show where appliesTo/check are defined
rg -n "async appliesTo\\(|async check\\(" "$V"
# show a window around the first occurrence
START=$(rg -n "async appliesTo\\(|async check\\(" "$V" | head -n1 | cut -d: -f1)
END=$((START+120))
sed -n "${START},${END}p" "$V" | awk '{print NR+START-1 "\t" $0}'
echo
echo "=== Effect/FileSystem usage scan in $V ==="
rg -n "Effect\\.|FileSystem\\.|HttpClient\\.|ChildProcess" "$V" || true
doneRepository: AltimateAI/altimate-code
Length of output: 10594
Effectify ValidatorRegistry.runAll (and validator dispatch) to preserve traced Effect composition
packages/opencode/src/session/validators/registry.ts implements ValidatorRegistry.runAll as raw async/Promise control flow (await v.appliesTo / await v.check), which bypasses the Effect.gen(...) + Effect.fn("Domain.method") traced Effect composition guidelines for packages/opencode/**/*.{ts,tsx}. Refactor runAll (and the Validator appliesTo/check contracts) to return Effect so the caller (packages/opencode/src/session/prompt.ts) can execute it within the Effect runtime.
🤖 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/session/validators/registry.ts` around lines 14 - 71,
ValidatorRegistry.runAll uses raw async/Promise calls and must be converted to
return an Effect so tracing via Effect.gen/Effect.fn is preserved; update the
Validator interface so appliesTo and check return Effect<...> instead of
Promise, change runAll (and its signature) to build and return an Effect that
iterates validators.values() inside an Effect.gen, invoking v.appliesTo and
v.check via Effect.fn("Validator.appliesTo")/Effect.fn("Validator.check") (or
wrap calls with Effect.attempt or Effect.try) and translate thrown errors into
the same skipped/soft-pass results inside the Effect; finally, update callers
(e.g., the caller in packages/opencode/src/session/prompt.ts) to execute/run the
returned Effect rather than awaiting Promises so existing behavior and logging
of validator errors is preserved.
Comment on lines
+87
to
+92
| ``` | ||
| [pending] Add position_descriptions to f1_dataset.yml sources | ||
| [pending] Create src_<model>.sql views in models/src/ pointing to source tables | ||
| [pending] Update staging models to reference src_ models instead of raw tables | ||
| [pending] Run dbt build to verify everything compiles and builds successfully | ||
| ``` |
There was a problem hiding this comment.
Add languages to fenced code blocks for markdownlint compliance.
Both fenced blocks are untyped; add a language (text, md, or bash) to satisfy MD040 and improve rendering consistency.
Also applies to: 209-216
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 87-87: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@research/kimi-k26-ade-bench-2026-05-10/findings.md` around lines 87 - 92, The
fenced code blocks in findings.md are missing language identifiers (e.g., the
triple-backtick lines are just "```"); update each untyped fenced block (the
block around the checklist shown and the other block at lines referenced in the
review) to include a language tag such as ```text or ```md (or ```bash if it's
shell commands) so they comply with MD040 and render consistently; locate the
untyped triple-backtick fences in the file and replace them with
language-specified fences for both occurrences.
Comment on lines
+511
to
+513
| Total skill invocations: **19** | ||
| Distinct trials that invoked any skill: **18 / 81** | ||
|
|
There was a problem hiding this comment.
Fix inconsistent trial denominator in skill-invocation summary.
Line 512 reports 18 / 81, but this report consistently uses 78 graded trials elsewhere, so this denominator likely needs correction.
🤖 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 `@research/kimi-k26-ade-bench-2026-05-10/findings.md` around lines 511 - 513,
The summary line "Distinct trials that invoked any skill: **18 / 81**" is using
the wrong denominator; update that fraction to use the consistent 78 graded
trials used elsewhere (i.e., change "**18 / 81**" to "**18 / 78**" in the
findings.md summary) and scan nearby summary lines like "Total skill
invocations: **19**" to ensure all denominators match the canonical 78.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
18 issues found across 28 files
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=".opencode/skills/dbt-unit-tests/SKILL.md">
<violation number="1" location=".opencode/skills/dbt-unit-tests/SKILL.md:43">
P2: The window-function guidance requires testing an "empty partition," but that case is not directly representable in SQL window output. This can mislead test authors into chasing an untestable condition.</violation>
</file>
<file name="packages/opencode/src/session/validators/registry.ts">
<violation number="1" location="packages/opencode/src/session/validators/registry.ts:48">
P2: `appliesTo()` exceptions are swallowed without any surfaced error details, so broken validators can be silently skipped and become hard to diagnose.</violation>
</file>
<file name=".opencode/skills/dbt-schema-verify/SKILL.md">
<violation number="1" location=".opencode/skills/dbt-schema-verify/SKILL.md:118">
P2: The fallback verification command is incomplete (and brittle): it only searches `schema.yml`, so `_models.yml` declarations can be missed during manual schema checks.</violation>
</file>
<file name="benchmark/ade-bench/altimate_code_agent/altimate-code-setup.sh">
<violation number="1" location="benchmark/ade-bench/altimate_code_agent/altimate-code-setup.sh:92">
P2: The config file contains API secrets but is written without restrictive permissions, which can leave credentials world-readable under default umask.</violation>
</file>
<file name="benchmark/ade-bench/altimate_code_agent/build-local-tarball.sh">
<violation number="1" location="benchmark/ade-bench/altimate_code_agent/build-local-tarball.sh:11">
P1: `REPO_ROOT` is derived with too many `..` segments, so the script can resolve paths outside the repository and fail to find build artifacts.</violation>
</file>
<file name="research/kimi-k26-ade-bench-2026-05-10/findings.md">
<violation number="1" location="research/kimi-k26-ade-bench-2026-05-10/findings.md:111">
P3: The “step-to-step gaps” row uses a start-to-start interval, which conflicts with the glossary’s gap definition (finish-to-next-start) and can mislead interpretation of timing breakdowns.</violation>
<violation number="2" location="research/kimi-k26-ade-bench-2026-05-10/findings.md:236">
P2: The f1011 failure description contradicts the appendix data (`check_option_b` is described as passed, but appendix rows indicate it failed).</violation>
<violation number="3" location="research/kimi-k26-ade-bench-2026-05-10/findings.md:250">
P3: The lead-in failure count is inconsistent with the per-domain breakdown in the same sentence (listed counts sum to 21, not 19).</violation>
</file>
<file name="packages/opencode/src/session/system.ts">
<violation number="1" location="packages/opencode/src/session/system.ts:109">
P2: Escape `skill.name` before interpolating it into the `name="..."` attribute to avoid malformed prompt XML and prompt-structure injection.</violation>
</file>
<file name="benchmark/ade-bench/altimate_code_agent/altimate_code_agent.py">
<violation number="1" location="benchmark/ade-bench/altimate_code_agent/altimate_code_agent.py:134">
P2: `num_turns` is undercounted because it only increments on `reason == "stop"` instead of every `step_finish` event.</violation>
<violation number="2" location="benchmark/ade-bench/altimate_code_agent/altimate_code_agent.py:228">
P1: `--model` value is not shell-escaped, which allows malformed or injected shell arguments from model configuration.</violation>
</file>
<file name="packages/dbt-tools/src/commands/build.ts">
<violation number="1" location="packages/dbt-tools/src/commands/build.ts:76">
P1: Project-wide schema verification uses non-unique model selectors (`name`), so duplicate model names can be verified against the wrong manifest node.</violation>
</file>
<file name="docs/docs/configure/skills.md">
<violation number="1" location="docs/docs/configure/skills.md:65">
P2: The docs describe auto-loaded skills as being appended after `<available_skills>`, but the implementation places them before that block. This mismatch can mislead users debugging prompt composition.</violation>
</file>
<file name=".opencode/skills/dbt-develop/SKILL.md">
<violation number="1" location=".opencode/skills/dbt-develop/SKILL.md:278">
P2: This guidance incorrectly describes `CONCAT_WS` behavior/support for Snowflake and BigQuery. It can lead to invalid BigQuery SQL and wrong NULL-handling assumptions in Snowflake.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| continue | ||
| } | ||
| try { | ||
| const v = await schemaVerify(adapter, ["--model", name]) |
There was a problem hiding this comment.
P1: Project-wide schema verification uses non-unique model selectors (name), so duplicate model names can be verified against the wrong manifest node.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/dbt-tools/src/commands/build.ts, line 76:
<comment>Project-wide schema verification uses non-unique model selectors (`name`), so duplicate model names can be verified against the wrong manifest node.</comment>
<file context>
@@ -36,7 +46,77 @@ export async function test(adapter: DBTProjectIntegrationAdapter, args: string[]
+ continue
+ }
+ try {
+ const v = await schemaVerify(adapter, ["--model", name])
+ if ("error" in v) {
+ errored.push({ model: name, error: String((v as { error: unknown }).error) })
</file context>
Comment on lines
+278
to
+280
| Use `CONCAT_WS()` if your dialect supports it (Snowflake, BigQuery) — it | ||
| skips `NULL` operands instead of propagating them, which is usually safer | ||
| than a static placeholder. |
There was a problem hiding this comment.
P2: This guidance incorrectly describes CONCAT_WS behavior/support for Snowflake and BigQuery. It can lead to invalid BigQuery SQL and wrong NULL-handling assumptions in Snowflake.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .opencode/skills/dbt-develop/SKILL.md, line 278:
<comment>This guidance incorrectly describes `CONCAT_WS` behavior/support for Snowflake and BigQuery. It can lead to invalid BigQuery SQL and wrong NULL-handling assumptions in Snowflake.</comment>
<file context>
@@ -127,6 +202,203 @@ Use `altimate-dbt children` and `altimate-dbt parents` to verify the DAG is inta
+-- Right: explicit placeholder
+COALESCE(region, 'UNKNOWN') || '-' || COALESCE(segment, 'UNKNOWN') AS geo_segment
+```
+Use `CONCAT_WS()` if your dialect supports it (Snowflake, BigQuery) — it
+skips `NULL` operands instead of propagating them, which is usually safer
+than a static placeholder.
</file context>
Suggested change
| Use `CONCAT_WS()` if your dialect supports it (Snowflake, BigQuery) — it | |
| skips `NULL` operands instead of propagating them, which is usually safer | |
| than a static placeholder. | |
| Use dialect-specific null-safe concatenation patterns instead. BigQuery does not provide `CONCAT_WS()`; use `ARRAY_TO_STRING([region, segment], '-')` when you want to skip `NULL` values. | |
| In Snowflake, `CONCAT_WS()` returns `NULL` if any argument is `NULL`, so keep explicit `COALESCE(...)` placeholders there. |
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
Multi-Persona Review — Verdict: comment
Multi-persona review completed.
6/6 agents completed · 118s · 0 findings (0 critical, 0 high, 0 medium)
Multi-Persona Review · vllm:qwen3-next-80b (waves) + vllm-fallback (synth) ·
All issues identified in the consensus review (Claude + 8 models) are addressed in this commit. **MAJOR fixes (blocking merge)** - Add subprocess timeout (`ALTIMATE_VALIDATORS_TIMEOUT_MS`, default 60 s) to `runDbtTest` and `runSchemaVerify` — prevents the agent loop from hanging indefinitely on stalled DuckDB connections or warehouse I/O. Kills the child process with SIGKILL on timeout. - Fix shadow telemetry gate: move `ValidatorRegistry.runAll()` and the per-validator `Telemetry.track()` loop outside the `validatorsEnabled` check in `prompt.ts`. Telemetry now fires regardless of the feature flag, fulfilling the "measure before enforce" promise stated in the inline comment. - Fix nested dbt project cwd bug: replace `isDbtProject(cwd): boolean` with `findDbtProjectRoot(cwd): Promise<string | null>` which returns the directory that actually contains `dbt_project.yml`. Both validators now pass that root as `cwd` to subprocess invocations and as the base for `modelsModifiedSince`, preventing the "not a dbt project" error when the project is one level below the working dir. - Extract shared helpers into `validator-utils.ts`: `findDbtProjectRoot`, `modelsModifiedSince`, `modelNameFromPath`, `extractLastJsonObject`. Both validator files now import from the shared module. The validated `extractLastJsonObject` rejects stray JSON fragments (checks for `verdict`/`error`/`model`/`stdout`/`columns_*` keys) — the laxer version that was only in `dbt-tests-pass.ts` is gone. - Add tests: 39 unit tests covering `extractLastJsonObject` (8 cases), `modelNameFromPath`, `findDbtProjectRoot` (5 cases), `modelsModifiedSince` (7 cases), and `parseDbtTestOutput` (10 cases including dbt 1.x format, ANSI prefixes, NO-OP variant, duplicate names, `[FAIL`/`[ERROR` token exclusion). **MINOR fixes** - Track spawn failures separately in `dbt-tests-pass.check()` and `dbt-schema-verify.check()`. `details.spawn_failures` now appears in the validator result so operators can distinguish "skipped model" from "passed model". - Add retries-exhausted telemetry: when `validatorRetryCount >= maxValidatorRetries` with failures outstanding, emit `validator_retries_exhausted` event and a `log.warn` so the session doesn't silently appear as "completed" in the operator dashboard. - Parallel model checking: both `check()` functions now use `Promise.all` instead of a sequential `for` loop. **NITS** - Named regex groups in `parseDbtTestOutput` — replaces positional `summaryMatch[1]` / `[3]` / `[5]` captures; resilient to dbt reordering summary fields. - Path separator: `modelsModifiedSince` and `modelNameFromPath` now use `path.sep` / `path.basename` instead of hardcoded `"/"`. - Fix `ls | head -1` in `build-local-tarball.sh`: derive exact tarball name from `VERSION` variable; error out explicitly if not present. - Fix stale comment "Limited to two-level deep search" in `dbt-schema-verify.ts` (actual depth was 4; comment is removed). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/opencode/src/session/prompt.ts (1)
1117-1120:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass the session/project directory to validators, not process cwd.
Using
process.cwd()can point outside the active session tree, causing validators to scan/run in the wrong location.Suggested fix
const vCtx = { sessionID, - workingDirectory: process.cwd(), + workingDirectory: Instance.directory, sessionStartMs: sessionStartTime, step, retryCount: validatorRetryCount, }🤖 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/session/prompt.ts` around lines 1117 - 1120, vCtx currently sets workingDirectory to process.cwd(), which can point outside the active session; locate the session's actual directory variable (e.g., session.root, sessionDir, projectDir, or the value used when the session was created) and set vCtx.workingDirectory to that session/project directory instead of process.cwd(); update any callers that rely on vCtx (the vCtx object created near the sessionID/workingDirectory/sessionStartMs block) so validators receive the correct session path.packages/opencode/src/altimate/validators/dbt-schema-verify.ts (1)
170-172:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFail closed when schema checks cannot be executed.
Spawn failures are counted but still allow
ok: truewhen there are no mismatches, which can bypass completion gating for unverified models.Suggested fix
- if (mismatches.length === 0) { + if (mismatches.length === 0 && spawnFailures === 0 && errored === 0) { return { ok: true, details: { @@ } } + + if (mismatches.length === 0 && (spawnFailures > 0 || errored > 0)) { + return { + ok: false, + reason: "Some touched models could not be schema-verified.", + fixHint: "Resolve validator/runtime issues and rerun so every touched model is verified.", + details: { + models_touched: touched.length, + verified: results.length, + errored, + spawn_failures: spawnFailures, + }, + } + }Also applies to: 180-191
🤖 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/validators/dbt-schema-verify.ts` around lines 170 - 172, The code currently increments spawnFailures but still allows ok: true when no mismatches are found; update the final decision logic so any spawnFailures force a failure: when computing the overall result (the variable/property named ok and/or the results object), set ok = false if spawnFailures > 0 and add a descriptive failure entry (or push an error into the mismatches/diagnostics array) so spawn failures are treated the same as schema mismatches; adjust the block around spawnFailures++ and the later aggregation (the code that produces ok) to ensure spawn failures between lines ~170 and ~191 flip the overall status and surface an error message.packages/opencode/src/altimate/validators/dbt-tests-pass.ts (1)
113-116:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDo not treat subprocess startup failures as passing validation.
A spawn failure currently resolves to
nulland can still end inok: true, which bypasses the completion gate for unverified models.Suggested fix
- child.on("error", () => { + child.on("error", (e) => { clearTimeout(timer) - resolve(null) + resolve({ model, error: `failed to start altimate-dbt: ${e.message}` }) }) @@ - const results: TestRunOutput[] = [] - for (const out of outputs) { - if (out) results.push(out) - else spawnFailures++ - } + const results: TestRunOutput[] = [] + for (const out of outputs) { + if (out) results.push(out) + else spawnFailures++ + } @@ - if (failures.length === 0 && errored.length === 0) { + if (failures.length === 0 && errored.length === 0 && spawnFailures === 0) { return { ok: true, details: {Also applies to: 193-215
🤖 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/validators/dbt-tests-pass.ts` around lines 113 - 116, The child.on("error") handler currently clears the timeout and calls resolve(null), which lets startup failures be treated as successful/unknown and can lead to ok:true; change the handler to clearTimeout(timer) and instead reject the Promise with the error (e.g., reject(err)) or resolve a failure result object (e.g., { ok: false, error: err.message }) so startup errors are propagated as failures; apply this change to both occurrences of child.on("error") (the handler that uses clearTimeout(timer) and resolve(null) around lines with child.on("error") and the similar block at lines 193–215), and ensure the surrounding code that awaits this Promise handles the rejection/failure shape.
🧹 Nitpick comments (1)
packages/opencode/test/altimate/validators/validator-utils.test.ts (1)
131-140: ⚡ Quick winUse
tmpdir()+await usingfixture pattern for temp dirs in tests.This test manages temp directories manually; switch to the shared
tmpdirhelper so setup/cleanup behavior is consistent and less error-prone.As per coding guidelines, "Use the
tmpdirfunction fromfixture/fixture.tsto create temporary directories for tests with automatic cleanup in test files" and "Always useawait usingsyntax withtmpdir()for automatic cleanup when the variable goes out of scope".Also applies to: 185-191
🤖 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/validators/validator-utils.test.ts` around lines 131 - 140, Replace the manual temp-dir setup/teardown in the findDbtProjectRoot test (the tmpDir variable and beforeEach/afterEach that call fs.mkdtemp/fs.rm) with the shared tmpdir fixture using the "await using" pattern: call await using (const tmpDir = await tmpdir()) inside the test scope (or top-level describe where appropriate) so temporary directories are created and automatically cleaned up; update both the findDbtProjectRoot block and the other similar manual setup (the second occurrence around the same file) to use tmpdir() instead of manual fs.mkdtemp/fs.rm and remove the beforeEach/afterEach teardown functions.
🤖 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/validators/validator-utils.ts`:
- Line 87: The scanner's file check uses entry.name.endsWith(".sql") which is
case-sensitive and misses files like "FOO.SQL"; change that check to a
case-insensitive comparison (e.g., use entry.name.toLowerCase().endsWith(".sql")
or a case-insensitive regex) so it matches any .sql variant and stays consistent
with modelNameFromPath's handling; update the branch that uses entry.isFile() &&
entry.name.endsWith(".sql") accordingly.
- Line 25: The exported VALIDATOR_TIMEOUT_MS currently uses
Number(process.env.ALTIMATE_VALIDATORS_TIMEOUT_MS ?? "60000") which can produce
NaN, 0, or negative values and lead to immediate/no-op timeouts; change the
initialization in validator-utils.ts to parse the env value safely (e.g.,
parseInt or Number), validate that it's a finite positive integer, and if not
set or invalid fall back to the default 60000 (optionally clamp to a minimum
like 1 ms). Update the VALIDATOR_TIMEOUT_MS assignment to perform this
validation and fallback so the constant is always a sane positive timeout value.
In `@packages/opencode/test/altimate/validators/validator-utils.test.ts`:
- Line 255: The test currently uses platform-dependent splitting: replace the
expression result.map((p) => p.split("/").pop()) with result.map((p) =>
path.basename(p)) and add an import for Node's path module (e.g., import path
from 'path') at the top of the test file so basename is used for OS-independent
extraction; update any expectations if necessary to match path.basename
behavior.
---
Duplicate comments:
In `@packages/opencode/src/altimate/validators/dbt-schema-verify.ts`:
- Around line 170-172: The code currently increments spawnFailures but still
allows ok: true when no mismatches are found; update the final decision logic so
any spawnFailures force a failure: when computing the overall result (the
variable/property named ok and/or the results object), set ok = false if
spawnFailures > 0 and add a descriptive failure entry (or push an error into the
mismatches/diagnostics array) so spawn failures are treated the same as schema
mismatches; adjust the block around spawnFailures++ and the later aggregation
(the code that produces ok) to ensure spawn failures between lines ~170 and ~191
flip the overall status and surface an error message.
In `@packages/opencode/src/altimate/validators/dbt-tests-pass.ts`:
- Around line 113-116: The child.on("error") handler currently clears the
timeout and calls resolve(null), which lets startup failures be treated as
successful/unknown and can lead to ok:true; change the handler to
clearTimeout(timer) and instead reject the Promise with the error (e.g.,
reject(err)) or resolve a failure result object (e.g., { ok: false, error:
err.message }) so startup errors are propagated as failures; apply this change
to both occurrences of child.on("error") (the handler that uses
clearTimeout(timer) and resolve(null) around lines with child.on("error") and
the similar block at lines 193–215), and ensure the surrounding code that awaits
this Promise handles the rejection/failure shape.
In `@packages/opencode/src/session/prompt.ts`:
- Around line 1117-1120: vCtx currently sets workingDirectory to process.cwd(),
which can point outside the active session; locate the session's actual
directory variable (e.g., session.root, sessionDir, projectDir, or the value
used when the session was created) and set vCtx.workingDirectory to that
session/project directory instead of process.cwd(); update any callers that rely
on vCtx (the vCtx object created near the
sessionID/workingDirectory/sessionStartMs block) so validators receive the
correct session path.
---
Nitpick comments:
In `@packages/opencode/test/altimate/validators/validator-utils.test.ts`:
- Around line 131-140: Replace the manual temp-dir setup/teardown in the
findDbtProjectRoot test (the tmpDir variable and beforeEach/afterEach that call
fs.mkdtemp/fs.rm) with the shared tmpdir fixture using the "await using"
pattern: call await using (const tmpDir = await tmpdir()) inside the test scope
(or top-level describe where appropriate) so temporary directories are created
and automatically cleaned up; update both the findDbtProjectRoot block and the
other similar manual setup (the second occurrence around the same file) to use
tmpdir() instead of manual fs.mkdtemp/fs.rm and remove the beforeEach/afterEach
teardown functions.
🪄 Autofix (Beta)
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
Run ID: 9c935fc7-0292-4705-9d8a-ebf9833196e8
📒 Files selected for processing (7)
benchmark/ade-bench/altimate_code_agent/build-local-tarball.shpackages/opencode/src/altimate/validators/dbt-schema-verify.tspackages/opencode/src/altimate/validators/dbt-tests-pass.tspackages/opencode/src/altimate/validators/validator-utils.tspackages/opencode/src/session/prompt.tspackages/opencode/test/altimate/validators/dbt-tests-pass.test.tspackages/opencode/test/altimate/validators/validator-utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- benchmark/ade-bench/altimate_code_agent/build-local-tarball.sh
There was a problem hiding this comment.
4 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="benchmark/ade-bench/altimate_code_agent/altimate_code_agent.py">
<violation number="1" location="benchmark/ade-bench/altimate_code_agent/altimate_code_agent.py:134">
P2: `num_turns` is undercounted because it only increments on `reason == "stop"` instead of every `step_finish` event.</violation>
</file>
<file name="packages/dbt-tools/src/commands/build.ts">
<violation number="1" location="packages/dbt-tools/src/commands/build.ts:76">
P1: Project-wide schema verification uses non-unique model selectors (`name`), so duplicate model names can be verified against the wrong manifest node.</violation>
</file>
<file name="docs/docs/configure/skills.md">
<violation number="1" location="docs/docs/configure/skills.md:65">
P2: The docs describe auto-loaded skills as being appended after `<available_skills>`, but the implementation places them before that block. This mismatch can mislead users debugging prompt composition.</violation>
</file>
<file name=".opencode/skills/dbt-develop/SKILL.md">
<violation number="1" location=".opencode/skills/dbt-develop/SKILL.md:278">
P2: This guidance incorrectly describes `CONCAT_WS` behavior/support for Snowflake and BigQuery. It can lead to invalid BigQuery SQL and wrong NULL-handling assumptions in Snowflake.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Expands the validator test suite from 39 to 117 tests, covering
boundary conditions, malformed inputs, and realistic dbt output patterns
that the original suite did not exercise.
**extractLastJsonObject adversarial cases**
- Stray JSON rejection: empty object, array with no envelope keys,
objects with only unknown keys, numeric keys
- Envelope key guard: null/false/empty-string values still accepted when
key is present; `error: null` does not invalidate the envelope
- Noisy stdout scanning: Python traceback + JSON, 50-line progress
noise, BOM prefix, CRLF line endings, > 10 KB leading noise,
whitespace-padded JSON
- Last-wins semantics: two valid envelopes (last wins), three valid
envelopes (last wins), stray fragments between valid envelopes,
same-line consecutive objects
- Brace/string parsing: nested braces in string values, escaped
backslashes, escaped double-quotes, multiline string values, stdout
field containing inner JSON, unicode characters, unicode escape
sequences, multi-line formatted JSON, unbalanced `{` in log noise
**parseDbtTestOutput adversarial cases**
- Null/empty guard: null, undefined, whitespace-only, truncated output,
dbt compile error (no Done. line)
- All-pass: clean run, SKIP-only, WARN-only
- NO-OP variant: zero tests, multiple NO-OP counts
- Failure extraction: deduplication of repeated test names, FAIL vs
ERROR lines, `[FAIL`/`[ERROR` token exclusion, test names with dots,
15+ failing tests captured
- Large counts: 99999 pass, 99999 error, zero counts, single test
- Format resilience: case-insensitive Done., named groups vs positional
(PASS=7 ERROR=3 TOTAL=11), timestamps, ANSI colours, CRLF line
endings, summary at very start/end of string, multiple summary lines
- Realistic full-output scenarios: dbt 1.8 all-pass, dbt 1.8 partial
failures, ANSI-coloured Docker output, no-tests-defined NO-OP,
SKIP from --exclude flag
**findDbtProjectRoot adversarial cases**
- Two-level deep search limit: project at depth 2 is NOT found
- dbt_project.yml is a directory (documents stat behavior)
- Many subdirs, only one has the file
- Direct takes precedence over nested
**modelsModifiedSince adversarial cases**
- Depth boundary: depth 4 included, depth 5 excluded (with path counts)
- Non-.sql files inside models/: yml, md, py, json all excluded
- File named `models.sql` outside a `models/` path component excluded
- Mtime boundary: file with mtime === sinceMs is included (>= semantics)
- Mixed modified/unmodified files
- Empty models/ directory (no SQL files)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/test/altimate/validators/validator-utils.test.ts (1)
353-362: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace manual
mkdtemplifecycle withawait using tmpdir()in tests.Line 357 and Line 444 (plus paired
afterEachcleanup blocks) should use the shared test fixture helper instead of manual temp-dir management, per repo test conventions.Suggested refactor pattern
-import { promises as fs } from "fs" +import { promises as fs } from "fs" +import { tmpdir } from "../fixture/fixture" @@ -describe("findDbtProjectRoot", () => { - let tmpDir: string - - beforeEach(async () => { - tmpDir = await fs.mkdtemp(join(osTmpdir(), "dbt-root-test-")) - }) - - afterEach(async () => { - await fs.rm(tmpDir, { recursive: true, force: true }) - }) +describe("findDbtProjectRoot", () => { @@ - test("returns cwd when dbt_project.yml is directly present", async () => { - await fs.writeFile(join(tmpDir, "dbt_project.yml"), "name: test_project\n") - expect(await findDbtProjectRoot(tmpDir)).toBe(tmpDir) + test("returns cwd when dbt_project.yml is directly present", async () => { + await using tmp = await tmpdir() + await fs.writeFile(join(tmp.path, "dbt_project.yml"), "name: test_project\n") + expect(await findDbtProjectRoot(tmp.path)).toBe(tmp.path) })Apply the same pattern in
modelsModifiedSincetests as well.As per coding guidelines, “Use the
tmpdirfunction fromfixture/fixture.tsto create temporary directories for tests with automatic cleanup in test files” and “Always useawait usingsyntax withtmpdir()for automatic cleanup when the variable goes out of scope.”Also applies to: 443-449
🤖 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/validators/validator-utils.test.ts` around lines 353 - 362, Replace the manual temp-dir lifecycle in the findDbtProjectRoot (and modelsModifiedSince) tests: remove the beforeEach/afterEach using fs.mkdtemp/fs.rm and instead acquire the temporary directory via the shared fixture by using "await using tmpdir()" to create the tmpDir variable; update any references to tmpDir in those tests to use the fixture-provided path and rely on automatic cleanup when the using scope ends; ensure you import tmpdir from fixture/fixture.ts and remove the manual fs.mkdtemp/fs.rm teardown code around the describe blocks.
🤖 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.
Outside diff comments:
In `@packages/opencode/test/altimate/validators/validator-utils.test.ts`:
- Around line 353-362: Replace the manual temp-dir lifecycle in the
findDbtProjectRoot (and modelsModifiedSince) tests: remove the
beforeEach/afterEach using fs.mkdtemp/fs.rm and instead acquire the temporary
directory via the shared fixture by using "await using tmpdir()" to create the
tmpDir variable; update any references to tmpDir in those tests to use the
fixture-provided path and rely on automatic cleanup when the using scope ends;
ensure you import tmpdir from fixture/fixture.ts and remove the manual
fs.mkdtemp/fs.rm teardown code around the describe blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 20fd2692-6a4c-4bc9-a768-f65af7d11a14
📒 Files selected for processing (2)
packages/opencode/test/altimate/validators/dbt-tests-pass.test.tspackages/opencode/test/altimate/validators/validator-utils.test.ts
|
Actionable comments posted: 0 |
…fixes Addresses all unresolved review threads from coderabbitai and cubic-dev-ai on PR #849 (feat/validator-framework). **Critical fixes** - `build-local-tarball.sh`: REPO_ROOT traversal depth 6→3 `..` segments (script is at `benchmark/ade-bench/altimate_code_agent/`, 3 levels deep) - `prompt.ts`: explicit `continue` after `validatorRetryCount++` to make the retry loop intent unambiguous (was falling through to bottom-of-loop `continue` correctly but implicitly) - `prompt.ts`: `workingDirectory` now uses `Instance.directory` instead of `process.cwd()` to match the session's actual working directory **Major fixes** - `altimate_code_agent.py`: `shlex.quote()` on `self._model_name` before shell interpolation to prevent injection via model name strings - `altimate-code-setup.sh`: `@latest` fallback replaced with `exit 1` for benchmark reproducibility; config file now written with `chmod 600` - `dbt-schema-verify.ts`: fail closed on errors — spawn failures now pushed into `results` as error entries; `ok` check requires `errored === 0` so unverifiable models don't silently pass the completion gate - `system.ts`: XML-escape `skill.name` before embedding in `<auto_loaded_skill name="...">` attribute via `escapeXmlAttr()` - `system.ts`: remove `.catch(() => [])` inside `anyMatchInWorktree` so errors propagate to the outer try/catch; `autoLoadLog.warn(...)` is now reachable - `registry.ts`: `appliesTo()` exceptions now surface error details as a soft-pass result entry instead of being silently swallowed **Minor fixes — validator-utils.ts hardening** - `VALIDATOR_TIMEOUT_MS`: finite/positive guard against NaN, 0, or negative env-var values (all fall back to the 60 s default) - `modelsModifiedSince`: case-insensitive `.sql` check (`.toLowerCase()`) for consistency with `modelNameFromPath` which already uses `/\.sql$/i` - New `runWithConcurrencyLimit` helper (max 4 concurrent by default, env override via `ALTIMATE_VALIDATORS_CONCURRENCY`) replaces unbounded `Promise.all` in both `dbt-tests-pass.ts` and `dbt-schema-verify.ts` **Docs / research corrections** - `findings.md`: timing table row renamed "Step-to-step intervals (start-to-start)" to clarify it includes step duration, not just gaps - `findings.md`: f1011 description corrected — `check_option_b` FAILED (per Appendix C), not passed - `findings.md`: per-domain failure counts corrected (asana 4→3, f1 5→4; total now sums to 19, consistent with headline 78-59=19) - `dbt-unit-tests/SKILL.md`: removed untestable "empty partition" window function guidance; replaced with partition-of-1 and tie-break row cases - `dbt-schema-verify/SKILL.md`: fallback verification glob broadened from `schema.yml` to `**/*.yml` to catch `_models.yml` and other conventions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/opencode/src/altimate/validators/validator-utils.ts (1)
12-13: 🏗️ Heavy liftModule uses raw
fs/promisesand plain async instead of the mandated Effect services.This new module under
packages/opencode/**does effectful file I/O viafs.stat/fs.readdirand exports plainasynchelpers, which deviates from the repo's Effect conventions. Given this subsystem handles subprocesses, file scanning, and timeouts, adopting the Effect services would also give it tracing and proper resource/timeout management. This complements the same systemic feedback raised onregistry.ts.As per coding guidelines: "Prefer
FileSystem.FileSysteminstead of rawfs/promisesfor effectful file I/O" and "UseEffect.gen(function* () { ... })for composing Effect computations".🤖 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/validators/validator-utils.ts` around lines 12 - 13, The module imports raw fs/promises (import { promises as fs } from "fs") and uses plain async helpers; replace direct use of fs.stat/fs.readdir and any exported async functions in validator-utils.ts with the repo Effect FileSystem service (FileSystem.FileSystem) and rewrite the helpers to return Effect values using Effect.gen(...) so they get tracing and resource/timeout management; specifically, inject/obtain the FileSystem service and call its stat/readdir equivalents instead of fs.stat/fs.readdir, convert exported async helpers into Effect.gen-based generators, and ensure callers are updated to run the Effects rather than awaiting plain Promises.
🤖 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.
Nitpick comments:
In `@packages/opencode/src/altimate/validators/validator-utils.ts`:
- Around line 12-13: The module imports raw fs/promises (import { promises as fs
} from "fs") and uses plain async helpers; replace direct use of
fs.stat/fs.readdir and any exported async functions in validator-utils.ts with
the repo Effect FileSystem service (FileSystem.FileSystem) and rewrite the
helpers to return Effect values using Effect.gen(...) so they get tracing and
resource/timeout management; specifically, inject/obtain the FileSystem service
and call its stat/readdir equivalents instead of fs.stat/fs.readdir, convert
exported async helpers into Effect.gen-based generators, and ensure callers are
updated to run the Effects rather than awaiting plain Promises.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d1ce922f-d8ef-4da7-9580-ff40ea712f9b
📒 Files selected for processing (13)
.opencode/skills/dbt-schema-verify/SKILL.md.opencode/skills/dbt-unit-tests/SKILL.mdbenchmark/ade-bench/altimate_code_agent/.gitignorebenchmark/ade-bench/altimate_code_agent/altimate-code-setup.shbenchmark/ade-bench/altimate_code_agent/altimate_code_agent.pybenchmark/ade-bench/altimate_code_agent/build-local-tarball.shpackages/opencode/src/altimate/validators/dbt-schema-verify.tspackages/opencode/src/altimate/validators/dbt-tests-pass.tspackages/opencode/src/altimate/validators/validator-utils.tspackages/opencode/src/session/prompt.tspackages/opencode/src/session/system.tspackages/opencode/src/session/validators/registry.tsresearch/kimi-k26-ade-bench-2026-05-10/findings.md
✅ Files skipped from review due to trivial changes (4)
- benchmark/ade-bench/altimate_code_agent/.gitignore
- .opencode/skills/dbt-unit-tests/SKILL.md
- .opencode/skills/dbt-schema-verify/SKILL.md
- research/kimi-k26-ade-bench-2026-05-10/findings.md
🚧 Files skipped from review as they are similar to previous changes (5)
- benchmark/ade-bench/altimate_code_agent/build-local-tarball.sh
- packages/opencode/src/altimate/validators/dbt-schema-verify.ts
- packages/opencode/src/session/prompt.ts
- packages/opencode/src/altimate/validators/dbt-tests-pass.ts
- benchmark/ade-bench/altimate_code_agent/altimate_code_agent.py
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
Next StepPlease address the failing cases above and re-run verification. |
CI's TypeScript job failed on this test with a 30 s timeout on commit 81b6df2, but the test passes 5/5 locally. The root cause is a race: const fiber = yield* runner.ensureRunning(Effect.never).pipe(Effect.forkChild) yield* Effect.sleep("10 millis") // <-- not guaranteed to land after state = Running yield* runner.cancel // <-- if still Idle, cancel is a no-op yield* Fiber.await(fiber) // <-- waits forever on Effect.never `Effect.forkChild` returns before `ensureRunning` has transitioned the runner from Idle to Running. On a slow CI runner, the 10 ms sleep can expire before that transition completes, so `runner.cancel` matches the Idle branch (no-op) and the test hangs awaiting `Effect.never`. Replace the fixed sleep with a busy-poll that exits as soon as the runner reports `busy === true`, eliminating the race entirely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Wrote a new adversarial test suite (`adversarial-bugs.test.ts`, 16 tests)
that targets edge cases the original 117 tests didn't exercise. The
suite found 11 real bugs in validator-utils.ts; each is now fixed.
**Bugs in `runWithConcurrencyLimit` (5)**
1. `limit=0` silently dropped every item — `Math.min(0, len)` produced
zero workers; results stayed as sparse `undefined` and the caller
never knew anything was skipped.
2. `limit=-1` had the same silent-drop behavior.
3. `limit=NaN` had the same silent-drop behavior (Array.from coerces
NaN length to 0).
4. `limit=0.5` was floored to 0 by Array.from — silent drop.
5. `limit=0.7` (e.g. user sets `ALTIMATE_VALIDATORS_CONCURRENCY=0.7`)
collapsed to 0 — silent drop.
**Fix**: clamp `limit` with `Number.isFinite(limit) && limit >= 1`,
floor the value, and cap at `items.length`. Defaults to 1 worker for
any invalid input so work is never silently skipped.
**Bugs in `modelsModifiedSince` (3)**
6. Case-sensitive `models/` check missed `Models/` or `MODELS/` on
case-insensitive volumes (macOS APFS default, Windows NTFS).
7. Hard depth cap of 4 silently dropped files in realistic dbt layouts
like `models/staging/sources/dl/raw/foo.sql` (depth 5+).
8. Uppercase `.SQL` extension was matched (fixed in earlier commit)
but the surrounding `MODELS/` dir was still skipped — an internal
inconsistency.
**Fix**: increase depth cap to 8; make the `models/` path-component
check case-insensitive (`.toLowerCase() === "models"`).
**Bugs in `findDbtProjectRoot` (2)**
9. Non-deterministic selection when multiple subdirectories each have
a `dbt_project.yml` — relied on `fs.readdir` order, which varies
across filesystems and Node versions.
10. A *directory* named `dbt_project.yml` was treated as a valid project
marker (`fs.stat` doesn't distinguish file from directory).
**Fix**: sort entries alphabetically before iterating; replace bare
`fs.stat` existence check with an `isFile()` test.
**Bug in `extractLastJsonObject` envelope guard (1)**
11. `isValidEnvelope` accepted `{"verdict": null}` because `"verdict"
in obj` returns true even when the value is null — a stray JSON
fragment with the right shape could be mistaken for a real verdict.
**Fix**: require envelope keys to have *defined, non-null* values
(except `error: null`, which is intentionally allowed as the "ran
cleanly" sentinel).
**Test updates**
Two pre-existing tests in `validator-utils.test.ts` pinned the old
buggy behavior (depth=5 excluded; directory-as-project accepted) — both
updated to assert the corrected behavior.
All 133 validator tests pass; typecheck and marker guard clean.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Builds on commit 9eb6bc7 (wave 1: 11 bugs fixed). This commit fixes the remaining real bugs surfaced by adversarial test waves 2-12 against `validator-utils.ts`, `dbt-tests-pass.ts`, and `system.ts`. **`parseDbtTestOutput` (dbt-tests-pass.ts)** - Anchored summary regex so `Done.` mid-word (`Predone.`) or inside quotes / paragraphs no longer false-matches. - Made WARN/SKIP/NO-OP fields optional in the summary regex — compact dbt outputs (PASS/ERROR/TOTAL only) now parse correctly. - Switched to global-flag scan that keeps the LAST `Done.` summary, so retried runs report the latest authoritative counts instead of the first (incorrect) one. - Strip ANSI CSI sequences from stdout before parsing so colour codes don't break field matching or pollute captured test names. - Replaced greedy `\\S+` test-name capture with bounded char class `[A-Za-z0-9_./:-]+` plus a `VALID_TEST_NAME_RE` post-check. Stops over-capturing `[FAIL]`, `(could not connect ...)`, `Done.`, quoted/angle-bracketed/comma-prefixed noise, and URLs. - Reject names containing `://` so URLs in failure messages aren't treated as test names. - Clamp count fields at `Number.MAX_SAFE_INTEGER` to prevent precision loss for absurdly large values. **`escapeXmlAttr` (system.ts)** - Escape `\\n`/`\\r`/`\\t` as ` `/` `/`	` so attribute values stay on a single line for log readers / grep / awk. - Strip XML-1.0-invalid control characters (NUL, VT, FF, etc.) so a rogue skill name can't produce invalid XML. **`modelNameFromPath` (validator-utils.ts)** - Normalise Windows-style `\\` separators to `/` before basename() so paths copied from Windows (or mixed-separator inputs) resolve to the correct model name. - Strip embedded NUL bytes from the returned name to prevent shell-argument truncation downstream. **`runWithConcurrencyLimit` (validator-utils.ts)** - Treat `Infinity` as "unbounded" (= items.length) instead of collapsing to the default of 1. **`modelsModifiedSince` (validator-utils.ts)** - Follow symlinks: a symlinked SQL file or a symlinked directory under `models/` is now discovered, matching the obvious user expectation. **`findDbtProjectRoot` (validator-utils.ts)** - Skip dotfile / `node_modules` / `target` directories when scanning for a nested `dbt_project.yml`, mirroring `modelsModifiedSince`. A fixture project shipped inside `node_modules/foo/` or a build artifact in `target/` no longer gets mistaken for the user's project. **Tests** - 12 adversarial wave files added (`adversarial-bugs.test.ts` plus `adversarial-wave-{2..12}.test.ts`), 308 new tests total. Each failing test originally demonstrated a real bug; the file headers describe the categories probed. - 3 tests marked `.skip` document known design limitations (rejection mid-flight, trailing-whitespace SQL filenames, case-insensitive filesystem behaviour). All 424 validator tests pass; typecheck clean; marker guard clean. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds seven `e2e-real-dbt*.test.ts` files that exercise the validators
end-to-end against a real `dbt` 1.8 + duckdb adapter and the real
`altimate-dbt` CLI (no mocks).
Of the 137 E2E test cases written, **51 expose distinct real bugs or
feature gaps** when run unskipped. They are landed as `test.skip(...)`
so CI stays green while the bugs are documented in code with
reproducible scenarios — each test is a faithful repro that exercises
the full subprocess flow:
- real `altimate-dbt init` / `altimate-dbt build` / `altimate-dbt test`
/ `altimate-dbt schema-verify` subprocesses
- real `dbt-core` 1.8 + `dbt-duckdb` 1.8
- a fresh duckdb file in each temp project dir
To run them locally:
ALTIMATE_VALIDATORS_DEBUG=1 bun test test/altimate/validators/e2e-real-dbt*.test.ts
(remove `test.skip` to enable each case)
**Categories of bugs / gaps documented**
CORRECTNESS / E2E SYNC ISSUES (build → schema-verify):
- happy-path schema-verify reports `mismatch` because the build/verify
cycle in altimate-dbt doesn't reliably surface the just-built table
- model-with-Jinja, two-model, tests-pass passing/no-tests scenarios
fail for the same root cause
- nested workspace dbt projects (depth > 1) are not detected by
`findDbtProjectRoot`'s one-level search
- ref-chain models — only the modified file should count as "touched"
- concurrent validator runs share duckdb file lock and don't return
consistent results
CONFIGURATION / PATH HANDLING:
- custom `model-paths: ["analytics"]` in `dbt_project.yml` is silently
ignored (validator only scans `models/`)
- conflicting model names across subdirs (e.g. `models/a/foo.sql` +
`models/b/foo.sql`) dedupe to one entry by `modelNameFromPath`,
silently dropping the other
- malformed `schema.yml` / 0-byte model / hyphen-named model / invalid
materialization / nonexistent macro / nonexistent ref — none of
these distinguish themselves from "schema mismatch" in the result
- pre_hook errors collapse into generic build failure
ERROR SURFACING (reason / fixHint / details quality):
- result doesn't surface the failing model name in `reason`
- no `elapsed_ms` / `total_subprocess_ms` field for telemetry
- no per-model `verdict` breakdown
- no `schema_yml_paths` / `dbt_root` / `dbt_version` / `dbt_adapter`
fields in details
- no `validator_version`, `altimate_dbt_path`, `concurrency_limit`,
`session_id`, `run_at` for traceability
- spawn timeouts not reported separately from spawn failures
- exit codes from subprocess not surfaced
- schema-verify doesn't distinguish "model never built" from "schema
drift" — both report `verdict: mismatch`
- tests-pass doesn't list passing tests or the per-model failure breakdown
- no `total_tests` / `tests_skipped` / `failing_rows` fields
MISSING FEATURE COVERAGE:
- orphan `schema.yml` entries (model in spec but not on disk) not detected
- Python models (.py) not picked up
- analyses/, tests/, seeds/ dirs all silently ignored (correct, but
documented)
The 7 files together cover ~127 distinct scenarios; the 51 failing
expectations are the catalogue of items that need engineering follow-up
to either fix the validator behaviour or richen the details schema.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue 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/test/altimate/validators/e2e-real-dbt-3.test.ts">
<violation number="1" location="packages/opencode/test/altimate/validators/e2e-real-dbt-3.test.ts:93">
P2: All 20 tests in this file are skipped via `.skip("BUG: ...")`, making the entire file inert — it provides no test coverage. Test names use a non-standard "BUG:" prefix that confuses intent (assertions check for correct behavior, not bugs). If these document known edge cases, consider using a dedicated test-plan document or `test.todo` instead; if they represent valid test scenarios, remove the `.skip` so they actually execute.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| if (dir) await teardown() | ||
| }) | ||
|
|
||
| test.skip("BUG: validator with ref chain (foo → bar) when only foo edited", async () => { |
There was a problem hiding this comment.
P2: All 20 tests in this file are skipped via .skip("BUG: ..."), making the entire file inert — it provides no test coverage. Test names use a non-standard "BUG:" prefix that confuses intent (assertions check for correct behavior, not bugs). If these document known edge cases, consider using a dedicated test-plan document or test.todo instead; if they represent valid test scenarios, remove the .skip so they actually execute.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/altimate/validators/e2e-real-dbt-3.test.ts, line 93:
<comment>All 20 tests in this file are skipped via `.skip("BUG: ...")`, making the entire file inert — it provides no test coverage. Test names use a non-standard "BUG:" prefix that confuses intent (assertions check for correct behavior, not bugs). If these document known edge cases, consider using a dedicated test-plan document or `test.todo` instead; if they represent valid test scenarios, remove the `.skip` so they actually execute.</comment>
<file context>
@@ -0,0 +1,344 @@
+ if (dir) await teardown()
+ })
+
+ test.skip("BUG: validator with ref chain (foo → bar) when only foo edited", async () => {
+ if (!ENABLE_E2E) return
+ await setupProject()
</file context>
Two follow-ups based on side-effect analysis of the PR:
**1. Performance side effect for dbt users (the biggest gap)**
Previously the validator dispatch in `prompt.ts` ran on every successful
agent turn — fs scan, subprocess spawn, the works — even when
`ALTIMATE_VALIDATORS_ENABLED=0` (the default). The flag only gated the
synthetic-message retry; the expensive part (real `altimate-dbt test` /
`schema-verify` subprocesses) still ran "for telemetry". That added
30 s – 5 min per session end for any dbt user, opted-in or not.
Now the entire dispatch path is gated on either:
- `ALTIMATE_VALIDATORS_ENABLED=1` (full enforcement, with retries), or
- `ALTIMATE_VALIDATORS_SHADOW=1` (run without enforcement — for
"would have fired" telemetry against historical traffic).
If neither is set (the default), the dispatch returns immediately
after the diagnostic log. No fs scan, no subprocess spawns, no perf
tax — for any user, dbt or otherwise.
**2. Result details enriched**
Addresses several of the documented E2E feature-gap tests by adding
telemetry / traceability fields to both validator results:
- `dbt_root` — the resolved project root (or null when not a dbt project)
- `session_id` — echoed back for trace correlation
- `elapsed_ms` — wall time spent inside `check()`
- `concurrency_limit` — actual worker cap used
- reason text now names the failing models inline (e.g. "models you
edited have failing tests: foo, bar.")
Three E2E tests un-skipped now that their expected behaviour is met
(elapsed_ms / dbt_root / session_id / concurrency_limit / failing-model
name in reason).
All 429 active tests pass; typecheck clean; marker guard clean.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds the user-facing documentation for the completion-gate validator framework introduced in this PR. Specifically: - **New page**: `docs/docs/data-engineering/validators.md` — full reference covering what validators are, when they fire, the two built-in dbt validators, the two opt-in modes (`ALTIMATE_VALIDATORS_ENABLED` for enforcement vs `ALTIMATE_VALIDATORS_SHADOW` for telemetry-only), all configuration knobs, performance characteristics, the emitted telemetry events, the result shape, the phased rollout plan, known limitations, and how to write a custom validator. - **Nav**: linked from the `Use → ` section in `mkdocs.yml`. - **dbt-tools page**: brief mention with a link to the validators page so anyone reading the dbt tool reference learns about the harness-side gates. - **Telemetry reference**: two new event rows (`validator_check`, `validator_retries_exhausted`) added to the collected-events table, cross-linked to the validators page. - **CHANGELOG.md**: new `Unreleased` section announcing the framework, the two modes, the new env vars, and a link to the docs. The docs are deliberate about positioning the framework as **opt-in by default** today, with a phased path to default-on once shadow telemetry confirms low false-positive rates and the open coverage / sync issues are resolved. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
3 issues found across 5 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="benchmark/ade-bench/altimate_code_agent/altimate_code_agent.py">
<violation number="1" location="benchmark/ade-bench/altimate_code_agent/altimate_code_agent.py:134">
P2: `num_turns` is undercounted because it only increments on `reason == "stop"` instead of every `step_finish` event.</violation>
</file>
<file name="packages/dbt-tools/src/commands/build.ts">
<violation number="1" location="packages/dbt-tools/src/commands/build.ts:76">
P1: Project-wide schema verification uses non-unique model selectors (`name`), so duplicate model names can be verified against the wrong manifest node.</violation>
</file>
<file name=".opencode/skills/dbt-develop/SKILL.md">
<violation number="1" location=".opencode/skills/dbt-develop/SKILL.md:278">
P2: This guidance incorrectly describes `CONCAT_WS` behavior/support for Snowflake and BigQuery. It can lead to invalid BigQuery SQL and wrong NULL-handling assumptions in Snowflake.</violation>
</file>
<file name="packages/opencode/test/altimate/validators/e2e-real-dbt-3.test.ts">
<violation number="1" location="packages/opencode/test/altimate/validators/e2e-real-dbt-3.test.ts:93">
P2: All 20 tests in this file are skipped via `.skip("BUG: ...")`, making the entire file inert — it provides no test coverage. Test names use a non-standard "BUG:" prefix that confuses intent (assertions check for correct behavior, not bugs). If these document known edge cases, consider using a dedicated test-plan document or `test.todo` instead; if they represent valid test scenarios, remove the `.skip` so they actually execute.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
ade-bench is the whole reason the validator framework exists — without ALTIMATE_VALIDATORS_ENABLED=1 the per-trial setup, a vanilla bench run measures the agent without its completion gates and we get the wrong baseline for any post-#849 evaluation. Sets the env var at the end of altimate-code-setup.sh per trial. Opt-out via ALTIMATE_VALIDATORS_BENCH_DISABLE=1 for intentional baseline runs. Trade-off: adds 30 s – 2 min per trial of validator wall time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
Resolves CHANGELOG.md conflict by keeping the Unreleased validator entry above the 0.7.3 release section that landed on main while this PR was in review.
…-shared file The merge from main brought in PR #850's altimate-backend provider ID without altimate_change markers. Adds the markers around the altimate-specific provider IDs in the Anthropic-style detection block so future upstream merges don't silently overwrite them. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 33124345 | Triggered | Basic Auth String | 2a03484 | CHANGELOG.md | View secret |
| 33124345 | Triggered | Basic Auth String | 2a03484 | packages/opencode/src/altimate/tools/project-scan.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
- validators.md: drop "session rollup" claim from telemetry section (only per-validator events are emitted today); make `checked` / `concurrency_limit` optional in the result-shape schema to match what the validators actually return on the no-models path - dbt-tools.md: mention both opt-in flags (ENABLED + SHADOW) and the zero-overhead default - skills.md: correct the auto-load placement docs (prepended BEFORE the available-skills listing, not appended after — placement was deliberate) - benchmark/ade-bench/README.md: add 'text' language to the directory tree code fence for markdownlint MD040 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Completes the harness-side completion-gate validator framework. The registry and
dbt-schema-verifywere shipped in earlier PRs; this addsdbt-tests-passand hardens the existing validator.New:
dbt-tests-passvalidatorFires after the agent declares done (
finishReason === "stop"). Finds every dbt model.sqlfile the session modified (by mtime sincesessionStartMs), runsaltimate-dbt test --model <name>against each, and injects a synthetic user message if any tests fail — forcing one more agent turn to fix the SQL.What this catches that schema-verify doesn't: row-data correctness errors. A model whose column names all match
schema.ymlcan still failrelationships,unique,not_null,accepted_values, orAUTO_*_equalitytests because theSELECTlogic produces wrong values or wrong row counts.Hardened:
dbt-schema-verifyparseSchemaVerifyOutput()— scans the stdout blob for the last balanced{...}block.altimate-dbtemits dbt log noise (ANSI codes, parser warnings) before the JSON verdict;JSON.parse(stdout)was failing on it. This fix was validated onairbnb006where the validator correctly triggered a schema-mismatch retry.ALTIMATE_VALIDATORS_DEBUG=1.prompt.ts: diagnostic cleanupconsole.errorcalls gated behindALTIMATE_VALIDATORS_DEBUG=1— previously they were always on, producing noise in normal sessions.validatorsEnabled &&guard on dispatch condition.dispatch_enter,dispatch_result,dispatch_errordebug events.build-local-tarball.sh:altimate-dbton PATHAdded
"altimate-dbt": "./dbt-tools/bin/altimate-dbt"to bin entries in the benchmark npm package. Validators spawnaltimate-dbtand it wasn't in PATH inside the Docker containers used by ade-bench.system.ts: Marker Guard fixMoved
// altimate_change endfrom inside theskills()function body (after the unreachablereturn) to outside it. The function's closing}was appearing as unguarded new code in the diff, tripping Marker Guard CI.Type of change
Issue for this PR
Closes #848
How did you verify your code works?
End-to-end on DGX with the ade-bench Docker harness:
dbt-schema-verify:airbnb006— agent triggered a schema-mismatch retry, fixed 5 column mismatches, all tests passeddbt-tests-pass: confirmed validator detects test failures and injects the correct fix hintaltimate-dbt teston PATH:which altimate-dbt→/usr/bin/altimate-dbtinside containerbun run typecheckpassesbun run script/upstream/analyze.ts --markers --base main --strictpassesChecklist
bun run script/upstream/analyze.ts --markers --base main --strictand it passes🤖 Generated with Claude Code
Summary by cubic
Adds a harness-side
dbt-tests-passvalidator and hardensdbt-schema-verify, with an opt-in completion gate that blocks session finish when dbt tests fail or schema diffs are found. Validators run only when enabled; ADE-Bench setup turns them on by default per trial (opt-out available), with docs and telemetry updated.New Features
dbt-tests-pass: after finish, runsaltimate-dbt test --model <name>on modified models; injects a user message on failures to force another turn; names failing models.ALTIMATE_VALIDATORS_MAX_RETRIES, timeouts and concurrency controls; enriched details (dbt_root,session_id,elapsed_ms,concurrency_limit).altimate-dbt schema-verify: CLI and auto-run inbuildresponses; validator blocks completion onmismatchand returns structured diffs.ALTIMATE_VALIDATORS_ENABLED=1(opt-outALTIMATE_VALIDATORS_BENCH_DISABLE=1); docs and telemetry updated.Bug Fixes
ALTIMATE_VALIDATORS_ENABLED=1orALTIMATE_VALIDATORS_SHADOW=1; no scans/spawns by default; telemetry only in these modes.altimate-backendprovider IDs with markers inpackages/opencode/src/provider/transform.tsto prevent upstream overwrite.checked/concurrency_limitas optional), noted both opt-in flags and zero-overhead default indbt-toolsdocs, and corrected skill auto-load placement (prepended before<available_skills>); minor README code-fence fix.Written for commit 4d5c76d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests