fix: address Greptile review — entry validation, size limit, DRY, no-op saves

- Validate individual skill entries in catalog (id, version, content
  must all be strings) not just the top-level shape
- Add 1MB response size limit on catalog fetch to prevent resource
  exhaustion from compromised/misconfigured CDN
- Skip manifest save when sync cycle had no changes (avoids
  unnecessary disk I/O every 45 minutes)
- Share extractVersion via remote-sync.ts export, remove duplicate
  from seed.ts

Author: shivammittal274

packages/browseros-agent/apps/server/src/skills/remote-sync.ts

@@ -1,6 +1,7 @@
 import { createHash } from 'node:crypto'
 import { mkdir, readFile, writeFile } from 'node:fs/promises'
 import { join } from 'node:path'
+import { SKILLS_LIMITS } from '@browseros/shared/constants/limits'
 import { TIMEOUTS } from '@browseros/shared/constants/timeouts'
 import { EXTERNAL_URLS } from '@browseros/shared/constants/urls'
 import { INLINED_ENV } from '../env'
@@ -18,6 +19,11 @@ export const MANIFEST_FILE = '.remote-manifest.json'
 
 let syncTimer: ReturnType<typeof setInterval> | null = null
 
+export function extractVersion(content: string): string {
+  const match = content.match(/^\s*version:\s*["']?([^"'\n]+)["']?/m)
+  return match?.[1]?.trim() || '1.0'
+}
+
 export function contentHash(content: string): string {
   return createHash('sha256').update(content).digest('hex')
 }
@@ -32,10 +38,24 @@ function isValidManifest(data: unknown): data is SkillManifest {
   return typeof d.lastSyncedAt === 'string' && typeof d.skills === 'object' && d.skills !== null
 }
 
+function isValidSkillEntry(entry: unknown): entry is RemoteSkillEntry {
+  if (typeof entry !== 'object' || entry === null) return false
+  const e = entry as Record<string, unknown>
+  return (
+    typeof e.id === 'string' &&
+    typeof e.version === 'string' &&
+    typeof e.content === 'string'
+  )
+}
+
 function isValidCatalog(data: unknown): data is RemoteSkillCatalog {
   if (typeof data !== 'object' || data === null) return false
   const d = data as Record<string, unknown>
-  return typeof d.version === 'number' && Array.isArray(d.skills)
+  return (
+    typeof d.version === 'number' &&
+    Array.isArray(d.skills) &&
+    d.skills.every(isValidSkillEntry)
+  )
 }
 
 export async function loadManifest(): Promise<SkillManifest> {
@@ -71,7 +91,14 @@ export async function fetchRemoteCatalog(): Promise<RemoteSkillCatalog | null> {
       })
       return null
     }
-    const data: unknown = await response.json()
+    const text = await response.text()
+    if (text.length > SKILLS_LIMITS.MAX_CATALOG_BYTES) {
+      logger.warn('Remote skill catalog response too large', {
+        size: text.length,
+      })
+      return null
+    }
+    const data: unknown = JSON.parse(text)
     if (!isValidCatalog(data)) {
       logger.warn('Remote skill catalog has invalid format')
       return null
@@ -171,8 +198,10 @@ export async function syncRemoteSkills(): Promise<{
     }
   }
 
-  manifest.lastSyncedAt = new Date().toISOString()
-  await saveManifest(manifest)
+  if (result.installed > 0 || result.updated > 0) {
+    manifest.lastSyncedAt = new Date().toISOString()
+    await saveManifest(manifest)
+  }
 
   return result
 }

packages/browseros-agent/apps/server/src/skills/seed.ts

@@ -4,6 +4,7 @@ import { logger } from '../lib/logger'
 import { DEFAULT_SKILLS } from './defaults'
 import {
   contentHash,
+  extractVersion,
   installSkill,
   loadManifest,
   saveManifest,
@@ -20,11 +21,6 @@ async function hasExistingSkills(skillsDir: string): Promise<boolean> {
   }
 }
 
-function extractVersion(content: string): string {
-  const match = content.match(/^\s*version:\s*["']?([^"'\n]+)["']?/m)
-  return match?.[1]?.trim() || '1.0'
-}
-
 export async function seedDefaultSkills(): Promise<void> {
   const skillsDir = getSkillsDir()
   if (await hasExistingSkills(skillsDir)) return

packages/browseros-agent/apps/server/tests/skills/remote-sync.test.ts

@@ -139,6 +139,31 @@ describe('fetchRemoteCatalog', () => {
     fetchSpy.mockRestore()
   })
 
+  it('returns null when skill entries have invalid shape', async () => {
+    const fetchSpy = spyOn(globalThis, 'fetch').mockResolvedValue(
+      new Response(
+        JSON.stringify({
+          version: 1,
+          skills: [{ id: 123, version: '1.0', content: null }],
+        }),
+        { status: 200 },
+      ),
+    )
+    const result = await fetchRemoteCatalog()
+    assert.strictEqual(result, null)
+    fetchSpy.mockRestore()
+  })
+
+  it('returns null for oversized response', async () => {
+    const huge = 'x'.repeat(1_100_000)
+    const fetchSpy = spyOn(globalThis, 'fetch').mockResolvedValue(
+      new Response(huge, { status: 200 }),
+    )
+    const result = await fetchRemoteCatalog()
+    assert.strictEqual(result, null)
+    fetchSpy.mockRestore()
+  })
+
   it('returns null when skills field is missing', async () => {
     const fetchSpy = spyOn(globalThis, 'fetch').mockResolvedValue(
       new Response(JSON.stringify({ version: 1 }), { status: 200 }),

packages/browseros-agent/packages/shared/src/constants/limits.ts

@@ -78,6 +78,10 @@ export const CDP_LIMITS = {
   RECONNECT_MAX_RETRIES: 3,
 } as const
 
+export const SKILLS_LIMITS = {
+  MAX_CATALOG_BYTES: 1_000_000,
+} as const
+
 export const CONTENT_LIMITS = {
   BODY_CONTEXT_SIZE: 10_000,
   MAX_QUEUE_SIZE: 1_000,