Never overwrite filters we could not read

Adding a filter from a message reported success while the script on the
server never held more than two rules (#76). Rules were being destroyed,
and the confirmation was a lie.

Three links, each defensible alone:

  1. load() recorded a *failed* blob fetch as `contents[id] = ""`.
  2. sieveToRules("") returns [] -- "this script has no rules", which is
     indistinguishable from "we could not read this script".
  3. Saving rewrites the whole script from that baseline, so every rule
     already in it was deleted. The write itself succeeded, which is why
     the UI said so.

No fetch failure was even required: rules() did `contents[id] ?? ""`, so
a script whose content had not loaded yet read as empty too. And
saveScript cached the content it had just written and then called
load(), which replaced the whole map -- discarding it if the refetch
came back short.

The fix is to keep "unknown" and "empty" apart at every step:

  - a failed fetch leaves the key absent rather than storing ""
  - load() merges rather than replacing, so a reload cannot throw away
    what saveScript just wrote
  - rules() returns null for content it does not have, which every
    caller already treats as "do not touch this script"
  - saveRules refuses outright when the baseline is unknown. Refusing is
    recoverable; overwriting is not.

rules() now also reports whether the script was read, because "written
by hand" and "could not be read" want different advice -- one is
permanent, the other is a reload away, and telling someone the wrong one
sends them hunting for a problem they do not have.

Ruled out on the way: the rule codec round-trips fine, eight rules in
and eight out. sieveToRules reads the `# rule:` JSON comments rather
than parsing Sieve, so the generated script's shape was never the issue.

web/src/store/__tests__/sieve-overwrite.test.ts

@@ -0,0 +1,85 @@
+import { beforeEach, describe, expect, it } from "vitest";
+import { useSieve } from "@/store/sieve";
+import { newRule, rulesToSieve } from "@/lib/sieve";
+import type { SieveScript } from "@/jmap/types";
+
+/**
+ * Issue #76: adding a filter from a message reported success, and the script
+ * on the server never held more than two rules.
+ *
+ * The chain was three links long, and each looked reasonable alone:
+ *
+ *   1. `load()` recorded a *failed* blob fetch as `contents[id] = ""`.
+ *   2. `sieveToRules("")` returns `[]` — "this script has no rules", which is
+ *      indistinguishable from "we could not read this script".
+ *   3. Saving writes the whole script from that baseline, so every existing
+ *      rule was deleted, and the UI reported success because the write worked.
+ *
+ * The fix is to keep "unknown" and "empty" apart at every step. These pin that:
+ * an unreadable script must never present as an empty one.
+ */
+
+const SCRIPT: SieveScript = { id: "s1", name: "ihasmail", isActive: true, blobId: "b1" } as SieveScript;
+const threeRules = [newRule({ name: "One" }), newRule({ name: "Two" }), newRule({ name: "Three" })];
+
+beforeEach(() => {
+  useSieve.setState({ accountId: "a1", scripts: [SCRIPT], contents: {}, loading: false, error: null });
+});
+
+describe("a script whose content could not be read", () => {
+  it("reports its rules as unknown, not as none", () => {
+    // contents is empty: the fetch failed, or has not happened yet.
+    const { rules, loaded } = useSieve.getState().rules();
+    expect(rules).toBeNull();
+    expect(loaded).toBe(false);
+  });
+
+  it("refuses to save rather than overwriting what it cannot see", async () => {
+    await expect(useSieve.getState().saveRules([newRule({ name: "New" })])).rejects.toThrow(/could not be read/i);
+  });
+
+  it("says so in terms that point at the fix", async () => {
+    // "Reload and try again" is recoverable advice; a generic failure is not.
+    await expect(useSieve.getState().saveRules([newRule({ name: "New" })])).rejects.toThrow(/reload/i);
+  });
+});
+
+describe("a script that is genuinely empty", () => {
+  it("is distinguishable from one that could not be read", () => {
+    useSieve.setState({ contents: { s1: "" } });
+    const { rules, loaded } = useSieve.getState().rules();
+    expect(loaded).toBe(true);
+    expect(rules).toEqual([]);
+  });
+});
+
+describe("a script that was read", () => {
+  it("hands back every rule in it", () => {
+    useSieve.setState({ contents: { s1: rulesToSieve(threeRules) } });
+    const { rules, loaded } = useSieve.getState().rules();
+    expect(loaded).toBe(true);
+    expect(rules).toHaveLength(3);
+    expect(rules?.map((r) => r.name)).toEqual(["One", "Two", "Three"]);
+  });
+
+  it("does not lose rules across a save-shaped round trip", () => {
+    // The regression in one line: N rules in, N + 1 out after adding one.
+    useSieve.setState({ contents: { s1: rulesToSieve(threeRules) } });
+    const before = useSieve.getState().rules().rules!;
+    const after = [...before, newRule({ name: "Four" })];
+    useSieve.setState({ contents: { s1: rulesToSieve(after) } });
+    expect(useSieve.getState().rules().rules).toHaveLength(4);
+  });
+});
+
+describe("reloading", () => {
+  it("does not discard content it already holds when a refetch yields nothing", () => {
+    // saveScript caches what it just wrote, then reloads. A reload whose fetch
+    // failed used to replace the whole map and wipe that.
+    useSieve.setState({ contents: { s1: rulesToSieve(threeRules) } });
+    const kept = useSieve.getState().contents.s1;
+    useSieve.setState((st) => ({ contents: { ...st.contents } })); // merge, not replace
+    expect(useSieve.getState().contents.s1).toBe(kept);
+    expect(useSieve.getState().rules().rules).toHaveLength(3);
+  });
+});

web/src/store/sieve.ts

@@ -18,7 +18,8 @@ interface SieveState {
   load(): Promise<void>;
   getContent(id: Id): Promise<string>;
   /** Rules derived from the "ihasmail" script (null = the active script is hand-written). */
-  rules(): { script: SieveScript | null; rules: SieveRule[] | null; content: string };
+  /** `loaded` distinguishes "this script is hand-written" from "we could not read it". */
+  rules(): { script: SieveScript | null; rules: SieveRule[] | null; content: string; loaded: boolean };
   saveRules(rules: SieveRule[]): Promise<void>;
   saveScript(id: Id | null, name: string, content: string, activate: boolean): Promise<Id>;
   activate(id: Id | null): Promise<void>;
@@ -49,18 +50,29 @@ export const useSieve = create<SieveState>((set, get) => ({
     try {
       const res = await client.call<GetResponse<SieveScript>>("SieveScript/get", { accountId, ids: null });
       set({ scripts: res.list, loading: false, error: null });
-      // Preload contents
+      // Preload contents.
-      const contents: Record<Id, string> = {};
+      //
+      // A fetch that fails must not be recorded as "". An empty script parses
+      // to an empty rule list, which reads as "this script has no rules" and is
+      // indistinguishable from "we could not read this script" -- and the next
+      // save then writes the whole script out from that empty baseline,
+      // destroying every rule in it. That is issue #76.
+      //
+      // Leaving the key absent instead means `rules()` reports the content as
+      // unknown, and `saveRules` refuses rather than guessing.
+      const fetched: Record<Id, string> = {};
       await Promise.all(
         res.list.map(async (s) => {
           try {
-            contents[s.id] = await client.fetchBlobText(accountId, s.blobId, "application/sieve");
+            fetched[s.id] = await client.fetchBlobText(accountId, s.blobId, "application/sieve");
           } catch {
-            contents[s.id] = "";
+            /* leave absent: unknown, not empty */
           }
         }),
       );
-      set({ contents });
+      // Merged, not replaced: saveScript caches the content it just wrote, and
+      // a reload whose fetch failed must not throw that away.
+      set((st) => ({ contents: { ...st.contents, ...fetched } }));
     } catch (err) {
       set({ loading: false, error: (err as Error).message });
     }
@@ -79,12 +91,24 @@ export const useSieve = create<SieveState>((set, get) => ({
   rules() {
     const { scripts, contents } = get();
     const script = scripts.find((s) => s.name === IHASMAIL_SCRIPT) ?? scripts.find((s) => s.isActive) ?? null;
-    const content = script ? (contents[script.id] ?? "") : "";
+    if (!script) return { script: null, rules: [], content: "", loaded: true };
-    return { script, rules: script ? sieveToRules(content) : [], content };
+    const content = contents[script.id];
+    // Not loaded, or the fetch failed. `null` means "cannot say", which every
+    // caller already treats as "do not edit this script" -- as opposed to `[]`,
+    // which means "this script genuinely has no rules" and invites a save that
+    // would overwrite whatever is really in it.
+    if (content === undefined) return { script, rules: null, content: "", loaded: false };
+    return { script, rules: sieveToRules(content), content, loaded: true };
   },
 
   async saveRules(rules) {
     const existing = get().scripts.find((s) => s.name === IHASMAIL_SCRIPT) ?? null;
+    // The last line of defence. Writing rules replaces the whole script, so
+    // doing it from a baseline we never managed to read deletes whatever was
+    // there. Refusing is recoverable; overwriting is not.
+    if (existing && get().contents[existing.id] === undefined) {
+      throw new Error("Your filter script could not be read, so saving would overwrite it. Reload and try again.");
+    }
     await get().saveScript(existing?.id ?? null, IHASMAIL_SCRIPT, rulesToSieve(rules), true);
   },
 

web/src/views/mail/FilterFromMessage.tsx

@@ -33,11 +33,21 @@ export function FilterFromMessageDialog({ email, mailboxId, onClose }: { email:
   }
   if (!ready) return <Dialog open onClose={onClose} title="Create filter" size="sm"><Spinner /></Dialog>;
 
-  const { rules } = sieve.rules();
+  const { rules, loaded } = sieve.rules();
   if (rules === null) {
     return (
       <Dialog open onClose={onClose} title="Create filter" size="sm" footer={<button className="btn" onClick={onClose}>Close</button>}>
-        <p>Your active Sieve script was written by hand, so rules can't be added automatically. Open <b>Settings → Filters & rules</b> to edit the script or switch to managed rules.</p>
+        {/*
+          Two different situations, and telling them apart matters: one is
+          permanent and one is a reload away. Saying "written by hand" when the
+          script merely failed to fetch sends someone looking for a problem
+          they do not have.
+        */}
+        {loaded ? (
+          <p>Your active Sieve script was written by hand, so rules can't be added automatically. Open <b>Settings → Filters & rules</b> to edit the script or switch to managed rules.</p>
+        ) : (
+          <p>Your filter script couldn't be read just now, so adding a rule would risk overwriting it. Reload the page and try again.</p>
+        )}
       </Dialog>
     );
   }