Skip to content

Commit a03901b

Browse files
anandgupta42claude
anandgupta42
and
claude
committed
fix: address cubic round-2 review — runner PII extraction + consumer 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 <[email protected]>
1 parent 00d0a02 commit a03901b

9 files changed

Lines changed: 67 additions & 20 deletions

File tree

‎packages/opencode/src/altimate/native/engine-coerce.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,3 +21,15 @@ export function classificationToString(c: unknown, fallback = "PII"): string {
2121
export function dialectHint(dialect: string | undefined | null): string | undefined {
2222
return dialect || undefined
2323
}
24+
25+
/** Map the engine's numeric confidence (0..1) to a string band. */
26+
export function bandConfidence(c: unknown): "high" | "medium" | "low" {
27+
if (typeof c === "string") {
28+
const s = c.toLowerCase()
29+
if (s === "high" || s === "medium" || s === "low") return s
30+
}
31+
const n = typeof c === "number" ? c : NaN
32+
if (n >= 0.8) return "high"
33+
if (n >= 0.5) return "medium"
34+
return "low"
35+
}

‎packages/opencode/src/altimate/native/schema/pii-detector.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
*/
55

66
import * as core from "@altimateai/altimate-core"
7-
import { classificationToString } from "../engine-coerce"
7+
import { bandConfidence, classificationToString } from "../engine-coerce"
88
import { getCache } from "./cache"
99
import * as Registry from "../connections/registry"
1010
import type {
@@ -97,7 +97,7 @@ export async function detectPii(params: PiiDetectParams): Promise<PiiDetectResul
9797
column: col.name,
9898
data_type: col.data_type,
9999
pii_category: classificationToString(piiCol.classification, "UNKNOWN"),
100-
confidence: piiCol.confidence ?? "medium",
100+
confidence: bandConfidence(piiCol.confidence),
101101
})
102102
tablesWithPii.add(`${col.warehouse}.${col.schema_name}.${col.table}`)
103103
}
@@ -190,7 +190,7 @@ async function detectPiiLive(params: PiiDetectParams): Promise<PiiDetectResult>
190190
column: piiCol.column || "",
191191
data_type: columns.find((c) => c.name === piiCol.column)?.data_type,
192192
pii_category: classificationToString(piiCol.classification, "UNKNOWN"),
193-
confidence: piiCol.confidence ?? "medium",
193+
confidence: bandConfidence(piiCol.confidence),
194194
})
195195
tablesWithPii.add(`${params.warehouse}.${schemaName}.${tableInfo.name}`)
196196
}

‎packages/opencode/src/altimate/review/runner.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,9 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun
228228
// RAW issues (which still carry column/target/name) — the normalized
229229
// `issues` drop those fields, so mapping over them would always miss.
230230
const piiColumns = [
231-
...asArray<any>(data.pii).map(piiColumnOf),
231+
// data.pii is the engine PiiQueryResult ({ pii_columns, … }); the
232+
// legacy array shape is kept as a fallback.
233+
...asArray<any>((data.pii as any)?.pii_columns ?? data.pii).map(piiColumnOf),
232234
...rawIssues
233235
.filter((i: any) => /pii|sensitive/i.test(String(i.category ?? i.rule ?? i.code ?? i.kind ?? "")))
234236
.map(piiColumnOf),

‎packages/opencode/src/altimate/tools/altimate-core-check.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -99,15 +99,19 @@ export function formatCheck(data: Record<string, any>): string {
9999
}
100100

101101
lines.push("\n=== PII ===")
102-
// Engine PiiQueryResult: { accesses_pii, pii_columns, risk_level, … }
102+
// Engine PiiQueryResult: { accesses_pii, pii_columns, risk_level, parse_error? }
103103
const piiCols = (data.pii?.pii_columns ?? data.pii?.findings ?? []) as any[]
104-
if (!piiCols.length) {
104+
if (data.pii?.parse_error) {
105+
// Abstention, not a clean verdict — the engine could not parse the query.
106+
lines.push(`PII check skipped: ${data.pii.parse_error}`)
107+
} else if (!piiCols.length) {
105108
lines.push("No PII detected.")
106109
} else {
107110
for (const p of piiCols) {
108111
const cls = classificationToString(p.classification ?? p.category)
109112
const where = [p.table, p.column ?? "unknown"].filter(Boolean).join(".")
110-
lines.push(` ${where}: ${cls}${p.suggested_masking ? ` (masking: ${p.suggested_masking})` : ""}`)
113+
const via = Array.isArray(p.query_targets) && p.query_targets.length ? ` exposed via: ${p.query_targets.join(", ")}` : ""
114+
lines.push(` ${where}: ${cls}${via}${p.suggested_masking ? ` (masking: ${p.suggested_masking})` : ""}`)
111115
}
112116
}
113117

‎packages/opencode/src/altimate/tools/altimate-core-classify-pii.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,13 @@ import z from "zod"
22
import { Tool } from "../../tool/tool"
33
import { Dispatcher } from "../native"
44
import { classificationToString } from "../native/engine-coerce"
5+
import { piiColumnsFromReport } from "../native/schema/pii-detector"
56

67
/** Engine PiiReport lists EVERY column; classification "None" means not PII. */
78
function realPiiColumns(data: Record<string, any>): any[] {
8-
const columns = (data.columns ?? data.findings ?? []) as any[]
9-
return columns.filter((c) => c.classification !== "None")
9+
if (Array.isArray(data.columns)) return piiColumnsFromReport(data)
10+
// Legacy fallback shape.
11+
return ((data.findings ?? []) as any[]).filter((c) => c.classification !== "None")
1012
}
1113

1214
export const AltimateCoreClassifyPiiTool = Tool.define("altimate_core_classify_pii", {

‎packages/opencode/src/altimate/tools/altimate-core-semantics.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,9 @@ export const AltimateCoreSemanticsTool = Tool.define("altimate_core_semantics",
4444
return {
4545
title: hasError ? "Semantics: ERROR" : `Semantics: ${issueCount === 0 ? "VALID" : `${issueCount} issues`}`,
4646
metadata: {
47-
success: true, // engine ran — semantic issues are findings, not failures
47+
// Semantic issues are findings, not failures — but a failed engine
48+
// call must not report success.
49+
success: result.success,
4850
valid: data.valid,
4951
issue_count: issueCount,
5052
has_schema: hasSchema,

‎packages/opencode/src/cli/cmd/check.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,8 @@ async function runValidate(sql: string, file: string, schemaPath?: string): Prom
7272
return errors.map((f) => {
7373
// Engine ValidationError: { code, kind, message, location: {line, column} | null, suggestions }
7474
const location = f.location as { line?: number; column?: number } | null | undefined
75-
const suggestions = f.suggestions as Array<Record<string, unknown>> | undefined
75+
const s0 = (f.suggestions as unknown[] | undefined)?.[0]
76+
const engineSuggestion = typeof s0 === "string" ? s0 : ((s0 as Record<string, unknown> | undefined)?.message as string | undefined)
7677
return {
7778
file,
7879
line: (location?.line ?? f.line) as number | undefined,
@@ -81,7 +82,7 @@ async function runValidate(sql: string, file: string, schemaPath?: string): Prom
8182
rule: "validate",
8283
severity: "error" as const,
8384
message: (f.message ?? f.description ?? "") as string,
84-
suggestion: (f.suggestion ?? suggestions?.[0]?.message) as string | undefined,
85+
suggestion: (f.suggestion ?? engineSuggestion) as string | undefined,
8586
}
8687
})
8788
}
@@ -111,9 +112,10 @@ async function runSafety(sql: string, file: string): Promise<Finding[]> {
111112
[]) as Array<Record<string, unknown>>
112113
if (issues.length > 0) {
113114
return issues.map((f) => {
114-
// ThreatFinding.location is [byteOffset, byteLength], not [start, end].
115+
// ThreatFinding.location is [byteOffset, byteLength] — label as bytes,
116+
// since byte offsets diverge from character indexes on multibyte SQL.
115117
const loc = f.location as [number, number] | undefined
116-
const at = Array.isArray(loc) ? ` (chars ${loc[0]}-${loc[0] + loc[1]})` : ""
118+
const at = Array.isArray(loc) ? ` (bytes ${loc[0]}-${loc[0] + loc[1]})` : ""
117119
return {
118120
file,
119121
line: f.line as number | undefined,

‎packages/opencode/test/altimate/altimate-core-e2e.test.ts‎

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1242,15 +1242,25 @@ describeIf("consumer contract sync (round 2)", () => {
12421242
})
12431243

12441244
describe("failure output consistency (title and body must agree)", () => {
1245-
test("classify-pii engine error renders Error output, not 'No PII columns detected'", async () => {
1245+
test("classify-pii engine error renders ERROR title and Error output", async () => {
12461246
const { initTool } = await import("./tool-fixture")
12471247
const { AltimateCoreClassifyPiiTool } = await import("../../src/altimate/tools/altimate-core-classify-pii")
12481248
const tool = await initTool(AltimateCoreClassifyPiiTool)
1249-
// Nonexistent schema_path → engine failure envelope
1250-
const result = await tool.execute({ schema_path: "/nonexistent/schema.json" }, toolCtx())
1251-
if (result.title === "PII Classification: ERROR") {
1249+
// Malformed schema file → deterministic engine failure envelope
1250+
// (a nonexistent path would silently fall back to an empty schema).
1251+
const fs = await import("fs/promises")
1252+
const os = await import("os")
1253+
const path = await import("path")
1254+
const badSchema = path.join(await fs.mkdtemp(path.join(os.tmpdir(), "pii-err-")), "schema.json")
1255+
await fs.writeFile(badSchema, "not json{{{")
1256+
try {
1257+
const result = await tool.execute({ schema_path: badSchema }, toolCtx())
1258+
expect(result.title).toBe("PII Classification: ERROR")
1259+
expect(result.metadata.success).toBe(false)
12521260
expect(result.output).toContain("Error")
12531261
expect(result.output).not.toContain("No PII columns detected")
1262+
} finally {
1263+
await fs.rm(path.dirname(badSchema), { recursive: true, force: true })
12541264
}
12551265
})
12561266

@@ -1279,8 +1289,21 @@ describeIf("consumer contract sync (round 2)", () => {
12791289
)
12801290
expect(result.output).toContain("=== PII ===")
12811291
expect(result.output).toContain("customers.email")
1292+
expect(result.output).toContain("exposed via: contact")
12821293
expect(result.output).not.toContain("No PII detected")
12831294
})
1295+
1296+
test("check tool PII section reports abstention for unparseable SQL, not a clean verdict", async () => {
1297+
const { formatCheck } = await import("../../src/altimate/tools/altimate-core-check")
1298+
const output = formatCheck({
1299+
validation: { valid: false, errors: [{ message: "syntax" }] },
1300+
lint: { clean: true },
1301+
safety: { safe: true },
1302+
pii: { accesses_pii: false, pii_columns: [], parse_error: "Syntax error: Expected: identifier" },
1303+
})
1304+
expect(output).toContain("PII check skipped")
1305+
expect(output).not.toContain("No PII detected")
1306+
})
12841307
})
12851308

12861309
describe("grade CLI mapping (engine shape: overall_grade/scores.overall)", () => {

‎packages/opencode/test/cli/check-e2e.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -624,8 +624,8 @@ describe("check command E2E", () => {
624624
const j = parseJson(r.stdout)
625625
expect(j.results.safety.findings).toHaveLength(1)
626626
expect(j.results.safety.findings[0].rule).toBe("unbalanced_quote")
627-
// ThreatFinding.location is [byteOffset, byteLength] — rendered as an offset range.
628-
expect(j.results.safety.findings[0].message).toBe("Unbalanced quote suggests injection breakout (chars 37-44)")
627+
// ThreatFinding.location is [byteOffset, byteLength] — rendered as a byte range.
628+
expect(j.results.safety.findings[0].message).toBe("Unbalanced quote suggests injection breakout (bytes 37-44)")
629629
expect(j.results.safety.findings[0].suggestion).toBe("Quote count is odd within a single statement")
630630
// Engine severity "high" must normalize to error, not degrade to info —
631631
// otherwise --fail-on/--severity filters silently pass high-risk injections.

0 commit comments

Comments
 (0)