mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-08-28 05:25:25 +00:00
* fix(fts): keep binary payloads out of the indexed description column Issue #2889 reports embedded binary and serialized data reaching LadybugDB through `description`. The vector is real, but not for the reason the report gives, and the detector that was supposed to stop it cannot see it. Every file enters the pipeline through a lossy `utf-8` decode — the CSV emitter's own content cache reads with `fs.readFile(path, 'utf-8')`, and so does the parse worker. An invalid byte sequence therefore never survives as invalid bytes; it is replaced with U+FFFD. `isBinaryContent` counted control bytes and DEL only, and charCode 0xFFFD is neither, so a wholly corrupt payload scored as clean text: on a real repro, a Vue/JS file carrying a class file constant pool produced the description `用户服务 handles 数据 <7×U+FFFD>MethCw` and the detector returned false. Counting U+FFFD toward the existing 10% threshold is what makes the function see the case it exists for. A legitimate source file carries no replacement characters at all unless it was mis-decoded, and a handful still score far under the bar. `formatFtsDescription` then gates on it. `content` has always been gated inside `extractContent`; `description` never was, so a symbol whose doc comment is really a slice of an embedded payload had that payload copied verbatim into an FTS-indexed column. Empty string rather than a sentinel: unlike `content`, a description has no reader that needs to be told why it is missing. This does not address the `Failed calling LOWER: Invalid UTF-8` build error itself. That error cannot originate in this layer — every value handed to COPY is encoded from a JS string, which is always well-formed UTF-8. The two other gaps the issue names are a no-op and dead code respectively; see the pull request for the evidence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017oEY2i74d1HLa5FuGVuPiT * fix(fts): confine an unbuildable index to its own table One untokenizable row cost far more than its own table's index. `createSearchFTSIndexes` let the first rejection leave the loop, and by then `dropFTSIndex` had already run for that table — so the failing table ended with no index, and every table after it in `FTS_INDEXES` order was never reached. On a fresh build, or on the incremental path where `dropSearchFTSIndexes` clears all of them up front, those later tables ended with no index either. `verifySearchFTSIndexes` never ran to report it, because the throw skipped it. That is the mechanism behind the multi-table degradation in #2889: the report lists Function, Method, Property and Variable as failing together, which is loop control flow, not four independent bad rows. It also explains why `--repair-fts` felt useless — repair runs the same loop, so it stopped at the same table and left everything after it unbuilt, then failed with a list of missing indexes and no reason attached. Each index now builds inside its own try/catch and the run continues, so the damage stops at the table that actually holds the bad row and repair can recover everything else. Failures are returned rather than thrown so the caller sees all of them instead of the first: `buildSearchIndexesOrDegrade` names every failing table with its raw LadybugDB message, and repair appends those reasons to the missing-index error. The aggregate failure class is computed per failure, with integrity winning. Classification checks capability signatures first, so folding the messages into one string would have let an untokenizable row mask a genuinely broken write and downgrade an abort into a degrade. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017oEY2i74d1HLa5FuGVuPiT * refactor(fts): verify before reporting, and fold the derivable state away Cleanup pass over the two #2889 commits. No behaviour change except the verification ordering, which was a real placement error. `buildSearchIndexesOrDegrade` reported build failures and returned BEFORE `verifySearchFTSIndexes` ran. A partial build is exactly when "the other tables are fine" needs proving rather than asserting, and a stale name+content-only index succeeds at build time while leaving description search broken (#2299). Verification now always runs, and a table that failed to build is subtracted from the missing list so it is reported once, with its reason, instead of twice. `FtsIndexBuildFailure.failureClass` was `classifyFtsBuildError(error)` stored beside the string it derives from — two fields that had to agree, and a test about loop isolation that broke if classification rules changed. Classify at the one place that asks. `describeFtsIndexBuildFailures` becomes `summarizeFtsIndexBuildFailures` and owns the whole sentence, including the denominator only this module knows. Analyze and `--repair-fts` were rendering the same failure two different ways. `isBinaryContent` drops the `slice` for a bounded loop and folds the U+FFFD arm into the existing predicate — the two arms had identical bodies over provably disjoint conditions. Measured on this box: 349ns vs 388ns per 200 character description, and it skips a SlicedString allocation past 1000 characters. Its doc moves onto the exported function whose contract changed. Tests: three isolation tests collapse into one (same setup, three channels), the duplicate capability-class test folds into the existing single-rejection test, the two integration tests become one graph covering both emission branches, and the CJK unit case goes — an equality check on one code point cannot be reached by a CJK character, so it could not fail. `afterEach` uses `resetAllMocks` so every mock's `...Once` queue is drained, not just one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017oEY2i74d1HLa5FuGVuPiT --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
192 lines
6.2 KiB
TypeScript
192 lines
6.2 KiB
TypeScript
/**
|
||
* P0 Unit Tests: CSV Escaping Functions
|
||
*
|
||
* Tests: escapeCSVField, escapeCSVNumber, sanitizeUTF8, isBinaryContent
|
||
* Covers hardening fix #23 (keyword arrays with backslashes and commas)
|
||
*/
|
||
import { describe, it, expect } from 'vitest';
|
||
import {
|
||
escapeCSVField,
|
||
escapeCSVNumber,
|
||
sanitizeUTF8,
|
||
isBinaryContent,
|
||
} from '../../src/core/lbug/csv-generator.js';
|
||
|
||
// ─── escapeCSVField ──────────────────────────────────────────────────
|
||
|
||
describe('escapeCSVField', () => {
|
||
it('returns empty quoted string for null', () => {
|
||
expect(escapeCSVField(null)).toBe('""');
|
||
});
|
||
|
||
it('returns empty quoted string for undefined', () => {
|
||
expect(escapeCSVField(undefined)).toBe('""');
|
||
});
|
||
|
||
it('returns quoted empty string for empty input', () => {
|
||
expect(escapeCSVField('')).toBe('""');
|
||
});
|
||
|
||
it('wraps simple string in quotes', () => {
|
||
expect(escapeCSVField('hello')).toBe('"hello"');
|
||
});
|
||
|
||
it('doubles internal double quotes', () => {
|
||
expect(escapeCSVField('say "hello"')).toBe('"say ""hello"""');
|
||
});
|
||
|
||
it('handles strings with commas', () => {
|
||
expect(escapeCSVField('a,b,c')).toBe('"a,b,c"');
|
||
});
|
||
|
||
it('handles strings with newlines', () => {
|
||
expect(escapeCSVField('line1\nline2')).toBe('"line1\nline2"');
|
||
});
|
||
|
||
it('converts numbers to quoted strings', () => {
|
||
expect(escapeCSVField(42)).toBe('"42"');
|
||
});
|
||
|
||
it('handles strings with both quotes and commas', () => {
|
||
expect(escapeCSVField('"hello",world')).toBe('"""hello"",world"');
|
||
});
|
||
|
||
// Hardening fix #23: keyword arrays with backslashes
|
||
it('handles strings with backslashes', () => {
|
||
const result = escapeCSVField('path\\to\\file');
|
||
expect(result).toBe('"path\\to\\file"');
|
||
});
|
||
|
||
it('handles code content with special characters', () => {
|
||
const code = 'function foo() {\n return "bar";\n}';
|
||
const result = escapeCSVField(code);
|
||
expect(result).toContain('function foo()');
|
||
expect(result).toContain('""bar""');
|
||
});
|
||
});
|
||
|
||
// ─── escapeCSVNumber ─────────────────────────────────────────────────
|
||
|
||
describe('escapeCSVNumber', () => {
|
||
it('returns default value for null', () => {
|
||
expect(escapeCSVNumber(null)).toBe('-1');
|
||
});
|
||
|
||
it('returns default value for undefined', () => {
|
||
expect(escapeCSVNumber(undefined)).toBe('-1');
|
||
});
|
||
|
||
it('returns custom default value', () => {
|
||
expect(escapeCSVNumber(null, 0)).toBe('0');
|
||
});
|
||
|
||
it('returns string representation of number', () => {
|
||
expect(escapeCSVNumber(42)).toBe('42');
|
||
});
|
||
|
||
it('handles zero', () => {
|
||
expect(escapeCSVNumber(0)).toBe('0');
|
||
});
|
||
|
||
it('handles negative numbers', () => {
|
||
expect(escapeCSVNumber(-5)).toBe('-5');
|
||
});
|
||
|
||
it('handles floating point', () => {
|
||
expect(escapeCSVNumber(3.14)).toBe('3.14');
|
||
});
|
||
});
|
||
|
||
// ─── sanitizeUTF8 ────────────────────────────────────────────────────
|
||
|
||
describe('sanitizeUTF8', () => {
|
||
it('passes through clean strings unchanged', () => {
|
||
expect(sanitizeUTF8('hello world')).toBe('hello world');
|
||
});
|
||
|
||
it('normalizes CRLF to LF', () => {
|
||
expect(sanitizeUTF8('line1\r\nline2')).toBe('line1\nline2');
|
||
});
|
||
|
||
it('normalizes lone CR to LF', () => {
|
||
expect(sanitizeUTF8('line1\rline2')).toBe('line1\nline2');
|
||
});
|
||
|
||
it('strips null bytes', () => {
|
||
expect(sanitizeUTF8('hello\x00world')).toBe('helloworld');
|
||
});
|
||
|
||
it('strips control characters', () => {
|
||
expect(sanitizeUTF8('hello\x01\x02\x03world')).toBe('helloworld');
|
||
});
|
||
|
||
it('preserves tabs', () => {
|
||
expect(sanitizeUTF8('hello\tworld')).toBe('hello\tworld');
|
||
});
|
||
|
||
it('preserves newlines', () => {
|
||
expect(sanitizeUTF8('hello\nworld')).toBe('hello\nworld');
|
||
});
|
||
|
||
it('strips lone surrogates', () => {
|
||
expect(sanitizeUTF8('hello\uD800world')).toBe('helloworld');
|
||
});
|
||
|
||
it('strips BOM-like characters (FFFE/FFFF)', () => {
|
||
expect(sanitizeUTF8('hello\uFFFEworld')).toBe('helloworld');
|
||
});
|
||
});
|
||
|
||
// ─── isBinaryContent ─────────────────────────────────────────────────
|
||
|
||
describe('isBinaryContent', () => {
|
||
it('returns false for empty string', () => {
|
||
expect(isBinaryContent('')).toBe(false);
|
||
});
|
||
|
||
it('returns false for normal text', () => {
|
||
expect(isBinaryContent('hello world\nline two')).toBe(false);
|
||
});
|
||
|
||
it('returns false for code content', () => {
|
||
const code = 'function foo() {\n return 42;\n}\n';
|
||
expect(isBinaryContent(code)).toBe(false);
|
||
});
|
||
|
||
it('returns true when >10% non-printable characters', () => {
|
||
// Create a string that's ~20% null bytes
|
||
const binary = 'a'.repeat(80) + '\x00'.repeat(20);
|
||
expect(isBinaryContent(binary)).toBe(true);
|
||
});
|
||
|
||
it('returns false when just under 10% threshold', () => {
|
||
// 9% non-printable should not be binary
|
||
const borderline = 'a'.repeat(91) + '\x01'.repeat(9);
|
||
expect(isBinaryContent(borderline)).toBe(false);
|
||
});
|
||
|
||
it('only samples first 1000 characters', () => {
|
||
// Binary content past 1000 chars should be ignored
|
||
const text = 'a'.repeat(1000) + '\x00'.repeat(500);
|
||
expect(isBinaryContent(text)).toBe(false);
|
||
});
|
||
|
||
// #2889 — every file enters through a lossy `utf-8` decode, so an embedded
|
||
// binary payload reaches this function as U+FFFD, never as the raw bytes.
|
||
it('returns true when >10% U+FFFD replacement characters', () => {
|
||
const decoded = 'a'.repeat(80) + '<27>'.repeat(20);
|
||
expect(isBinaryContent(decoded)).toBe(true);
|
||
});
|
||
|
||
it('counts U+FFFD toward the same threshold as control bytes', () => {
|
||
// 5 control + 6 replacement = 11% of 100 — neither group crosses 10% alone.
|
||
const mixed = 'a'.repeat(89) + '\x01'.repeat(5) + '<27>'.repeat(6);
|
||
expect(isBinaryContent(mixed)).toBe(true);
|
||
});
|
||
|
||
it('returns false for text carrying a few replacement characters', () => {
|
||
// A mis-decoded latin-1 comment in an otherwise clean file stays indexable.
|
||
const mostlyText = 'a'.repeat(95) + '<27>'.repeat(5);
|
||
expect(isBinaryContent(mostlyText)).toBe(false);
|
||
});
|
||
});
|