Thin-channel/thick-runner refactor: strangler-fig migration + auth/reliability hardening + MCP interface - #4
Conversation
…e endpoints
ApiChannel gains a fetch() override (auth resolved from AuthManager's
encrypted store, falling back to legacy env/inline config); collector.py
now routes every channel through run_channel() instead of gating on
capabilities.incremental, with FetchResult/RunResult carrying metadata so
opencli/skill's node_url/awaiting_confirm still reach PipelineResult.
New POST/GET /sources/{id}/credentials + DELETE .../{key_name} endpoints
and AuthManager.list_keys()/.delete() let a source migrate off plaintext
channel_config.auth. Frontend: ChannelConfigForm gets a CredentialField
widget wired to sourceId on the edit form.
…lient fetch() read config.timeout but only applied it when building its own one-shot client; in production ctx.http is always the runner's shared client (fixed 30s), so the per-source timeout was silently dropped.
collect() wraps connection-refused/DNS-failure/etc into a clean ChannelResult.fail; fetch() only caught TimeoutException/HTTPStatusError, letting those bypass ChannelFetchError and the runner's retry/backoff contract.
…elper AuthManager.resolve_context() and ApiChannel._resolve_auth_headers() each hand-rolled the same token/key/username+password convention and diverged on empty credentials: resolve_context() sent a placeholder empty Basic header, _resolve_auth_headers() fell back to legacy config instead. Both now delegate to backend.auth.header_builder.build_auth_header(), which never sends a placeholder credential.
AuthManager.store() did a select-then-insert with no upsert semantics; two concurrent stores for the same (source_id, key_name) — e.g. a double-click on the credential-save button — could both miss each other's row and both attempt an INSERT, and the loser hit an unhandled IntegrityError (500) instead of updating. Recovers by rolling back and retrying as an UPDATE.
Pydantic allowed up to 100 chars but SourceCredential.key_name is String(64); a 65-100 char value passed validation then hit an unhandled DataError on Postgres (SQLite doesn't enforce VARCHAR length, so this was invisible in the existing test suite).
source_credentials had no DB-level FK/cascade to data_sources (AuthManager writes via a separate session, so a real FK would need cross-module coordination). delete_source() now cleans up matching source_credentials rows in the same transaction, so encrypted secrets don't outlive the source that owned them.
…nmigrated channels collector.py's routing generalization (every channel now goes through run_channel()) added an unconditional DBCursorStore SELECT and an unused RateLimitedClient/httpx.AsyncClient/TokenBucket construction on every collect() call, even for channels whose fetch() is the default collect()-bridging adapter that never touches cursor state or ctx.http (opencli/skill/cli/web_scraper). Both are now skipped when the channel hasn't actually migrated onto the thick contract; RSS's incremental + rate-limited path is unaffected (its fetch() is a real override).
…plicated lines collect() had drifted into a second, independently-maintained copy of fetch()'s request/parse logic with no live production caller left (every real collection now routes through fetch() via run_channel()) — a future bugfix to one silently wouldn't reach the other. collect() is now a thin FetchContext(config, params) wrapper that converts fetch()'s raise-based ChannelFetchError contract back to its own return-based ChannelResult.fail, so every existing collect()-level test passes unchanged.
…hinese labels
name was derived via formFieldName(label, 'credential'), which strips
non-ASCII characters — the basic-auth username/password fields' Chinese
labels ('用户名'/'密码') both collapsed to an empty slug, so both
password-type inputs rendered with the identical name attribute
(browser autofill/credential-manager could cross-fill them). Derives
the name from keyName instead, which is already unique and ASCII.
…s unknown' listSourceCredentials() failures were swallowed silently, leaving the placeholder indistinguishable from a confirmed-absent credential. A user could see an 'unconfigured' field for a source that actually had a working stored credential (the status check merely failed to load), and overwrite it without realizing one existed.
…ping
collector.py injected plain cursor_pending/cursor_source_id keys into
every channel's metadata dict via {**run_result.metadata, ...}, silently
overwriting any channel-emitted key with the same name (no current
channel collides, but nothing prevented it). Renamed to
__cursor_pending__/__cursor_source_id__ so a channel's own metadata can
never collide with the runner's cursor bookkeeping.
…policy No current channel is both paginated and metadata-bearing, so there's no real use case to derive a different merge policy from yet — document the existing behavior as intentional rather than leaving it looking like an unreviewed oversight.
run_channel()'s pagination loop had no visibility when chan.fetch() failed partway through: already-accumulated items from prior pages were silently discarded even though the cursor may have already advanced past them. Exception propagation and cursor-commit timing are unchanged (that's a separate, deliberate design question the team already flagged as needing its own decision) — this only makes the loss observable via a warning log.
frontend/src/components/ChannelConfigForm.tsx and frontend/src/pages/SourcesPage.tsx already carried mixed CRLF/LF line endings before this branch touched them; editing any line made git's line-based diff show most of the file as changed (git diff -w on this branch's earlier commits shows the real content delta is small: 154 and 1 lines respectively). Forcing LF stops the same class of noise on future edits — this does not rewrite the already-pushed history that caused it.
…eout forward - ChannelResult gains error_type (failing exception class name) so a future retryable-vs-permanent error taxonomy has real data to build on, instead of re-parsing free-text error strings. - All 6 channels' collect()/fetch() catch-all except blocks now tag error_type; api_channel derives it from ChannelFetchError.__cause__. - pipeline.py surfaces error_type in both collect-failure and (newly) the previously-silent sink-write-failure event, and step2-3 sink exceptions now emit a "store" TaskRunEvent (was log-only before). - rss_channel.py fetch(): ctx.http.get() now forwards the configured per-source timeout (was silently using the shared client's hardcoded 30s default), matching the GOAL-3 PR1 fix already applied to api_channel. Non-destructive: no control-flow change, only new observability fields. 616 passed, 7 skipped, zero regressions.
… (GOAL-4 PR-A) - backend/pipeline/error_taxonomy.py: is_retryable()/is_retryable_http_status() classify exception-type/status-code as transient (retry helps) vs deterministic (retry reproduces the same failure). Used by pipeline.py in PR-B to decide whether to re-raise for celery's retry policy. - Verified collect->persist idempotency: sequential re-runs of the same batch already dedupe correctly via content_hash (existing test coverage). - Found and closed a real gap: storer.store_records()'s existence-check-then- insert is not atomic, so a concurrent writer landing the same content_hash between the check and the flush raises IntegrityError and loses the whole batch. Now catches it, rolls back, rechecks against the DB, and inserts survivors individually so one collision doesn't discard unrelated new records. This becomes reachable once PR-B makes celery retries real (a retry racing the original attempt), not just theoretical today. 639 passed, 7 skipped, zero regressions.
…(GOAL-4 PR-B) - pipeline.py: both step1/collect and step2-3/sink except blocks now check error_taxonomy.is_retryable() (via effective_error_type(), which unwraps ChannelFetchError.__cause__) and re-raise instead of swallowing into a failed PipelineResult when the fault is transient. The collect()-only ChannelResult.fail(error_type=...) path (no live exception object) raises ChannelFetchError so it propagates the same way. Permanent/unclassified errors are unchanged: still swallowed, matching every existing test. - runner.py: run_pipeline()'s call is now wrapped in try/except. A re-raised retryable exception still gets recorded as a failed TaskRun/CollectionTask (Phase 4's finalize block never runs for a raised exception, so this closes what would otherwise be a run stuck at status="running" for the whole celery backoff window) before propagating further. - worker/tasks.py: run_collection gains autoretry_for=(Exception,). Broad on purpose and safe here — pipeline.py already filtered to only retryable faults before anything reaches this boundary, so this isn't duplicating that classification, just consuming its result. max_retries=3/ default_retry_delay=60 were already declared but dead (nothing ever raised through to the task function); this is what actually activates them. Investigated during PR-A/PR-B: run_scheduled_collection (the cron-fired sibling task) has no autoretry_for and is out of this PR's declared scope — its failures now surface as a raised exception instead of a returned error dict (more consistent with run_collection, arguably better observability) but still don't retry. Not fixed here; GOAL-4 PR-B only covers run_collection. 645 passed, 7 skipped, zero regressions.
… sync (GOAL-4 PR-C) Discovered while scoping this PR: celery beat was never actually wired up at all. worker/beat_schedule.py::build_beat_schedule() existed but had zero callers, was never assigned to celery_app.conf.beat_schedule, and no beat process definition referenced it. In task_executor="celery" mode, scheduled (cron) collection never fired — only manual/webhook dispatch worked. This isn't a "redbeat swap", there was nothing running to swap. - Added celery-redbeat dependency; celery_app.py sets beat_scheduler="redbeat.RedBeatScheduler" + redbeat_redis_url. - backend/worker/redbeat_sync.py: sync_entry()/remove_entry() write/delete redis-backed redbeat entries directly, so redbeat's per-tick redis read reflects a schedule change immediately — no beat restart needed (the problem the old dead build_beat_schedule() would have had even if it were wired up: celery's static beat_schedule dict is only read at beat startup). populate_all() bulk-syncs all enabled schedules once, for drift from anything that changed the DB without going through the CRUD endpoints. - schedule_service.py's create/update/delete_schedule call the sync/remove after each write, gated on task_executor == "celery" (mirrors the existing browser_pool use_redis gate) so local/dev mode never touches redis. Failures are logged and swallowed — the DB write is the source of truth, redbeat sync is best-effort, a redis hiccup must not 500 the endpoint. - main.py calls populate_all() at startup in celery mode (parallel to the existing local-scheduler startup branch). - worker/beat_schedule.py stripped down to just parse_cron_expression (the one piece redbeat_sync.py actually reuses); the dead build_beat_schedule() and its DB-loader are removed. - backend/scheduler.py (the local in-process polling loop) is UNCHANGED and kept: it's still the only cron mechanism for task_executor=="local", and local mode has no reason to require redis. The two are mutually exclusive via task_executor, not competing. Caught and fixed a bug before it shipped: populate_all() first drafted with the same asyncio.new_event_loop().run_until_complete() pattern worker/tasks.py uses for celery's sync task threads — wrong here, since its only caller (main.py's async lifespan) is already inside a running loop and that pattern raises "Cannot run the event loop while another loop is running". Made populate_all() async instead. No real redis in this environment (no fixture, no local server) — new tests mock the redbeat library boundary, same approach already used elsewhere in this codebase for other external I/O. 657 passed, 7 skipped, zero regressions.
…li/skill (GOAL-4 PR-D) - web_scraper_channel.py: collect() is now a thin wrapper delegating to a new fetch(), same pattern as GOAL-3 PR8's api_channel migration. Goes through ctx.http (the runner's RateLimitedClient) when present, so a scraper hitting a site's rate limit now backs off and retries instead of failing outright. Headers can't be baked into the shared client's constructor (it's reused across sources), so the shared-client path sends them per-request; the owns-client path keeps constructor-time headers to match existing collect() test mocks exactly. All 16 pre-existing test_collect_* assertions pass unchanged (migration DoD); 3 new tests cover the fetch()/ctx.http path. - opencli: evaluated, not migrated. It's subprocess+browser-pool driven, not an HTTP client — fetch() would never read ctx.http, so overriding it would just reimplement what the default collect()-bridging adapter already does for free, no rate-limit/retry benefit gained. Capabilities also declare no auth/cursor/pagination to pick up either. - cli/skill: evaluated, no standalone retry wrapper added. Realized this during evaluation: PR-B's celery-level retry already covers them — pipeline.py's is_retryable() check reads ChannelResult.error_type, which every channel populates on failure since PR0's instrumentation, not just fetch()-migrated ones. A wrapper retrying inside a single collect() call would just duplicate what celery's autoretry_for already does by re-calling collect() on the next task attempt. 660 passed, 7 skipped, zero regressions.
…readiness (GOAL-4 PR-E)
health_check() widened from () -> bool to (config=None, source_id=None) ->
bool across AbstractChannel and all overrides — needed real per-source data
to probe anything beyond "is a binary on PATH". source_service.py's
test_source_connectivity now passes source.channel_config/source.id through.
Backward compatible: both params default to None, so calling with zero args
(existing tests) is unchanged.
- api_channel: real HEAD (falling back to GET on 404/405) against
base_url+endpoint, with real auth headers via the existing
_resolve_auth_headers (encrypted store when source_id migrated, legacy
inline config otherwise) — not just a bare unauthenticated ping, which
would report a 401-gated API as unreachable even when it's fine.
- web_scraper: two-tier — the lxml parser ("driver") must be usable at all
(cheap, no network), then the configured url must actually be reachable
(HEAD falling back to GET).
- opencli: two-tier — binary-on-PATH liveness (existing check) always runs
first, then a deep readiness probe hits {cdp_endpoint}/json/version through
a real browser_pool.acquire()/release() (held only for the probe's
duration, not kept open). Skipped for agent mode (dispatches to a remote
node — a different health concern) and bridge mode (no local endpoint to
hit); pool-not-initialized (e.g. a channel constructed standalone outside
the app lifecycle) falls back to the binary check.
- cli: signature widened for uniformity only, behaviour unchanged (still
binary-checked per-collect, as already documented).
Explicitly NOT done, per GOAL-4: wiring health_check into task dispatch as a
skip-if-unhealthy gate. That's a feature (changes what runs), not a fix
(this PR is only about health_check reporting the truth) — out of scope.
675 passed, 7 skipped, zero regressions.
…rollback, last_failing_trace Self-healing loop for the skill subsystem: skill_channel now checks terminal_conditions against step text, persists the last failing journey_trace_v1 for humans to review, and flags (never runs) a redistill proposal after a configurable fail streak. Dock/API gets dismiss-correction (reset the streak, false alarm) and rollback (revert to a prior distilled version) endpoints. auto_confirm can no longer bypass a matched red line.
…ages Adds channel_type="skill" so a distilled skill can be scheduled as a regular data source. backend/skills/record.py drives a live browser recording session (start/stop) that feeds the existing distill path; skill_record.py exposes it over the API. backend/cli.py is a thin httpx client over the same REST API the dock uses (list/show/record/ redistill/dismiss/rollback) so the capability travels outside one React admin panel. Frontend gets SkillsPage/SkillDetailPage wired into nav + routes, plus a dismiss-correction action in the agent dock.
Adds list_providers (read) + update_provider (write, proposal/confirm gated like update_schedule) so the natural-language dock can switch the default model or toggle a provider on/off, instead of requiring a separate admin screen. Reuses the existing ModelProvider model and toolcall parsing helpers unchanged.
No prior test file touched backend/api/v1/chat.py at all — these were landing untested. Exercises _run_read_tool/_build_proposal directly (the LLM tool-calling round trip itself has no mocking precedent in this repo, so out of scope) plus a real POST /chat/confirm through the ASGI test client for the write-applies-to-DB path.
…ookie persistence proof issue07's live E2E had never actually been run against a real browser in this repo (always environment-gated to skip). Ran it for real against both a launched Chrome and Edge over CDP -- both pass, confirming connect_over_cdp is genuinely Chromium-family-agnostic, not just Chrome in practice. Adds test_cookie_persistence_live.py: sets a cookie, detaches (connect_over_cdp browser.close() does not kill the real browser), reattaches fresh, and asserts the cookie survived -- the same reuse pattern two separate task runs hit against the same browser_pool endpoint in production. TESTING.md: documents the Edge repro steps and a real gotcha found running it --remote-debugging-address=127.0.0.1 does not stop Edge from binding ::1 only; Chrome does not have this quirk.
…raper cookie auth api_channel and web_scraper_channel had no way to reach content behind a login -- auth_kind only covered none/bearer/api_key/basic. Adds auth.type "cookie": AuthManager.resolve_cookies(domain) serves a real synced browser session instead. backend/auth/cookiecloud_sync.py is the only module that speaks CookieCloud's own protocol (PyCookieCloud client does the HTTP GET + AES decrypt); it immediately normalizes into our own Fernet-encrypted cookie_jar table, keyed by (domain, cookie_name) -- not source_id, since one sync yields a whole browser's cookie jar across many domains at once, not a single source's secret. POST /api/v1/cookies/sync triggers a sync manually (v1: no scheduled re-sync -- a stale cookie just gets re-synced on demand, not worth a celery job yet). skill_channel/opencli_channel are untouched: they already reuse a real login via session_affinity + connect_over_cdp to an already-running browser, so this only fills the gap for the channels that had nothing.
…i-detection New channel_type "crawl4ai": CSS-structured extraction only (JsonCssExtractionStrategy) -- no LLMExtractionStrategy, AI enrichment stays the pipeline's downstream job like every other channel. Deliberately does not attach to backend.browser_pool like skill/opencli (session_affinity + connect_over_cdp to an already-running browser) -- Crawl4AI manages its own browser with enable_stealth/magic mode, which is the entire reason to reach for it over web_scraper_channel; routing it through an externally-attached browser would throw that capability away for nothing. Reuses the CookieCloud plumbing from the previous commit for auth.type "cookie" (AuthManager.resolve_cookies already returns Playwright-shaped dicts, which is exactly what BrowserConfig.cookies wants). Frontend: crawl4ai config form (url/list_selector/selectors/wait_for/cookie toggle), source type picker entry. Also surfaced the cookie auth option on ApiConfig and WebScraperConfig, which had the backend support (prior commit) but no UI to turn it on.
POST /api/v1/sources/discover-feed {url}: parses <link rel="alternate"> feed
tags off the page, falls back to probing common feed paths (/feed, /rss.xml,
...) when a site declares none. Returns every candidate found, never
auto-picks "the main one" -- ambiguous on purpose, a human picks.
POST /api/v1/sources/import-opml: bulk-creates channel_type="rss" sources
from an OPML export. Lands disabled (a human reviews and enables, not an
auto-live firehose from one file), dedups against already-stored feed_urls
and duplicates within the same file.
Zero new dependencies -- feedparser/beautifulsoup4/lxml were already there.
Setup-time convenience only; neither function is ever called from a
scheduled collect().
Thin stdio MCP server (backend/mcp_server.py) wrapping the existing REST API over HTTP -- 7 tools covering the highest-frequency actions (list/create/test sources, discover_feed, trigger_task, get_task, list_records), not a 1:1 mirror of all ~56 endpoints. Runs as its own process, decoupled from the FastAPI/uvicorn lifecycle. No new auth: the REST API itself has none today (api_key_enabled is read but never enforced), so this inherits that trust boundary -- point OPENCLI_ADMIN_API_URL at a trusted host only. New `mcp` optional dependency group + `opencli-mcp` console script. Live-verified against a running instance (create/list/test source, discover_feed, list_records, get_task error path) before committing.
Two extraction paths now: CSS-structured (JsonCssExtractionStrategy) when 'selectors' is configured -- unchanged, no AI cost -- falling back to LLMExtractionStrategy when it isn't, for targeted sources where writing a CSS selector up front isn't practical. Config gains 'instruction' (required to trigger the fallback), optional 'extraction_schema' (schema-mode vs free-form block-mode), and optional 'provider_id'. This is extraction (deciding what the items are), not the pipeline's separate downstream AI enrichment step -- the two don't overlap. LLM credentials reuse the same ModelProvider row the enrichment step and AIAgent already use; no provider_id -> first enabled provider, same autonomous-default convention as pipeline/runner.py. 9 new unit tests (LLM strategy construction, schema vs block mode, provider-type -> litellm prefix mapping, missing instruction/provider error paths), reusing the AsyncSessionLocal-patch pattern from test_manager.py. Full suite: 767 passed.
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR adds an end-to-end browser-skill subsystem (record→distill→correct→execute), refactors channels into a thick fetch/collect contract with retryable error classification, introduces CookieCloud sync and encrypted credential/cookie storage, adds RSS feed discovery/OPML import, chat-based AI provider management tools, a Crawl4AI channel, and switches Celery Beat scheduling to RedBeat. ChangesSkills Subsystem
Channels Thick Contract
Auth, CookieCloud & Onboarding
Chat Provider Management
Celery RedBeat
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SkillsPage
participant RecordAPI as skill_record API
participant RecordSession
participant DistillAPI as distill endpoint
participant DB as Skill table
User->>SkillsPage: start recording
SkillsPage->>RecordAPI: POST /skills/record/start
RecordAPI->>RecordSession: start_recording(cdp_endpoint)
RecordSession-->>RecordAPI: session_id
RecordAPI-->>SkillsPage: session_id, cdp_endpoint
User->>SkillsPage: stop recording
SkillsPage->>RecordAPI: POST /skills/record/{id}/stop
RecordAPI->>RecordSession: stop(status)
RecordSession-->>RecordAPI: journey_trace_v1
RecordAPI-->>SkillsPage: trace
SkillsPage->>DistillAPI: POST /skills/distill(trace, domain, capability)
DistillAPI->>DB: create Skill(version=1, status=draft)
DB-->>DistillAPI: skill
DistillAPI-->>SkillsPage: skill id/version
sequenceDiagram
participant Channel as Channel.collect()
participant Runner as run_channel
participant Fetch as chan.fetch()
participant Pipeline
participant ErrorTaxonomy as error_taxonomy
Pipeline->>Runner: run_channel(chan, ctx)
Runner->>Fetch: fetch(ctx)
Fetch-->>Runner: FetchResult(items, metadata) or raises ChannelFetchError
Runner-->>Pipeline: RunResult(items, metadata)
Pipeline->>ErrorTaxonomy: effective_error_type(exc)
ErrorTaxonomy-->>Pipeline: error_type
Pipeline->>Pipeline: is_retryable(error_type)?
alt retryable
Pipeline-->>Pipeline: re-raise for Celery autoretry
else permanent
Pipeline-->>Pipeline: return failed PipelineResult
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several major features, including a human-driven recording leg for browser skill capture, CookieCloud sync for authenticated scraping, a Crawl4AI channel, dynamic Celery schedule syncing via Redbeat, and an MCP server for agent tools. It also enhances error taxonomy and concurrency handling. The code review feedback highlights critical improvements: utilizing nested transactions (begin_nested()) in storer.py to prevent batch insert rollbacks, cleaning up stale recording sessions to avoid browser slot leaks, handling network errors in the MCP server client, resolving cookies/headers during channel health checks, filtering out disabled data sources in the Redbeat startup population, and batching cookie sync database commits to reduce transaction overhead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- storer.py: per-record retry now uses session.begin_nested() so a failed insert only rolls back its own SAVEPOINT, not every earlier survivor's flush in the same recovery loop (session.rollback() was undoing all of them, not just the failing one). - skill_record.py: /record/start clears stale _SESSIONS entries (page + pool slot) before acquiring a new one -- a session whose /stop was never called would otherwise hold its browser_pool slot forever. - mcp_server.py: _request wraps the call in try/except httpx.HTTPError so a backend that's down or times out returns a structured error instead of crashing the MCP process; non-JSON 4xx/5xx bodies no longer need raise_for_status() to surface. - crawl4ai_channel.py / web_scraper_channel.py: health_check now resolves cookies (crawl4ai) and cookies+headers (web_scraper) the same way fetch() already does, so authenticated sources' health checks stop false-failing. - redbeat_sync.py: populate_all joins DataSource and requires it enabled too, so a disabled source's schedule isn't pushed to redis at startup. - auth/manager.py + cookiecloud_sync.py: store_cookie takes an optional session param; sync_from_cookiecloud now opens one session for the whole batch (can be hundreds of cookies) and commits once, instead of a session + commit per cookie. Full suite: 767 passed, 9 skipped, no regressions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/api/v1/skills.py (1)
175-187: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward the redistill provider override.
body["provider"]is currently ignored, so callers cannot choose the stronger provider that the correction flow expects/redistillto support.Proposed fix
body = body or {} + provider = body.get("provider") traces = body.get("trace") or body.get("traces") or skill.last_failing_trace if not traces: raise HTTPException( @@ try: - skill = await correction.re_distill(db, skill, traces) + skill = await correction.re_distill(db, skill, traces, provider=provider)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/skills.py` around lines 175 - 187, Forward the provider override through the redistill flow: in the skills redistill handler, `body["provider"]` is currently ignored before calling `correction.re_distill`, so update the request handling to read the provider from `body` and pass it along to `re_distill` (or into whatever config object it uses). Make sure the change is applied in the redistill endpoint logic around `skill`, `traces`, and `correction.re_distill` so callers can select the stronger provider expected by `/redistill`.frontend/src/pages/SourcesPage.tsx (1)
652-679: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
sourceTarget()has nocrawl4aibranch.Every other URL-bearing channel (
rss,api,web_scraper) has a branch surfacing its target URL;crawl4aisources (which also store their page underconfig.url) fall through to the genericsource.description || '未配置目标', so node cards/inspectors won't show the actual crawl target.🩹 Suggested fix
if (source.channel_type === 'web_scraper') { return String(config.url ?? config.start_url ?? config.startUrl ?? CHANNEL_META.web_scraper.hint) } + if (source.channel_type === 'crawl4ai') { + return String(config.url ?? CHANNEL_META.crawl4ai.hint) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/SourcesPage.tsx` around lines 652 - 679, Add a dedicated crawl4ai case in sourceTarget() so sources with channel_type equal to crawl4ai surface their actual target URL from config.url, similar to the rss/api/web_scraper branches. Use the existing sourceTarget function and CHANNEL_META fallback pattern so crawl4ai does not fall through to source.description || '未配置目标' and node cards/inspectors show the real crawl target.
🟠 Major comments (20)
backend/api/v1/chat.py-123-123 (1)
123-123: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDon’t send provider
base_urlto the chat model.
list_providersresults are fed back into the LLM tool loop, so returningbase_urlcan expose internal model endpoints. The agent only needsid,name,provider_type,default_model, andenabledto propose provider changes.Proposed fix
- "description": "列出所有模型提供商 (返回 id / name / provider_type / default_model / base_url / enabled)。AI 富化阶段用哪个模型由 provider 决定。只读, 立即执行。", + "description": "列出所有模型提供商 (返回 id / name / provider_type / default_model / enabled)。AI 富化阶段用哪个模型由 provider 决定。只读, 立即执行。",{ "id": p.id, "name": p.name, "provider_type": p.provider_type, - "default_model": p.default_model, "base_url": p.base_url, "enabled": p.enabled, + "default_model": p.default_model, "enabled": p.enabled, }Also applies to: 225-231
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/chat.py` at line 123, The list_providers tool schema currently exposes base_url in the description/returned fields, which should not be sent back into the chat model. Update the tool definition in chat.py for list_providers so it only advertises and returns id, name, provider_type, default_model, and enabled, and remove base_url from any related descriptions or response shaping used in the tool loop.backend/api/v1/chat.py-293-300 (1)
293-300: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate
update_providerinputs before building or applying the proposal.
bool(args["enabled"])will treat strings like"false"asTrue, and/chat/confirmappliesproposal.argsdirectly, so invaliddefault_modelvalues can still be persisted there. Validateenabledas a real boolean anddefault_modelas a non-empty string within the column limit in both paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/chat.py` around lines 293 - 300, Validate the `update_provider` proposal inputs before populating `out_args` and before `/chat/confirm` applies `proposal.args` directly. In `update_provider`, treat `enabled` as a real boolean instead of coercing with `bool(...)`, and reject invalid `default_model` values unless they are non-empty strings within the allowed column limit. Apply the same validation in the confirm path that persists `proposal.args`, using the `update_provider` and `/chat/confirm` handling to ensure only valid `enabled` and `default_model` values can be saved.backend/services/schedule_service.py-53-53 (1)
53-53: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefer RedBeat side effects until the DB commit succeeds.
These calls run after
flush()but before the request-level commit, so any later rollback leaves Redis/RedBeat inconsistent with the DB. Move sync/remove to an after-commit hook, outbox job, or router layer afterawait db.commit().Also applies to: 65-65, 73-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/schedule_service.py` at line 53, RedBeat sync/remove is happening in schedule_service before the transaction is actually committed, which can leave Redis inconsistent if the DB later rolls back. Move the side effects in the schedule_service flow (the _sync_redbeat call and the corresponding remove path around the related schedule operations) to an after-commit hook, outbox job, or a caller/router step that runs only after await db.commit(). Use the existing _sync_redbeat helper and the schedule create/update/delete methods as the lookup points when refactoring.backend/worker/redbeat_sync.py-61-88 (1)
61-88: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReconcile disabled schedules during startup populate.
Line 78 only loads enabled rows, so a schedule disabled by migration/direct DB edit can leave its old RedBeat entry firing forever. Load all schedules and let
sync_entry()remove disabled entries, or add explicit stale-key pruning.Suggested minimal fix
- result = await session.execute(select(CronSchedule).where(CronSchedule.enabled.is_(True))) + result = await session.execute(select(CronSchedule))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/worker/redbeat_sync.py` around lines 61 - 88, The startup bulk sync in populate_all only queries enabled CronSchedule rows, so stale RedBeat entries for newly-disabled schedules are never cleaned up. Update populate_all to include all schedules (or add explicit stale-key pruning) and rely on sync_entry to remove disabled entries; keep the existing load-and-sync flow in redbeat_sync.populate_all and preserve the per-schedule exception handling.backend/services/source_service.py-102-145 (1)
102-145: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftSSRF: server-side fetch of an unrestricted, caller-supplied URL.
discover_feedsissues anhttpxGET (with redirects) directly againsturl, then — if no<link>candidates are found — probes six additional fixed paths on the same resolved host. There's no scheme allowlist (http/httpsonly) and no check against private/reserved IP ranges (loopback, link-local incl. cloud metadata169.254.169.254, RFC1918), so this endpoint can be used to reach internal services. Because both the homepage's declared<link>candidates and the probe results (URL + content-type match) are reflected back in the response, this also functions as a limited internal-network reconnaissance oracle.
follow_redirects=Truemakes a naive pre-check (validateurlbefore calling) insufficient — the redirect target must be checked too (or redirects disabled and handled manually).💡 Mitigation approach
+import ipaddress +import socket + +def _is_safe_host(hostname: str) -> bool: + try: + addrs = socket.getaddrinfo(hostname, None) + except socket.gaierror: + return False + return all(not ipaddress.ip_address(a[4][0]).is_private for a in addrs) + async def discover_feeds(url: str) -> list[dict[str, Any]]: + parsed = urlparse(url) + if parsed.scheme not in ("http", "https") or not _is_safe_host(parsed.hostname or ""): + return [] ...Also validate the final URL after each redirect hop (e.g. via an
httpxevent hook), sincefollow_redirects=Truecan otherwise bypass a one-time check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/source_service.py` around lines 102 - 145, The SSRF issue is in discover_feeds, which fetches a caller-supplied URL and follows redirects without any destination safety checks. Add strict allowlisting for http/https and block private/reserved IP ranges for both the initial URL and any redirect target, or disable follow_redirects and validate each hop before requesting it. Apply the same validation to the fallback probe_url requests so the feed discovery and probe logic cannot reach internal or metadata endpoints.backend/services/source_service.py-148-172 (1)
148-172: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse
defusedxmlfor OPML uploads.import_opml()passes untrusted file content straight intoparse_opml(), andxml.etree.ElementTree.fromstring()still leaves entity-expansion DoS exposure here. Adddefusedxmland mapDefusedXmlExceptioninto the existing 400 path, or enforce a hard size/depth limit if you want to avoid the dependency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/source_service.py` around lines 148 - 172, `parse_opml()` is parsing untrusted OPML with `xml.etree.ElementTree.fromstring`, which leaves entity-expansion exposure in `import_opml()`. Update `parse_opml` in `source_service` to use `defusedxml` for XML parsing and catch `DefusedXmlException` alongside the existing parse error path so malformed or unsafe uploads still map to the same 400 response. Keep the current `import_opml()` flow and preserve the `ValueError` behavior for callers.backend/api/v1/cookies.py-22-28 (1)
22-28: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGate
backend/api/v1/cookies.pybehind an admin dependency.backend/api/v1/__init__.pyandbackend/main.pymount this router without any auth/authz guard, so/api/v1/cookies/syncaccepts an arbitraryurland forwards it toPyCookieCloudviasync_from_cookiecloud, leaving an SSRF-capable admin action publicly reachable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/cookies.py` around lines 22 - 28, The /sync endpoint in cookies.py is currently public and allows arbitrary CookieCloud URLs to be forwarded through sync_from_cookiecloud, creating an SSRF-capable admin action. Add the existing admin/authz dependency to the cookies router or directly to sync_cookies so the route is only accessible to admins, and ensure the router mounting in api/v1/__init__.py and main.py does not expose it without that guard.backend/auth/cookiecloud_sync.py-25-34 (1)
25-34: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the CookieCloud fetch with a timeout
PyCookieClouddoesn’t expose a timeout here, soasyncio.to_thread(_fetch_decrypted, ...)can still block indefinitely if the server hangs. Wrap the call inasyncio.wait_for(...)with a sane deadline, or switch to a client that accepts request timeouts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/auth/cookiecloud_sync.py` around lines 25 - 34, The CookieCloud fetch in _fetch_decrypted can block indefinitely because it relies on PyCookieCloud without any timeout. Update the call site that uses asyncio.to_thread(_fetch_decrypted, ...) to enforce a deadline with asyncio.wait_for, or replace PyCookieCloud with a timeout-aware client, so hangs from get_decrypted_data() are bounded. Keep the existing CookieCloudSyncError path for failed fetch/decrypt and ensure the timeout is handled cleanly in the sync flow.frontend/src/pages/SkillsPage.tsx-168-199 (1)
168-199: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancelling during "recording" leaks the held browser session.
The wizard's Cancel button (line 169) always calls
onClose()directly, regardless ofstep. Per the upstream contract,/skills/record/startHolds the pool's per-endpoint mutex for the session's lifetime (released on/stop), released only viarecordStop(line 51-61), which in turn hits/skills/record/{session_id}/stopthat Releases the held Chrome back to the pool and closes the CDP connection. If the user opens the wizard, starts recording (sessionIdset,step === 'recording'), then clicks 取消 instead of one of the "标记成功/失败并停止" buttons, the wizard just closes —recordStopis never called, and the Chrome endpoint's pool lock is leaked until the backend restarts.🔧 Proposed fix
+ const handleClose = () => { + if (step === 'recording' && sessionId) { + recordStop(sessionId, { status: 'failed' }).catch(() => {}) + } + onClose() + } + return ( <div className="fixed inset-0 z-50 flex items-center justify-center bg-black/75 p-4 backdrop-blur-sm"> ... <div className="flex justify-end gap-3 border-t border-white/10 p-5"> - <Button type="button" variant="outline" onClick={onClose}>取消</Button> + <Button type="button" variant="outline" onClick={handleClose}>取消</Button>This mirrors the same abandonment gap in
backend/cli.py'scmd_record(Ctrl+C mid-recording) — both client surfaces need to guarantee/stopfires on early exit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/SkillsPage.tsx` around lines 168 - 199, The Cancel button in the SkillsPage wizard closes the modal without stopping an active recording session, which can leak the held browser session. Update the Cancel handler in SkillsPage so it checks the current step/session state and, when step is 'recording' (with a sessionId), it calls the existing recordStop flow before onClose; keep onClose as the fallback for non-recording steps. Reuse the existing stop mutation and the recordStop logic already used by the “标记成功/失败并停止” actions so the session is always released from the backend before the UI closes.backend/cli.py-110-150 (1)
110-150: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancelling/interrupting
recordmid-session leaks the held browser.Per the upstream contract (
backend/api/v1/skill_record.py),/skills/record/startHolds the pool's per-endpoint mutex for the session's lifetime (released on/stop). Incmd_record, if the user hits Ctrl+C (or any exception occurs) while the twoinput()prompts (lines 122, 125) are blocking, the process exits without ever calling/skills/record/{session_id}/stop. The backend session/mutex is never released, permanently locking that Chrome endpoint until the backend process is restarted.Wrap the interactive portion in try/finally so an interrupt still stops the session.
🔧 Proposed fix
session = _unwrap(c.post("/skills/record/start", json=start_body, timeout=60.0)) session_id = session["session_id"] print(f"recording started (session={session_id}, chrome={session['cdp_endpoint']})") print("go demo the task in that Chrome window now.") - input("press Enter here when done recording... ") - - status = "success" - if input("mark as success? [Y/n] ").strip().lower() == "n": - status = "failed" - stop_result = _unwrap( - c.post(f"/skills/record/{session_id}/stop", json={"status": status}, timeout=30.0) - ) + try: + input("press Enter here when done recording... ") + status = "success" + if input("mark as success? [Y/n] ").strip().lower() == "n": + status = "failed" + except (KeyboardInterrupt, EOFError): + status = "failed" + stop_result = _unwrap( + c.post(f"/skills/record/{session_id}/stop", json={"status": status}, timeout=30.0) + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cli.py` around lines 110 - 150, `cmd_record` leaves an active recording session open if Ctrl+C or another exception happens during the interactive `input()` prompts, because `/skills/record/{session_id}/stop` is never called. Wrap the interactive body in `try/finally` and ensure the `stop` request is sent from the `finally` block whenever `session_id` has been created, so the backend mutex held by the session is always released. Use the existing `cmd_record` flow and `session_id`/`stop_result` handling to locate the cleanup path.backend/skills/correction.py-190-195 (1)
190-195: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject rollback when the corrected evidence lacks a snapshot.
Legacy
correctedentries won’t haveprev_skill_md/prev_elements; the current fallbacks would commitskill_md=""andelements={}instead of failing cleanly.Proposed guard
from_version = skill.version - skill.skill_md = corrected.get("prev_skill_md") or "" - skill.elements = dict(corrected.get("prev_elements") or {}) + if "prev_skill_md" not in corrected or "prev_elements" not in corrected: + raise ValueError( + f"skill {skill.id} correction v{to_version} has no rollback snapshot" + ) + prev_elements = corrected["prev_elements"] + if not isinstance(prev_elements, dict): + raise ValueError( + f"skill {skill.id} correction v{to_version} has invalid rollback elements" + ) + + skill.skill_md = corrected["prev_skill_md"] + skill.elements = dict(prev_elements) skill.distill_model = corrected.get("prev_distill_model") skill.source_trace = corrected.get("prev_source_trace")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/skills/correction.py` around lines 190 - 195, Reject rollback in correction.py’s rollback path when the corrected evidence has no snapshot data. In the logic that restores `skill.skill_md` and `skill.elements` from `corrected`, add a guard in the same branch that handles `from_version`/`prev_*` so legacy entries without `prev_skill_md` or `prev_elements` do not fall back to empty values. Update the rollback flow around the `skill.version` reassignment to fail cleanly instead of committing an empty skill state.backend/api/v1/__init__.py-9-9 (1)
9-9: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd auth and URL allowlisting for CookieCloud sync
backend/api/v1/__init__.py:31exposes/api/v1/cookies/syncwithout any auth guard, andbackend/api/v1/cookies.pyforwardsbody.urlstraight intosync_from_cookiecloud. Gate it behind the intended admin/auth check and validate/allowlist the target URL before calling out.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/__init__.py` at line 9, The CookieCloud sync endpoint is currently exposed without protection and passes the user-supplied URL through unchanged. Update the `cookies` route registration in `backend/api/v1/__init__.py` and the sync handler in `backend/api/v1/cookies.py` so `/api/v1/cookies/sync` requires the intended admin/auth guard, then validate `body.url` against an allowlist before calling `sync_from_cookiecloud`. Use the existing `cookies` module entrypoints and the sync handler’s URL handling logic as the fix points.backend/skills/record.py-213-228 (1)
213-228: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLeak the opened page if
session.start()fails.
open_skill_page(cdp_endpoint)is awaited beforesession.start(); ifstart()raises (e.g.expose_bindingfailure per the concern above), the already-openedSkillPage(CDP connection + Playwright driver) is never closed, unlike the API layer's own careful "never leak a held Chrome on failure" handling inbackend/api/v1/skill_record.py'srecord_start.🩹 Proposed fix
page = await open_skill_page(cdp_endpoint) session = RecordSession( session_id=uuid.uuid4().hex, domain=domain, capability=capability, page=page, ) - await session.start() + try: + await session.start() + except Exception: + await page.aclose() + raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/skills/record.py` around lines 213 - 228, The start_recording flow opens a SkillPage before calling RecordSession.start(), so a failure in session.start() can leak the already-opened page/driver. Update start_recording to handle exceptions around session.start() and ensure the opened page is always closed or cleaned up on failure, mirroring the safe failure handling used by record_start in backend/api/v1/skill_record.py. Use the existing symbols open_skill_page, RecordSession.start, and logger to locate and adjust the lifecycle cleanup path.frontend/src/components/ChannelConfigForm.tsx-589-655 (1)
589-655: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCrawl4AI config form has no way to configure LLM/instruction-based extraction.
The form only exposes CSS
selectors(marked required) pluswait_forand cookie auth. The backend channel (backend/channels/crawl4ai_channel.py) explicitly supports aninstruction-only path (noselectors) for LLM extraction, and this PR's own commit summary lists "Crawl4AI LLM fallback for selector-free extraction" as a delivered capability — but there's no UI path to configureinstruction,extraction_schema, orprovider_id. Users can only create CSS-based Crawl4AI sources through this form; the LLM fallback is effectively unreachable from the UI.♻️ Suggested addition (sketch)
function Crawl4AIConfig({ config, onChange }: {...}) { ... + const useLlm = !selectors.length return ( <div className="space-y-3"> ... - <Field label={t('channelConfig.fieldSelectors')} hint={t('channelConfig.fieldSelectorsHint')} required> + <Field label={t('channelConfig.fieldSelectors')} hint={t('channelConfig.fieldSelectorsHint')}> <KVList pairs={selectors} onChange={updateSelectors} keyPlaceholder="field name" valuePlaceholder="CSS selector" /> </Field> + {selectors.length === 0 && ( + <Field label="instruction (LLM extraction)" required> + <TextInput value={(config.instruction as string) ?? ''} onChange={(v) => update({ instruction: v })} placeholder="extract the article title and author" required /> + </Field> + )} ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/ChannelConfigForm.tsx` around lines 589 - 655, The Crawl4AI config UI only exposes CSS selector-based extraction, so the LLM/instruction-based path is unreachable from the form. Update Crawl4AIConfig to add fields for instruction, extraction_schema, and provider_id, and wire them through update/onChange alongside selectors and wait_for. Also make selectors optional in the UI when instruction-based extraction is used, so the form can represent the backend’s selector-free path supported by crawl4ai_channel.backend/channels/crawl4ai_channel.py-61-99 (1)
61-99: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCookie/LLM-provider resolution failures escape the
ChannelFetchErrortaxonomy.
_resolve_cookies(line 73) and_build_llm_strategy→_resolve_llm_config(line 91, hitting the DB) run before/outside thetry/except ImportErrorand the latertry/except Exceptionblock. IfAuthManager().resolve_cookies()or the DB query raises, the exception propagates out offetch()unclassified — and sincecollect()(lines 54-58) only catchesChannelFetchError, it escapescollect()entirely, bypassing the retry/error-taxonomy contract this refactor is meant to establish for exactly this kind of external-call hazard.🛡️ Proposed fix
cookies: list[dict] = [] if auth_config.get("type") == "cookie": - cookies = await self._resolve_cookies(url) + try: + cookies = await self._resolve_cookies(url) + except Exception as exc: + raise ChannelFetchError(f"crawl4ai: cookie resolution failed: {exc}") from excApply the same guard around
_resolve_llm_config's DB lookup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/channels/crawl4ai_channel.py` around lines 61 - 99, The external resolution steps in fetch() are leaking raw exceptions instead of ChannelFetchError, so wrap the calls to _resolve_cookies and _build_llm_strategy (including the _resolve_llm_config DB lookup it uses) in the same error-handling path as the rest of crawl4ai_channel.py. Catch failures from AuthManager/DB access and re-raise them as ChannelFetchError with context, so collect() can handle them consistently alongside the existing ImportError and fetch-time taxonomy.tests/unit/pipeline/test_pipeline_errors.py-130-132 (1)
130-132: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPoint these tests at the current
run_pipelinecontract.These calls pass
db_sessionastask_idandtask.idasparameters. Also, Line 204 patchesbackend.pipeline.storer.store_records, but the current pipeline writes throughactive_sink.write_batch; the retryable sink test won’t exercise the intended path.🧪 Suggested test shape
+class _FailingSink: + async def write_batch(self, ctx, items): + raise ConnectionError("pool exhausted") + ... - await run_pipeline(db_session, source, task.id) + await run_pipeline(task.id, source) ... - result = await run_pipeline(db_session, source, task.id) + result = await run_pipeline(task.id, source) ... - await run_pipeline(db_session, source, task.id) + await run_pipeline(task.id, source) ... with ( patch("backend.pipeline.collector.collect", return_value=channel_result), - patch("backend.pipeline.storer.store_records", side_effect=ConnectionError("pool exhausted")), ): with pytest.raises(ConnectionError, match="pool exhausted"): - await run_pipeline(db_session, source, task.id) + await run_pipeline(task.id, source, sink=_FailingSink())Also applies to: 152-153, 178-180, 202-207
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/pipeline/test_pipeline_errors.py` around lines 130 - 132, Update these pipeline error tests to match the current run_pipeline contract: ensure the call sites pass task.id as task_id and the parameters object in the correct position, rather than passing db_session where the task ID belongs. Also change the retryable sink test to patch the actual write path used by the pipeline, active_sink.write_batch, instead of backend.pipeline.storer.store_records, so the intended retry behavior is exercised against the real sink implementation.backend/channels/base.py-133-134 (1)
133-134: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve
ChannelResult.error_typewhen bridging toChannelFetchError.Line 134 drops
result.error_type. With collect-only channels now routed throughAbstractChannel.fetch(),ChannelResult.fail(..., error_type="TimeoutException")becomes a bareChannelFetchError, so downstream retry taxonomy can treat a retryable failure as permanent.🔧 Suggested direction
result = await self.collect(ctx.config, ctx.params) if not result.success: - raise ChannelFetchError(result.error or f"{self.channel_type} collect failed") + exc = ChannelFetchError(result.error or f"{self.channel_type} collect failed") + exc.error_type = result.error_type + raise excAlso ensure
effective_error_type()checksgetattr(exc, "error_type", None)before falling back to the exception class/cause.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/channels/base.py` around lines 133 - 134, Preserve the error taxonomy when `AbstractChannel.fetch()` converts a failed `ChannelResult` into `ChannelFetchError`: in `base.py`, update the failure path in `fetch()` so it carries `result.error_type` forward instead of only raising with `result.error` text. Also adjust `effective_error_type()` to prefer `getattr(exc, "error_type", None)` before falling back to the exception class or cause, so retryable failures like `TimeoutException` stay classified correctly.backend/worker/tasks.py-23-34 (1)
23-34: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBlanket
autoretry_for=(Exception,)outruns the taxonomy filtering it's justified by.The decorator comment's invariant ("run_pipeline() only re-raises retryable errors") only holds for the try/except wrapping the Phase 3
run_pipeline()call insiderun_collection_pipeline. Phase 1 (create TaskRun), Phase 2 (load source/agent), and Phase 4 (finalize status) have no such filtering — any exception there (DB error, code bug) reaches this Celery boundary unfiltered and is retried the same as a genuinely transient failure.Concretely: if Phase 4 throws after Phase 3 already succeeded, retrying re-runs the whole pipeline from Phase 1 (creating a new TaskRun row) while the prior attempt's TaskRun — which already completed the actual collection — is never marked failed/completed and is left stuck at
"running"forever. That's the exact symptom the Phase-3 bookkeeping was added to prevent, just for a different phase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/worker/tasks.py` around lines 23 - 34, The `run_collection` Celery task is retrying every exception, but only the Phase 3 `run_pipeline()` path is taxonomy-filtered; exceptions from TaskRun creation, source/agent loading, and final status updates can be non-retryable and should not be blanket retried. Update the task’s retry handling so only transient/retryable failures are retried at the Celery boundary, and let unexpected Phase 1/2/4 exceptions fail normally or be handled explicitly, preserving the existing `run_collection_pipeline` and `run_collection` behavior without recreating a new TaskRun on a Phase 4 failure.backend/channels/opencli_channel.py-477-510 (1)
477-510: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
health_checkignoresconfig/source_id, so it probes an arbitrary pool endpoint instead of this source's bound one.
AbstractChannel.health_check's contract saysconfig/source_idlet an override do "a real per-source probe instead of a generic liveness check," andcollect()in this same file does resolve a site-specific endpoint (viachrome_endpointparam / agent routing). This override drops straight topool.acquire()with no endpoint, so with multiple pool members the probe result doesn't necessarily reflect the endpoint this particular source actually uses.Route to the source's bound endpoint when available
from backend.browser_pool import get_pool try: pool = get_pool() except RuntimeError: return True # pool not initialized yet (e.g. tested standalone) — binary check stands + acquire_endpoint: str | None = None + site = (config or {}).get("site") + if site: + from backend.services import browser_service + from backend.database import AsyncSessionLocal + async with AsyncSessionLocal() as session: + binding = await browser_service.get_binding_by_site(session, site) + if binding: + acquire_endpoint = binding.browser_endpoint + try: - async with pool.acquire() as cdp_endpoint: + async with pool.acquire(endpoint=acquire_endpoint) as cdp_endpoint:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/channels/opencli_channel.py` around lines 477 - 510, The health_check override in opencli_channel.py is ignoring the passed config/source_id and probing a random acquired pool endpoint instead of the source’s bound endpoint. Update health_check to resolve and use the same per-source endpoint selection path used by collect() and the channel routing logic (for example via the site-specific chrome_endpoint/agent resolution) before falling back to pool.acquire(), so the probe reflects the actual source being checked. Keep the existing binary liveness and agent/bridge-mode short-circuit behavior, but make the CDP readiness check target the resolved endpoint for that source.backend/channels/web_scraper_channel.py-101-119 (1)
101-119: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStatus-based retry handling is missing for
HTTPStatusError
effective_error_type()reduces a wrappedHTTPStatusErrorto the bare class name, andis_retryable()only checks class-name sets, so every HTTP status falls through as permanent. That means 429/5xx won’t retry here, despiteis_retryable_http_status()already encoding the intended split. Wire the status code into retry classification or mapHTTPStatusErrorbefore the class-name check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/channels/web_scraper_channel.py` around lines 101 - 119, Status-based retry classification for wrapped HTTPStatusError is missing, so 429/5xx are treated as permanent instead of retryable. Update the retry path in `is_retryable()` and/or `effective_error_type()` to preserve or inspect the HTTP status from `ChannelFetchError` created in `_get()`, rather than relying only on the exception class name. Use `is_retryable_http_status()` to decide retryability for HTTP status failures, and ensure `HTTPStatusError` cases map to retryable vs non-retryable based on the response code before the generic class-name check.
🟡 Minor comments (7)
TESTING.md-496-498 (1)
496-498: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the verification date.
2026-07-02is future-dated here, so the claim reads as if the browser checks were verified after this revision. Use the actual run date or drop the date.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TESTING.md` around lines 496 - 498, The verification note in the Chrome/Edge CDP guidance uses a future-dated timestamp, so update the date to the actual run date or remove it entirely. Edit the affected markdown text in TESTING.md where the browser verification claim appears, keeping the existing `connect_over_cdp` guidance and the Chrome/Edge reference unchanged.frontend/src/pages/SkillDetailPage.tsx-102-102 (1)
102-102: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
canRollbackstays true even after the correction was already rolled back.It checks for any
correctedevent in history rather than whether the most recent correction/rollback event is stillcorrected. After a rollback, the button remains enabled with nothing left to revert (backend will presumably reject it, surfaced only via an error toast).🐛 Proposed fix
- const canRollback = (skill.evidence ?? []).some((ev) => ev.event === 'corrected') + const rollbackRelevant = (skill.evidence ?? []).filter( + (ev) => ev.event === 'corrected' || ev.event === 'rolled_back', + ) + const canRollback = + rollbackRelevant.length > 0 && + rollbackRelevant[rollbackRelevant.length - 1].event === 'corrected'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/SkillDetailPage.tsx` at line 102, The `canRollback` flag in `SkillDetailPage` is checking the full `skill.evidence` history for any `corrected` event, so it can stay enabled after a rollback has already consumed the correction. Update the logic near `canRollback` to inspect only the current/latest evidence state and determine whether there is an unapplied correction still available to revert, rather than using `.some(...)` over all events. Use the `canRollback` computation and the `skill.evidence` event history shape to locate the change.backend/api/v1/skills.py-110-110 (1)
110-110: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAddress the new Ruff B008 warnings.
The new endpoints use
Depends(get_db)in default arguments; Ruff is flagging these lines. Prefer a module-level dependency alias orAnnotatedpattern so lint stays clean.Also applies to: 122-122, 140-140, 158-158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/skills.py` at line 110, Ruff is flagging the direct use of Depends(get_db) in default parameters on the skill endpoint handlers. Update get_skill and the other affected endpoint functions to use either a module-level dependency alias or the Annotated pattern for the db parameter, keeping the existing get_db dependency behavior while removing the B008 warning. Use the endpoint function names in this module to locate and apply the same fix consistently across all affected signatures.Source: Linters/SAST tools
tests/skills/test_record_live.py-116-118 (1)
116-118: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPage is left open on the success path.
session.stoppedis setTrueinsidestop()before it returns, soif not session.stopped: await session.page.aclose()skips cleanup precisely whenstop()succeeded — the CDP connection is only closed on the (rarer) error path. This is asymmetric withrecord_stopin the API, which always closes the page in afinally. Leaving live CDP connections open across repeated live-test runs can accumulate stale Playwright driver processes/websockets.🧹 Proposed fix
finally: - if not session.stopped: - await session.page.aclose() + await session.page.aclose()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/skills/test_record_live.py` around lines 116 - 118, The cleanup in the live test is inverted: `session.stopped` is already set by `session.stop()`, so the current `finally` block in `test_record_live` only closes `session.page` on the error path and leaves successful runs open. Update the `finally` cleanup to always close the page, using the same unconditional pattern as `record_stop`, and keep the fix anchored around `session.stop()` and `session.page.aclose()`.backend/api/v1/skill_record.py-147-163 (1)
147-163: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winHandle the insert race on
/skills/distill.
uq_skill_domain_capabilityalready prevents duplicate rows, but this select-then-insert path can still race and surface as an unhandledIntegrityErroratsession.commit(). Catch it, rollback, and return409instead of a 500.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/skill_record.py` around lines 147 - 163, The select-then-insert flow in the `/skills/distill` path can still race even though `uq_skill_domain_capability` exists, so an `IntegrityError` may escape from `session.commit()` and become a 500. Update the skill creation logic in `skill_record.py` around the existing `AsyncSessionLocal`, `existing`, and insert/commit flow to catch `IntegrityError`, roll back the session, and return an HTTP 409 with the same duplicate-skill message instead of letting the exception propagate.frontend/src/pages/SourcesPage.tsx-1072-1075 (1)
1072-1075: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLegacy
toggleMuthas no error handling, unlike its canvas-page counterpart.Every other mutation here (
createMut,updateMut,deleteMut, and thetoggleMutin the newerSourcesPagecomponent at lines 2236-2243) shows an error toast viaonError. This one silently no-ops on failure, so a failed enable/pause action gives no user feedback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/SourcesPage.tsx` around lines 1072 - 1075, The legacy toggleMut mutation in SourcesPage is missing failure feedback, unlike createMut/updateMut/deleteMut and the newer SourcesPage toggle mutation. Update this useMutation setup to add an onError handler that surfaces a toast or equivalent user-visible error message when updateSource(id, { enabled }) fails, while keeping the existing onSuccess invalidation for ['sources'].backend/channels/crawl4ai_channel.py-214-241 (1)
214-241: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
health_checkdoesn't mirrorfetch()'s auth/anti-detection setup;source_idis unused.The liveness probe builds
BrowserConfig(headless=True)/CrawlerRunConfig(...)withoutenable_stealth,magic=True, or resolved cookies — so a source behind cookie auth or anti-bot protection can fail this probe (false "unhealthy") even though the realfetch()(which does set all three) would succeed.source_idis accepted but never read, so there's also no path to look up cookies/config from it when onlysource_idis passed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/channels/crawl4ai_channel.py` around lines 214 - 241, The health_check path in crawl4ai_channel.py is using a different browser setup than fetch(), which can produce false unhealthy results for protected sources. Update Crawl4AiChannel.health_check to reuse the same auth and anti-detection configuration as fetch(), including enable_stealth, magic=True, and resolved cookies, and make sure source_id is actually used to load source-specific config when config is missing. Keep the probe lightweight, but build the BrowserConfig/CrawlerRunConfig via the same helper or shared logic used by fetch() so both paths behave consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 694a9be3-7b14-484d-ad34-7e6b30f4ce0c
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (90)
.gitattributesTESTING.mdbackend/api/v1/__init__.pybackend/api/v1/chat.pybackend/api/v1/cookies.pybackend/api/v1/skill_record.pybackend/api/v1/skills.pybackend/api/v1/sources.pybackend/auth/cookiecloud_sync.pybackend/auth/header_builder.pybackend/auth/manager.pybackend/channels/api_channel.pybackend/channels/base.pybackend/channels/cli_channel.pybackend/channels/crawl4ai_channel.pybackend/channels/opencli_channel.pybackend/channels/registry.pybackend/channels/rss_channel.pybackend/channels/skill_channel.pybackend/channels/web_scraper_channel.pybackend/cli.pybackend/main.pybackend/mcp_server.pybackend/migrations/versions/s9n0o1p2q3r4_add_skill_last_failing_trace.pybackend/migrations/versions/t0o1p2q3r4s5_add_cookie_jar.pybackend/models/__init__.pybackend/models/cookie_jar.pybackend/models/skill.pybackend/pipeline/channel_runner.pybackend/pipeline/collector.pybackend/pipeline/error_taxonomy.pybackend/pipeline/pipeline.pybackend/pipeline/runner.pybackend/pipeline/storer.pybackend/schemas/credential.pybackend/schemas/source.pybackend/services/schedule_service.pybackend/services/source_service.pybackend/skills/correction.pybackend/skills/record.pybackend/skills/risk.pybackend/skills/trace.pybackend/worker/beat_schedule.pybackend/worker/celery_app.pybackend/worker/redbeat_sync.pybackend/worker/tasks.pyfrontend/src/App.tsxfrontend/src/api/endpoints.tsfrontend/src/api/types.tsfrontend/src/components/ChannelConfigForm.tsxfrontend/src/components/Layout.tsxfrontend/src/components/StatusBadge.tsxfrontend/src/i18n/en.tsfrontend/src/i18n/zh.tsfrontend/src/labs/topology/AgentDock.tsxfrontend/src/pages/SkillDetailPage.tsxfrontend/src/pages/SkillsPage.tsxfrontend/src/pages/SourcesPage.tsxpyproject.tomltests/integration/test_chat_api.pytests/integration/test_cookies_api.pytests/integration/test_skills_api.pytests/integration/test_sources_api.pytests/skills/test_cookie_persistence_live.pytests/skills/test_correction.pytests/skills/test_record.pytests/skills/test_record_live.pytests/skills/test_risk.pytests/skills/test_skill_channel.pytests/unit/auth/test_cookiecloud_sync.pytests/unit/auth/test_header_builder.pytests/unit/auth/test_manager.pytests/unit/channels/test_api_channel.pytests/unit/channels/test_crawl4ai_channel.pytests/unit/channels/test_opencli_channel.pytests/unit/channels/test_rss_fetch.pytests/unit/channels/test_web_scraper_channel.pytests/unit/pipeline/test_channel_runner.pytests/unit/pipeline/test_collector.pytests/unit/pipeline/test_collector_incremental.pytests/unit/pipeline/test_error_taxonomy.pytests/unit/pipeline/test_pipeline_cursor.pytests/unit/pipeline/test_pipeline_errors.pytests/unit/pipeline/test_storer.pytests/unit/test_runner.pytests/unit/test_schedule_service_redbeat.pytests/unit/test_source_onboarding.pytests/unit/worker/test_beat_schedule.pytests/unit/worker/test_redbeat_sync.pytests/unit/worker/test_tasks.py
… + health_check parity - record.py CAPTURE_JS: the change listener now skips capturing type=password field values (critical, coderabbit) -- raw passwords typed during a recording session were flowing into StepRecord.args["text"] and persisted verbatim in journey_trace_v1 / Skill.evidence. - crawl4ai_channel.py fetch(): cookie resolution and LLM-strategy setup (_build_llm_strategy, which hits the DB for a provider) are now wrapped so a failure raises ChannelFetchError instead of an unclassified exception escaping collect()'s catch entirely, bypassing the retry/error-taxonomy contract. - crawl4ai_channel.py health_check(): now sets enable_stealth=True and magic=True, same as fetch() -- a probe without them could false-fail against a source that's only reachable with anti-detection on. 767 passed, 9 skipped, no regressions.
…beat startup sync; close resource/race gaps - channels/base.py: ChannelFetchError now carries an optional error_type, and fetch()'s bridge from ChannelResult passes result.error_type through instead of dropping it -- a channel that already classified its own failure (e.g. ChannelResult.fail(..., error_type="TimeoutException")) no longer degrades to a bare, unclassified wrapper exception. - error_taxonomy.py: effective_error_type() prefers the explicit error_type when present, before falling back to __cause__/class name. - web_scraper_channel.py: HTTPStatusError is now classified by status code (via is_retryable_http_status) into error_type=Retryable/ PermanentHTTPStatus instead of falling through as an unclassified, always-permanent "HTTPStatusError" -- 429/5xx now actually retry. - worker/redbeat_sync.py: populate_all reconciles ALL schedules (not just enabled ones) against redbeat, calling remove_entry for anything effectively disabled (schedule.enabled=False directly in the DB, or its DataSource disabled) instead of silently skipping them -- a schedule disabled outside the normal CRUD path would otherwise keep a stale redbeat entry firing forever, since a startup pass that only looked at enabled rows never even saw it. - skills/record.py: start_recording() closes the already-opened page if session.start() raises, instead of leaking the CDP connection. - api/v1/skill_record.py: /skills/distill's select-then-insert can still race a concurrent distill for the same (domain, capability); IntegrityError on commit now becomes the same 409 the pre-check gives, not an unhandled 500. - tests/skills/test_record_live.py: fixed an inverted cleanup guard (`if not session.stopped: aclose()`) that skipped closing the page on exactly the success path, since stop() already sets stopped=True before returning. Not fixed (deferred, reasons noted separately): SSRF hardening for discover_feeds/OPML fetch, defusedxml for OPML parsing, auth-gating cookies/sync (the whole REST API has no auth today, not specific to this route), frontend gaps (Crawl4AI LLM-fields UI, cancel-button session leak, canRollback staleness), CLI record ctrl+c cleanup, opencli_channel health_check per-source endpoint routing, and a few test call-signature mismatches in test_pipeline_errors.py predating this PR. 767 passed, 9 skipped, no regressions.
health_check() ignored config/source_id and probed an arbitrary pool member via pool.acquire() with no endpoint. With multiple pool members the probe result didn't necessarily reflect the endpoint this source's collect() actually binds to (site-keyed browser binding resolved the same way pipeline.py does before collect()). Now resolves config.site -> browser_service.get_binding_by_site and passes the bound endpoint to pool.acquire(endpoint=...); no binding falls back to the prior acquire(endpoint=None) behavior. Flagged by coderabbit on PR #4 (opencli-admin/2233admin), scoped out at merge time as a follow-up precision fix.
…review Left scoped-out at PR #4 merge time; fixing now: - SourcesPage.tsx: sourceTarget() had no crawl4ai branch, fell back to the generic "未配置目标" hint instead of showing the crawl4ai url. - ChannelConfigForm.tsx: Crawl4AI form only exposed CSS selectors; no UI path for the LLM-fallback extraction (instruction/extraction_schema/ provider_id) this PR's own backend change shipped. Added a CSS/LLM mode toggle with the missing fields. - SkillDetailPage.tsx: canRollback stayed true after a correction was already rolled back (checked for any 'corrected' event instead of the most recent correction/rollback event). - SourcesPage.tsx: legacy toggleMut had no onError toast, unlike every other mutation on the page and its newer-component counterpart. - backend/cli.py cmd_record: Ctrl+C (or EOF) during either interactive prompt skipped /stop, leaking the pool's per-endpoint mutex for that Chrome session until the backend process restarted. Now caught and treated as status=failed so /stop always fires. Verified live: started backend+frontend, created a real crawl4ai source through the new LLM-mode UI, confirmed it persisted with instruction set and rendered its url in the sources list. Added tests/unit/test_cli.py (cmd_record normal/no/Ctrl+C/EOF paths). 773 passed (was 769), tsc -b + vite build clean.
Issue 08 verification found all five PR #4 deferred frontend findings (SourcesPage crawl4ai target + toggle error handling, SkillsPage recording cancel leak, ChannelConfigForm LLM extraction fields, SkillDetailPage canRollback) already fixed by 2f5944b; the only live defect was this stale test asserting the pre-d157881 source->task:triggers edge for a manual task, which d157881 intentionally changed to source->task:manual. Frontend suite: 19/19 green.
…PR-C) New channel_type "browser_act" (registered). Generic interpreter of a pack's channel.manifest.json (no per-pack code): resolves the pack via PackCatalog, replays manifest.steps against a browser-act session (PR-B), and for each eval_script step runs the pack's scripts/*.py to emit JS then evals it in the browser and JSON-parses the DOM result. - backend/browser_act/scripts.py: run_pack_script() — the second subprocess hop, create_subprocess_exec(sys.executable, ...) argv-only (decision #6), timeout+kill, ScriptError. - backend/channels/browser_act_channel.py: collect/validate_config/health_check. Login/anti-bot detection (decision #4): a script's {error:true,message} matching an auth keyword -> error_type="needs_human", stops, never retries or bypasses. Pagination interprets url_page + "result_count<N" stop_when, else falls back to max_pages(5) + empty-page stop (limitations documented). Bad-manifest KeyError/JSON/schema errors return a ChannelResult, never crash. Env secrets never enter error text or metadata. Registering the channel legitimately expands list_channel_types(), so the workflow-capabilities exact-set test gains "browser_act" (+1 line). 17 tests (happy/needs_human/generic-error/validate/health/registry/ secret-not-leaked/script-hop injection-safety/template-mismatch). 1467 -> 1484 passed, zero regression (full suite verified).
1 participant
Summary
Strangler-fig migration of the channel/runner architecture toward "thin channel, thick runner" (~100 lines per new source), plus everything built on top of it: real per-channel health probes, error-taxonomy-driven retries, CookieCloud auth sync, a Crawl4AI channel (CSS + LLM-fallback extraction), RSS source onboarding, a skill record->distill->CLI workflow, and an MCP interface exposing the system to agents.
29 commits, incremental -- each is independently green. No open PR existed for this branch before (push-trigger allowlist only covers
main/develop/codex/**, so CI hadn't run on this work until now).What's in it
Core refactor
api_channelgets a thick-contractfetch();collect()becomes a thin delegator (kills ~50 lines of duplication), then the same pattern spreads toweb_scraper/crawl4ai.AuthManager) with unified bearer/api_key/basic header building; recovers from concurrent-store races on the unique constraint instead of 500ing.CredentialCreate.key_namemax_length now matches the DB column.Reliability
Auth
cookie_jartable, domain-keyed, Fernet-encrypted) viaPyCookieCloud-- the only place in the codebase that speaks the CookieCloud wire protocol.api_channel/web_scraper_channelgainedauth.type: cookie.Crawl4AI channel
browser_pool-- its anti-detection browser management is the whole point of using it).JsonCssExtractionStrategy) is the default, zero AI cost.LLMExtractionStrategywhenselectorsisn't configured -- give aninstructiondescribing what to pull instead, optionalextraction_schemafor schema-mode vs free-form block-mode. Reuses the sameModelProviderrow the downstream AI-enrichment step already uses (no new credential UI). This is extraction (deciding what the items are), not enrichment (adding fields to existing records) -- the two pipeline stages don't overlap.Sources
discover-feed(parses<link rel=alternate>, falls back to common paths) andimport-opml(bulk import, new sources default disabled). Zero new dependencies.Skills
opencli-skillCLI entry point, and a dock-chat tool for configuring the AI enrichment provider conversationally (list/update providers via chat tool-calling).Live E2E proof
issue07: skill execute-loop verified against real Chrome and real Edge (not headless-only) -- surfaced a real bug (Edge's--remote-debugging-address=127.0.0.1doesn't bind; needslocalhost). Cookie persistence proven end-to-end (disconnect, reconnect, read back).New in this PR: MCP interface
backend/mcp_server.py-- a thin stdio MCP server wrapping the REST API over HTTP. 7 tools covering the highest-frequency actions (list/create/test sources, discover_feed, trigger_task, get_task, list_records) -- not a 1:1 mirror of all ~56 REST endpoints. Runs as its own process, decoupled from the FastAPI/uvicorn lifecycle, so agents (Claude Code or otherwise) can drive collection without going through the UI or raw curl.api_key_enabledis read but never enforced by any dependency) -- this server inherits that trust boundary. Only pointOPENCLI_ADMIN_API_URLat a host you trust.mcpoptional dependency group +opencli-mcpconsole script.Testing
767 passed, 9 skipped, cov ~88%. Everything auth/browser-touching was verified against real infrastructure, not just mocks:
Why no earlier PR
The push-trigger allowlist in
.github/workflows/ci.ymlonly coversmain/develop/codex/**branches, and no PR was open, so CI never ran on this branch until now -- not a CI failure, just CI that never fired. This PR is what triggers the first real run.