Merge pull request #1370 from TheWizardsCode/copilot/unescape-strings-before-inserting-db

Unescape plain-text fields before persisting to SQLite

Author: SorraTheOrc

src/persistent-store.ts

@@ -67,6 +67,26 @@ export function normalizeSqliteBindings(values: unknown[]): Array<number | strin
   return values.map(normalizeSqliteValue);
 }
 
+/**
+ * Unescape backslash escape sequences in a plain-text string before persisting.
+ * Converts common two-character escape artifacts (e.g. backslash-n from CLI
+ * argument passing) into their actual character equivalents so stored text is
+ * human-readable and free of accidental escape artifacts.
+ *
+ * Only the following sequences are converted (single-pass, left-to-right):
+ *   \n  -> newline
+ *   \t  -> tab
+ *   \r  -> carriage return
+ *   \\  -> single backslash
+ *
+ * All other characters (including quotes and backticks) are left unchanged.
+ * This function must NOT be applied to JSON strings or structured fields.
+ */
+export function unescapeText(s: string): string {
+  const map: Record<string, string> = { '\\': '\\', n: '\n', t: '\t', r: '\r' };
+  return s.replace(/\\(\\|n|t|r)/g, (_, c: string) => map[c]);
+}
+
 export class SqlitePersistentStore {
   private db: Database.Database;
   private dbPath: string;
@@ -337,14 +357,27 @@ export class SqlitePersistentStore {
     // runtime normalization elsewhere.
     const normalizedStatus = normalizeStatusValue(item.status) ?? item.status;
 
+    // Unescape plain-text fields so backslash escape artifacts (e.g. \n from
+    // CLI argument passing) are stored as the intended characters.
+    // Structured/JSON fields (tags, refs, audit JSON) must NOT be unescaped here.
+    const titleVal = unescapeText(item.title ?? '');
+    const descriptionVal = unescapeText(item.description ?? '');
+    const deleteReasonVal = unescapeText(item.deleteReason ?? '');
+    // Unescape only the plain-text field within the structured audit object.
+    let auditVal: string | null = null;
+    if (item.audit) {
+      const auditCopy = { ...item.audit, text: unescapeText(item.audit.text ?? '') };
+      auditVal = JSON.stringify(auditCopy);
+    }
+
     // Ensure we never pass `undefined` into better-sqlite3 bindings (it only
     // accepts numbers, strings, bigints, buffers and null). Normalize tags to
     // a JSON string and convert any undefined to null before running.
     const tagsVal = Array.isArray(item.tags) ? JSON.stringify(item.tags) : JSON.stringify([]);
     const values: any[] = [
       item.id,
-      item.title,
-      item.description,
+      titleVal,
+      descriptionVal,
       normalizedStatus,
       item.priority,
       item.sortIndex,
@@ -357,14 +390,14 @@ export class SqlitePersistentStore {
       item.issueType ?? '',
       item.createdBy ?? '',
       item.deletedBy ?? '',
-      item.deleteReason ?? '',
+      deleteReasonVal,
       item.risk ?? '',
       item.effort ?? '',
       item.githubIssueNumber ?? null,
       item.githubIssueId ?? null,
       item.githubIssueUpdatedAt ?? null,
       item.needsProducerReview ? 1 : 0,
-      item.audit ? JSON.stringify(item.audit) : null,
+      auditVal,
     ];
 
     const normalized = normalizeSqliteBindings(values);
@@ -601,11 +634,14 @@ export class SqlitePersistentStore {
     // Pre-construction: stringify references, coerce optional fields.
     // Preserve existing || behavior for githubCommentUpdatedAt so that
     // falsy values (including empty string) become null.
+    // Unescape the comment body so backslash escape artifacts are stored as
+    // the intended characters. The refs JSON and other structured fields are
+    // intentionally left unchanged.
     const values: unknown[] = [
       comment.id,
       comment.workItemId,
       comment.author,
-      comment.comment,
+      unescapeText(comment.comment),
       comment.createdAt,
       JSON.stringify(comment.references),
       comment.githubCommentId ?? null,

tests/normalize-sqlite-bindings.test.ts

@@ -1,10 +1,10 @@
 /**
- * Tests for normalizeSqliteValue and normalizeSqliteBindings
+ * Tests for normalizeSqliteValue, normalizeSqliteBindings, and unescapeText
  * (WL-0MLRSV1XF14KM6WT)
  */
 
 import { describe, it, expect, beforeEach, afterEach } from 'vitest';
-import { normalizeSqliteValue, normalizeSqliteBindings } from '../src/persistent-store.js';
+import { normalizeSqliteValue, normalizeSqliteBindings, unescapeText } from '../src/persistent-store.js';
 import { WorklogDatabase } from '../src/database.js';
 import { createTempDir, cleanupTempDir, createTempJsonlPath, createTempDbPath } from './test-utils.js';
 
@@ -287,3 +287,155 @@ describe('SQLite binding round-trip', () => {
     expect(outbound[0].toId).toBe(b.id);
   });
 });
+
+// ---------------------------------------------------------------------------
+// Unit tests for unescapeText
+// ---------------------------------------------------------------------------
+
+describe('unescapeText', () => {
+  it('returns an empty string unchanged', () => {
+    expect(unescapeText('')).toBe('');
+  });
+
+  it('passes through plain text with no escape sequences', () => {
+    expect(unescapeText('Hello World')).toBe('Hello World');
+  });
+
+  it('converts \\n to a real newline', () => {
+    expect(unescapeText('Line\\nBreak')).toBe('Line\nBreak');
+  });
+
+  it('converts \\t to a real tab', () => {
+    expect(unescapeText('Col\\tValue')).toBe('Col\tValue');
+  });
+
+  it('converts \\r to a real carriage return', () => {
+    expect(unescapeText('Foo\\rBar')).toBe('Foo\rBar');
+  });
+
+  it('converts \\\\ to a single backslash', () => {
+    expect(unescapeText('path\\\\file')).toBe('path\\file');
+  });
+
+  it('handles multiple escape sequences in a single string', () => {
+    expect(unescapeText('a\\nb\\tc\\\\d')).toBe('a\nb\tc\\d');
+  });
+
+  it('does not double-decode when a backslash precedes a backslash-n', () => {
+    // Input: 4 chars: \  \  n  ->  backslash + n (not a newline)
+    expect(unescapeText('\\\\n')).toBe('\\n');
+  });
+
+  it('preserves double quotes unchanged', () => {
+    expect(unescapeText('say "hello"')).toBe('say "hello"');
+  });
+
+  it('preserves backticks unchanged', () => {
+    expect(unescapeText('use `code`')).toBe('use `code`');
+  });
+
+  it('preserves unrecognised backslash sequences unchanged', () => {
+    // \x is not a recognised sequence; the backslash is kept as-is
+    expect(unescapeText('foo\\xbar')).toBe('foo\\xbar');
+  });
+});
+
+// ---------------------------------------------------------------------------
+// Integration round-trip tests: unescaping applied on DB write
+// ---------------------------------------------------------------------------
+
+describe('unescapeText round-trip via DB', () => {
+  let tempDir: string;
+  let dbPath: string;
+  let jsonlPath: string;
+  let db: WorklogDatabase;
+
+  beforeEach(() => {
+    tempDir = createTempDir();
+    dbPath = createTempDbPath(tempDir);
+    jsonlPath = createTempJsonlPath(tempDir);
+    db = new WorklogDatabase('UT', dbPath, jsonlPath, true, true);
+  });
+
+  afterEach(() => {
+    db.close();
+    cleanupTempDir(tempDir);
+  });
+
+  it('stores description with real newline when input contains \\n escape artifact', () => {
+    const created = db.create({
+      title: 'Escape test',
+      description: 'Line\\nBreak',
+    });
+
+    const loaded = db.get(created.id);
+    expect(loaded).toBeDefined();
+    // Stored text must contain a real newline, not the two-char sequence \n
+    expect(loaded!.description).toBe('Line\nBreak');
+    expect(loaded!.description).not.toContain('\\n');
+  });
+
+  it('stores title with real newline when input contains \\n escape artifact', () => {
+    const created = db.create({
+      title: 'Title\\nWith Escape',
+    });
+
+    const loaded = db.get(created.id);
+    expect(loaded).toBeDefined();
+    expect(loaded!.title).toBe('Title\nWith Escape');
+    expect(loaded!.title).not.toContain('\\n');
+  });
+
+  it('stores comment body with real newline when input contains \\n escape artifact', () => {
+    const item = db.create({ title: 'Escape comment test' });
+
+    db.createComment({
+      workItemId: item.id,
+      author: 'tester',
+      comment: 'First\\nSecond',
+      references: [],
+    });
+
+    const comments = db.getCommentsForWorkItem(item.id);
+    expect(comments).toHaveLength(1);
+    expect(comments[0].comment).toBe('First\nSecond');
+    expect(comments[0].comment).not.toContain('\\n');
+  });
+
+  it('unescapes audit text field but leaves audit JSON structure intact', () => {
+    const created = db.create({
+      title: 'Audit escape test',
+      audit: {
+        time: '2026-01-01T00:00:00.000Z',
+        author: 'tester',
+        text: 'Ready to close: Yes\\nExtra detail',
+        status: 'Complete',
+      },
+    });
+
+    const loaded = db.get(created.id);
+    expect(loaded).toBeDefined();
+    expect(loaded!.audit).toBeDefined();
+    // audit.text should have a real newline
+    expect(loaded!.audit!.text).toBe('Ready to close: Yes\nExtra detail');
+    expect(loaded!.audit!.text).not.toContain('\\n');
+    // Structured audit fields must remain intact
+    expect(loaded!.audit!.author).toBe('tester');
+    expect(loaded!.audit!.status).toBe('Complete');
+  });
+
+  it('does not alter tags (JSON field) when description contains escape artifacts', () => {
+    const created = db.create({
+      title: 'Tags intact',
+      description: 'Desc\\nValue',
+      tags: ['tag\\none', 'normal'],
+    });
+
+    const loaded = db.get(created.id);
+    expect(loaded).toBeDefined();
+    // Description should be unescaped
+    expect(loaded!.description).toBe('Desc\nValue');
+    // Tags are JSON-structured; the raw tag values are preserved as-is
+    expect(loaded!.tags).toEqual(['tag\\none', 'normal']);
+  });
+});