diff --git a/src/cli.ts b/src/cli.ts index 55807b9..1d0d2a8 100644 --- a/src/cli.ts +++ b/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(", ")}`); + } } }); diff --git a/src/daemon/http/index.ts b/src/daemon/http/index.ts index 71a2be2..9991e5d 100644 --- a/src/daemon/http/index.ts +++ b/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( + (deps.discoveryAdapter.getManuallyEnabledProviders?.() || []) as string[], ); - const manuallyDisabled = new Set( - state.manuallyDisabledProviders || [], + const manuallyDisabled = new Set( + (deps.discoveryAdapter.getManuallyDisabledProviders?.() || []) as string[], ); - const disabledProviders: string[] = [ - ...new Set([ - ...(state.disabledProviders || []), - ...(state.manuallyDisabledProviders || []), - ]), - ].filter((url) => !manuallyEnabled.has(url)); + const disabledProviders = new Set( + [ + ...((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, 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 diff --git a/src/daemon/http/providers.test.ts b/src/daemon/http/providers.test.ts new file mode 100644 index 0000000..21fdba2 --- /dev/null +++ b/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, + reviewRejected: Set, +) { + 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; +}) { + const baseUrlsList = overrides.baseUrlsList ?? [PROVIDER_A, PROVIDER_B]; + const store = makeStore(baseUrlsList); + const adapter = makeAdapter(baseUrlsList); + const reviewRejected = overrides.reviewRejected ?? new Set(); + 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); + }); +});