fix(providers): address review — fail-closed nostr-sync, adapter-sourced list, strict indices

Blocker: nostr-sync now recomputes the kind-38425 review verdict via
modelManager.syncReviewedProvidersFromNostr() after clearing manual
overrides and mirrors it into the store. Previously a provider that was
manually enabled across a scheduled review refresh was absent from the
review-disabled set (the SDK skips manually-enabled providers), so
nostr-sync would fail OPEN for a review-rejected provider.

- GET /providers now derives disabled/manual status from the discovery
  adapter (the router's source of truth), like /providers/reviews,
  instead of the store mirror that lags scheduled review syncs.
  URL comparison is normalization-aware (adapter stores trailing-slash
  URLs).
- nostr-sync validates indices with Number.isInteger, returns 400 when
  no valid indices are given, dedupes, and reports skipped indices on
  partial success.
- CLI parses indices strictly (/^\d+$/) for disable/enable/nostr-sync;
  parseInt previously accepted '3abc' and '0x5'.
- Add src/daemon/http/providers.test.ts: regression coverage for the
  review-disabled -> manual-enable -> refresh -> nostr-sync lifecycle,
  list/routing/nostr-sync agreement, manual status labels, and
  fractional/duplicate/out-of-range indices.

src/cli.ts

@@ -1132,6 +1132,24 @@ const providersCmd = program
   .command("providers")
   .description("List and manage providers");
 
+// Strict index parsing: parseInt accepts garbage like "3abc" (-> 3) or
+// "0x5" (-> 0), which would silently act on the wrong provider.
+const parseProviderIndices = (raw: string[]): number[] => {
+  const valid: number[] = [];
+  const invalid: string[] = [];
+  for (const s of raw) {
+    if (/^\d+$/.test(s)) {
+      valid.push(parseInt(s, 10));
+    } else {
+      invalid.push(s);
+    }
+  }
+  if (invalid.length > 0) {
+    console.log(`Ignoring invalid index argument(s): ${invalid.join(", ")}`);
+  }
+  return valid;
+};
+
 providersCmd
   .command("list")
   .description("List all providers with their enabled/disabled status")
@@ -1188,9 +1206,7 @@ providersCmd
   .action(async (indices: string[]) => {
     await ensureDaemonRunning();
 
-    const indexNums = indices
-      .map((s) => parseInt(s, 10))
-      .filter((n) => Number.isFinite(n));
+    const indexNums = parseProviderIndices(indices);
     if (indexNums.length === 0) {
       console.log("No valid indices provided.");
       process.exit(1);
@@ -1225,9 +1241,7 @@ providersCmd
   .action(async (indices: string[]) => {
     await ensureDaemonRunning();
 
-    const indexNums = indices
-      .map((s) => parseInt(s, 10))
-      .filter((n) => Number.isFinite(n));
+    const indexNums = parseProviderIndices(indices);
     if (indexNums.length === 0) {
       console.log("No valid indices provided.");
       process.exit(1);
@@ -1262,9 +1276,7 @@ providersCmd
   .action(async (indices: string[]) => {
     await ensureDaemonRunning();
 
-    const indexNums = indices
-      .map((s) => parseInt(s, 10))
-      .filter((n) => Number.isFinite(n));
+    const indexNums = parseProviderIndices(indices);
     if (indexNums.length === 0) {
       console.log("No valid indices provided.");
       process.exit(1);
@@ -1284,6 +1296,7 @@ providersCmd
       | {
           message: string;
           providers: Array<{ baseUrl: string; disabled: boolean }>;
+          skipped?: unknown[];
         }
       | undefined;
     if (output) {
@@ -1294,6 +1307,9 @@ providersCmd
           : "enabled";
         console.log(`  - ${provider.baseUrl} -> ${status}`);
       }
+      if (output.skipped && output.skipped.length > 0) {
+        console.log(`Skipped invalid indices: ${output.skipped.join(", ")}`);
+      }
     }
   });
 

src/daemon/http/index.ts

@@ -1304,6 +1304,33 @@ export function createDaemonRequestHandler(deps: {
 
         const state = deps.store.getState();
         const baseUrlsList: string[] = state.baseUrlsList || [];
+
+        const validIndices: number[] = [];
+        const skippedIndices: unknown[] = [];
+        for (const idx of indices) {
+          if (
+            typeof idx === "number" &&
+            Number.isInteger(idx) &&
+            idx >= 0 &&
+            idx < baseUrlsList.length
+          ) {
+            if (!validIndices.includes(idx)) validIndices.push(idx);
+          } else {
+            skippedIndices.push(idx);
+          }
+        }
+
+        if (validIndices.length === 0) {
+          res.writeHead(400, { "Content-Type": "application/json" });
+          res.end(
+            JSON.stringify({
+              error: `No valid indices provided (valid range: 0-${Math.max(baseUrlsList.length - 1, 0)}).`,
+              skipped: skippedIndices,
+            }),
+          );
+          return;
+        }
+
         // Clearing both manual lists returns the provider to the
         // review-based state: the effective disabled set is the union of
         // disabledProviders (kind-38425 review sync) and the manual lists,
@@ -1316,23 +1343,17 @@ export function createDaemonRequestHandler(deps: {
         ];
 
         const synced: string[] = [];
-        for (const idx of indices) {
-          if (
-            typeof idx === "number" &&
-            idx >= 0 &&
-            idx < baseUrlsList.length
-          ) {
-            const baseUrl = baseUrlsList[idx]!;
-            const disabledPos = manuallyDisabledProviders.indexOf(baseUrl);
-            if (disabledPos !== -1) {
-              manuallyDisabledProviders.splice(disabledPos, 1);
-            }
-            const enabledPos = manuallyEnabledProviders.indexOf(baseUrl);
-            if (enabledPos !== -1) {
-              manuallyEnabledProviders.splice(enabledPos, 1);
-            }
-            synced.push(baseUrl);
+        for (const idx of validIndices) {
+          const baseUrl = baseUrlsList[idx]!;
+          const disabledPos = manuallyDisabledProviders.indexOf(baseUrl);
+          if (disabledPos !== -1) {
+            manuallyDisabledProviders.splice(disabledPos, 1);
           }
+          const enabledPos = manuallyEnabledProviders.indexOf(baseUrl);
+          if (enabledPos !== -1) {
+            manuallyEnabledProviders.splice(enabledPos, 1);
+          }
+          synced.push(baseUrl);
         }
 
         deps.store.getState().setManuallyDisabledProviders(manuallyDisabledProviders);
@@ -1340,17 +1361,30 @@ export function createDaemonRequestHandler(deps: {
         deps.store.getState().setManuallyEnabledProviders(manuallyEnabledProviders);
         deps.discoveryAdapter.setManuallyEnabledProviders(manuallyEnabledProviders);
 
-        // Report each synced provider's resulting state so the CLI can show
-        // whether Nostr reviews now have it enabled or disabled. Read from
-        // the discovery adapter (the source the router uses) after the
-        // writes above; with both manual lists cleared, its disabled set is
-        // exactly the review-based set.
+        // Recompute the review verdict now that the manual overrides are
+        // gone. This is load-bearing, not just cosmetic: the SDK's review
+        // sync skips manually-enabled providers when it rebuilds the
+        // review-disabled set, so a provider that was manually enabled
+        // across a review refresh is absent from that set — reading the
+        // stale set here would fail OPEN for a review-rejected provider.
+        const reviewed =
+          await deps.modelManager.syncReviewedProvidersFromNostr(baseUrlsList);
+        if (reviewed !== null) {
+          deps.store.getState().setDisabledProviders(reviewed);
+        }
+
+        // Report each synced provider's resulting state from the discovery
+        // adapter (the source the router uses). With both manual lists
+        // cleared for these URLs, membership in the adapter's disabled set
+        // is exactly the review verdict.
         const reviewDisabled = new Set(
           deps.discoveryAdapter.getDisabledProviders() || [],
         );
         const resulting = synced.map((baseUrl) => ({
           baseUrl,
-          disabled: reviewDisabled.has(baseUrl),
+          disabled:
+            reviewDisabled.has(baseUrl) ||
+            reviewDisabled.has(normalizeProviderBaseUrl(baseUrl)),
         }));
 
         res.writeHead(200, { "Content-Type": "application/json" });
@@ -1359,6 +1393,7 @@ export function createDaemonRequestHandler(deps: {
             output: {
               message: `Reverted ${synced.length} provider(s) to Nostr review-based state`,
               providers: resulting,
+              ...(skippedIndices.length > 0 ? { skipped: skippedIndices } : {}),
             },
           }),
         );
@@ -1570,25 +1605,33 @@ export function createDaemonRequestHandler(deps: {
 
         const state = deps.store.getState();
         const baseUrlsList: string[] = state.baseUrlsList || [];
-        const manuallyEnabled = new Set(
-          state.manuallyEnabledProviders || [],
+        // Read disable/override state from the discovery adapter (the same
+        // source the router/ProviderManager uses), not the mirror in the
+        // SdkStore, which can lag behind the scheduled kind-38425 review
+        // sync — that updates the adapter without touching the store.
+        const manuallyEnabled = new Set<string>(
+          (deps.discoveryAdapter.getManuallyEnabledProviders?.() || []) as string[],
         );
-        const manuallyDisabled = new Set(
-          state.manuallyDisabledProviders || [],
+        const manuallyDisabled = new Set<string>(
+          (deps.discoveryAdapter.getManuallyDisabledProviders?.() || []) as string[],
         );
-        const disabledProviders: string[] = [
-          ...new Set([
-            ...(state.disabledProviders || []),
-            ...(state.manuallyDisabledProviders || []),
-          ]),
-        ].filter((url) => !manuallyEnabled.has(url));
+        const disabledProviders = new Set<string>(
+          [
+            ...((deps.discoveryAdapter.getDisabledProviders() || []) as string[]),
+            ...manuallyDisabled,
+          ].filter((url) => !manuallyEnabled.has(url)),
+        );
+        // Adapter URLs are normalized (trailing slash); the store list may
+        // not be. Compare against both forms.
+        const inSet = (set: Set<string>, baseUrl: string) =>
+          set.has(baseUrl) || set.has(normalizeProviderBaseUrl(baseUrl));
 
         const providers = baseUrlsList.map((baseUrl, index) => ({
           index,
           baseUrl,
-          disabled: disabledProviders.includes(baseUrl),
-          manuallyDisabled: manuallyDisabled.has(baseUrl),
-          manuallyEnabled: manuallyEnabled.has(baseUrl),
+          disabled: inSet(disabledProviders, baseUrl),
+          manuallyDisabled: inSet(manuallyDisabled, baseUrl),
+          manuallyEnabled: inSet(manuallyEnabled, baseUrl),
         }));
 
         // Only count disabled providers that are actually in the current list

src/daemon/http/providers.test.ts

@@ -0,0 +1,271 @@
+import { describe, expect, it } from "bun:test";
+import { EventEmitter } from "events";
+import { createDaemonRequestHandler } from "./index";
+
+const PROVIDER_A = "https://provider-a.example/";
+const PROVIDER_B = "https://provider-b.example/";
+
+function makeReq(method: string, path: string, body?: unknown) {
+  const req = new EventEmitter() as any;
+  req.method = method;
+  req.url = path;
+  req.headers = { host: "localhost" };
+  // readBody attaches listeners synchronously; emit on the next tick.
+  setImmediate(() => {
+    if (body !== undefined) req.emit("data", Buffer.from(JSON.stringify(body)));
+    req.emit("end");
+  });
+  return req;
+}
+
+function makeRes() {
+  const res: any = {
+    status: 0,
+    body: "",
+    writeHead(status: number) {
+      res.status = status;
+      return res;
+    },
+    end(chunk?: string) {
+      if (chunk) res.body += chunk;
+      return res;
+    },
+    json() {
+      return JSON.parse(res.body);
+    },
+  };
+  return res;
+}
+
+function makeStore(baseUrlsList: string[]) {
+  const state: any = {
+    baseUrlsList,
+    disabledProviders: [] as string[],
+    manuallyDisabledProviders: [] as string[],
+    manuallyEnabledProviders: [] as string[],
+    setDisabledProviders: (urls: string[]) => {
+      state.disabledProviders = urls;
+    },
+    setManuallyDisabledProviders: (urls: string[]) => {
+      state.manuallyDisabledProviders = urls;
+    },
+    setManuallyEnabledProviders: (urls: string[]) => {
+      state.manuallyEnabledProviders = urls;
+    },
+  };
+  return { getState: () => state, state };
+}
+
+/**
+ * Mimics the SDK discovery adapter: URLs are normalized, the effective
+ * disabled set is review-disabled ∪ manually-disabled minus
+ * manually-enabled, and setDisabledProviders *replaces* the review set.
+ */
+function makeAdapter(baseUrlsList: string[]) {
+  let reviewDisabled: string[] = [];
+  let manualDisabled: string[] = [];
+  let manualEnabled: string[] = [];
+  return {
+    getBaseUrlsList: () => [...baseUrlsList],
+    getDisabledProviders: () =>
+      [...new Set([...reviewDisabled, ...manualDisabled])].filter(
+        (u) => !manualEnabled.includes(u),
+      ),
+    getManuallyDisabledProviders: () => [...manualDisabled],
+    getManuallyEnabledProviders: () => [...manualEnabled],
+    setDisabledProviders: (urls: string[]) => {
+      reviewDisabled = [...urls];
+    },
+    setManuallyDisabledProviders: (urls: string[]) => {
+      manualDisabled = [...urls];
+    },
+    setManuallyEnabledProviders: (urls: string[]) => {
+      manualEnabled = [...urls];
+    },
+    // Test hook: what the kind-38425 review sync would compute.
+    _setReviewDisabled: (urls: string[]) => {
+      reviewDisabled = [...urls];
+    },
+    _getReviewDisabled: () => [...reviewDisabled],
+  };
+}
+
+/**
+ * Mimics ModelManager.syncReviewedProvidersFromNostr semantics: rebuild the
+ * review-disabled set from the current verdicts, SKIPPING manually enabled
+ * providers (this skip is what makes a stale disabled set fail open).
+ */
+function makeModelManager(
+  adapter: ReturnType<typeof makeAdapter>,
+  reviewRejected: Set<string>,
+) {
+  return {
+    syncReviewedProvidersFromNostr: async (baseUrls?: string[]) => {
+      const urls = baseUrls ?? adapter.getBaseUrlsList();
+      const enabled = new Set(adapter.getManuallyEnabledProviders());
+      const recomputed = urls.filter(
+        (u) => reviewRejected.has(u) && !enabled.has(u),
+      );
+      adapter.setDisabledProviders(recomputed);
+      return recomputed;
+    },
+  };
+}
+
+function makeDeps(overrides: {
+  baseUrlsList?: string[];
+  reviewRejected?: Set<string>;
+}) {
+  const baseUrlsList = overrides.baseUrlsList ?? [PROVIDER_A, PROVIDER_B];
+  const store = makeStore(baseUrlsList);
+  const adapter = makeAdapter(baseUrlsList);
+  const reviewRejected = overrides.reviewRejected ?? new Set<string>();
+  const modelManager = makeModelManager(adapter, reviewRejected);
+  const handler = createDaemonRequestHandler({
+    store,
+    discoveryAdapter: adapter,
+    modelManager,
+  } as any);
+  return { handler, store, adapter, modelManager, reviewRejected };
+}
+
+async function call(
+  handler: Function,
+  method: string,
+  path: string,
+  body?: unknown,
+) {
+  const res = makeRes();
+  await handler(makeReq(method, path, body), res);
+  return res;
+}
+
+describe("GET /providers", () => {
+  it("derives status from the discovery adapter, not the stale store mirror", async () => {
+    const { handler, adapter } = makeDeps({
+      reviewRejected: new Set([PROVIDER_B]),
+    });
+    // Simulate a scheduled review sync that updated ONLY the adapter
+    // (the store mirror lags behind by design).
+    adapter._setReviewDisabled([PROVIDER_B]);
+
+    const res = await call(handler, "GET", "/providers");
+    expect(res.status).toBe(200);
+    const { providers } = res.json().output;
+    expect(providers[0]).toMatchObject({
+      baseUrl: PROVIDER_A,
+      disabled: false,
+      manuallyDisabled: false,
+      manuallyEnabled: false,
+    });
+    expect(providers[1]).toMatchObject({
+      baseUrl: PROVIDER_B,
+      disabled: true,
+      manuallyDisabled: false,
+      manuallyEnabled: false,
+    });
+  });
+
+  it("reports manual enable/disable flags from the adapter", async () => {
+    const { handler, adapter } = makeDeps({});
+    adapter.setManuallyDisabledProviders([PROVIDER_A]);
+    adapter.setManuallyEnabledProviders([PROVIDER_B]);
+
+    const res = await call(handler, "GET", "/providers");
+    const { providers } = res.json().output;
+    expect(providers[0]).toMatchObject({
+      disabled: true,
+      manuallyDisabled: true,
+      manuallyEnabled: false,
+    });
+    expect(providers[1]).toMatchObject({
+      disabled: false,
+      manuallyDisabled: false,
+      manuallyEnabled: true,
+    });
+  });
+});
+
+describe("POST /providers/nostr-sync", () => {
+  it("re-enables nothing fail-open: recomputes the review verdict after clearing overrides", async () => {
+    // Scenario from review: review-disabled -> manual enable -> scheduled
+    // review refresh (adapter review set loses the provider) -> nostr-sync.
+    const { handler, store, adapter, modelManager, reviewRejected } =
+      makeDeps({ reviewRejected: new Set([PROVIDER_A]) });
+    adapter._setReviewDisabled([PROVIDER_A]);
+    store.state.disabledProviders = [PROVIDER_A];
+
+    // User manually enables the review-rejected provider.
+    const enableRes = await call(handler, "POST", "/providers/enable", {
+      indices: [0],
+    });
+    expect(enableRes.status).toBe(200);
+    expect(adapter.getDisabledProviders()).not.toContain(PROVIDER_A);
+
+    // Scheduled review refresh runs: SDK skips manually-enabled providers
+    // and overwrites the review-disabled set without PROVIDER_A.
+    await modelManager.syncReviewedProvidersFromNostr();
+    expect(adapter._getReviewDisabled()).toEqual([]);
+
+    // nostr-sync must restore the review verdict, not the stale empty set.
+    const res = await call(handler, "POST", "/providers/nostr-sync", {
+      indices: [0],
+    });
+    expect(res.status).toBe(200);
+    const out = res.json().output;
+    expect(out.providers).toEqual([{ baseUrl: PROVIDER_A, disabled: true }]);
+
+    // Routing state and the store mirror must agree with the verdict.
+    expect(adapter.getDisabledProviders()).toContain(PROVIDER_A);
+    expect(store.state.disabledProviders).toContain(PROVIDER_A);
+    expect(adapter.getManuallyEnabledProviders()).toEqual([]);
+    expect(adapter.getManuallyDisabledProviders()).toEqual([]);
+
+    // And providers list must agree with routing.
+    const listRes = await call(handler, "GET", "/providers");
+    const { providers } = listRes.json().output;
+    expect(providers[0].disabled).toBe(true);
+  });
+
+  it("returns a manually review-clean provider to enabled", async () => {
+    const { handler, adapter } = makeDeps({});
+    adapter.setManuallyDisabledProviders([PROVIDER_B]);
+
+    const res = await call(handler, "POST", "/providers/nostr-sync", {
+      indices: [1],
+    });
+    expect(res.status).toBe(200);
+    expect(res.json().output.providers).toEqual([
+      { baseUrl: PROVIDER_B, disabled: false },
+    ]);
+    expect(adapter.getManuallyDisabledProviders()).toEqual([]);
+  });
+
+  it("rejects requests with no valid indices", async () => {
+    const { handler } = makeDeps({});
+    for (const indices of [[0.5], [-1], [99], ["x"], [0.5, 99]]) {
+      const res = await call(handler, "POST", "/providers/nostr-sync", {
+        indices,
+      });
+      expect(res.status).toBe(400);
+      expect(res.json().error).toContain("No valid indices");
+    }
+  });
+
+  it("reports skipped indices on partial success and dedupes", async () => {
+    const { handler } = makeDeps({});
+    const res = await call(handler, "POST", "/providers/nostr-sync", {
+      indices: [0, 99, 0.5, 0],
+    });
+    expect(res.status).toBe(200);
+    const out = res.json().output;
+    expect(out.providers).toEqual([{ baseUrl: PROVIDER_A, disabled: false }]);
+    expect(out.skipped).toEqual([99, 0.5]);
+  });
+
+  it("rejects a missing indices field", async () => {
+    const { handler } = makeDeps({});
+    const res = await call(handler, "POST", "/providers/nostr-sync", {});
+    expect(res.status).toBe(400);
+  });
+});