chore: upgrade @altimateai/altimate-core to 0.7.0 and sync consumer contracts - #1090
Conversation
… contracts
Engine upgrade 0.5.1 → 0.7.0 (through 0.6.0). Both releases are correctness
releases whose output differs where the old output was wrong: more lineage
edges (derived tables, CTE chains, CTAS/INSERT…SELECT), more PII exposures,
stricter migration verdicts, a new `unbalanced_quote` safety rule, and
validate/transpile/equivalence fixes. One type-surface addition:
`PiiColumnAccess.query_targets`.
Contract sync (consumers still read legacy pre-native-core shapes):
- `altimate-core-migration` tool: read `findings`/`safe`/`overall_risk`
(engine `MigrationResult`) instead of nonexistent `risks` — previously ANY
migration, including `DROP COLUMN`, rendered "Migration: SAFE"; never render
SAFE when the engine call errored; count only non-"safe" findings as risks.
- `altimate_core.migration` handler: coerce empty dialect `"" → undefined`
(`|| undefined`, same as the equivalence handler) so `Schema.fromDdl` does
not throw on the default empty-string dialect.
- `check --checks safety`: read engine `threats[]` (`rule`/`message`/`detail`)
so real threats (e.g. `unbalanced_quote`) render with rule and message
instead of a generic warning.
- `normalizeSeverity`: map engine severities `high → error`, `medium →
warning` — previously both degraded to `info`, so `--fail-on`/`--severity`
silently passed high-risk injections.
- `check --checks pii`: map engine `PiiColumnAccess` (`table`/`column`/
`classification`/`query_targets`/`suggested_masking`); stop assigning the
column NAME to the numeric column-position field; report the exposing alias;
stringify `{ Custom: string }` classifications (also in the query-pii tool
renderer, which printed `[object Object]`).
- `altimate-core-track-lineage` tool: collect edges from `queries[].edges`
(engine `LineageResult`) instead of flat `edges` — previously always
"0 edges"; render `impact_map`; format `{table, column}` refs; render ERROR
instead of "0 edges" when the engine call fails.
- `altimate-core-query-pii` tool: surface new 0.7.0 `query_targets` field as
"Exposed via: …".
- `altimate-core-check` tool: safety renderer and telemetry read
`rule`/`message` with legacy fallback.
Test updates for 0.7.0 behavior:
- Dialect-forwarding fixture: `payload:f` now parses in the default dialect,
so it no longer discriminates; switched to Snowflake time-travel
`AT(OFFSET => -60)` which still does.
- Grade comparison made apples-to-apples (same table, projection-only diff).
- New real-engine tests: `query_targets` contract + rendering, migration
destructive/safe/empty-dialect/error + tool-title regression, track-lineage
edge surfacing + error rendering, check CLI safety `ThreatFinding` (incl.
`high → error` normalization) and PII `PiiColumnAccess` shapes (incl.
`{ Custom }` classification).
- Strengthened previously vacuous migration e2e assertions.
Verification: typecheck clean; test/altimate + test/cli fully green (4750
pass / 0 fail); full-suite failures (MCP/TUI-sound/subprocess) reproduced
identically on 0.5.1 or shown run-to-run flaky with no engine coupling —
pre-existing. Marker check: no upstream-shared files modified. Codex reviewed
twice (found the legacy-shape consumers; verified all fixes non-tautological).
Closes #1089
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates Altimate Core to ChangesEngine contract synchronization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 1
🧹 Nitpick comments (1)
packages/opencode/src/altimate/tools/altimate-core-track-lineage.ts (1)
21-27: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse the flattened edge list.
collectEdges(data)runs at Line 21 and Line 55. This traverses every edge twice and allocates two arrays. Pass the first result toformatTrackLineageto avoid duplicate work for large responses.Proposed refactor
- const edgeCount = collectEdges(data).length + const edges = collectEdges(data) + const edgeCount = edges.length ... - output: error ? `Error: ${error}` : formatTrackLineage(data), + output: error ? `Error: ${error}` : formatTrackLineage(data, edges), ... -function formatTrackLineage(data: Record<string, any>): string { +function formatTrackLineage(data: Record<string, any>, edges: any[]): string { ... - const edges = collectEdges(data)Also applies to: 53-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/tools/altimate-core-track-lineage.ts` around lines 21 - 27, Reuse the flattened edge list created by collectEdges in the surrounding track-lineage execution flow: update formatTrackLineage and its call site to accept and use that existing list, removing the second collectEdges invocation while preserving the current output behavior.
🤖 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/test/cli/check-e2e.test.ts`:
- Around line 596-630: Make the dispatcher-based tests, including the safety
check around setDispatcherResponse and installDispatcherMocks, safe from
concurrent execution by serializing them or using an isolated dispatcher
instance. Ensure Dispatcher.reset() cannot clear or replace handlers used by
another test, and populate and restore savedHandlers when applying the test
setup.
---
Nitpick comments:
In `@packages/opencode/src/altimate/tools/altimate-core-track-lineage.ts`:
- Around line 21-27: Reuse the flattened edge list created by collectEdges in
the surrounding track-lineage execution flow: update formatTrackLineage and its
call site to accept and use that existing list, removing the second collectEdges
invocation while preserving the current output behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d28e9bc-8c65-4703-86d5-a101948b8b32
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
packages/opencode/package.jsonpackages/opencode/src/altimate/native/altimate-core.tspackages/opencode/src/altimate/tools/altimate-core-check.tspackages/opencode/src/altimate/tools/altimate-core-migration.tspackages/opencode/src/altimate/tools/altimate-core-query-pii.tspackages/opencode/src/altimate/tools/altimate-core-track-lineage.tspackages/opencode/src/cli/cmd/check-helpers.tspackages/opencode/src/cli/cmd/check.tspackages/opencode/test/altimate/altimate-core-e2e.test.tspackages/opencode/test/altimate/altimate-core-native.test.tspackages/opencode/test/cli/check-e2e.test.ts
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review (90e2eae..b35057f)Reviewed the incremental commit since the last review. The changes extend the
New real-engine tests are non-tautological: they verify the stale-risk reset, Files Reviewed (3 files)
Previous Review Summaries (13 snapshots, latest commit 90e2eae)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 90e2eae)Status: No Issues Found | Recommendation: Merge Overview
Incremental review (2762415..90e2eae)Reviewed the single incremental commit since the last review. The changes Other fixes: No issues found. Files Reviewed (6 files)
Previous review (commit 2762415)Status: No Issues Found | Recommendation: Merge Overview
Incremental review (d4b1282..2762415)Reviewed the 5-file incremental diff since the last review. The changes add Files Reviewed (5 files)
Previous review (commit d4b1282)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Previous review (commit 494407e)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Incremental review: 7 files (4358bb4 → 494407e)Incremental pass over the post-approval follow-up commit. The correctness changes are sound:
Only one minor redundancy (see inline). No bugs, security, or fail-open regressions in the changed lines. Fix these issues in Kilo Cloud Previous review (commit 4358bb4)Status: No Issues Found | Recommendation: Merge Incremental review from
No redundancy or simplification opportunities in the changed lines — the defensive Files Reviewed (2 files)
Previous review (commit 846cf24)Status: No Issues Found | Recommendation: Merge Incremental review from
No redundancy or simplification opportunities in the changed lines — the defensive casts against the Files Reviewed (3 files)
Previous review (commit 3974e35)Status: No Issues Found | Recommendation: Merge Incremental review of commit
Each change is covered by new real-shape tests in Files Reviewed (5 files)
Previous review (commit 960229d)Status: No Issues Found | Recommendation: Merge Incremental review of commit
The remaining changes are consistent and well-covered by new tests: fail-closed handling of Files Reviewed (12 files)
Previous review (commit a00fb38)Status: 2 Issues Found | Recommendation: Address before merge Incremental review of commit Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous review (commit a03901b)Status: 2 Issues Found | Recommendation: Address before merge Incremental review of commit Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (16 files)
Fix these issues in Kilo Cloud Previous review (commit e3db0a4)The review did not run because the selected model is no longer available. Choose another model in Kilo Code review settings: https://app.kilo.ai/code-reviews Previous review (commit e3db0a4)Status: No Issues Found | Recommendation: Merge Incremental review of commit No new issues found in the changed code. Files Reviewed (2 files)
Previous review (commit 6b16d7f)Status: 1 Issue Found | Recommendation: Address before merge Fix these issues in Kilo Cloud Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (12 files)
Reviewed by glm-5.2 · Input: 77.6K · Output: 9.3K · Cached: 304.4K Review guidance: REVIEW.md from base branch |
`f.suggestion ?? f.suggested_masking` leaked `null` into the `suggestion` field when the engine emits `suggested_masking: null`. Coerce to `undefined` and lock with a test. (Kilo review follow-up on #1090.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round-2 contract sync for PR #1090, fixing every Critical/Major finding from the 4-model consensus review (all verified against the live 0.7.0 binary; all latent since the Python-engine elimination, not 0.7.0 breakage): Dead gates (silent false-passes): - `check --checks validate`: gated on envelope `success`, which the handler sets even for invalid SQL — every file passed. Now gates on `data.valid`, maps `ValidationError` (`location.line/column`, `suggestions`) and fails closed on engine failure. - `check --checks semantic` + `altimate_core_semantics` tool: engine returns `valid:true` WITH `findings` (cartesian product) — `valid` means "plannable", not "clean". Both consumers now read `findings` and never gate on `valid`; tool formatter/title/telemetry follow the findings list. - `check --checks grade`: read `grade`/`score`/`recommendations`, none of which `evaluate()` returns — no grade or finding ever surfaced. Now reads `overall_grade`/`scores.overall`/`lint.findings` and fails closed on the failure envelope. The check-e2e mock enshrined the fictional shape — fixed. - `schema.detect_pii` (pii-detector): read `piiData.findings`; engine `PiiReport` is `{ columns, pii_count, … }` — schema PII scanning returned zero findings for every scan. Shared `piiColumnsFromReport` now filters `classification !== "None"` on both the cache and live paths. Wrong shapes / crashes: - `altimate-core-compare` tool: read `differences` (engine: `identical`/ `diff_count`/`diffs`, `DiffEntry.change_type`) — different queries rendered "Compare: IDENTICAL". Error-gated title added. - `altimate-core-policy` tool: titled on `pass` (engine: `allowed`) — clean SQL always rendered "VIOLATIONS FOUND"; `metadata.success` now reflects the envelope and error output no longer contradicts the ERROR title. - Empty-dialect coercion centralized: new `dialectHint()` in `native/engine-coerce.ts` applied to all 7 dialect-forwarding handlers (columnLineage, formatSql, extractMetadata, compareQueries, importDdl + the two already fixed) — the compare/column-lineage/extract-metadata/ import-ddl tools crashed with `unknown dialect ''` whenever `dialect` was omitted (the common invocation path). - `{ Custom: string }` PII classifications rendered `[object Object]` in classify-pii, the composite check renderer, and the review runner's signed verdict — shared `classificationToString()` used everywhere; classify-pii also no longer counts `classification: "None"` rows and error-gates its title/output; query-pii treats engine `parse_error` as an abstention (previously rendered CLEAN for unparseable SQL). - Composite `altimate_core.check` now computes query PII (fail-safe) — the tool's "=== PII ===" section previously always printed "No PII detected" because the handler never populated it. - `ThreatFinding.location` is `[byteOffset, byteLength]` — surfaced as a `(chars a-b)` range in `check --checks safety` (was dropped entirely). Tests: real-engine "consumer contract sync (round 2)" block (semantics valid-gate, compare, policy pass/violation, classify-pii None-filter, no-dialect tool invocations, query-pii/classify-pii/policy error gating, composite-check PII, grade contract); CLI-shape tests for validate/semantic/ grade incl. fail-closed cases; pii-detector unit tests against the live engine + a DuckDB-gated e2e; legacy formatter tests updated to real shapes. Also filed upstream: stale `SafetyRule` union in the engine's index.d.ts (altimate-core-internal#764). Codex-verified twice: all shape corrections match the installed 0.7.0 engine; its five review findings (parse_error abstention, grade fail-closed, byte-length location, contradictory failure outputs, untracked files) are addressed in this commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00d0a02467
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 16 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…polish
- `review/runner.ts`: the composite check's `data.pii` is now the engine
`PiiQueryResult` object — extract columns from `pii_columns` (legacy array
shape kept as fallback) so check-derived PII columns reach the signed
review verdict.
- Composite check renderer: surface `query_targets` ("exposed via: …") and
honor the engine's `parse_error` abstention ("PII check skipped: …")
instead of rendering "No PII detected" for unparseable SQL.
- `pii-detector`: band numeric engine confidence (0..1) to
`high`/`medium`/`low` via shared `bandConfidence` — `PiiFinding.confidence`
is a string field.
- `check --checks validate`: normalize string-shaped `suggestions` entries as
well as `Suggestion` objects.
- Safety location label: `bytes a-b` (engine offsets are byte-based and
diverge from char indexes on multibyte SQL).
- Semantics tool metadata reports `result.success` instead of hardcoding
true (consistent with the policy/compare tools).
- `classify-pii` tool reuses `piiColumnsFromReport` instead of a second
None-filter implementation.
- Tests: classify-pii error case made deterministic (malformed schema file —
previously guarded by an `if`, so it could pass vacuously); composite-check
alias + parse-error abstention assertions added; bytes label updated.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a03901b841
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/opencode/src/altimate/tools/altimate-core-classify-pii.ts (2)
58-59: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNormalize confidence with
bandConfidence.The engine can return a numeric confidence. This line then renders text such as "0.85 confidence", while
pii-detector.tsreports "high"/"medium"/"low" for the same engine field. ReusebandConfidencefrom../native/engine-coerceto keep one presentation.♻️ Proposed change
-import { classificationToString } from "../native/engine-coerce" +import { bandConfidence, classificationToString } from "../native/engine-coerce"- const confidence = f.confidence ?? "high" + const confidence = bandConfidence(f.confidence)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/tools/altimate-core-classify-pii.ts` around lines 58 - 59, Update the confidence assignment in the classification flow to import and reuse bandConfidence from ../native/engine-coerce, converting f.confidence into the shared high/medium/low band while preserving the existing high fallback when confidence is absent.
4-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving
piiColumnsFromReportintoengine-coerce.ts.
pii-detector.tsalso pulls in the schema cache and the connector registry. Importing it from a tool module widens the tool's dependency graph for one pure coercion function.engine-coerce.tsis the existing home for engine shape normalization, and this file already imports it.If you move the helper, keep a re-export from
packages/opencode/src/altimate/native/schema/pii-detector.tssopackages/opencode/test/altimate/pii-detector-e2e.test.tskeeps its current import path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/tools/altimate-core-classify-pii.ts` around lines 4 - 12, Move the pure piiColumnsFromReport helper into engine-coerce.ts alongside the existing engine shape normalization utilities, and update realPiiColumns to use that location. Preserve a re-export from pii-detector.ts so the existing pii-detector-e2e.test.ts import path remains valid.packages/opencode/src/altimate/tools/altimate-core-policy.ts (1)
32-36: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider a fallback verdict when
allowedis absent.If a future engine result omits both
allowedandpasswhile the call succeeds,allowedisundefinedand the title renders "VIOLATIONS FOUND" with an empty violations list. That reproduces the false-positive gate this PR fixes. Deriving the verdict from the violations count when the flag is missing keeps the output consistent with the body.♻️ Proposed defensive fallback
- const allowed = (data.allowed ?? data.pass) as boolean | undefined + const allowed = (data.allowed ?? data.pass ?? violations.length === 0) as boolean🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/tools/altimate-core-policy.ts` around lines 32 - 36, Update the verdict logic in the policy result handling to fall back to the violations count when both data.allowed and data.pass are absent, treating an empty violations list as passing. Preserve the existing explicit allowed/pass values and error title behavior, and ensure the title remains consistent with the rendered violations body.packages/opencode/src/altimate/native/schema/pii-detector.ts (1)
16-27: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd an
Array.isArrayguard oncolumns.If the engine returns a non-array
columnsvalue (for example an object keyed by column name),columns.filterthrows aTypeErrorinsidedetectPii, and the surroundingcatchswallows the scan silently. The consumer inaltimate-core-classify-pii.tsalready checksArray.isArray(data.columns)before calling this helper, so the check belongs here for consistency.♻️ Proposed guard
export function piiColumnsFromReport(piiData: unknown): Array<Record<string, any>> { - const columns = ((piiData as Record<string, any>)?.columns ?? []) as Array<Record<string, any>> - return columns.filter((c) => c.classification !== "None") + const columns = (piiData as Record<string, any>)?.columns + if (!Array.isArray(columns)) return [] + return (columns as Array<Record<string, any>>).filter((c) => c?.classification !== "None") }🤖 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/native/schema/pii-detector.ts` around lines 16 - 27, Update piiColumnsFromReport to validate that piiData.columns is an array before filtering; fall back to an empty array for missing or non-array values, while preserving the existing classification filter for valid arrays.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/altimate/tools/altimate-core-check.ts`:
- Line 66: Update the PII status logic in formatCheck so data.pii.parse_error
takes precedence over clean pii_columns and findings, returning an explicit
skipped or error title state instead of PASS. Preserve the existing PII detected
behavior when findings or columns are present and the normal PASS result when
the check completes cleanly.
In `@packages/opencode/test/altimate/altimate-core-e2e.test.ts`:
- Around line 1186-1191: In the PII classification test, add an assertion that
the computed piiCount is greater than zero before comparing it with
result.metadata.finding_count. Keep the existing equality and “: None” output
assertions unchanged so the test verifies a non-empty PII result and the
filtering behavior.
In `@packages/opencode/test/altimate/pii-detector-e2e.test.ts`:
- Around line 50-57: Update the test teardown in afterAll to close the
duck_pii_e2e connector before calling Registry.reset(), and preserve and restore
the prior ALTIMATE_TELEMETRY_DISABLED environment value rather than
unconditionally deleting it.
---
Nitpick comments:
In `@packages/opencode/src/altimate/native/schema/pii-detector.ts`:
- Around line 16-27: Update piiColumnsFromReport to validate that
piiData.columns is an array before filtering; fall back to an empty array for
missing or non-array values, while preserving the existing classification filter
for valid arrays.
In `@packages/opencode/src/altimate/tools/altimate-core-classify-pii.ts`:
- Around line 58-59: Update the confidence assignment in the classification flow
to import and reuse bandConfidence from ../native/engine-coerce, converting
f.confidence into the shared high/medium/low band while preserving the existing
high fallback when confidence is absent.
- Around line 4-12: Move the pure piiColumnsFromReport helper into
engine-coerce.ts alongside the existing engine shape normalization utilities,
and update realPiiColumns to use that location. Preserve a re-export from
pii-detector.ts so the existing pii-detector-e2e.test.ts import path remains
valid.
In `@packages/opencode/src/altimate/tools/altimate-core-policy.ts`:
- Around line 32-36: Update the verdict logic in the policy result handling to
fall back to the violations count when both data.allowed and data.pass are
absent, treating an empty violations list as passing. Preserve the existing
explicit allowed/pass values and error title behavior, and ensure the title
remains consistent with the rendered violations body.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5495fa1e-a85f-4b9f-8b58-c30f65d554c6
📒 Files selected for processing (16)
packages/opencode/src/altimate/native/altimate-core.tspackages/opencode/src/altimate/native/engine-coerce.tspackages/opencode/src/altimate/native/schema/pii-detector.tspackages/opencode/src/altimate/review/runner.tspackages/opencode/src/altimate/tools/altimate-core-check.tspackages/opencode/src/altimate/tools/altimate-core-classify-pii.tspackages/opencode/src/altimate/tools/altimate-core-compare.tspackages/opencode/src/altimate/tools/altimate-core-policy.tspackages/opencode/src/altimate/tools/altimate-core-query-pii.tspackages/opencode/src/altimate/tools/altimate-core-semantics.tspackages/opencode/src/cli/cmd/check.tspackages/opencode/test/altimate/altimate-core-check-formatters.test.tspackages/opencode/test/altimate/altimate-core-e2e.test.tspackages/opencode/test/altimate/altimate-core-semantics-formatters.test.tspackages/opencode/test/altimate/pii-detector-e2e.test.tspackages/opencode/test/cli/check-e2e.test.ts
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- Move `piiColumnsFromReport` into `native/engine-coerce.ts` (re-exported from `pii-detector` for compatibility) so `altimate-core-classify-pii` no longer pulls the native NAPI binding eagerly at tool-registry load time. - `bandConfidence`: missing confidence maps to "medium" (unknown), restoring the previous default instead of degrading to "low". - classify-pii error test: temp dir created outside `try` but cleaned in `finally` regardless of setup failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/altimate/native/engine-coerce.ts`:
- Around line 39-50: Update piiColumnsFromReport to validate that
piiData.columns exists as an array and that every row has the expected object
shape and classification field; return an explicit invalid-report result instead
of defaulting or silently filtering malformed data. In both cached and live
detection paths, propagate invalid reports as success: false while preserving
normal filtering of classification "None" for valid reports. Update the
malformed-report test and add coverage for missing columns, non-array columns,
and malformed rows.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 53f7c4f5-8e6a-41f1-92a9-382f2c1d9cf6
📒 Files selected for processing (4)
packages/opencode/src/altimate/native/engine-coerce.tspackages/opencode/src/altimate/native/schema/pii-detector.tspackages/opencode/src/altimate/tools/altimate-core-classify-pii.tspackages/opencode/test/altimate/altimate-core-e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/opencode/src/altimate/native/schema/pii-detector.ts
- packages/opencode/src/altimate/tools/altimate-core-classify-pii.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a00fb38668
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Abstention and fail-closed hardening:
- `check --checks pii`: engine `parse_error` abstentions now produce an
error finding ("PII analysis skipped: …") instead of an empty pass —
`--fail-on` no longer PASSes files whose PII analysis never ran.
- Composite `altimate_core.check`: a thrown `checkQueryPii` now marks the
PII section as an abstention (`parse_error`) instead of leaving `{}`,
which rendered a false-clean "No PII detected"; `formatCheckTitle`
reports "PII check skipped" instead of PASS for abstained-but-clean runs.
- `schema.detect_pii`: per-column classify failures are counted and flip
`success` to false (fail closed) instead of being silently swallowed;
`piiColumnsFromReport` now throws on malformed reports (missing/non-array
`columns`) rather than yielding zero findings.
- `altimate_core_query_pii` tool: abstentions set `metadata.success: false`
so telemetry's soft-failure classification records them.
Correctness:
- `check --checks grade`: per-file grades (`results.grade.grades`) — the
shared `gradeValue`/`gradeScore` raced across concurrent batch promises,
keeping whichever file finished last; flat `grade`/`score` retained for
single-file runs only.
Module shape / dedup:
- `engine-coerce.ts` gets the AGENTS.md-prescribed `EngineCoerce`
self-reexport; all consumers import the namespace projection.
- `review/runner.ts` drops its local `bandConfidence` for the shared one;
shared version now bands missing/non-numeric confidence as "medium"
(matching the runner's previous behavior).
Tests: CLI PII-abstention and multi-file grade regression tests;
"PII check skipped" title test; strict malformed-PiiReport assertions;
classify-pii e2e asserts a positive `pii_count` lower bound; DuckDB e2e
closes its connector and restores the telemetry env var on teardown.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/altimate/native/engine-coerce.ts (1)
29-42: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winApply
EngineCoerce.bandConfidenceto PII columns output.When
data.columnsis present, mapf.confidencethroughEngineCoerce.bandConfidence. The current fallback displays missing confidence as"high"and numeric values as raw numbers. Keep the"high"fallback for legacyfindings.🤖 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/native/engine-coerce.ts` around lines 29 - 42, Update the PII columns output mapping to pass each column finding’s f.confidence through EngineCoerce.bandConfidence, replacing the raw numeric/missing-confidence handling so missing column confidence uses the mapper’s medium default. Preserve the existing high fallback behavior for legacy findings.
🤖 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/cli/cmd/check.ts`:
- Around line 530-534: In the flat grade-field emission block around graded,
check the invocation’s input file count rather than graded.length. Emit
results.grade.grade and results.grade.score only when exactly one input file was
provided, while preserving grades for multi-file invocations even when only one
file has grade metadata.
---
Outside diff comments:
In `@packages/opencode/src/altimate/native/engine-coerce.ts`:
- Around line 29-42: Update the PII columns output mapping to pass each column
finding’s f.confidence through EngineCoerce.bandConfidence, replacing the raw
numeric/missing-confidence handling so missing column confidence uses the
mapper’s medium default. Preserve the existing high fallback behavior for legacy
findings.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d1c84d2f-e537-43c8-8bc4-13d38b77f93d
📒 Files selected for processing (12)
packages/opencode/src/altimate/native/altimate-core.tspackages/opencode/src/altimate/native/engine-coerce.tspackages/opencode/src/altimate/native/schema/pii-detector.tspackages/opencode/src/altimate/review/runner.tspackages/opencode/src/altimate/tools/altimate-core-check.tspackages/opencode/src/altimate/tools/altimate-core-classify-pii.tspackages/opencode/src/altimate/tools/altimate-core-query-pii.tspackages/opencode/src/cli/cmd/check.tspackages/opencode/test/altimate/altimate-core-check-formatters.test.tspackages/opencode/test/altimate/altimate-core-e2e.test.tspackages/opencode/test/altimate/pii-detector-e2e.test.tspackages/opencode/test/cli/check-e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/opencode/src/altimate/review/runner.ts
- packages/opencode/src/altimate/tools/altimate-core-classify-pii.ts
- packages/opencode/test/cli/check-e2e.test.ts
- packages/opencode/test/altimate/pii-detector-e2e.test.ts
- packages/opencode/test/altimate/altimate-core-check-formatters.test.ts
- packages/opencode/src/altimate/native/schema/pii-detector.ts
- packages/opencode/src/altimate/tools/altimate-core-query-pii.ts
- packages/opencode/src/altimate/tools/altimate-core-check.ts
- packages/opencode/test/altimate/altimate-core-e2e.test.ts
- packages/opencode/src/altimate/native/altimate-core.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 960229d4ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
All reported issues were addressed across 12 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…urface - `schema_detect_pii` tool: honor `success: false` from `detectPii` — a scan that failed for any column now renders "PII Scan: ERROR" (with any partial findings attached) instead of a clean "no findings" verdict. The previous round made the detector fail closed but left its only user-facing consumer branching on `finding_count` alone. (cursor High / cubic P1 / Codex P2) - `check --checks grade`: flat `grade`/`score` gated on the invocation file count, not surviving-grade count — with several files where all but one grade call failed, the flat fields would have misattributed the survivor. (CodeRabbit Major) - `altimate_core_policy` tool: `allowed: true` with non-empty `warnings` now renders the warnings with the pass instead of hiding them behind the early "passes all policy checks" return. (Codex P2) - `review/runner.ts`: PII column extraction includes `query_targets` output aliases (e.g. `SELECT email AS contact` exposes `contact`), not just source columns. (Codex P2) - Tests: fail-closed detect-pii tool cases (zero and partial findings), policy warnings-on-pass case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- `review/runner.ts`: when a PII column has `query_targets`, report ONLY the output aliases — the source column name is not present in the model output, so including it flagged a column that isn't there (`SELECT email AS contact` now reports `contact`, not `email` + `contact`). - `check --checks grade`: findings now come from all nested EvalResult sections (lint.findings + validation.errors + safety.threats) — a failing grade with clean lint no longer yields an empty, passing finding list. - Regression test for the nested-findings case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 846cf24bc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
`runGrade`'s nested `validation.errors`/`safety.threats` went through the
shared flat mapper, dropping the line numbers and fixes that `runValidate`/
`runSafety` already surface. Normalize each shape first: `ValidationError`
hoists `location.{line,column}` and the first `suggestions[]` entry;
`ThreatFinding` renders its `[byteOffset, byteLength]` range into the message
and maps `detail` to the suggestion. Regression assertions added.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4358bb4d46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Approved — with a few optional follow-ups
Nice work on this one. I checked every claimed engine shape against the published @altimateai/altimate-core@0.7.0 index.d.ts rather than against the inline comments, and the contract sync is accurate throughout: CompareResult, PolicyResult.allowed, EvalResult.overall_grade/scores.overall, SemanticResult.findings, LineageResult.queries[].edges, MigrationResult.findings[].risk, PiiReport/PiiColumnAccess, ThreatFinding.location, and ValidationError.location/suggestions all match. EquivalenceResult.differences genuinely still exists, so leaving those consumers alone was right. Version pinning is consistent across package.json and every bun.lock entry including the five platform optionals.
Two things worth calling out as done well: fixing runValidate's dead gate (result.success alone was making check --checks validate pass every file) is the highest-value correctness fix here, and the error-vs-verdict separation applied across the seven tools — never rendering SAFE/CLEAN/IDENTICAL/PASS on a failed engine call — is consistent and each case has a test.
Note on scope: this review was started against
a00fb38668. Several findings were fixed by the commits that landed since (960229d4ac,3974e35c93,846cf24bc8,4358bb4d46) and have been redacted rather than reported — specifically the composite-check PII abstention, the policyallowed: truesuppression, the duplicatebandConfidence, the unguardedpiiColumnsFromReport, the PII scan reporting success after swallowing failures, and the multi-file grade race. Everything below was re-verified against4358bb4d46.
Nothing outstanding blocks merge. The items below are follow-ups, split by whether they came in with this PR.
Introduced by this PR
1. classificationToString fallbacks are inconsistent across call sites — the default is "PII" (altimate-core-check.ts:112, altimate-core-query-pii.ts:57, altimate-core-classify-pii.ts:57), "UNKNOWN" in pii-detector.ts:90,186, and "" in runner.ts:403. An unrecognized classification object therefore renders as a positive PII assertion in tool output and as an empty string in the review runner. "UNKNOWN" seems like the honest convention for all three.
2. pii-detector-e2e.test.ts fails hard where the other core tests skip — altimate-core-e2e.test.ts:19-24 guards on require.resolve("@altimateai/altimate-core") and uses describe.skip when the NAPI binary is absent. The new file imports pii-detector (which imports the package at module scope) and calls require("@altimateai/altimate-core") inside an ungated test, so without the binary it throws at import instead of skipping. Worth adding the same guard.
Related: the only test of detectPii itself is the DuckDB path behind ALTIMATE_RUN_WAREHOUSE_E2E === "1", so the pii-detector rewrite has no test that runs by default. A non-live unit test over the schema path would cover the biggest behavioural change here.
3. unbalanced_quote isn't a rule the 0.7.0 engine can emit — check-e2e.test.ts:610. SafetyRule is a closed union of ten values and this isn't one of them. The severity mapping and byte-range assertions in that test are genuine and valuable; just the rule name in the fixture isn't engine-faithful. tautology_attack would keep it real.
Pre-existing — not from this PR, but adjacent enough to be worth a follow-up
4. Policy remediation and warnings[] never reach output — PolicyViolation.remediation is not read anywhere, and runPolicy in check.ts maps f.suggestion, which PolicyViolation doesn't have, so every policy finding loses its fix hint. PolicyResult.warnings[] is likewise dropped by the CLI. formatPolicy now surfaces warnings on the pass path, so only the CLI side is left:
suggestion: (f.remediation ?? f.suggestion) as string | undefined5. Two direct engine calls still forward a raw empty dialect — sql/register.ts:186 (core.formatSql(params.sql, params.dialect)) and :465 (core.columnLineage(params.sql, params.dialect ?? undefined, …)). ?? doesn't coerce "", only || does, and siblings at :364/:433/:434 already use || undefined. Since ReviewConfig.dialect defaults to "", these are reachable and will throw unknown dialect ''. They fail closed rather than returning a wrong answer, so it's low urgency — but EngineCoerce.dialectHint would finish the job.
6. The review runner drops composite validation.errors and safety.threats — runner.ts:206-209 concatenates lint.findings, issues, violations, and findings, but the comment just above says validation failures surface via data.validation.errors, and nothing reads that or data.safety.threats. So validation errors and injection threats from the composite check don't reach the review output. Since this PR wired PII into the same block, it's a natural place to pick up the other two.
7. runSafety's dispatcher-failure path emits warning instead of error — every other check routes engine failures through dispatcherErrorFinding, which uses severity: "error" with the stated rationale "so CI doesn't false-pass". The safety fallback hardcodes "warning", so a safety-engine crash still passes --fail-on error.
8. runner.ts:293 reads the legacy grade field first — data.grade ?? data.overall_grade. Harmless today since data.grade is always undefined in 0.7.0, but it's the reverse of the ordering check.ts now uses.
Nits
runSemantic'svalid === falsefallback emits a bare "Semantic check found issues" and dropsSemanticResult.validation_errors, which carries the detail.extractSemanticsErrorsalready does this on the tool side.runSafetystill mapsline,column, andcode, none of which exist onThreatFinding— they're alwaysundefined.- The byte range prints an exclusive end as if inclusive:
bytes 37-44for[37, 7]actually covers 37–43. Worth noting this formula is now in two places, since the nested-findings mapper inrunGradecopied it. normalizeSeverityleaveslow→infountouched while explicitly reasoning abouthighandmedium. Display is unaffected (--severitydefaults toinfo), but a low-severity safety threat never trips--fail-on warning. Probably intended — a one-line comment would settle it.runPiidropping the numericcolumnfield is correct (the engine'scolumnis a name, not a position), but the name now lives only inside the message string, so JSON consumers can't key on it. AcolumnNamefield would help.
All items from the approving human review, except where noted in the PR reply: Introduced by this PR: - `classificationToString` default fallback unified to "UNKNOWN" across all call sites (was "PII"/"UNKNOWN"/"" depending on caller — an unrecognized classification no longer renders as a positive PII assertion). - `pii-detector-e2e.test.ts` gets the same NAPI-availability guard as `altimate-core-e2e.test.ts` (skip, not crash, when the binary is absent). - The `unbalanced_quote` check-e2e fixture SQL is now engine-faithful (a dangling quote, which the live 0.7.0 runtime genuinely flags with that rule — only the stale `SafetyRule` union omits it; upstream issue #764). Adjacent pre-existing: - `check --checks policy`: violations map `remediation` into the suggestion. - `sql.format` / `sql.column_lineage` handlers: last two raw dialect forwards coerced via `EngineCoerce.dialectHint`. - Review runner: composite `validation.errors` and `safety.threats` now flow into review issues alongside lint findings. - `runSafety` dispatcher-failure fallback fails closed (error severity via `dispatcherErrorFinding`, matching every other check). - Runner grade reads `overall_grade` before the legacy `grade`. Nits: - `runSemantic`'s valid:false fallback surfaces `validation_errors` detail. - Dead `line`/`column`/`code` reads dropped from the threats mapper. - Byte ranges render the INCLUSIVE end (`[37,7]` → `bytes 37-43`) in both places; tests updated. - `normalizeSeverity` documents the intentional low→info mapping. - PII findings carry a machine-readable `columnName` field. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@sahrizvi Thanks for the thorough review — especially for verifying against the published Fixed (introduced by this PR):
Fixed (pre-existing follow-ups): Nits: all taken — Full suite still green: 4782 pass / 0 fail, typecheck + marker check clean. |
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 494407e9ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…il-closed partials - Review runner: composite `safety.threats` are normalized before joining review issues — engine severities (`critical`/`high` → error, `medium` → warning, `low` → info) and the byte-tuple `location` dropped, so the quality lane classifies threats as errors instead of suggestions. - `check --checks safety`: a `success:false` envelope now appends the fail-closed error finding even when partial sub-error threats were returned — `--fail-on error` can no longer pass a crashed scanner. - Semantics tool: abstentions (`validation_errors`) set `metadata.success: false` so telemetry records the soft failure (consistent with query-pii); contract test updated accordingly. - Shared `byteRange()` helper replaces the duplicated inclusive-range formula in `runSafety`/`runGrade`. - `pii-detector-e2e.test.ts` defers the pii-detector import until after the NAPI-availability guard (static import defeated the guard). - Fixture cleanup: stale `matched_pattern`/comment from the old OR 1=1 payload removed; new fail-closed partial-threats test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4b12827d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… safety, CLI policy warnings - Review runner: validation errors from the composite check are surfaced ONLY when a real schema exists — in lint-only mode the throwaway `_altimate_lint_` schema marks every real table unknown and would flood the review (restores the documented intent of that mode). - Composite `altimate_core.check`: safety threats are diff-scoped like lint when `base_sql` is supplied — threats already present in the base (matched by rule + matched_pattern) are pre-existing, not introduced by the change. Real-engine regression test added. - `check --checks policy`: advisory `warnings[]` on an allowed result now surface as info findings (they do not fail the check). Test added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27624151eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…schema bug - Safety diff-scoping is now a MULTISET subtraction (one base occurrence consumes one head occurrence of the same rule + matched_pattern) — a PR that adds a second identical injection still reports it; `safe` and `risk_score` are recomputed when every threat is pre-existing so the verdict doesn't stay stale-unsafe. - PII exposures are diff-scoped the same way: (table, column) pairs already exposed by the base are not reported as introduced. - Validation is deliberately NOT diff-scoped: the engine validates fail-fast (only the first error is reported), so base subtraction can hide genuinely new breakage behind a pre-existing error — proven by test; re-reporting a pre-existing error is the safe mode, documented inline. - Pre-existing bug found while testing: the composite check with `base_sql` + a flat agent schema_context crashed with "missing field tables" — lintDiff takes SchemaDefinition JSON; now normalized via `normalizeSchemaContext` (exported from schema-resolver). This made every diff-scoped composite call with a flat schema fail closed since the lintDiff wiring landed. - Semantic CLI abstention severity: error when a schema was provided (the analysis should have run), warning schema-less (abstains routinely). - Policy tool telemetry includes advisory warnings (`policy_warning`). - `pii-detector` gets the AGENTS.md namespace self-reexport. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 90e2eae. Configure here.
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90e2eaebd4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- PII diff-scoping identity now includes the sorted `query_targets` — adding or renaming a SELECT-list alias for an already-exposed column is a NEW output exposure and surfaces; identical exposures stay filtered. Regression test covers rename vs identical. - `risk_score` is recomputed (documented severity-based approximation) when only SOME threats are pre-existing, instead of keeping the full head scan's score; `risk_level` resets to "None" when every PII exposure was pre-existing. - `schema-resolver` gets the AGENTS.md `SchemaResolver` self-reexport; the new `normalizeSchemaContext` consumer uses the namespace projection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b35057fbdb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2 participants
Issue for this PR
Closes #1089
Type of change
What does this PR do?
Upgrades
@altimateai/altimate-core0.5.1 → 0.7.0 and syncs every consumer to the real engine output shapes.The 0.6.0/0.7.0 releases are correctness releases: more lineage edges (derived tables, CTE chains, CTAS/
INSERT…SELECT), more PII exposures, stricter migration verdicts, a newunbalanced_quoteinjection rule, and validate/transpile/equivalence fixes. The only type-surface change is a new requiredPiiColumnAccess.query_targets: string[](the SELECT-list aliases exposing a PII column).Integrating the new engine surfaced consumers still reading legacy (pre-native-core) shapes, all fixed here:
data.risks, which the engine never returns (findings/safe/overall_risk), so evenALTER TABLE … DROP COLUMNrendered "Migration: SAFE". An empty dialect string also crashedSchema.fromDdland still rendered SAFE. Now reads the realMigrationResultshape, coerces"" → undefined(same pattern as the equivalence handler), and never renders SAFE on an engine error.altimate check --checks safetyignoreddata.threats, collapsing realThreatFindings into one generic warning;normalizeSeverityalso degradedhigh/mediumtoinfo, so--fail-on warningsilently passed high-risk injections. Now mapsrule/message/detailandhigh → error,medium → warning.altimate check --checks piireadcolumn_name/pii_type(emitting "PII detected: unknown") and stuffed the column name into the numeric column-position field. Now mapsPiiColumnAccess, reports the exposing alias fromquery_targets, and stringifies{ Custom: string }classifications (the query-pii tool renderer printed[object Object]for those).data.edgesbut the engine returnsqueries[].edges+impact_map. Also renders ERROR instead of "0 edges" when the engine call fails.payload:f, which 0.7.0's default dialect now parses, so it stopped discriminating — switched to Snowflake time-travelAT(OFFSET => -60), which still does. The grade star-vs-explicit comparison compared different queries (WHERE + different tables); made projection-only.Why these work: every mapping was written against the installed engine's
.d.tsand verified by executing the real napi binary (not mocks) in the new tests.How did you verify your code works?
bun run typecheckclean;bun test test/altimate test/cli: 4750 pass / 0 fail (includes ~4700 real-engine tests).query_targetscontract + rendering, migration destructive/safe/empty-dialect/engine-error + tool-title, track-lineage edge surfacing + error rendering, check CLIThreatFinding+PiiColumnAccessshapes (incl.{ Custom }andhigh → errornormalization). Previously vacuous migration e2e assertions (toBeDefined()) strengthened.altimate checkon a SQL file against the 0.7.0 engine; injection-breakout payload now flagged byisSafe; BigQuery equivalence decidable.analyze.ts --markers --base main --strict— no upstream-shared files modified.Screenshots / recordings
Not a UI change.
Round 2 — consensus-review fixes (00d0a02)
A 4-model consensus review of this PR found the contract sync incomplete: the same legacy-shape bug class survived in consumers this PR had not touched — including two silently-dead CI gates (
check --checks validatepassed every invalid file;check --checks semanticand the semantics tool hid findings behind thevalidflag, which means "plannable", not "clean"), a dead grade check reading fieldsevaluate()never returns, schema-level PII detection readingfindingswhere the engine returnscolumns(zero findings ever), a compare tool that rendered IDENTICAL for different queries, a policy tool that always rendered VIOLATIONS FOUND, four tools crashing onunknown dialect ''when dialect was omitted, and{ Custom }PII classifications rendering[object Object](including in the signed review verdict). All are latent since the Python-engine elimination — the 0.5.1↔0.7.0 type contracts are byte-identical exceptquery_targets(verified by diffing both npm tarballs). All fixed in00d0a02467with shareddialectHint/classificationToStringhelpers, fail-closed gates, and ~25 new real-engine + CLI-shape regression tests. Codex verified each fix against the installed binary; its five review findings (parse_error abstention, grade fail-closed,[byteOffset, byteLength]location semantics, contradictory failure outputs, untracked files) are addressed. Upstream issue filed for the staleSafetyRuleunion: altimate-core-internal#764.Checklist
🤖 Generated with Claude Code
Note
Medium Risk
Touches CI gates (validate, safety, PII, semantic, grade) and PR review check composition; incorrect mapping could still false-pass or over-fail, but changes are heavily regression-tested against the real engine.
Overview
Upgrades
@altimateai/altimate-corefrom 0.5.1 to 0.7.0 and aligns CLI, tools, review runner, and native handlers with the engine’s real JSON shapes so checks and UIs stop false-passing or mislabeling results.Introduces
engine-coerce(dialectHint, PII classification stringification, confidence bands,piiColumnsFromReport) and appliesdialectHinteverywhere dialect is optional so""means auto-detect instead of crashing.altimate_core.checknow wires PII viacheckQueryPii, diff-scopes safety threats and PII againstbase_sql(validation stays full-scan), and normalizes flatschema_contextforlintDiff.Tools read the correct fields (
allowed/violations/warnings,diffs/identical,findingsvsvalid,queries[].edges,pii_columns/query_targets) and use ERROR titles when the engine fails or abstains (parse_error), not SAFE/IDENTICAL/CLEAN. Schema PII detection usescolumnsfromPiiReportand fails closed on scan errors.altimate checkfixes dead gates: validate ondata.valid, semantic onfindingsnotvalid, safety onthreatswithhigh→error severity, PII/grade/policy mappings, per-file grades, and fail-closed envelopes. Review runner folds validation/safety into check issues, maps PII viaquery_targets, and uses shared coercions.Large regression test additions lock these contracts against the live NAPI binary.
Reviewed by Cursor Bugbot for commit b35057f. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Upgrades
@altimateai/altimate-coreto 0.7.0 and realigns CLI/tools/review to the engine’s JSON contracts so checks stop false‑passing and titles/severities match results. Safety and PII are now diff‑scoped; validation stays non‑diff‑scoped to avoid hiding new breakage.query_targets); recomputerisk_level/risk_scorewhen exposures are pre‑existing or partially filtered; stringify{ Custom }; drop nullsuggested_masking; detector fails closed and reportssuccess:falseon partial column errors.base_sql; recomputesafe/risk_score;high|critical → error,medium → warning; envelope failures append an error finding even when partial threats exist.diffs/identical; policy usesallowedand surfaces advisorywarnings; grade usesoverall_gradeand aggregates nested validation/safety/lint findings with locations/suggestions; semantics readsfindings(notvalid); validation gates ondata.valid; lineage readsqueries[].edges.parse_errormarksmetadata.success:falseand renders “check skipped”; semantics abstentions are error‑severity with a schema, warning when schema‑less.""means auto‑detect via sharedEngineCoerce.dialectHintacross all dialect‑taking handlers; composite diff path fixes flatschema_contextviaSchemaResolver.normalizeSchemaContext.query_targetsand avoid duplicating source columns; CLI renders byte‑range locations.Written for commit b35057f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes