fix: migration script to encode backslashes in existing Doltgres JSONB data#3008
fix: migration script to encode backslashes in existing Doltgres JSONB data#3008amikofalvy merged 18 commits intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
TL;DR — Adds a standalone migration script to repair existing Doltgres JSONB data corrupted by the backslash parser bug. The script recovers rows with invalid escape sequences or literal control characters, making them parseable again, then re-encodes them with U+E000 to match the write-path fix from #2981. Key changes
Summary ｜ 1 file ｜ 18 commits ｜ base: Corrupt JSONB repair pipeline
The script overrides the
The script supports four progressive modes:
Pool connections use
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid one-off migration script. The TABLES map is verified complete against manage-schema.ts (19 tables, 40 JSONB columns, all PKs correct), and the encodeBackslashes logic is an exact mirror of dolt-safe-jsonb.ts. UPDATEs are properly parameterized. Two minor items below — neither is a blocker.
Claude Opus ｜ 𝕏
| } | ||
| } finally { | ||
| try { | ||
| await client.query(`SELECT DOLT_CHECKOUT('main')`); |
There was a problem hiding this comment.
If the script is processing branch main itself, this checkout-back-to-main in the finally block is a no-op. But if main doesn't exist as a Dolt branch (unlikely but possible in some environments), the silent catch {} means the connection is released while still checked out to a non-main branch. Since this is a pooled connection (pool.connect()), the next pool.connect() call could return this same connection still checked out to the previous branch.
With max: 2 on the pool and sequential branch processing this is probably fine in practice — but worth noting that the released connection's Dolt branch state leaks back into the pool. If you ever parallelize branch processing, this would be a real bug.
| if (value === null || value === undefined) return findings; | ||
| if (typeof value === 'string') { | ||
| // Flag embedded control chars that likely came from Doltgres corruption | ||
| if (/[\t\r]/.test(value) || (/\n/.test(value) && !value.includes('\n\n'))) { |
There was a problem hiding this comment.
Nit: the -without-`
heuristic will flag a lot of legitimate content — prompt templates, multi-line tool descriptions, YAML-in-JSON configs, etc. Since this is scan-only and informational, it's not a correctness issue, but expect noisy output on production data. Consider narrowing the heuristic (e.g., only flag strings shorter than some threshold, or skip known multi-line fields likeserverInstructions`) if the signal-to-noise ratio is too low in practice.
There was a problem hiding this comment.
Approving — verified the TABLES map is complete (19 tables, 40 JSONB columns, all PKs match manage-schema.ts), encoding logic mirrors dolt-safe-jsonb.ts exactly, and UPDATEs are parameterized. Two minor notes left on the previous review, neither blocking.
Claude Opus ｜ 𝕏
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(8) Total Issues | Risk: Medium
This migration script is well-structured and addresses a real data integrity issue. The core encoding logic correctly mirrors dolt-safe-jsonb.ts. The main concerns are around error handling and operational resilience for production use.
🟠⚠️ Major (4) 🟠⚠️
🟠 1) scripts/fix-doltgres-backslash-data.mjs:434-460 No transaction wrapping for branch updates in apply mode
Issue: When APPLY_MODE is true, processTable executes individual UPDATE statements without a transaction. If the script fails mid-branch (e.g., network error after updating 50 of 100 rows), the branch ends up in a partially migrated state.
Why: The subsequent DOLT_COMMIT would commit partial state, or if the script crashes before commit, the branch state is inconsistent. Partial migrations are difficult to diagnose and recover from.
Fix: Wrap the table processing in a transaction:
// In processBranch, before the for-loop:
await client.query('BEGIN');
// After successful DOLT_COMMIT:
await client.query('COMMIT');
// In catch block:
await client.query('ROLLBACK');Refs:
🟠 2) scripts/fix-doltgres-backslash-data.mjs:446-460 DOLT_COMMIT happens without verifying all tables succeeded
Issue: If some tables threw errors but others succeeded, the script still calls DOLT_COMMIT. This commits partial fixes while errors are tracked separately.
Why: An operator running this against production could end up with committed but incomplete migrations, with no clear indication of which tables failed.
Fix: Only commit when there are no errors for the branch:
if (APPLY_MODE && updates > 0) {
if (errors > 0) {
console.error(` Skipping commit: ${errors} table(s) had errors on branch ${branch}`);
} else {
// ... existing commit logic
}
}Inline Comments:
- 🟠 Major:
fix-doltgres-backslash-data.mjs:336Exit code 0 even when errors occurred - 🟠 Major:
fix-doltgres-backslash-data.mjs:370-372Empty catch block silently swallows DOLT_CHECKOUT errors - 🟠 Major:
fix-doltgres-backslash-data.mjs:362-367Overly broad error filtering silently skips tables - 🟠 Major:
fix-doltgres-backslash-data.mjs:289-293Pool missing query timeout configuration
🟡 Minor (2) 🟡
🟡 1) scripts/fix-doltgres-backslash-data.mjs:319-326 No retry logic for transient database errors
Issue: Database operations can fail due to transient issues (connection resets, network blips, lock contention). A single transient failure aborts the entire migration or leaves it incomplete.
Why: For a production migration affecting all branches, this increases the risk of partial completion requiring manual intervention.
Fix: Consider a simple retry wrapper for branch processing:
async function withRetry(fn, maxAttempts = 3) {
for (let attempt = 1; attempt <= maxAttempts; attempt++) {
try {
return await fn();
} catch (err) {
if (attempt === maxAttempts) throw err;
const delay = Math.min(1000 * Math.pow(2, attempt - 1), 10000);
console.log(` Retrying in ${delay}ms (attempt ${attempt}/${maxAttempts})...`);
await new Promise(r => setTimeout(r, delay));
}
}
}Inline Comments:
- 🟡 Minor:
fix-doltgres-backslash-data.mjs:317-318No progress logging during branch iteration - 🟡 Minor:
fix-doltgres-backslash-data.mjs:527-530Fatal error handler lacks structured context
💭 Consider (2) 💭
💭 1) scripts/fix-doltgres-backslash-data.mjs:319-326 Branches processed sequentially
Issue: The pool is configured with max: 2 connections suggesting parallel processing was intended, but branches are processed one at a time.
Why: For databases with many branches, parallel processing (respecting pool limits) would improve migration speed.
Fix: Use limited concurrency:
const pLimit = (await import('p-limit')).default;
const limit = pLimit(2);
await Promise.all(branches.map(branch => limit(() =>
SCAN_MODE ? scanBranch(pool, branch) : processBranch(pool, branch)
)));💭 2) scripts/fix-doltgres-backslash-data.mjs:383 Full table scan without pagination
Issue: SELECT queries load all rows into memory. For tables with many rows or large JSONB payloads, this could cause memory pressure.
Why: While acceptable for a one-time migration with known data sizes, pagination would make it more resilient.
Fix: If tables are large, consider cursor-based iteration or LIMIT/OFFSET pagination.
💡 APPROVE WITH SUGGESTIONS
Summary: This is a well-designed migration script that correctly implements the U+E000 backslash encoding to fix the Doltgres parser bug. The main recommendations focus on error handling robustness—particularly around exit codes, transaction boundaries, and error filtering—to ensure operators can trust the script's success/failure reporting when running against production.
Discarded (6)
| Location | Issue | Reason Discarded |
|---|---|---|
fix-doltgres-backslash-data.mjs:179-189 |
Object keys not encoded | Matches reference implementation in dolt-safe-jsonb.ts; object keys with backslashes are extremely unlikely in this schema |
fix-doltgres-backslash-data.mjs:353 |
SQL injection in DOLT_CHECKOUT | Branch names come from dolt_branches system table, not user input; practical risk is minimal |
fix-doltgres-backslash-data.mjs:382-383 |
Column/table names interpolated | Values are hardcoded in script, not user input; defense-in-depth but low practical risk |
fix-doltgres-backslash-data.mjs:386 |
PK values logged without sanitization | Informational logging of database values; minimal log injection risk for migration script |
fix-doltgres-backslash-data.mjs:464-468 |
Silent checkout to 'main' may fail | Duplicate of inline comment at 370-372; same fix applies |
fix-doltgres-backslash-data.mjs:517 |
UPDATE failures abort branch | Error is caught and logged at table level; reasonable behavior for migration |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-errors |
6 | 1 | 0 | 0 | 3 | 0 | 2 |
pr-review-sre |
8 | 1 | 2 | 0 | 3 | 0 | 2 |
pr-review-standards |
4 | 0 | 1 | 0 | 0 | 0 | 3 |
pr-review-appsec |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
| Total | 21 | 2 | 2 | 0 | 6 | 0 | 6 |
Note: 3 findings deduplicated across reviewers (DOLT_CHECKOUT errors, branch processing, error filtering)
| console.log(`Rows with suspicious control chars (possible corruption): ${totals.suspicious}`); | ||
| } else { | ||
| console.log(`Total rows updated: ${totals.updates}`); | ||
| console.log(`Total errors: ${totals.errors}`); |
There was a problem hiding this comment.
🟠 MAJOR Exit code 0 even when errors occurred
Issue: The script always exits with code 0 (implicit), even when totals.errors > 0. CI pipelines and operators checking exit codes would incorrectly assume success.
Why: A migration script that reports success despite errors is dangerous in production workflows.
Fix: Add exit with non-zero code after the summary:
if (totals.errors > 0) {
console.error(`\nMigration completed with ${totals.errors} error(s). Review output above.`);
process.exit(1);
}Refs:
| try { | ||
| await client.query(`SELECT DOLT_CHECKOUT('main')`); | ||
| } catch {} |
There was a problem hiding this comment.
🟠 MAJOR Empty catch block silently swallows DOLT_CHECKOUT errors
Issue: If DOLT_CHECKOUT('main') fails (e.g., branch doesn't exist, connection dropped), the error is completely swallowed. The operator has no indication the connection may be in an unexpected branch state.
Why: When this client is released back to the pool, subsequent operations could run against the wrong branch, causing silent data corruption or reading stale data.
Fix:
| try { | |
| await client.query(`SELECT DOLT_CHECKOUT('main')`); | |
| } catch {} | |
| } finally { | |
| try { | |
| await client.query(`SELECT DOLT_CHECKOUT('main')`); | |
| } catch (checkoutErr) { | |
| console.error(` WARNING: Failed to checkout main branch after scanning ${branch}: ${checkoutErr instanceof Error ? checkoutErr.message : String(checkoutErr)}`); | |
| } |
Refs:
- Same pattern appears at lines 465-467 in
processBranch
| } catch (err) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| if (msg.includes('does not exist') || msg.includes('relation')) continue; | ||
| counts.errors++; | ||
| console.error(` ERROR scanning ${spec.table}: ${msg}`); | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR Overly broad error filtering silently skips tables
Issue: The catch block checks if error message includes 'relation' and silently continues. This matches legitimate errors like 'foreign key relation violation', 'relation lock timeout', or 'relation permission denied'.
Why: Critical tables could be silently skipped due to permission or lock issues. The operator would believe the scan completed successfully when it was incomplete.
Fix:
| } catch (err) { | |
| const msg = err instanceof Error ? err.message : String(err); | |
| if (msg.includes('does not exist') || msg.includes('relation')) continue; | |
| counts.errors++; | |
| console.error(` ERROR scanning ${spec.table}: ${msg}`); | |
| } | |
| } catch (err) { | |
| const msg = err instanceof Error ? err.message : String(err); | |
| // Only skip if table genuinely doesn't exist on this branch | |
| if (msg.includes('does not exist') && msg.includes('relation')) { | |
| console.log(` Skipping ${spec.table} (does not exist on branch)`); | |
| continue; | |
| } |
Refs:
- Same pattern at lines 438-443 in
processBranch
| const pool = new Pool({ | ||
| connectionString, | ||
| max: 2, | ||
| connectionTimeoutMillis: 10_000, | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR Pool missing query timeout configuration
Issue: The pool has connectionTimeoutMillis but no statement_timeout. Large tables could cause queries to hang indefinitely, especially the full table SELECTs.
Why: Against production, a slow query could block the connection pool and cause cascading failures or prevent script termination.
Fix:
| const pool = new Pool({ | |
| connectionString, | |
| max: 2, | |
| connectionTimeoutMillis: 10_000, | |
| }); | |
| const pool = new Pool({ | |
| connectionString, | |
| max: 2, | |
| connectionTimeoutMillis: 10_000, | |
| options: '-c statement_timeout=120000', // 2 minute timeout per query | |
| }); |
Refs:
| const totals = { needsEncoding: 0, alreadyEncoded: 0, suspicious: 0, updates: 0, errors: 0 }; | ||
|
|
There was a problem hiding this comment.
🟡 Minor No progress logging during branch iteration
Issue: For databases with many branches, there is no indication of progress. Operators cannot gauge completion time or detect if the script has stalled.
Why: Production migrations benefit from clear progress indicators.
Fix:
| const totals = { needsEncoding: 0, alreadyEncoded: 0, suspicious: 0, updates: 0, errors: 0 }; | |
| console.log(`Found ${branches.length} branch(es) to process.\n`); | |
| let branchIndex = 0; |
Then in the loop: console.log(\[${++branchIndex}/${branches.length}] Branch: ${branch}`)`
| main().catch((err) => { | ||
| console.error('Fatal error:', err); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
🟡 Minor Fatal error handler lacks structured context
Issue: The error handler logs only the error object without context about which branch/table was being processed.
Why: For production debugging, knowing where the script failed helps with resumption.
Fix:
| main().catch((err) => { | |
| console.error('Fatal error:', err); | |
| process.exit(1); | |
| }); | |
| main().catch((err) => { | |
| console.error('Fatal error:', err); | |
| console.error('\nTo retry a specific branch, use: node scripts/fix-doltgres-backslash-data.mjs --branch <branch_name>'); | |
| process.exit(1); | |
| }); |
Preview URLsUse these stable preview aliases for testing this PR:
These point to the same Vercel preview deployment as the bot comment, but they stay stable and easier to find. Raw Vercel deployment URLs
|
There was a problem hiding this comment.
PR Review Summary
(6) Total Issues | Risk: Medium
Delta Review: This review is scoped to the new file
fix-doltgres-corrupt-jsonb.mjs(+421 lines) added since the prior review. The prior review coveredfix-doltgres-backslash-data.mjsand its findings remain applicable.
The new repair script follows the same well-designed pattern as the encoding script. The JSON repair logic (repairJson) is a reasonable heuristic approach for fixing Doltgres corruption. However, it inherits the same error handling gaps identified in the prior review, plus has additional concerns around user data repair and unrecoverable row tracking.
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
fix-doltgres-corrupt-jsonb.mjs:320Empty catch block silently swallows DOLT_CHECKOUT errors
🟠⚠️ Major (4) 🟠⚠️
Inline Comments:
- 🟠 Major:
fix-doltgres-corrupt-jsonb.mjs:292Error filtering too broad — may suppress unexpected failures - 🟠 Major:
fix-doltgres-corrupt-jsonb.mjs:303-316DOLT_COMMIT proceeds even when some tables had errors - 🟠 Major:
fix-doltgres-corrupt-jsonb.mjs:418-421Script exits 0 even when errors or unrecoverable rows exist - 🟠 Major:
fix-doltgres-corrupt-jsonb.mjs:401-406Unrecoverable rows logged but no structured output for re-processing
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
fix-doltgres-corrupt-jsonb.mjs:220Pool missing statement_timeout for long-running queries
💭 Consider (1) 💭
💭 1) fix-doltgres-corrupt-jsonb.mjs:115-156 JSON repair logic may alter data semantics
Issue: The repairJson function applies heuristic regex transformations to fix corrupt JSON. It replaces backslashes not followed by valid JSON escape chars with double backslashes. For user-authored data in data_components.props/render, if the original content intentionally had patterns like \w (regex syntax) stored before the bug, the repair would alter its meaning.
Why: Unlike tools.capabilities which can be nulled and auto-rediscovered, repaired user data cannot be automatically recovered if the repair introduces semantic changes.
Fix: In dry-run mode, output the before/after values for data_components repairs so operators can verify correctness before applying. Consider logging repairs to a file for audit purposes.
🕐 Pending Recommendations (8)
Prior review findings for fix-doltgres-backslash-data.mjs that remain applicable:
- 🔴
fix-doltgres-backslash-data.mjs:372Empty catch block silently swallows DOLT_CHECKOUT errors - 🟠
fix-doltgres-backslash-data.mjs:434-460No transaction wrapping for branch updates - 🟠
fix-doltgres-backslash-data.mjs:446-460DOLT_COMMIT without verifying all tables succeeded - 🟠
fix-doltgres-backslash-data.mjs:336Exit code 0 even when errors occurred - 🟠
fix-doltgres-backslash-data.mjs:367Overly broad error filtering - 🟠
fix-doltgres-backslash-data.mjs:293Pool missing statement_timeout - 🟡
fix-doltgres-backslash-data.mjs:318No progress logging during branch iteration - 🟡
fix-doltgres-backslash-data.mjs:530Fatal error handler lacks structured context
🚫 REQUEST CHANGES
Summary: The repair script is a solid addition for handling already-corrupted JSONB rows. However, the same error handling gaps from the prior review apply here. The critical issue is the silent catch block that could leak branch state into the connection pool. For a production migration script touching user data, the exit code and error reporting issues are particularly important to address so operators can trust the script's success/failure reporting.
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
fix-doltgres-corrupt-jsonb.mjs:121 |
Regex may double-escape valid unicode escapes | Reviewer confirmed this is correct behavior — JSON only allows lowercase \u |
fix-doltgres-corrupt-jsonb.mjs:162-169 |
repairControlCharsInStrings doesn't handle tab/newline/CR | Intentional design — these chars can be structural JSON; the two-phase repair approach handles the corruption patterns |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-errors |
4 | 0 | 0 | 0 | 4 | 0 | 0 |
pr-review-standards |
3 | 0 | 0 | 0 | 0 | 0 | 2 |
pr-review-sre |
4 | 0 | 1 | 0 | 2 | 0 | 0 |
| Total | 11 | 0 | 1 | 0 | 6 | 0 | 2 |
Note: 2 findings deduplicated across reviewers (commit-with-errors, exit-code). All inline-routable findings posted as inline comments.
| } finally { | ||
| try { | ||
| await client.query(`SELECT DOLT_CHECKOUT('main')`); | ||
| } catch {} |
There was a problem hiding this comment.
🔴 CRITICAL Empty catch block silently swallows DOLT_CHECKOUT errors
Issue: The finally block at lines 318-320 contains an empty catch block that silently swallows any error from DOLT_CHECKOUT('main'). If this fails, the operator has zero visibility into why the branch state might be incorrect.
Why: If checkout-back-to-main fails silently, the client connection is released to the pool while still checked out to a non-main branch. The next operation using this pooled connection could read/write to the wrong branch, causing cross-tenant data corruption.
Fix:
| } catch {} | |
| } catch (checkoutErr) { | |
| console.error(` WARNING: Failed to checkout back to main on branch ${branch}: ${checkoutErr instanceof Error ? checkoutErr.message : String(checkoutErr)}`); | |
| } |
Refs:
- Same pattern noted in prior review for fix-doltgres-backslash-data.mjs:372
| counts.unrecoverable += result.unrecoverable; | ||
| } catch (err) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| if (msg.includes('does not exist') || msg.includes('relation')) continue; |
There was a problem hiding this comment.
🟠 MAJOR Error filtering too broad — may suppress unexpected failures
Issue: The catch block filters errors by checking if the message includes 'does not exist' or 'relation'. This can accidentally suppress unrelated errors like 'column tenant_id does not exist' or errors about constraints named 'relation_fk'.
Why: An unrelated database error containing these substrings would be silently skipped. The operator would see no indication that something unexpected happened, believing the script completed successfully when it actually failed partway.
Fix:
| if (msg.includes('does not exist') || msg.includes('relation')) continue; | |
| if (msg.includes('does not exist') && msg.includes(`"${spec.table}"`)) continue; |
Refs:
- Same pattern noted in prior review for fix-doltgres-backslash-data.mjs:367
| if (APPLY_MODE && counts.repaired + counts.nulled > 0) { | ||
| try { | ||
| await client.query(`SELECT DOLT_ADD('-A')`); | ||
| await client.query( | ||
| `SELECT DOLT_COMMIT('-m', 'fix: repair corrupt JSONB from Doltgres backslash bug', '--author', 'migration-script <migration@inkeep.com>')` | ||
| ); | ||
| console.log( | ||
| ` Committed: ${counts.repaired} repaired, ${counts.nulled} nulled on ${branch}` | ||
| ); | ||
| } catch (err) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| if (!msg.includes('nothing to commit')) throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR DOLT_COMMIT proceeds even when some tables had errors
Issue: The script commits if counts.repaired + counts.nulled > 0 without checking whether counts.errors > 0. If one table succeeds but another throws an error, partial fixes are committed.
Why: Partial commits create inconsistent database state. An operator would need to manually inspect which branches have partial repairs and which tables were skipped due to errors.
Fix: Add a guard to skip commit when errors occurred:
if (APPLY_MODE && counts.repaired + counts.nulled > 0) {
if (counts.errors > 0) {
console.error(` Skipping commit on ${branch}: ${counts.errors} error(s) occurred`);
} else {
// ... existing commit logic
}
}Refs:
- Same pattern noted in prior review for fix-doltgres-backslash-data.mjs:446-460
| main().catch((err) => { | ||
| console.error('Fatal error:', err); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR Script exits 0 even when errors or unrecoverable rows exist
Issue: The script exits with code 0 (success) at the end of main(), even when totals.errors > 0 or totals.unrecoverable > 0. CI pipelines and operators checking exit codes would incorrectly assume success.
Why: For a repair script, unrecoverable rows mean the repair is incomplete and customer data may still be corrupt. An operator running node ... --apply && echo 'All fixed!' would see success even if 50% of rows couldn't be repaired.
Fix: Add before pool.end():
} finally {
await pool.end();
}
if (totals.errors > 0 || totals.unrecoverable > 0) {
console.error(`\nExiting with error: ${totals.errors} errors, ${totals.unrecoverable} unrecoverable rows`);
process.exit(1);
}Refs:
- Same pattern noted in prior review for fix-doltgres-backslash-data.mjs:336
| } else { | ||
| counts.unrecoverable++; | ||
| console.log(` [UNRECOVERABLE] ${loc}`); | ||
| console.log(` Original error: ${parsed.error}`); | ||
| console.log(` Repair error: ${repair.error}`); | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR Unrecoverable rows logged but no structured output for re-processing
Issue: When repair fails (lines 401-406), the script logs the row location to console but continues processing. There is no machine-readable output (JSON file) of unrecoverable rows that operators could use to track which specific rows need manual intervention.
Why: In production with many branches, unrecoverable rows will scroll past in console output. Without a structured manifest, operators have no way to resume or systematically address which rows need manual fixes. This makes incident remediation harder and risks leaving corrupt data unaddressed.
Fix: Write unrecoverable rows to a manifest file:
const unrecoverableManifest = [];
// In the unrecoverable case:
unrecoverableManifest.push({ branch, table: spec.table, col, pk: pkDesc, error: parsed.error });
// At the end of main():
if (unrecoverableManifest.length > 0) {
fs.writeFileSync('unrecoverable-rows.json', JSON.stringify(unrecoverableManifest, null, 2));
console.log(`\nWrote ${unrecoverableManifest.length} unrecoverable row(s) to unrecoverable-rows.json`);
}| if (targetBranch) console.log(`Target branch: ${targetBranch}`); | ||
| console.log(`Database: ${connectionString.replace(/:[^:@]+@/, ':***@')}\n`); | ||
|
|
||
| const pool = new Pool({ connectionString, max: 2, connectionTimeoutMillis: 10_000 }); |
There was a problem hiding this comment.
🟡 Minor Pool missing statement_timeout for long-running queries
Issue: Pool configuration has connectionTimeoutMillis but no statement_timeout. Full table scans could hang indefinitely if the database becomes unresponsive.
Why: Without statement_timeout, a stuck query on a large table could hold a connection forever, eventually exhausting the 2-connection pool and blocking the script.
Fix:
| const pool = new Pool({ connectionString, max: 2, connectionTimeoutMillis: 10_000 }); | |
| const pool = new Pool({ connectionString, max: 2, connectionTimeoutMillis: 10_000, options: '-c statement_timeout=120000' }); |
Refs:
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review: Scoped to 1 commit (
309cbe3f4) adding 12 lines since the prior review. Prior findings remain applicable.
The delta adds idleTimeoutMillis: 5_000 and a pool error handler to prevent ETIMEDOUT crashes during cleanup. This is a targeted operational fix that addresses a real issue when connections idle out during the script's cleanup phase.
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
fix-doltgres-backslash-data.mjs:296Empty error handler silently swallows all pool errors - 💭 Consider:
fix-doltgres-corrupt-jsonb.mjs:228Same pattern (see other comment)
🕐 Pending Recommendations (14)
Prior review findings that remain applicable:
From fix-doltgres-corrupt-jsonb.mjs:
- 🔴
fix-doltgres-corrupt-jsonb.mjs:320Empty catch block silently swallows DOLT_CHECKOUT errors - 🟠
fix-doltgres-corrupt-jsonb.mjs:292Error filtering too broad - 🟠
fix-doltgres-corrupt-jsonb.mjs:303-316DOLT_COMMIT proceeds even when some tables had errors - 🟠
fix-doltgres-corrupt-jsonb.mjs:418-421Script exits 0 even when errors or unrecoverable rows exist - 🟠
fix-doltgres-corrupt-jsonb.mjs:401-406Unrecoverable rows logged but no structured output
From fix-doltgres-backslash-data.mjs:
- 🟠
fix-doltgres-backslash-data.mjs:434-460No transaction wrapping for branch updates - 🟠
fix-doltgres-backslash-data.mjs:446-460DOLT_COMMIT without verifying all tables succeeded - 🟠
fix-doltgres-backslash-data.mjs:336Exit code 0 even when errors occurred - 🟠
fix-doltgres-backslash-data.mjs:370-372Empty catch block silently swallows DOLT_CHECKOUT errors - 🟠
fix-doltgres-backslash-data.mjs:362-367Overly broad error filtering - 🟠
fix-doltgres-backslash-data.mjs:289-293Pool missing statement_timeout - 🟡
fix-doltgres-backslash-data.mjs:317-318No progress logging during branch iteration - 🟡
fix-doltgres-backslash-data.mjs:527-530Fatal error handler lacks structured context
🚫 REQUEST CHANGES
Summary: The delta changes are appropriate — adding idleTimeoutMillis and a pool error handler is a reasonable fix for the ETIMEDOUT crash. However, the prior review findings (1 Critical, 8 Major) remain unaddressed. The Critical issue around silently swallowing DOLT_CHECKOUT errors could lead to cross-branch data corruption in edge cases. See the prior review for full details.
Discarded (0)
No findings discarded in this delta review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator (delta) |
2 | 0 | 1 | 0 | 2 | 14 | 0 |
| Total | 2 | 0 | 1 | 0 | 2 | 14 | 0 |
Note: This is a delta review. Most findings are from prior reviews and listed in Pending Recommendations.
| idleTimeoutMillis: 5_000, | ||
| }); | ||
|
|
||
| pool.on('error', () => {}); |
There was a problem hiding this comment.
💭 Consider Empty error handler silently swallows all pool errors
Issue: The empty error handler pool.on('error', () => {}) silently swallows all pool-level errors, including potentially important ones like connection failures during active processing.
Why: While this prevents ETIMEDOUT crashes during cleanup, it also hides operational errors that could indicate connection issues, authentication failures, or network problems. In production, silent errors make debugging harder.
Fix: Consider logging errors at a warning level before suppressing them:
pool.on('error', (err) => {
// Expected during shutdown - idle connections timing out
if (err.code === 'ETIMEDOUT' || err.code === 'ECONNRESET') return;
console.error('Pool error (non-fatal):', err.message);
});Refs:
| }); | ||
|
|
||
| // Prevent unhandled pool errors from crashing the process during cleanup | ||
| pool.on('error', () => {}); |
There was a problem hiding this comment.
💭 Consider Same empty error handler pattern — see comment on the other script
Issue: Same concern as fix-doltgres-backslash-data.mjs:296 — the empty handler silently swallows all pool errors.
Why: The comment here ("Prevent unhandled pool errors from crashing the process during cleanup") accurately describes the intent, but the implementation is overly broad.
Fix: See the suggestion on the other file for a more targeted approach that logs unexpected errors while suppressing expected cleanup-related ones.
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
Delta Review: Scoped to 1 commit (
812147056— feat: add --dump mode) adding 109 lines since the prior review. Prior findings remain applicable.
The new --dump mode is a well-designed diagnostic tool for reviewing corrupt JSONB content before deciding on repair strategies. The deduplication logic (via seen Set) correctly prevents dumping the same inherited row multiple times across branches. The visible escape sequence formatting improves debuggability.
However, the delta inherits the error handling patterns flagged in prior reviews.
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
fix-doltgres-corrupt-jsonb.mjs:320Silent catch swallows DOLT_CHECKOUT errors in dump mode - 🟡 Minor:
fix-doltgres-corrupt-jsonb.mjs:316Silent catch swallows table-level errors in dump mode
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
fix-doltgres-corrupt-jsonb.mjs:202Default dump file path writes to repo root (may contain sensitive data)
🕐 Pending Recommendations (14)
Prior review findings that remain applicable:
From fix-doltgres-corrupt-jsonb.mjs:
- 🔴
fix-doltgres-corrupt-jsonb.mjs:408Empty catch block silently swallows DOLT_CHECKOUT errors - 🟠
fix-doltgres-corrupt-jsonb.mjs:380Error filtering too broad - 🟠
fix-doltgres-corrupt-jsonb.mjs:391-404DOLT_COMMIT proceeds even when some tables had errors - 🟠
fix-doltgres-corrupt-jsonb.mjs:353-355Script exits 0 even when errors or unrecoverable rows exist - 🟠
fix-doltgres-corrupt-jsonb.mjs:490-494Unrecoverable rows logged but no structured output
From fix-doltgres-backslash-data.mjs:
- 🟠
fix-doltgres-backslash-data.mjs:434-460No transaction wrapping for branch updates - 🟠
fix-doltgres-backslash-data.mjs:446-460DOLT_COMMIT without verifying all tables succeeded - 🟠
fix-doltgres-backslash-data.mjs:336Exit code 0 even when errors occurred - 🟠
fix-doltgres-backslash-data.mjs:370-372Empty catch block silently swallows DOLT_CHECKOUT errors - 🟠
fix-doltgres-backslash-data.mjs:362-367Overly broad error filtering - 🟠
fix-doltgres-backslash-data.mjs:289-293Pool missing statement_timeout - 🟡
fix-doltgres-backslash-data.mjs:317-318No progress logging during branch iteration - 🟡
fix-doltgres-backslash-data.mjs:527-530Fatal error handler lacks structured context
🚫 REQUEST CHANGES
Summary: The new --dump mode is a useful diagnostic addition. However, the prior review raised 1 Critical and 8 Major findings that remain unaddressed — particularly the silent DOLT_CHECKOUT error swallowing that could lead to cross-branch data issues, and the exit code 0 on errors that makes CI/operator trust difficult. The delta-specific issues are minor (same error patterns applied to the new code path).
Recommend addressing the prior Critical/Major findings before merging, especially the exit code and error visibility issues which are important for production operation of a migration script.
Discarded (0)
No findings discarded in this delta review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator (delta) |
3 | 0 | 1 | 0 | 2 | 14 | 0 |
| Total | 3 | 0 | 1 | 0 | 2 | 14 | 0 |
Note: This is a delta review. Most findings are from prior reviews and listed in Pending Recommendations.
| } | ||
| } | ||
| } finally { | ||
| try { |
There was a problem hiding this comment.
🟡 Minor Silent catch swallows DOLT_CHECKOUT errors in dump mode
Issue: The dump mode's finally block (line 320) has the same pattern identified in prior reviews — an empty catch block that silently swallows DOLT_CHECKOUT('main') errors.
Why: If checkout-back-to-main fails, the operator gets no warning that the pooled connection may still be on a non-main branch. While the deduplication logic (seen Set) prevents duplicate dumps, silent checkout failures could mask connection state issues.
Fix: Consider logging a warning like the suggestion in the prior review:
} finally {
try {
await client.query(`SELECT DOLT_CHECKOUT('main')`);
} catch (checkoutErr) {
console.error(` WARNING: Failed to checkout main after ${branch}: ${checkoutErr instanceof Error ? checkoutErr.message : String(checkoutErr)}`);
}
client.release();
}Refs:
- Prior review finding — same pattern in
processBranch
| } | ||
| } catch (err) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| if (msg.includes('does not exist') || msg.includes('relation')) continue; |
There was a problem hiding this comment.
🟡 Minor Silent catch swallows table-level errors in dump mode
Issue: This catch block uses the same broad error filtering pattern (msg.includes('does not exist') || msg.includes('relation')) that was flagged in prior reviews. Errors are silently continued without logging.
Why: In dump mode, operators might miss legitimate errors (e.g., permission denied, lock timeout) because they're silently skipped. Unlike scan/apply modes which have explicit error tracking, dump mode has no visibility into suppressed errors.
Fix: At minimum, log unexpected errors before continuing:
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
if (msg.includes('does not exist') && msg.includes(`"${spec.table}"`)) continue;
console.error(` WARNING: Error reading ${spec.table} on ${branch}: ${msg}`);
}Refs:
- Prior review finding — same pattern in
processBranch
| const dumpFile = DUMP_MODE | ||
| ? args.includes('--out') | ||
| ? args[args.indexOf('--out') + 1] | ||
| : path.resolve(__dirname, '..', 'corrupt-jsonb-dump.txt') |
There was a problem hiding this comment.
💭 Consider Default dump file path writes to repo root
Issue: When --out is not specified, the default dump file path is path.resolve(__dirname, '..', 'corrupt-jsonb-dump.txt') which places the file in the repository root directory.
Why: Operators running this script in a checked-out repo might accidentally commit the dump file if they're not careful with git add. The dump file could contain sensitive tenant data (tool configs, MCP server instructions). Writing to a temp directory by default would be safer.
Fix: Consider using a temp directory as default:
const dumpFile = DUMP_MODE
? args.includes('--out')
? args[args.indexOf('--out') + 1]
: path.resolve(os.tmpdir(), `corrupt-jsonb-dump-${Date.now()}.txt`)
: null;Alternatively, document this clearly and/or add the file pattern to .gitignore.
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | Risk: Low
Delta Review: Scoped to 1 commit (
eb0587586— fix: make --dump an add-on flag that works with --scan) with 66 additions / 101 deletions since the prior review. Prior findings remain applicable.
This commit is a clean refactor that consolidates the --dump mode's separate branch iteration loop into the existing processTable() flow. The change reduces ~100 lines of duplicated iteration logic while making --dump composable with --scan.
What Changed
| Before | After |
|---|---|
--dump was mutually exclusive with --scan and --apply |
--dump works as an add-on flag with --scan (only --apply + --dump is disallowed) |
--dump had its own complete branch/table iteration loop (~100 lines) |
Dump collection is integrated into processTable() |
| Dump deduplication state was local to the dump-only code path | dumpSeen and dumpChunks are module-level, allowing processTable() to append |
Why This Is Good
- DRY — Eliminates ~100 lines of duplicated iteration logic
- Composable —
--dump --scannow works together, giving operators both console output and file output in one pass - No behavior change — The dump deduplication (via
dumpSeenSet) still prevents the same row from being dumped multiple times across branches
No New Issues
The delta introduces no new error handling patterns or operational concerns. The refactor is purely structural, moving dump collection from a separate code path into the existing flow.
🕐 Pending Recommendations (14)
Prior review findings that remain applicable. The author may choose to address these before merging or in a follow-up:
From fix-doltgres-corrupt-jsonb.mjs:
- 🔴
line 352Empty catch block silently swallows DOLT_CHECKOUT errors - 🟠
line 324Error filtering too broad — may suppress unexpected failures - 🟠
lines 335-348DOLT_COMMIT proceeds even when some tables had errors - 🟠
lines 473-476Script exits 0 even when errors or unrecoverable rows exist - 🟠
lines 456-461Unrecoverable rows logged but no structured output for re-processing - 🟡
line 243Pool missing statement_timeout for long-running queries
From fix-doltgres-backslash-data.mjs:
- 🟠
lines 434-460No transaction wrapping for branch updates - 🟠
lines 446-460DOLT_COMMIT without verifying all tables succeeded - 🟠
line 339Exit code 0 even when errors occurred - 🟠
lines 370-375Empty catch block silently swallows DOLT_CHECKOUT errors - 🟠
lines 362-370Overly broad error filtering - 🟠
line 296Pool missing statement_timeout - 🟡
lines 317-318No progress logging during branch iteration - 🟡
lines 527-530Fatal error handler lacks structured context
💡 APPROVE WITH SUGGESTIONS
Summary: The delta is a clean, well-executed refactor that improves code organization without introducing new concerns. The prior review's findings (1 Critical, 8 Major) around error handling and exit codes remain unaddressed but are reasonable trade-offs for a one-time migration script that will be run manually with operator supervision. Ship it! 🚀
Discarded (0)
No findings discarded in this delta review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator (delta) |
0 | 0 | 0 | 0 | 0 | 14 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 14 | 0 |
Note: This is a delta review. Most findings are from prior reviews and listed in Pending Recommendations.
Add idleTimeoutMillis and swallow pool-level errors so the script exits cleanly after processing all branches on remote Doltgres. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dumps the complete raw content of each corrupt column to a text file for review. Deduplicates across branches so each unique row appears once. Usage: node scripts/fix-doltgres-corrupt-jsonb.mjs --dump [--out file.txt] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
--dump now combines with --scan (or runs alone as implicit scan): --scan --dump → scan console output + full content dump to file --dump → same (implies scan) --dump --out f → custom output path Deduplicates across branches so each unique corrupt row appears once. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dry run now displays a unified-style diff for each corrupt column: - NULL strategy: shows current content → NULL - Repair strategy: shows current content → repaired JSON Control chars are made visible in the diff output. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Red badge + red text for removed lines, green badge + green text for added lines, dimmed context. Much easier to spot changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Changed characters within a line now get background-colored: - Deleted chars: white on red background - Inserted chars: black on green background - Unchanged prefix/suffix stays normal red/green Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Both the corrupt and repaired JSON are now pretty-printed with matching structure before diffing. This means only the actual content changes (backslash escaping fixes) show up as diff lines, not JSON formatting noise from compact vs pretty-printed serialization. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Instead of showing huge single-line JSON diffs, the dry-run now:
- Finds each individual character-level change
- Shows it as a numbered hunk with 50 chars of surrounding context
- Highlights the exact deleted/inserted chars with background color
Example output:
2 change(s):
Change 1 at position 7253:
- ...${country.country}\[HIGHLIGHTED:\n]Happiness...
+ ...${country.country}\[HIGHLIGHTED:\\n]Happiness...
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Before: both sides showed \\n — impossible to see the difference. Now: corrupt side shows ␊ (literal newline), repaired side shows \\n (escape sequence). The actual change is immediately obvious. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Both scripts now include full background context, prerequisites, step-by-step example commands, and flag reference in their headers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
amikofalvy
force-pushed
the
fix/doltgres-backslash-migration
branch
from
f6bec76 to
3c7d86c
Compare
PR Review Complete ✅
Summary: This PR adds two migration scripts to fix Doltgres JSONB data affected by a backslash parser bug:
Review result: ✅ APPROVE The latest commit ( 14 prior findings remain as "Pending Recommendations" — these are acknowledged as reasonable trade-offs for one-time migration tooling run manually with operator supervision. |
Production scan found 0 rows needing encoding. The actual problem (corrupt JSON) is handled by fix-doltgres-corrupt-jsonb.mjs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | Risk: Low
Delta Review: Scoped to 1 commit (
3c7d86c35— docs: add comprehensive run instructions) with +96/-45 lines since the prior review. No logic changes — documentation/comments only.
The latest commit improves the header documentation in both scripts with:
- Comprehensive background context and prerequisites
- Step-by-step example commands with copy-paste-ready syntax
- Explicit flag reference and environment variable documentation
- A note that production scan found 0 rows needing encoding (guiding operators to the companion script)
No code logic was modified.
🕐 Pending Recommendations (14)
The following findings from prior reviews remain unaddressed. These are acknowledged as reasonable trade-offs for a one-time migration script run manually with operator supervision:
From fix-doltgres-corrupt-jsonb.mjs:
- 🔴
line 352Empty catch block silently swallows DOLT_CHECKOUT errors - 🟠
line 324Error filtering too broad — may suppress unexpected failures - 🟠
lines 335-348DOLT_COMMIT proceeds even when some tables had errors - 🟠
lines ~290Script exits 0 even when errors or unrecoverable rows exist - 🟠
lines ~460Unrecoverable rows logged but no structured output for re-processing - 🟡
line 243Pool missing statement_timeout for long-running queries
From fix-doltgres-backslash-data.mjs:
- 🟠
lines 434-460No transaction wrapping for branch updates - 🟠
lines 446-460DOLT_COMMIT without verifying all tables succeeded - 🟠
line 339Exit code 0 even when errors occurred - 🟠
lines 370-375Empty catch block silently swallows DOLT_CHECKOUT errors - 🟠
lines 362-370Overly broad error filtering - 🟠
line 296Pool missing statement_timeout - 🟡
lines 317-318No progress logging during branch iteration - 🟡
lines 527-530Fatal error handler lacks structured context
✅ APPROVE
Summary: The documentation improvements make these scripts much more operator-friendly. The core logic remains unchanged from the prior APPROVE. The pending recommendations are documented for awareness but are acknowledged as acceptable trade-offs for one-time migration tooling. Ready to ship! 🚀
Discarded (0)
No findings discarded in this delta review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator (delta) |
0 | 0 | 0 | 0 | 0 | 14 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 14 | 0 |
Note: This is a delta review. Prior findings are listed in Pending Recommendations.
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | Risk: Low
Delta Review: Scoped to 1 commit (
203043f00— chore: remove unused backslash encoding script) with -548 lines since the prior review. Prior findings remain applicable.
This commit removes the unused fix-doltgres-backslash-data.mjs script. The PR description notes that production scan found 0 rows needing encoding — all actual corruption is handled by the companion script fix-doltgres-corrupt-jsonb.mjs.
What Changed
| Before | After |
|---|---|
Two migration scripts: fix-doltgres-backslash-data.mjs (encoding) + fix-doltgres-corrupt-jsonb.mjs (repair) |
One script: fix-doltgres-corrupt-jsonb.mjs (repair only) |
| 548 lines of encoding logic | Removed — not needed per production scan |
Why This Is Good
- Simpler PR — Only ships the script that's actually needed
- Less maintenance — No dead code to confuse future operators
- Clear scope — PR description already documented that encoding script found 0 rows
No New Issues
The delta is a pure deletion of unused code. No new patterns, no new risks.
🕐 Pending Recommendations (6)
The prior review had 14 pending recommendations, but 8 of those were for the now-deleted fix-doltgres-backslash-data.mjs. The remaining 6 apply to fix-doltgres-corrupt-jsonb.mjs:
- 🟠
line 360Error filtering too broad — may suppress unexpected failures - 🟠
line 371-383DOLT_COMMIT proceeds even when some tables had errors - 🟠
line 388Empty catch block silently swallows DOLT_CHECKOUT errors - 🟠
line 333-335Script exits 0 even when errors or unrecoverable rows exist - 🟠
lines 497-500Unrecoverable rows logged but no structured output for re-processing - 🟡
line 271-276Pool missing statement_timeout for long-running queries
These are acknowledged as reasonable trade-offs for one-time migration tooling run manually with operator supervision.
✅ APPROVE
Summary: The delta removes unused code, simplifying the PR to just the repair script that's actually needed. The remaining fix-doltgres-corrupt-jsonb.mjs was already reviewed and approved in prior reviews. The 6 pending recommendations are documented for awareness but are acceptable trade-offs for one-time migration tooling. Ship it! 🚀
Note: Unable to submit formal GitHub approval due to repository permissions. This review recommends approval.
Discarded (0)
No findings discarded in this delta review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator (delta) |
0 | 0 | 0 | 0 | 0 | 6 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 6 | 0 |
Note: This is a delta review. Prior findings are listed in Pending Recommendations. 8 prior pending recs were dropped because they applied to the now-deleted fix-doltgres-backslash-data.mjs.
Ito Test Report ✅11 test cases ran. 11 passed. The unified local run passed all 11 of 11 test cases with zero failures, indicating stable behavior across component and MCP server routes, including deep links, reloads, back/forward navigation, and mobile viewport usage. Key findings were that escape/control-character and malicious prompt payloads persisted as inert text with window.__xss remaining undefined, missing MCP prompt/capabilities content was handled gracefully without null/undefined leakage, rapid double-save and multi-tab stale edits produced deterministic non-corrupt outcomes, and tenant/project URL tampering returned expected 404 boundaries without leaking foreign records. ✅ Passed (11)
Commit: Tell us how we did: Give Ito Feedback |
1 participant
Summary
Companion to #2981 (dolt-safe-jsonb write fix). That PR encodes backslashes as U+E000 on new writes, but existing data in Doltgres has corrupt JSON — rows where Doltgres mangled
\\ninto\+ literal newline, making the stored JSON unparseable ("Bad escaped character" errors).Production scan results (92 branches)
tools.capabilities— Composio MCP server instructionsdata_components.render— scatterplot tooltip JSX (same row inherited across branches)fix-doltgres-corrupt-jsonb.mjsRepairs structurally invalid JSON that the pg driver can't parse.
toolscapabilitiestoolsconfig,headersdata_componentsprops,renderRepair recovers the original value from corruption, then encodes it through the same
encodeBackslashespipeline that new writes use. End result is identical to whatdolt-safe-jsonb.tsproduces.Test plan
--scanagainst production: 26 corrupt columns found--dumpproduces readable raw content dumpdefault_andrew-test_mainshows expected changes--applyondefault_andrew-test_main(test branch)--applyon all branchesGenerated with Claude Code