fix: use onSuccessActivateScript for sieve activation - fixes #21

lib/jmap/client.ts

@@ -1433,17 +1433,22 @@ export class JMAPClient {
     throw new Error('Invalid upload response: blobId not found');
   }
 
-  async createSieveScript(name: string, content: string): Promise<SieveScript> {
+  async createSieveScript(name: string, content: string, activate?: boolean): Promise<SieveScript> {
     const blobId = await this.uploadSieveBlob(content);
     const accountId = this.getSieveAccountId();
 
+    const setArgs: Record<string, unknown> = {
+      accountId,
+      create: {
+        "new-script": { name, blobId }
+      },
+    };
+    if (activate) {
+      setArgs.onSuccessActivateScript = "#new-script";
+    }
+
     const response = await this.request([
-      ["SieveScript/set", {
-        accountId,
-        create: {
-          "new-script": { name, blobId }
-        }
-      }, "0"]
+      ["SieveScript/set", setArgs, "0"]
     ], this.sieveUsing());
 
     if (response.methodResponses?.[0]?.[0] === "SieveScript/set") {
@@ -1462,17 +1467,22 @@ export class JMAPClient {
     throw new Error("Failed to create sieve script");
   }
 
-  async updateSieveScript(scriptId: string, content: string): Promise<void> {
+  async updateSieveScript(scriptId: string, content: string, activate?: boolean): Promise<void> {
     const blobId = await this.uploadSieveBlob(content);
     const accountId = this.getSieveAccountId();
 
+    const setArgs: Record<string, unknown> = {
+      accountId,
+      update: {
+        [scriptId]: { blobId }
+      },
+    };
+    if (activate) {
+      setArgs.onSuccessActivateScript = scriptId;
+    }
+
     const response = await this.request([
-      ["SieveScript/set", {
-        accountId,
-        update: {
-          [scriptId]: { blobId }
-        }
-      }, "0"]
+      ["SieveScript/set", setArgs, "0"]
     ], this.sieveUsing());
 
     if (response.methodResponses?.[0]?.[0] === "SieveScript/set") {
@@ -1513,44 +1523,36 @@ export class JMAPClient {
     const response = await this.request([
       ["SieveScript/set", {
         accountId,
-        update: {
-          [scriptId]: { isActive: true }
-        }
+        onSuccessActivateScript: scriptId,
       }, "0"]
     ], this.sieveUsing());
 
-    if (response.methodResponses?.[0]?.[0] === "SieveScript/set") {
-      const result = response.methodResponses[0][1];
-      if (result.notUpdated?.[scriptId]) {
-        const error = result.notUpdated[scriptId];
-        throw new Error(error.description || "Failed to activate sieve script");
-      }
-      return;
+    const [methodName, result] = response.methodResponses?.[0] || [];
+    if (methodName === "error") {
+      throw new Error(result?.description || "Failed to activate sieve script");
+    }
+    if (methodName !== "SieveScript/set") {
+      throw new Error("Failed to activate sieve script");
     }
-    throw new Error("Failed to activate sieve script");
   }
 
-  async deactivateSieveScript(scriptId: string): Promise<void> {
+  async deactivateSieveScript(): Promise<void> {
     const accountId = this.getSieveAccountId();
 
     const response = await this.request([
       ["SieveScript/set", {
         accountId,
-        update: {
-          [scriptId]: { isActive: false }
-        }
+        onSuccessActivateScript: null,
       }, "0"]
     ], this.sieveUsing());
 
-    if (response.methodResponses?.[0]?.[0] === "SieveScript/set") {
-      const result = response.methodResponses[0][1];
-      if (result.notUpdated?.[scriptId]) {
-        const error = result.notUpdated[scriptId];
-        throw new Error(error.description || "Failed to deactivate sieve script");
-      }
-      return;
+    const [methodName, result] = response.methodResponses?.[0] || [];
+    if (methodName === "error") {
+      throw new Error(result?.description || "Failed to deactivate sieve script");
+    }
+    if (methodName !== "SieveScript/set") {
+      throw new Error("Failed to deactivate sieve script");
     }
-    throw new Error("Failed to deactivate sieve script");
   }
 
   async validateSieveScript(content: string): Promise<{ isValid: boolean; errors?: string[] }> {

lib/sieve/__tests__/generator.test.ts

@@ -399,4 +399,72 @@ describe('generateScript', () => {
     const secondIdx = script.indexOf('# Rule: Second');
     expect(firstIdx).toBeLessThan(secondIdx);
   });
+
+  describe('edge cases', () => {
+    it('skips rules with empty conditions', () => {
+      const script = generateScript([makeRule({ name: 'Empty', conditions: [], actions: [{ type: 'keep' }] })]);
+      expect(script).not.toContain('# Rule: Empty');
+      expect(script).not.toContain('if ');
+    });
+
+    it('skips rules with empty actions', () => {
+      const script = generateScript([makeRule({ name: 'NoAction', actions: [] })]);
+      expect(script).not.toContain('# Rule: NoAction');
+      expect(script).not.toContain('if ');
+    });
+
+    it('uses X-Unknown for header field without headerName', () => {
+      const script = generateScript([makeRule({
+        conditions: [{ field: 'header', comparator: 'contains', value: 'test' }],
+      })]);
+      expect(script).toContain('header :contains "X-Unknown" "test"');
+    });
+
+    it('handles all enabled rules with different extensions combined', () => {
+      const rules = [
+        makeRule({ id: '1', actions: [{ type: 'move', value: 'A' }] }),
+        makeRule({ id: '2', actions: [{ type: 'reject', value: 'No' }] }),
+        makeRule({ id: '3', conditions: [{ field: 'body', comparator: 'contains', value: 'x' }], actions: [{ type: 'star' }] }),
+      ];
+      const script = generateScript(rules);
+      const requireLine = script.split('\n').find(l => l.startsWith('require'))!;
+      expect(requireLine).toContain('"fileinto"');
+      expect(requireLine).toContain('"reject"');
+      expect(requireLine).toContain('"body"');
+      expect(requireLine).toContain('"imap4flags"');
+    });
+
+    it('generates no require line when only keep/discard/stop/forward actions', () => {
+      const script = generateScript([makeRule({
+        actions: [{ type: 'keep' }, { type: 'forward', value: 'a@b.com' }],
+      })]);
+      expect(script).not.toContain('require');
+    });
+
+    it('handles multiple actions on same rule', () => {
+      const script = generateScript([makeRule({
+        actions: [
+          { type: 'move', value: 'Folder' },
+          { type: 'mark_read' },
+          { type: 'star' },
+          { type: 'stop' },
+        ],
+      })]);
+      expect(script).toContain('fileinto "Folder";');
+      expect(script).toContain('addflag "\\\\Seen";');
+      expect(script).toContain('addflag "\\\\Flagged";');
+      expect(script).toContain('stop;');
+    });
+
+    it('generates valid script for all-disabled rules', () => {
+      const rules = [
+        makeRule({ id: '1', enabled: false }),
+        makeRule({ id: '2', enabled: false }),
+      ];
+      const script = generateScript(rules);
+      expect(script).toContain('@metadata:begin');
+      expect(script).not.toContain('if ');
+      expect(script).not.toContain('require');
+    });
+  });
 });

lib/sieve/__tests__/parser.test.ts

@@ -144,4 +144,70 @@ describe('parseScript', () => {
       expect(result.rules).toEqual(rules);
     });
   });
+
+  describe('validation edge cases', () => {
+    it('returns isOpaque when rule has non-string id', () => {
+      const script = `/* @metadata:begin\n${JSON.stringify({ version: 1, rules: [{ id: 123, name: 'x', enabled: true, matchType: 'all', conditions: [{ field: 'from', comparator: 'contains', value: 'a' }], actions: [{ type: 'keep' }], stopProcessing: false }] })}\n@metadata:end */`;
+      expect(parseScript(script).isOpaque).toBe(true);
+    });
+
+    it('returns isOpaque when rule has non-boolean enabled', () => {
+      const script = `/* @metadata:begin\n${JSON.stringify({ version: 1, rules: [{ id: '1', name: 'x', enabled: 'yes', matchType: 'all', conditions: [{ field: 'from', comparator: 'contains', value: 'a' }], actions: [{ type: 'keep' }], stopProcessing: false }] })}\n@metadata:end */`;
+      expect(parseScript(script).isOpaque).toBe(true);
+    });
+
+    it('returns isOpaque when rule has invalid matchType', () => {
+      const script = `/* @metadata:begin\n${JSON.stringify({ version: 1, rules: [{ id: '1', name: 'x', enabled: true, matchType: 'none', conditions: [{ field: 'from', comparator: 'contains', value: 'a' }], actions: [{ type: 'keep' }], stopProcessing: false }] })}\n@metadata:end */`;
+      expect(parseScript(script).isOpaque).toBe(true);
+    });
+
+    it('returns isOpaque when condition missing value', () => {
+      const script = `/* @metadata:begin\n${JSON.stringify({ version: 1, rules: [{ id: '1', name: 'x', enabled: true, matchType: 'all', conditions: [{ field: 'from', comparator: 'contains' }], actions: [{ type: 'keep' }], stopProcessing: false }] })}\n@metadata:end */`;
+      expect(parseScript(script).isOpaque).toBe(true);
+    });
+
+    it('returns isOpaque when action missing type', () => {
+      const script = `/* @metadata:begin\n${JSON.stringify({ version: 1, rules: [{ id: '1', name: 'x', enabled: true, matchType: 'all', conditions: [{ field: 'from', comparator: 'contains', value: 'a' }], actions: [{ value: 'Inbox' }], stopProcessing: false }] })}\n@metadata:end */`;
+      expect(parseScript(script).isOpaque).toBe(true);
+    });
+
+    it('accepts valid empty rules array', () => {
+      const script = `/* @metadata:begin\n${JSON.stringify({ version: 1, rules: [] })}\n@metadata:end */`;
+      const result = parseScript(script);
+      expect(result.isOpaque).toBe(false);
+      expect(result.rules).toEqual([]);
+    });
+
+    it('preserves all comparator types through round-trip', () => {
+      const comparators = ['contains', 'not_contains', 'is', 'not_is', 'starts_with', 'ends_with', 'matches'] as const;
+      const rules = comparators.map((comparator, i) => makeRule({
+        id: `r${i}`,
+        name: `Rule ${comparator}`,
+        conditions: [{ field: 'from', comparator, value: 'test' }],
+      }));
+      const script = generateScript(rules);
+      const result = parseScript(script);
+      expect(result.isOpaque).toBe(false);
+      expect(result.rules).toEqual(rules);
+    });
+
+    it('preserves size comparators through round-trip', () => {
+      const rules = [
+        makeRule({ id: 'r1', conditions: [{ field: 'size', comparator: 'greater_than', value: '1000' }], actions: [{ type: 'discard' }] }),
+        makeRule({ id: 'r2', conditions: [{ field: 'size', comparator: 'less_than', value: '500' }], actions: [{ type: 'keep' }] }),
+      ];
+      const script = generateScript(rules);
+      const result = parseScript(script);
+      expect(result.rules).toEqual(rules);
+    });
+
+    it('preserves header field with custom headerName', () => {
+      const rules = [makeRule({
+        conditions: [{ field: 'header', comparator: 'contains', value: 'test', headerName: 'X-My-Header' }],
+      })];
+      const script = generateScript(rules);
+      const result = parseScript(script);
+      expect(result.rules).toEqual(rules);
+    });
+  });
 });