fix: preserve Nextcloud Mail filter markers across saves #201

lib/sieve/__tests__/external-rules.test.ts

@@ -264,4 +264,94 @@ describe('external rule preservation (issue #201)', () => {
       expect(externalAfter[0].conditions[0]).toMatchObject({ field: 'header', headerName: 'X-Spam' });
     });
   });
+
+  describe('Nextcloud Mail marker regions', () => {
+    const NEXTCLOUD_MARKER = "### Nextcloud Mail: Filters ### DON'T EDIT ###";
+
+    const nextcloudScript = [
+      NEXTCLOUD_MARKER,
+      'require ["imap4flags", "fileinto"];',
+      NEXTCLOUD_MARKER,
+      '',
+      NEXTCLOUD_MARKER,
+      '# FILTER: [{"name":"Wetterwarnungen"}]',
+      'if header :contains "From" "weather@x.com" {',
+      '    fileinto "Weather";',
+      '}',
+      '',
+      '# PayPal',
+      'if header :contains "From" "paypal.de" {',
+      '    addflag "$paypal";',
+      '}',
+      NEXTCLOUD_MARKER,
+      '',
+    ].join('\n');
+
+    it('collapses a Nextcloud marker-pair region into one opaque rule labeled "Nextcloud"', () => {
+      const result = parseScript(nextcloudScript);
+      const nextcloud = result.rules.filter(r => r.originLabel === 'Nextcloud');
+      expect(nextcloud).toHaveLength(1);
+      expect(nextcloud[0].origin).toBe('opaque');
+      // Every marker that wrapped actual rule content survives in the rawBlock.
+      const markerCount = (nextcloud[0].rawBlock || '').split(NEXTCLOUD_MARKER).length - 1;
+      expect(markerCount).toBe(2);
+      // Inner if-blocks are not exposed as separate external rules.
+      expect(result.rules.filter(r => r.originLabel === 'External')).toHaveLength(0);
+    });
+
+    it('merges require tokens from inside Nextcloud regions into externalRequires', () => {
+      const result = parseScript(nextcloudScript);
+      expect(result.externalRequires).toEqual(expect.arrayContaining(['imap4flags', 'fileinto']));
+    });
+
+    it('drops require-only Nextcloud regions (no separate opaque rule for them)', () => {
+      const script = [
+        NEXTCLOUD_MARKER,
+        'require ["fileinto"];',
+        NEXTCLOUD_MARKER,
+        '',
+      ].join('\n');
+      const result = parseScript(script);
+      expect(result.rules).toHaveLength(0);
+      expect(result.externalRequires).toContain('fileinto');
+    });
+
+    it('round-trips a Nextcloud region verbatim through parse → generate → parse', () => {
+      const bulwark = makeBulwarkRule({ name: 'Test' });
+      const initial = `${generateScript([bulwark])}\n${nextcloudScript}`;
+
+      const parsed = parseScript(initial);
+      const regenerated = generateScript(parsed.rules, parsed.vacation, {
+        externalRequires: parsed.externalRequires,
+      });
+
+      // Both wrapping markers remain, in the right positions.
+      const firstMarker = regenerated.indexOf(NEXTCLOUD_MARKER);
+      const lastMarker = regenerated.lastIndexOf(NEXTCLOUD_MARKER);
+      expect(firstMarker).toBeGreaterThanOrEqual(0);
+      expect(lastMarker).toBeGreaterThan(firstMarker);
+      expect(regenerated).toContain('# FILTER: [{"name":"Wetterwarnungen"}]');
+      expect(regenerated).toContain('# PayPal');
+
+      const reparsed = parseScript(regenerated);
+      expect(reparsed.rules.filter(r => r.originLabel === 'Nextcloud')).toHaveLength(1);
+      expect(reparsed.rules.filter(r => r.originLabel === 'External')).toHaveLength(0);
+    });
+
+    it('does not emit duplicate "External rules" headers across repeated saves', () => {
+      const bulwark = makeBulwarkRule({ name: 'Test' });
+      const initial = `${generateScript([bulwark])}\n${nextcloudScript}`;
+
+      let script = initial;
+      for (let i = 0; i < 3; i++) {
+        const parsed = parseScript(script);
+        script = generateScript(parsed.rules, parsed.vacation, {
+          externalRequires: parsed.externalRequires,
+        });
+      }
+
+      const headerCount = (script.match(/# --- External rules \(managed outside Bulwark\) ---/g) || []).length;
+      expect(headerCount).toBe(1);
+    });
+  });
 });

lib/sieve/parser.ts

@@ -21,6 +21,11 @@ const OPAQUE: ParseResult = { rules: [], isOpaque: true, externalRequires: [] };
 const METADATA_BEGIN = '/* @metadata:begin';
 const METADATA_END = '@metadata:end */';
 
+const NEXTCLOUD_BLOCK_MARKER = "### Nextcloud Mail: Filters ### DON'T EDIT ###";
+
+const BULWARK_EXTERNAL_HEADER_RE =
+  /^[ \t]*#[ \t]*---[ \t]*External rules \(managed outside Bulwark\)[ \t]*---[ \t]*\r?\n/m;
+
 const FIELD_FROM_HEADER: Record<string, FilterConditionField> = {
   from: 'from',
   to: 'to',
@@ -496,6 +501,103 @@ function makeOpaqueRule(block: TopBlock, idPrefix: string, index: number): Filte
   };
 }
 
+function escapeRegex(s: string): string {
+  return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+}
+
+/**
+ * Nextcloud Mail wraps its managed filter region with a pair of
+ * `### Nextcloud Mail: Filters ### DON'T EDIT ###` markers and typically
+ * emits two such regions — one enclosing its own `require [...]` line and
+ * another enclosing the if-blocks it generates from its `# FILTER: [...]`
+ * JSON comments. Parsing the interior blocks individually loses the outer
+ * markers (causing later rules to fall back to "External") and mis-attaches
+ * them to neighboring blocks on round-trip.
+ *
+ * Treat each marker-pair as one opaque external rule so the whole region
+ * round-trips verbatim, merge any require tokens into the top-level list,
+ * and drop regions that carry nothing but a require (their extensions are
+ * already represented in the merged require line).
+ */
+function extractNextcloudRegions(content: string): {
+  cleaned: string;
+  rules: FilterRule[];
+  requires: string[];
+} {
+  const marker = NEXTCLOUD_BLOCK_MARKER;
+  const positions: number[] = [];
+  let searchFrom = 0;
+  while (true) {
+    const idx = content.indexOf(marker, searchFrom);
+    if (idx === -1) break;
+    const atLineStart = idx === 0 || content[idx - 1] === '\n';
+    if (atLineStart) positions.push(idx);
+    searchFrom = idx + marker.length;
+  }
+
+  if (positions.length < 2) {
+    return { cleaned: content, rules: [], requires: [] };
+  }
+
+  const rules: FilterRule[] = [];
+  const requires: string[] = [];
+  const markerRe = new RegExp(escapeRegex(marker), 'g');
+
+  let cleaned = '';
+  let cursor = 0;
+
+  for (let i = 0; i + 1 < positions.length; i += 2) {
+    const start = positions[i];
+    const closeStart = positions[i + 1];
+    const lineEnd = content.indexOf('\n', closeStart + marker.length);
+    const end = lineEnd === -1 ? content.length : lineEnd + 1;
+
+    cleaned += content.slice(cursor, start);
+    const raw = content.slice(start, end);
+
+    for (const m of raw.matchAll(/require\s+\[([\s\S]*?)\]\s*;/g)) {
+      for (const tok of m[1].matchAll(/"([^"]+)"/g)) {
+        if (!requires.includes(tok[1])) requires.push(tok[1]);
+      }
+    }
+    const singleReq = /require\s+"([^"]+)"\s*;/.exec(raw);
+    if (singleReq && !requires.includes(singleReq[1])) requires.push(singleReq[1]);
+
+    const stripped = raw
+      .replace(markerRe, '')
+      .replace(/require\s+\[[\s\S]*?\]\s*;/g, '')
+      .replace(/require\s+"[^"]+"\s*;/g, '')
+      .replace(/"(?:[^"\\]|\\.)*"/g, '""')
+      .replace(/#[^\n]*/g, '')
+      .replace(/\/\*[\s\S]*?\*\//g, '')
+      .trim();
+
+    if (stripped.length > 0) {
+      rules.push({
+        id: `nextcloud-${rules.length}`,
+        name: 'Nextcloud Mail filters',
+        enabled: true,
+        matchType: 'all',
+        conditions: [],
+        actions: [],
+        stopProcessing: false,
+        origin: 'opaque',
+        originLabel: 'Nextcloud',
+        rawBlock: raw,
+      });
+    }
+
+    cursor = end;
+  }
+
+  cleaned += content.slice(cursor);
+  return { cleaned, rules, requires };
+}
+
+function stripBulwarkExternalHeader(content: string): string {
+  return content.replace(BULWARK_EXTERNAL_HEADER_RE, '');
+}
+
 function parseExternalRules(
   content: string,
   idPrefix: string,
@@ -557,17 +659,25 @@ export function parseScript(content: string): ParseResult {
       if (!isValidRule(rule)) return OPAQUE;
     }
 
-    // Scan the portion AFTER the metadata block for external rules.
-    const afterMetadata = content.slice(endIdx + METADATA_END.length);
-    const external = parseExternalRules(afterMetadata, 'ext');
+    // Scan the portion AFTER the metadata block for external rules. A prior
+    // Bulwark save may have emitted its "External rules" header here; strip
+    // it so it does not get re-attached to the first external rule's rawBlock
+    // and written out twice on the next save.
+    const afterMetadata = stripBulwarkExternalHeader(
+      content.slice(endIdx + METADATA_END.length),
+    );
+    const nextcloud = extractNextcloudRegions(afterMetadata);
+    const external = parseExternalRules(nextcloud.cleaned, 'ext');
 
     // Parsed bulwark rules intentionally omit an explicit `origin` field so
     // round-trip equality with metadata-only callers holds. Absence of origin
     // is treated as 'bulwark' everywhere downstream.
     const bulwarkRules: FilterRule[] = metadata.rules;
 
-    // Exclude requires and the vacation line that we emit ourselves from externalRequires.
-    const externalRequires = external.externalRequires;
+    const externalRequires = [
+      ...external.externalRequires,
+      ...nextcloud.requires.filter(r => !external.externalRequires.includes(r)),
+    ];
 
     // Drop any external "rules" that are really the bulwark-managed if-blocks or vacation.
     // Recognizable by the leading comment "# Rule: <name>" or "# Vacation auto-reply".
@@ -584,7 +694,7 @@ export function parseScript(content: string): ParseResult {
     });
 
     return {
-      rules: [...bulwarkRules, ...filteredExternal],
+      rules: [...bulwarkRules, ...nextcloud.rules, ...filteredExternal],
       isOpaque: false,
       vacation: metadata.vacation,
       externalRequires,
@@ -595,18 +705,29 @@ export function parseScript(content: string): ParseResult {
   const vacationOnly = detectVacationOnlyScript(content);
   if (vacationOnly) return vacationOnly;
 
-  // Try to parse the whole script as external rules.
-  const external = parseExternalRules(content, 'ext');
+  // Extract Nextcloud-managed marker regions first so their interior is not
+  // parsed as a series of loose if-blocks (which would lose the outer markers
+  // and mis-label later blocks as generic "External").
+  const nextcloud = extractNextcloudRegions(content);
+  const external = parseExternalRules(nextcloud.cleaned, 'ext');
 
-  if (!external.hasContent) {
+  const allRules = [...nextcloud.rules, ...external.rules];
+  const allRequires = [
+    ...external.externalRequires,
+    ...nextcloud.requires.filter(r => !external.externalRequires.includes(r)),
+  ];
+
+  if (!external.hasContent && allRules.length === 0) {
     // Entirely empty or whitespace/comments only - treat as empty, editable.
-    return { rules: [], isOpaque: false, externalRequires: [] };
+    // Preserve any require tokens lifted out of Nextcloud marker regions so
+    // they can be re-emitted in the top-level require line.
+    return { rules: [], isOpaque: false, externalRequires: allRequires };
   }
 
   // If at least one block parsed into a structured rule, expose them as external.
   const anyParsed = external.rules.some(r => r.origin === 'external');
-  if (anyParsed || external.rules.length > 0) {
-    return { rules: external.rules, isOpaque: false, externalRequires: external.externalRequires };
+  if (anyParsed || allRules.length > 0) {
+    return { rules: allRules, isOpaque: false, externalRequires: allRequires };
   }
 
   return OPAQUE;