fix(api): restore safe non-suspicious behavior

Author: ngutman

convex/httpApi.handlers.test.ts

@@ -112,6 +112,22 @@ describe('httpApi handlers', () => {
     })
   })
 
+  it('searchSkillsHttp prefers canonical nonSuspiciousOnly over legacy alias', async () => {
+    const runAction = vi.fn().mockResolvedValue([])
+    await __handlers.searchSkillsHandler(
+      makeCtx({ runAction }),
+      new Request(
+        'https://example.com/api/search?q=test&nonSuspiciousOnly=false&nonSuspicious=1',
+      ),
+    )
+    expect(runAction).toHaveBeenCalledWith(expect.anything(), {
+      query: 'test',
+      limit: undefined,
+      highlightedOnly: undefined,
+      nonSuspiciousOnly: undefined,
+    })
+  })
+
   it('getSkillHttp validates slug', async () => {
     const response = await __handlers.getSkillHandler(
       makeCtx({ runQuery: vi.fn() }),

convex/httpApi.ts

@@ -12,7 +12,7 @@ import type { ActionCtx } from './_generated/server'
 import { httpAction } from './functions'
 import { requireApiTokenUser } from './lib/apiTokenAuth'
 import { corsHeaders, mergeHeaders } from './lib/httpHeaders'
-import { parseBooleanQueryParam } from './lib/httpUtils'
+import { parseBooleanQueryParam, resolveBooleanQueryParam } from './lib/httpUtils'
 import { publishVersionForUser } from './skills'
 
 type SearchSkillEntry = {
@@ -47,9 +47,10 @@ async function searchSkillsHandler(ctx: ActionCtx, request: Request) {
   const limit = toOptionalNumber(url.searchParams.get('limit'))
   const approvedOnly = parseBooleanQueryParam(url.searchParams.get('approvedOnly'))
   const highlightedOnly = parseBooleanQueryParam(url.searchParams.get('highlightedOnly')) || approvedOnly
-  const nonSuspiciousOnly =
-    parseBooleanQueryParam(url.searchParams.get('nonSuspiciousOnly')) ||
-    parseBooleanQueryParam(url.searchParams.get('nonSuspicious'))
+  const nonSuspiciousOnly = resolveBooleanQueryParam(
+    url.searchParams.get('nonSuspiciousOnly'),
+    url.searchParams.get('nonSuspicious'),
+  )
 
   if (!query) return json({ results: [] })
 
@@ -168,7 +169,7 @@ async function cliPublishHandler(ctx: ActionCtx, request: Request) {
   try {
     const { userId } = await requireApiTokenUser(ctx, request)
     const args = parsePublishBody(body)
-    if (args.acceptLicenseTerms !== true) {
+    if (!hasAcceptedLegacyLicenseTerms(args.acceptLicenseTerms)) {
       return text('MIT-0 license terms must be accepted to publish skills', 400)
     }
     const result = await publishVersionForUser(ctx, userId, args)
@@ -180,6 +181,10 @@ async function cliPublishHandler(ctx: ActionCtx, request: Request) {
   }
 }
 
+function hasAcceptedLegacyLicenseTerms(acceptLicenseTerms: boolean | undefined) {
+  return acceptLicenseTerms !== false
+}
+
 export const cliPublishHttp = httpAction(cliPublishHandler)
 
 async function cliSkillDeleteHandler(ctx: ActionCtx, request: Request, deleted: boolean) {

convex/httpApiV1.handlers.test.ts

@@ -265,6 +265,26 @@ describe('httpApiV1 handlers', () => {
     })
   })
 
+  it('search prefers canonical nonSuspiciousOnly over legacy alias', async () => {
+    const runAction = vi.fn().mockResolvedValue([])
+    const runMutation = vi.fn().mockResolvedValue(okRate())
+    const response = await __handlers.searchSkillsV1Handler(
+      makeCtx({ runAction, runMutation }),
+      new Request(
+        'https://example.com/api/v1/search?q=test&nonSuspiciousOnly=false&nonSuspicious=1',
+      ),
+    )
+    if (response.status !== 200) {
+      throw new Error(await response.text())
+    }
+    expect(runAction).toHaveBeenCalledWith(expect.anything(), {
+      query: 'test',
+      limit: undefined,
+      highlightedOnly: undefined,
+      nonSuspiciousOnly: undefined,
+    })
+  })
+
   it('search rate limits', async () => {
     const runMutation = vi.fn().mockResolvedValue(blockedRate())
     const response = await __handlers.searchSkillsV1Handler(
@@ -623,6 +643,24 @@ describe('httpApiV1 handlers', () => {
     expect(response.status).toBe(200)
   })
 
+  it('lists skills prefers canonical nonSuspiciousOnly over legacy alias', async () => {
+    const runQuery = vi.fn(async (_query: unknown, args: Record<string, unknown>) => {
+      if ('sort' in args || 'cursor' in args || 'limit' in args) {
+        expect(args.nonSuspiciousOnly).toBeUndefined()
+        return { items: [], nextCursor: null }
+      }
+      return null
+    })
+    const runMutation = vi.fn().mockResolvedValue(okRate())
+    const response = await __handlers.listSkillsV1Handler(
+      makeCtx({ runQuery, runMutation }),
+      new Request(
+        'https://example.com/api/v1/skills?nonSuspiciousOnly=false&nonSuspicious=1',
+      ),
+    )
+    expect(response.status).toBe(200)
+  })
+
   it('get skill returns 404 when missing', async () => {
     const runQuery = vi.fn().mockResolvedValue(null)
     const runMutation = vi.fn().mockResolvedValue(okRate())

convex/httpApiV1/skillsV1.ts

@@ -2,7 +2,7 @@ import { api, internal } from '../_generated/api'
 import type { Doc, Id } from '../_generated/dataModel'
 import type { ActionCtx } from '../_generated/server'
 import { getOptionalApiTokenUserId, requireApiTokenUser } from '../lib/apiTokenAuth'
-import { parseBooleanQueryParam } from '../lib/httpUtils'
+import { parseBooleanQueryParam, resolveBooleanQueryParam } from '../lib/httpUtils'
 import { applyRateLimit, parseBearerToken } from '../lib/httpRateLimit'
 import { publishVersionForUser } from '../skills'
 import {
@@ -327,9 +327,10 @@ export async function searchSkillsV1Handler(ctx: ActionCtx, request: Request) {
   const query = url.searchParams.get('q')?.trim() ?? ''
   const limit = toOptionalNumber(url.searchParams.get('limit'))
   const highlightedOnly = parseBooleanQueryParam(url.searchParams.get('highlightedOnly'))
-  const nonSuspiciousOnly =
-    parseBooleanQueryParam(url.searchParams.get('nonSuspiciousOnly')) ||
-    parseBooleanQueryParam(url.searchParams.get('nonSuspicious'))
+  const nonSuspiciousOnly = resolveBooleanQueryParam(
+    url.searchParams.get('nonSuspiciousOnly'),
+    url.searchParams.get('nonSuspicious'),
+  )
 
   if (!query) return json({ results: [] }, 200, rate.headers)
 
@@ -408,9 +409,10 @@ export async function listSkillsV1Handler(ctx: ActionCtx, request: Request) {
   const rawCursor = url.searchParams.get('cursor')?.trim() || undefined
   const sort = parseListSort(url.searchParams.get('sort'))
   const cursor = sort === 'trending' ? undefined : rawCursor
-  const nonSuspiciousOnly =
-    parseBooleanQueryParam(url.searchParams.get('nonSuspiciousOnly')) ||
-    parseBooleanQueryParam(url.searchParams.get('nonSuspicious'))
+  const nonSuspiciousOnly = resolveBooleanQueryParam(
+    url.searchParams.get('nonSuspiciousOnly'),
+    url.searchParams.get('nonSuspicious'),
+  )
 
   const result = (await ctx.runQuery(api.skills.listPublicPage, {
     limit,
@@ -846,7 +848,7 @@ export async function publishSkillV1Handler(ctx: ActionCtx, request: Request) {
     if (contentType.includes('application/json')) {
       const body = await request.json()
       const payload = parsePublishBody(body)
-      if (payload.acceptLicenseTerms !== true) {
+      if (!hasAcceptedLegacyLicenseTerms(payload.acceptLicenseTerms)) {
         return text('MIT-0 license terms must be accepted to publish skills', 400, rate.headers)
       }
       const result = await publishVersionForUser(ctx, userId, payload)
@@ -855,7 +857,7 @@ export async function publishSkillV1Handler(ctx: ActionCtx, request: Request) {
 
     if (contentType.includes('multipart/form-data')) {
       const payload = await parseMultipartPublish(ctx, request)
-      if (payload.acceptLicenseTerms !== true) {
+      if (!hasAcceptedLegacyLicenseTerms(payload.acceptLicenseTerms)) {
         return text('MIT-0 license terms must be accepted to publish skills', 400, rate.headers)
       }
       const result = await publishVersionForUser(ctx, userId, payload)
@@ -869,6 +871,10 @@ export async function publishSkillV1Handler(ctx: ActionCtx, request: Request) {
   return text('Unsupported content type', 415, rate.headers)
 }
 
+function hasAcceptedLegacyLicenseTerms(acceptLicenseTerms: boolean | undefined) {
+  return acceptLicenseTerms !== false
+}
+
 type TransferDecisionAction = 'accept' | 'reject' | 'cancel'
 
 function transferErrorToResponse(error: unknown, headers: HeadersInit) {

convex/leaderboards.ts

@@ -1,13 +1,15 @@
 import { v } from 'convex/values'
 import { internal } from './_generated/api'
-import type { Id } from './_generated/dataModel'
 import { internalAction, internalMutation, internalQuery } from './functions'
 import {
-  buildTrendingLeaderboard,
-  compareTrendingEntries,
+  buildTrendingEntriesFromDailyRows,
+  buildTrendingEntryCandidates,
   getTrendingRange,
   queryDailyStats,
-  topN,
+  takeTopNonSuspiciousTrendingEntries,
+  takeTopTrendingEntries,
+  TRENDING_LEADERBOARD_KIND,
+  TRENDING_NON_SUSPICIOUS_LEADERBOARD_KIND,
 } from './lib/leaderboards'
 
 const MAX_TRENDING_LIMIT = 200
@@ -26,9 +28,27 @@ export const getDailyStats = internalQuery({
   },
 })
 
+export const filterTopNonSuspiciousTrendingEntries = internalQuery({
+  args: {
+    entries: v.array(
+      v.object({
+        skillId: v.id('skills'),
+        score: v.number(),
+        installs: v.number(),
+        downloads: v.number(),
+      }),
+    ),
+    limit: v.number(),
+  },
+  handler: async (ctx, { entries, limit }) => {
+    return takeTopNonSuspiciousTrendingEntries(ctx, entries, limit)
+  },
+})
+
 /** Writes the pre-computed leaderboard and prunes old entries. */
 export const writeTrendingLeaderboard = internalMutation({
   args: {
+    kind: v.string(),
     items: v.array(
       v.object({
         skillId: v.id('skills'),
@@ -40,11 +60,11 @@ export const writeTrendingLeaderboard = internalMutation({
     startDay: v.number(),
     endDay: v.number(),
   },
-  handler: async (ctx, { items, startDay, endDay }) => {
+  handler: async (ctx, { kind, items, startDay, endDay }) => {
     const now = Date.now()
 
     await ctx.db.insert('skillLeaderboards', {
-      kind: 'trending',
+      kind,
       generatedAt: now,
       rangeStartDay: startDay,
       rangeEndDay: endDay,
@@ -53,7 +73,7 @@ export const writeTrendingLeaderboard = internalMutation({
 
     const recent = await ctx.db
       .query('skillLeaderboards')
-      .withIndex('by_kind', (q) => q.eq('kind', 'trending'))
+      .withIndex('by_kind', (q) => q.eq('kind', kind))
       .order('desc')
       .take(KEEP_LEADERBOARD_ENTRIES + 5)
 
@@ -70,39 +90,32 @@ export const rebuildTrendingLeaderboardAction = internalAction({
   args: { limit: v.optional(v.number()) },
   handler: async (ctx, args): Promise<{ ok: true; count: number }> => {
     const limit = clampInt(args.limit ?? MAX_TRENDING_LIMIT, 1, MAX_TRENDING_LIMIT)
-    const { startDay, endDay } = getTrendingRange(Date.now())
-
+    const now = Date.now()
+    const { startDay, endDay } = getTrendingRange(now)
     const dayKeys = Array.from({ length: endDay - startDay + 1 }, (_, i) => startDay + i)
     const perDayRows = await Promise.all(
       dayKeys.map((day) => ctx.runQuery(internal.leaderboards.getDailyStats, { day })),
     )
-
-    const totals = new Map<string, { installs: number; downloads: number }>()
-    for (const rows of perDayRows) {
-      for (const row of rows) {
-        const current = totals.get(row.skillId) ?? { installs: 0, downloads: 0 }
-        current.installs += row.installs
-        current.downloads += row.downloads
-        totals.set(row.skillId, current)
-      }
-    }
-
-    const entries = Array.from(totals, ([skillId, t]) => ({
-      skillId: skillId as Id<'skills'>,
-      installs: t.installs,
-      downloads: t.downloads,
-      score: t.installs,
-    }))
-
-    const items = topN(entries, limit, compareTrendingEntries).sort((a, b) =>
-      compareTrendingEntries(b, a),
+    const entries = buildTrendingEntriesFromDailyRows(perDayRows)
+    const items = takeTopTrendingEntries(entries, limit)
+    const nonSuspicious = await ctx.runQuery(
+      internal.leaderboards.filterTopNonSuspiciousTrendingEntries,
+      { entries, limit },
     )
 
-    return await ctx.runMutation(internal.leaderboards.writeTrendingLeaderboard, {
+    await ctx.runMutation(internal.leaderboards.writeTrendingLeaderboard, {
+      kind: TRENDING_LEADERBOARD_KIND,
       items,
       startDay,
       endDay,
     })
+    await ctx.runMutation(internal.leaderboards.writeTrendingLeaderboard, {
+      kind: TRENDING_NON_SUSPICIOUS_LEADERBOARD_KIND,
+      items: nonSuspicious,
+      startDay,
+      endDay,
+    })
+    return { ok: true as const, count: items.length }
   },
 })
 
@@ -115,24 +128,37 @@ export const rebuildTrendingLeaderboardInternal = internalMutation({
   handler: async (ctx, args) => {
     const limit = clampInt(args.limit ?? MAX_TRENDING_LIMIT, 1, MAX_TRENDING_LIMIT)
     const now = Date.now()
-    const { startDay, endDay, items } = await buildTrendingLeaderboard(ctx, { limit, now })
+    const { startDay, endDay, entries } = await buildTrendingEntryCandidates(ctx, now)
+    const items = takeTopTrendingEntries(entries, limit)
+    const nonSuspicious = await takeTopNonSuspiciousTrendingEntries(ctx, entries, limit)
 
     await ctx.db.insert('skillLeaderboards', {
-      kind: 'trending',
+      kind: TRENDING_LEADERBOARD_KIND,
       generatedAt: now,
       rangeStartDay: startDay,
       rangeEndDay: endDay,
       items,
     })
+    await ctx.db.insert('skillLeaderboards', {
+      kind: TRENDING_NON_SUSPICIOUS_LEADERBOARD_KIND,
+      generatedAt: now,
+      rangeStartDay: startDay,
+      rangeEndDay: endDay,
+      items: nonSuspicious,
+    })
 
-    const recent = await ctx.db
-      .query('skillLeaderboards')
-      .withIndex('by_kind', (q) => q.eq('kind', 'trending'))
-      .order('desc')
-      .take(KEEP_LEADERBOARD_ENTRIES + 5)
-
-    for (const entry of recent.slice(KEEP_LEADERBOARD_ENTRIES)) {
-      await ctx.db.delete(entry._id)
+    for (const kind of [
+      TRENDING_LEADERBOARD_KIND,
+      TRENDING_NON_SUSPICIOUS_LEADERBOARD_KIND,
+    ]) {
+      const entriesForKind = await ctx.db
+        .query('skillLeaderboards')
+        .withIndex('by_kind', (q) => q.eq('kind', kind))
+        .order('desc')
+        .take(KEEP_LEADERBOARD_ENTRIES + 5)
+      for (const entry of entriesForKind.slice(KEEP_LEADERBOARD_ENTRIES)) {
+        await ctx.db.delete(entry._id)
+      }
     }
 
     return { ok: true as const, count: items.length }

convex/lib/httpUtils.test.ts

@@ -1,5 +1,9 @@
 import { describe, expect, it } from 'vitest'
-import { parseBooleanQueryParam } from './httpUtils'
+import {
+  parseBooleanQueryParam,
+  parseBooleanQueryParamOptional,
+  resolveBooleanQueryParam,
+} from './httpUtils'
 
 describe('parseBooleanQueryParam', () => {
   it('returns true for true-like values', () => {
@@ -15,4 +19,17 @@ describe('parseBooleanQueryParam', () => {
     expect(parseBooleanQueryParam('0')).toBe(false)
     expect(parseBooleanQueryParam('yes')).toBe(false)
   })
+
+  it('supports optional parsing for precedence-sensitive callers', () => {
+    expect(parseBooleanQueryParamOptional(null)).toBeUndefined()
+    expect(parseBooleanQueryParamOptional('false')).toBe(false)
+    expect(parseBooleanQueryParamOptional('1')).toBe(true)
+  })
+
+  it('prefers the primary param over the legacy alias when both are present', () => {
+    expect(resolveBooleanQueryParam('false', '1')).toBe(false)
+    expect(resolveBooleanQueryParam('true', '0')).toBe(true)
+    expect(resolveBooleanQueryParam(null, '1')).toBe(true)
+    expect(resolveBooleanQueryParam(null, null)).toBeUndefined()
+  })
 })