feat(enrichment): add screenshot pipeline with browser fetch and storage - #2708
Conversation
SafeDep Report Summary
No dependency changes detected. Nothing to scan. This report is generated by SafeDep Github App |
- Add screenshot pipeline service for capturing page screenshots via headless browser - Add browser fetch service with Puppeteer-based rendering - Add screenshot storage service with local filesystem support - Add screenshot repository with DB schema migration (0011) - Add enrichment config schema fields for screenshot settings - Update open-graph provider to support screenshot capture - Enhance safe-fetch with configurable retries and user agent - Add comprehensive test suite for all new services - Update Dockerfile with Chromium and Puppeteer dependencies - Add design docs for screenshot and knowledge base features
8fb2cce to
4061d14
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5468e1797d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| internal.live = true | ||
| this.scheduleIdleClose(internal) | ||
| } | ||
| this.flushWaiter() |
There was a problem hiding this comment.
When release(..., { discard: true }) is called, closeSlot() is fired asynchronously and then flushWaiter() runs immediately, so a queued acquire can receive the same slot before its agent-browser ... close finishes. Because closeSlot() later removes that slot from this.slots, the waiter may run on a session being torn down and then fail to return capacity on release (no matching internal slot), which can shrink effective pool size and cause stuck acquires under error bursts.
Useful? React with 👍 / 👎.
| const usage = await this.repository.getQuotaUsage() | ||
|
|
||
| let projectedCount = usage.count + addedItem | ||
| let projectedBytes = usage.totalBytes - existingBytes + newBytes | ||
|
|
There was a problem hiding this comment.
Quota enforcement is computed from a point-in-time getQuotaUsage() snapshot, but there is no lock/transaction spanning this check and the subsequent S3 upload + DB upsert. Two concurrent storeOrEvict calls can both observe headroom and proceed, causing maxItems/maxTotalBytes to be exceeded despite each call individually passing checks; this breaks the configured storage cap under normal parallel traffic.
Useful? React with 👍 / 👎.
…SSRF bypass CLI 0.26.0's batch sub-command parser rejects `eval -b <b64> --json` when `--json` is also on the sub-command, and misroutes screenshot `--screenshot-*` flags as a [selector]. Drop the inner `--json` on eval, and run viewport + screenshot as two standalone invocations against the same `--session` instead of a batch. webp is not a CLI output format; fall back to jpeg (sharp still re-encodes to webp downstream). Add an `isDev` short-circuit in `assertHostnameSafe` so local SSRF DNS validation does not reject loopback / fakeIp proxy resolvers like Surge. Add a live spec gated by `LIVE_BROWSER_FETCH=1` for end-to-end regression coverage.
Two issues surfaced in the codex review of #2708. BrowserSessionPool: `release({ discard: true })` fired `closeSlot()` asynchronously then immediately ran `flushWaiter`, so a queued acquire could pick up the same slot while its `agent-browser ... close` was still in flight and `slots.splice(...)` was pending — the waiter would run on a session being torn down and then leak capacity when its own release found no matching internal slot. Move the splice to the start of `closeSlot` (synchronously, before the await), give `flushWaiter` a fallback that mints a fresh slot when capacity exists but the pool is empty, and track in-flight closes in a Set drained by `shutdown` so callers can rely on shutdown() meaning "all chromium is gone". Adds a regression spec where a discard release with a queued waiter must not hand the discarded slot to the waiter. ScreenshotStorageService: `getQuotaUsage` → S3 PUT → DB upsert is not transactional, so two concurrent `storeOrEvict` calls inside the same pod could each see headroom and transiently overshoot `maxItems` / `maxTotalBytes`. Chain all `storeOrEvict` invocations through a single per-instance promise to serialize the critical section; absorb the chain link's rejection so one failing write does not poison subsequent callers. Cross-pod concurrency still relies on the next LRU pass to converge — quota stays a soft target, documented in the JSDoc. Adds two specs: concurrent storeOrEvict never interleaves, and a rejected call does not poison the chain.
1 participant
Summary
Add page-screenshot capture for the OG enrichment provider, with a bounded headless-browser session pool that doubles as a concurrency cap and an SSRF guard on the browser path.
Pipeline
agent-browserCLI driver; HTML batch + optional viewport screenshot in the same named sessionlast_accessed_attouchesenrichment_screenshotstable (migration 0011) with FK CASCADE toenrichment_cache.idEnrichmentServicevia a WeakMap channel so persistence orders correctly (row id → screenshot row)Browser Session Pool (this revision)
BrowserSessionPool— bounded, lazyog-pool-0..N-1named sessions, doubles as FIFO semaphoremaxSize=2,idleMs=60svia envAGENT_BROWSER_MAX_CONCURRENT/AGENT_BROWSER_IDLE_MSrelease({ discard: true })tears down sessions whose last command failedOnModuleDestroy→await shutdown()ensures every chromium close is issued before Nest exitsMAX_CONCURRENT=1+IDLE_MS=0SSRF Hardening
parseAndValidateUrl+assertHostnameSafeintourl-guard.ts(shared).internalhosts can no longer reach chromiumConfig / Ops
openGraph.fetchModetogglesfetch(default, HTTP) vsbrowser(chromium via agent-browser)openGraph.screenshot.*—enabled,maxItems,maxTotalBytes,maxBytesPerImage,webpQualityAGENT_BROWSER_EXECUTABLE_PATH=/usr/bin/chromium-browserMigration
0011_enrichment_screenshots.sql— new table; brand-new at deploy time so bareCREATE INDEXis allowed per migration-lint annotation.Docs
docs/superpowers/specs/2026-05-12-enrichment-screenshot-design.mddocs/superpowers/specs/2026-05-11-knowledge-base-book-tree-design.mddocs/superpowers/plans/2026-05-13-browser-fetch-session-pool.mdTests
Comprehensive coverage across all new services, including:
browser-session-pool.spec.ts— acquire / queue / discard / idle eviction / shutdown / abort (8 tests)browser-fetch.service.spec.ts— pool reuse, SSRF rejection, screenshot capture, timeoutopen-graph-screenshot.integration.spec.ts— end-to-end with per-harness poolscreenshot-storage.service.spec.ts— eviction, retry, S3 errors swallowed20260512-enrichment-screenshots.spec.ts— migration behaviourTotal: 158 test files / 1014 tests passing locally.