fix: follow-up PR bot review findings (cubic P1/P2 + coderabbit MAJOR/MINOR)

suryaiyer95 · claude · suryaiyer95 · commit 282876b8b506 · 2026-04-17T15:58:24.000-07:00
Addresses 5 substantive issues raised by the latest round of bot reviews. ### P1 / MAJOR - **MySQL/MariaDB week-partition values no longer corrupted** (cubic P1, data-diff.ts:610) — the prior ISO `yyyy-mm-dd` normalization applied to every dialect silently rewrote MySQL `DATE_FORMAT(%Y-%u)` outputs like `"2024-42"` into invalid dates, producing WHERE clauses that never match. Scope the normalization to T-SQL / Fabric only — those use `CONVERT(DATE, …, 23)` which is the only code path that requires ISO. Postgres, MySQL, ClickHouse, BigQuery, Oracle all get the raw value verbatim, matching their own `DATE_TRUNC`/`toStartOf*` output. - **Partitioned diff no longer drops extra_columns** (coderabbit MAJOR, data-diff.ts:824) — the partition fix wraps each side as a SELECT subquery before recursing. `discoverExtraColumns` skips SQL queries (only inspects plain table names), so the recursive `runDataDiff` fell through to key-only comparison, silently losing value-level diffs. Now `runPartitionedDiff` runs discovery ONCE on the plain source table up-front and passes the resolved `extra_columns` explicitly to each recursive call. Audit-column exclusion metadata is also propagated to the aggregated result for user reporting. ### P2 / MINOR - **`azure_resource_url` trailing slash normalized** (cubic P2, sqlserver.ts:50) — an explicit `"https://custom-host"` (no slash) would produce an invalid OAuth scope `"https://custom-host.default"`. Enforce a trailing slash in `resolveAzureResourceUrl`. - **`az account get-access-token` uses `execFile`** (coderabbit, sqlserver.ts:200) — replaces `exec(<shell command string>)` with `execFile("az", [args])` so user-supplied `azure_resource_url` can't introduce shell metacharacters into the command string. Also updates the test harness to stub both `exec` and `execFile`. ### Test isolation / coverage - **Added same-dialect cross-warehouse joindiff test** (cubic, data-diff-cross-dialect.test.ts:97) — two MSSQL servers with different hosts must still be gated by the joindiff guard; previous tests only exercised mixed dialects. - **Added MySQL week-partition regression tests** — prevent future revivals of the dialect-unaware ISO rewrite. - **Added trailing-slash `azure_resource_url` test.** Test results: - 44/44 sqlserver driver tests pass - 2824/2824 altimate tests pass, 0 fail - Remaining full-suite failures (`mcp/`, `tool/project-scan`, `plan-approval-phrase`) are pre-existing on `main`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
diff --git a/packages/drivers/src/sqlserver.ts b/packages/drivers/src/sqlserver.ts
@@ -44,18 +44,26 @@ function parseTokenExpiry(token: string): number | undefined {
  *   1. Explicit `config.azure_resource_url`.
  *   2. Inferred from host suffix (Azure Gov / China).
  *   3. Default Azure commercial cloud.
+ *
+ * The returned URL is guaranteed to end with `/` — callers append `.default`
+ * to build the OAuth scope (e.g. `https://database.windows.net/.default`).
+ * Without the trailing slash, an explicit user value like
+ * `"https://custom-host"` would produce an invalid scope
+ * `"https://custom-host.default"` and `credential.getToken` would fail.
  */
 function resolveAzureResourceUrl(config: ConnectionConfig): string {
   const explicit = config.azure_resource_url as string | undefined
-  if (explicit) return explicit
-  const host = (config.host as string | undefined) ?? ""
-  if (host.includes(".usgovcloudapi.net") || host.includes(".datawarehouse.fabric.microsoft.us")) {
-    return "https://database.usgovcloudapi.net/"
-  }
-  if (host.includes(".chinacloudapi.cn")) {
-    return "https://database.chinacloudapi.cn/"
-  }
-  return "https://database.windows.net/"
+  const raw = explicit ?? (() => {
+    const host = (config.host as string | undefined) ?? ""
+    if (host.includes(".usgovcloudapi.net") || host.includes(".datawarehouse.fabric.microsoft.us")) {
+      return "https://database.usgovcloudapi.net/"
+    }
+    if (host.includes(".chinacloudapi.cn")) {
+      return "https://database.chinacloudapi.cn/"
+    }
+    return "https://database.windows.net/"
+  })()
+  return raw.endsWith("/") ? raw : `${raw}/`
 }
 
 /** Visible for testing: reset the module-scoped token cache. */
@@ -176,18 +184,26 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
 
           if (!token) {
             try {
-              // Use async `exec` (not `execSync`) so the connection path stays
-              // non-blocking — `az account get-access-token` can take several
-              // seconds to round-trip and execSync would block the entire
-              // event loop for that duration.
+              // Use async `execFile` (NOT `exec`/`execSync`):
+              //   - `execFile` skips the shell entirely and passes args as an
+              //     array, preventing shell interpolation when `resourceUrl`
+              //     comes from user config (e.g. `"https://x/;other-cmd"`).
+              //   - async keeps the connect path non-blocking during the
+              //     multi-second `az` round trip.
               const childProcess = await import("node:child_process")
               const { promisify } = await import("node:util")
-              const execAsync = promisify(childProcess.exec)
-              const { stdout } = await execAsync(
-                `az account get-access-token --resource ${resourceUrl} --query accessToken -o tsv`,
+              const execFileAsync = promisify(childProcess.execFile)
+              const { stdout } = await execFileAsync(
+                "az",
+                [
+                  "account", "get-access-token",
+                  "--resource", resourceUrl,
+                  "--query", "accessToken",
+                  "-o", "tsv",
+                ],
                 { encoding: "utf-8", timeout: 15000 },
               )
-              const out = stdout.trim()
+              const out = String(stdout).trim()
               if (out) {
                 token = out
                 expiresAt = parseTokenExpiry(out)
diff --git a/packages/drivers/test/sqlserver-unit.test.ts b/packages/drivers/test/sqlserver-unit.test.ts
@@ -92,30 +92,50 @@ const cliState = {
 }
 const realChildProcess = await import("node:child_process")
 const realUtil = await import("node:util")
-// Stub `exec` with a custom `util.promisify.custom` so `promisify(exec)`
-// yields { stdout, stderr } exactly as the real implementation does. Also
-// keep the legacy callback form of `execSync` for tests that still use it.
-const execStub: any = (cmd: string, optsOrCb: any, maybeCb?: any) => {
-  cliState.lastCmd = cmd
-  const cb = typeof optsOrCb === "function" ? optsOrCb : maybeCb
-  if (cliState.throwError) {
-    const e: any = new Error(cliState.throwError.message ?? "az failed")
-    e.stderr = cliState.throwError.stderr
-    if (cb) cb(e, "", cliState.throwError.stderr ?? "")
+
+// Helper: build a mock with callback + util.promisify.custom support so
+// `promisify(child_process.exec)` or `promisify(child_process.execFile)`
+// yields { stdout, stderr } exactly like the real implementation.
+function makeChildProcessMock(captureCmd: (args: string) => void) {
+  const stub: any = (arg0: any, arg1: any, arg2: any, arg3: any) => {
+    // Accept both exec(cmd, opts?, cb?) and execFile(file, args?, opts?, cb?)
+    const cb = [arg0, arg1, arg2, arg3].find((x) => typeof x === "function")
+    // Pick the best "command" representation for test assertions:
+    //   - exec:     first arg is the full command string
+    //   - execFile: first arg is the program, second arg is the args array
+    if (Array.isArray(arg1)) {
+      captureCmd(`${arg0} ${arg1.join(" ")}`)
+    } else {
+      captureCmd(String(arg0))
+    }
+    if (cliState.throwError) {
+      const e: any = new Error(cliState.throwError.message ?? "az failed")
+      e.stderr = cliState.throwError.stderr
+      if (cb) cb(e, "", cliState.throwError.stderr ?? "")
+      return { on() {}, stdout: null, stderr: null }
+    }
+    if (cb) cb(null, cliState.output, "")
     return { on() {}, stdout: null, stderr: null }
   }
-  if (cb) cb(null, cliState.output, "")
-  return { on() {}, stdout: null, stderr: null }
-}
-execStub[realUtil.promisify.custom] = (cmd: string, _opts?: any) => {
-  cliState.lastCmd = cmd
-  if (cliState.throwError) {
-    const e: any = new Error(cliState.throwError.message ?? "az failed")
-    e.stderr = cliState.throwError.stderr
-    return Promise.reject(e)
+  stub[realUtil.promisify.custom] = (arg0: any, arg1: any) => {
+    if (Array.isArray(arg1)) {
+      captureCmd(`${arg0} ${arg1.join(" ")}`)
+    } else {
+      captureCmd(String(arg0))
+    }
+    if (cliState.throwError) {
+      const e: any = new Error(cliState.throwError.message ?? "az failed")
+      e.stderr = cliState.throwError.stderr
+      return Promise.reject(e)
+    }
+    return Promise.resolve({ stdout: cliState.output, stderr: "" })
   }
-  return Promise.resolve({ stdout: cliState.output, stderr: "" })
+  return stub
 }
+
+const execStub = makeChildProcessMock((c) => { cliState.lastCmd = c })
+const execFileStub = makeChildProcessMock((c) => { cliState.lastCmd = c })
+
 mock.module("node:child_process", () => ({
   ...realChildProcess,
   execSync: (cmd: string) => {
@@ -128,6 +148,7 @@ mock.module("node:child_process", () => ({
     return cliState.output
   },
   exec: execStub,
+  execFile: execFileStub,
 }))
 
 // Import after mocking
@@ -726,6 +747,20 @@ describe("SQL Server driver unit tests", () => {
       expect(azureIdentityState.lastScope).toBe("https://custom.sovereign.example/.default")
     })
 
+    test("azure_resource_url without trailing slash is normalized", async () => {
+      // Regression: without the slash, `${resourceUrl}.default` produced an
+      // invalid scope like "https://custom-host.default", and `getToken`
+      // would reject it.
+      resetMocks()
+      const c = await connect({
+        host: "x.database.windows.net", database: "d",
+        authentication: "azure-active-directory-default",
+        azure_resource_url: "https://custom-host",
+      })
+      await c.connect()
+      expect(azureIdentityState.lastScope).toBe("https://custom-host/.default")
+    })
+
     test("az CLI fallback uses the same resource URL", async () => {
       // Disable @azure/identity so we hit the az CLI fallback
       azureIdentityState.throwOnGetToken = true
diff --git a/packages/opencode/src/altimate/native/connections/data-diff.ts b/packages/opencode/src/altimate/native/connections/data-diff.ts
@@ -595,21 +595,29 @@ export function buildPartitionWhereClause(
 
   // date mode
   const expr = dateTruncExpr(granularity!, quotedCol, dialect)
-  // Normalize the partition value to ISO yyyy-mm-dd. The mssql driver returns
-  // date columns as JS Date objects which get `String()`-coerced upstream,
-  // producing output like "Mon Jan 01 2024 00:00:00 GMT+0000 (UTC)" —
-  // T-SQL `CONVERT(DATE, …, 23)` and Postgres date literals both reject that
-  // format. Parsing once here keeps the downstream SQL dialect-safe.
-  const isoDate = (() => {
-    const trimmed = partitionValue.trim()
-    // Already looks like yyyy-mm-dd — preserve as-is so pre-formatted values
-    // (e.g. from Postgres, BigQuery, DATE_FORMAT MySQL output) flow through
-    // without surprising timezone shifts.
-    if (/^\d{4}-\d{2}-\d{2}(\s|T|$)/.test(trimmed)) return trimmed.slice(0, 10)
-    const d = new Date(trimmed)
-    return Number.isNaN(d.getTime()) ? trimmed : d.toISOString().slice(0, 10)
-  })()
-  const escaped = isoDate.replace(/'/g, "''")
+  // Normalize to ISO `yyyy-mm-dd` ONLY for T-SQL / Fabric, which use
+  // `CONVERT(DATE, '…', 23)` (strict ISO-8601 parser). The mssql driver
+  // returns date columns as JS Date objects that coerce to strings like
+  // "Mon Jan 01 2024 00:00:00 GMT+0000 (UTC)" — that format must be parsed
+  // to ISO before CONVERT will accept it.
+  //
+  // For other dialects, pass the value through unchanged. MySQL/MariaDB
+  // produce non-ISO `DATE_FORMAT` outputs (e.g. `YYYY-%u` for ISO week,
+  // which is `YYYY-42` not `YYYY-MM-DD`), and forcing ISO conversion would
+  // corrupt them — the WHERE would never match. Postgres / BigQuery /
+  // ClickHouse accept whatever their own `DATE_TRUNC`/`toStartOf*`
+  // emits verbatim on the round trip.
+  const needsIso = dialect === "tsql" || dialect === "fabric" ||
+    dialect === "sqlserver" || dialect === "mssql"
+  const normalized = needsIso
+    ? (() => {
+        const trimmed = partitionValue.trim()
+        if (/^\d{4}-\d{2}-\d{2}(\s|T|$)/.test(trimmed)) return trimmed.slice(0, 10)
+        const d = new Date(trimmed)
+        return Number.isNaN(d.getTime()) ? trimmed : d.toISOString().slice(0, 10)
+      })()
+    : partitionValue
+  const escaped = normalized.replace(/'/g, "''")
 
   // Cast the literal appropriately per dialect
   switch (dialect) {
@@ -775,6 +783,26 @@ async function runPartitionedDiff(params: DataDiffParams): Promise<DataDiffResul
     }
   }
 
+  // Auto-discover extra_columns ONCE here on the plain source table — we wrap
+  // each partition's source/target as SELECT subqueries below, which
+  // `discoverExtraColumns` skips (it only works on plain table names). If we
+  // let the recursive `runDataDiff` try, it'd always see a wrapped query and
+  // regress to key-only comparison (value-level diffs silently lost).
+  let resolvedExtraColumns = params.extra_columns
+  let partitionExcludedAudit: string[] = []
+  if (!resolvedExtraColumns || resolvedExtraColumns.length === 0) {
+    const discovered = await discoverExtraColumns(
+      params.source,
+      params.key_columns,
+      sourceDialect,
+      params.source_warehouse,
+    )
+    if (discovered) {
+      resolvedExtraColumns = discovered.columns
+      partitionExcludedAudit = discovered.excludedAudit
+    }
+  }
+
   // Diff each partition
   const partitionResults: PartitionDiffResult[] = []
   let aggregatedOutcome: unknown = null
@@ -820,6 +848,10 @@ async function runPartitionedDiff(params: DataDiffParams): Promise<DataDiffResul
       target: targetSql,
       // Preserve the user's shared where_clause — it's dialect-neutral.
       where_clause: params.where_clause,
+      // Pass auto-discovered extras explicitly — `runDataDiff`'s own
+      // discovery path would skip these wrapped SELECT subqueries and
+      // regress to key-only comparison.
+      extra_columns: resolvedExtraColumns,
       partition_column: undefined, // prevent recursion
     })
 
@@ -840,6 +872,7 @@ async function runPartitionedDiff(params: DataDiffParams): Promise<DataDiffResul
     steps: totalSteps,
     outcome: aggregatedOutcome ?? { mode: "diff", stats: { rows_table1: 0, rows_table2: 0, exclusive_table1: 0, exclusive_table2: 0, updated: 0, unchanged: 0 }, diff_rows: [] },
     partition_results: partitionResults,
+    ...(partitionExcludedAudit.length > 0 ? { excluded_audit_columns: partitionExcludedAudit } : {}),
   }
 }
 
diff --git a/packages/opencode/test/altimate/data-diff-cross-dialect.test.ts b/packages/opencode/test/altimate/data-diff-cross-dialect.test.ts
@@ -130,6 +130,23 @@ describe("buildPartitionWhereClause — cross-dialect correctness (the CRITICAL
     const sql = buildPartitionWhereClause(col, "Mon Apr 01 2024 00:00:00 GMT+0000", "month", undefined, "tsql")
     expect(sql).toContain("CONVERT(DATE, '2024-04-01', 23)")
   })
+
+  test("mysql week-format values pass through unchanged (regression: no ISO rewrite)", () => {
+    // Regression guard: MySQL `DATE_FORMAT(%Y-%u)` emits e.g. "2024-42" for
+    // week 42 — that's not a parseable JS Date. An earlier revision tried
+    // to normalize it to ISO `yyyy-mm-dd`, which either produced NaN or a
+    // wildly wrong date (Dec 2024 in 0042 AD). Must be passed through.
+    const sql = buildPartitionWhereClause("ts", "2024-42", "week", undefined, "mysql")
+    expect(sql).toContain("= '2024-42'")
+    expect(sql).not.toContain("0042")
+    expect(sql).not.toContain("NaN")
+  })
+
+  test("mysql DATE_FORMAT month output flows through verbatim", () => {
+    const sql = buildPartitionWhereClause("ts", "2024-04-01", "month", undefined, "mariadb")
+    expect(sql).toContain("DATE_FORMAT(`ts`, '%Y-%m-01')")
+    expect(sql).toContain("= '2024-04-01'")
+  })
 })
 
 // The joindiff guard runs BEFORE `runDataDiff`'s NAPI import, so we can drive
@@ -147,7 +164,7 @@ describe("joindiff + cross-warehouse guard", () => {
     Registry.reset()
   })
 
-  test("explicit joindiff with different warehouses returns early error", async () => {
+  test("explicit joindiff with different warehouses (mixed dialect) returns early error", async () => {
     Registry.setConfigs({
       msrc: { type: "sqlserver", host: "mssql-host", database: "src" },
       ptgt: { type: "postgres", host: "pg-host", database: "tgt" },
@@ -166,6 +183,29 @@ describe("joindiff + cross-warehouse guard", () => {
     expect(result.steps).toBe(0)
   })
 
+  test("explicit joindiff with different warehouses (SAME dialect) still errors", async () => {
+    // Regression guard: if `crossWarehouse` were computed from dialect
+    // equality (as an earlier revision did) instead of resolved warehouse
+    // identity, this case would slip through and route a JOIN query to a
+    // warehouse that doesn't have the other side's tables. Two MSSQL
+    // servers share a dialect but are independent physical databases.
+    Registry.setConfigs({
+      mssql_a: { type: "sqlserver", host: "server-a", database: "src" },
+      mssql_b: { type: "sqlserver", host: "server-b", database: "tgt" },
+    })
+    const result = await runDataDiff({
+      source: "dbo.orders",
+      target: "dbo.orders",
+      key_columns: ["id"],
+      source_warehouse: "mssql_a",
+      target_warehouse: "mssql_b",
+      algorithm: "joindiff",
+    })
+    expect(result.success).toBe(false)
+    expect(result.error).toMatch(/joindiff requires both tables in the same warehouse/i)
+    expect(result.steps).toBe(0)
+  })
+
   test("same-name warehouse on both sides does NOT trigger the guard", async () => {
     // Guard compares resolved warehouse identity, not dialect — same name →
     // guard stays quiet. We can't drive the whole diff without NAPI, but we
