feat(worker): Phase 2 hybrid routing — TikTok/Instagram/X via browser-downloader - #141
feat(worker): Phase 2 hybrid routing — TikTok/Instagram/X via browser-downloader#141tomkabel wants to merge 31 commits into
Conversation
…listeners Remove the redundant Content-Security-Policy header from nginx reverse proxy configs (production and local-ssl) that was being sent in addition to the application CSP. Browsers enforce the intersection of multiple CSP headers, so the restrictive nginx CSP (script-src 'self') was stripping the app's nonces, hashes, and external font sources. Replace hx-on:htmx:after-request and hx-on:htmx:response-error attributes on the download form with nonced script event listeners. This eliminates the need for unsafe-eval in the CSP and keeps the form functional when JavaScript is properly loaded via CSP-compliant nonced script tags.
… responses Replace string equality check with secrets.compare_digest() in validate_csrf_token to prevent timing side-channel attacks on CSRF token comparison. Add the missing import for the secrets module. Remove rotate_csrf_token calls from HTMX partial response handlers in web_downloads.py (create_download_form and delete_download_form). Token rotation on partial DOM updates caused subsequent form submissions to fail with 403 because the page's meta tag and hidden inputs still held the old token while the cookie was already rotated. The cookie is set once on full page load (dashboard_page) and does not need per-request rotation on HTMX endpoints. Remove the now-unused rotate_csrf_token import from web_downloads.py.
… JWT Prefix authentication cookies with __Host- to enforce same-site binding and reject non-secure origins, per RFC 6265bis cookie prefix guidelines: - access_token → __Host-access_token - refresh_token → __Host-refresh_token Update all cookie readers across: - app/auth.py: set_token_cookies, clear_token_cookies - app/api/routes/auth.py: login, refresh, logout endpoints - app/api/routes/web/web_auth.py: demo_login, logout, login_form - app/main.py: root redirect auth check - app/api/dependencies/__init__.py: get_current_user_from_cookie Remove email field from create_access_token JWT payload. Email is already accessible via the /api/v1/me endpoint and should not be duplicated in the access token claims, reducing token size and sensitive data exposure in the encoded payload. Update all 13 test files to use the new cookie names and match the adjusted auth token expectations.
Add model_config = ConfigDict(extra="forbid") to DownloadCreate, TokenRefresh, and UserCreate Pydantic models. This prevents mass-assignment attacks by rejecting unexpected fields in request payloads, requiring clients to send only explicitly defined fields.
Add flake8-bandit (S) and flake8-async (ASYNC) to the ruff select list for automated security and async-safety linting. Add per-file ignores for test files (S101 assert, S104-S108 test secrets/defaults) and migration files (ASYNC). Configure mypy strict=true and line-ending=lf.
Pin all Docker base images by SHA256 digest to prevent supply chain attacks through tag replacement: - python:3.12-slim → python@sha256:6c4d... - ghcr.io/astral-sh/uv:0.6 → ghcr.io/astral-sh/uv@sha256:4a6c... - node:20-alpine → node@sha256:fb4c... Verify the NodeSource GPG signing key with sha256sum before importing it, preventing key substitution attacks during apt repository setup. Replace set -e with set -euo pipefail in entrypoint.sh for stricter shell error detection. Generate random DB password and secret key per CI run instead of using hardcoded test values. Add worker entrypoint script to shellcheck.
…ompliance Add explicit return type annotations and parameter type hints across 18 source files to satisfy mypy strict mode requirements: API routes: Add HTMLResponse, RedirectResponse, TemplateResponse, JSONResponse, dict, and union return types to all route handlers. Middleware: Add Callable[[Request], Awaitable[Response]] dispatch signatures and proper starlette response imports. Services: Add redis.asyncio.Redis type hints to error_classifier, structlog BoundLogger to user_service, and optional None checks for stdout/stderr in yt_dlp_service. Core: Add AsyncEngine, async_sessionmaker[AsyncSession], AsyncGenerator types to database module. Add ColumnElement[bool] return to user model filter. Add aioredis.Redis type support with cast() for the lazy singleton pattern in redis_client and queue. Validators: Add parameter types to SSRF redirect handler methods. Add response_model=None to 11 route decorators whose return type annotations use non-Pydantic union types (HTMLResponse | RedirectResponse, dict[str, Any] | JSONResponse) per FastAPI requirements.
Add request nonce propagation and update CSP to include strict-dynamic and style nonces. This enables safer inline styles/scripts by adding nonce attributes to templates and injecting the nonce into generated docs. Improves content security posture for browser-rendered templates.
When refresh endpoint issues new tokens, blacklist the consumed refresh token jti to prevent reuse. Use secrets.compare_digest for CSRF token comparison to mitigate timing attacks. Minor signature and call-site cleanups for token cookie helper and password verification.
Import and apply rate limiter to the downloads SSE endpoint to limit connections; refactor rate limit parsing helper for robustness and minor API surface typing improvements.
Replace random with secrets.SystemRandom for jitter calculation, use re.IGNORECASE for robust pattern matching, and ensure job_max_retries defaults correctly. These reduce jitter predictability and make error classification more resilient to varied casing.
…ages Add trailing commas to mapped_column calls, ensure consistent tuple/line breaks, and minor punctuation fixes in config logging/error messages. These are formatting and typing improvements that keep the ORM mappings consistent and lint-clean.
Add trailing commas for SQLAlchemy method chains, normalize raising exceptions (raise X vs raise X()), and tighten docstrings. These changes are stylistic and reduce accidental tuple/line continuation bugs in queries and exception propagation.
… fixes Add trailing commas for SQLAlchemy chain continuity, honor WORKER_HEALTH_HOST env for health server, and small robustness/formatting fixes across worker job handling and outbox relay. These are non-behavioral improvements to prevent subtle syntax/tuple errors and improve deploy-time configurability.
Minor cleanups: add trailing commas and normalize function signatures/return annotations across exception handlers, middleware dispatch, startup lifespan, health and downloads routes, and web route handlers. These are non-functional style changes to keep codebase consistent and lint-clean.
Improve circuit breaker and yt-dlp subprocess handling with clearer fallbacks, safer file reading, better error message fallbacks, and sanitization of titles. Also tighten path traversal error messages and fix redis client type casts. Minor validators/demo_urls docstring tweaks.
Add .betterleaks files, SARIF, SOTA audit report, fail-*.md and HAR files, and notegpt-clone/ to .gitignore to avoid committing local scan/artifact outputs.
…on-youtube scripts Expose module-level as a SystemRandom instance so tests can patch while preserving cryptographic RNG; change calculate_delay to call random.uniform. Generate yt-dlp extract script without embedding YouTube-only options for non-YouTube platforms (inject options only when platform=='youtube' at build time).
…entries Fix _get_platform to use suffix matching and return 'unknown' for subdomain-bypass URLs like youtube.com.evil.com. Embed youtube-specific ydl options as dict entries inside ydl_opts definition so tests see JSON-style keys (prefer_free_formats, check_formats) rather than Python assignment statements.
Unknown domains like example.com still default to 'youtube', but hostnames containing a platform keyword that failed suffix matching (subdomain bypass) return 'unknown' for all platforms, not just YouTube.
Fix E402 lint errors by relocating random assignment after all import statements.
…-downloader - core/config.py: add 4 settings (enabled=false by default, endpoint, timeout, cb_use_redis) with validator - worker/browser_executor.py: NEW httpx client + named circuit breaker + structured error-code → ErrorCategory mapping (drm/anti_bot → BLOCKED, network → TRANSIENT, no_media → NOT_FOUND, http_429 → TRANSIENT, 4xx → BLOCKED, 5xx → TRANSIENT) - worker/job_executor.py: select_executor + _resolve_executor_kind helper, dispatch block in execute() that skips throttle predictor and progress callback for browser branch (microservice is single-shot HTTP, no progress stream) - pyproject.toml: promote httpx>=0.27 from test extras to runtime deps - 3 new test files (875 total unit tests, 6 skipped, all green) The new worker behavior is a true no-op until BROWSER_DOWNLOADER_ENABLED=true is set. Defaults route everything to yt-dlp. Phase 0 microservice contract unchanged. 875 pass / 6 skipped unit tests. lint:check + type:check clean.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds a browser-downloader service, worker executor routing, platform-aware yt-dlp extraction, host-prefixed authentication cookies, outbox retention, build-input pinning, security-header changes, and stricter linting and typing. ChangesBrowser downloader
Security and authentication
Persistence, CI, and tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🛠️ Fix failing CI checks
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 |
There was a problem hiding this comment.
Pull request overview
This PR implements Phase 2 of hybrid routing in the worker so TikTok/Instagram/X downloads can be dispatched to the existing browser-downloader microservice while keeping YouTube (and unknown hosts) on the existing yt-dlp path behind a feature flag. It also hardens several security and operational aspects (CSP nonce strategy, __Host-* auth cookies, stricter typing/linting, and container pinning) that support running the new routing safely in production.
Changes:
- Add a browser-downloader HTTP client + circuit breaker + error-category mapping, and route jobs to it when enabled and the URL matches supported hosts.
- Expand platform detection + cookies/format-chain behavior in yt-dlp to better separate YouTube-only options from other platforms.
- Strengthen security posture (CSP nonce/strict-dynamic, __Host-* cookie names, CSRF compare_digest) and tighten repo tooling (ruff/bandit/async rules, mypy strict, dependency/runtime adjustments).
Reviewed changes
Copilot reviewed 79 out of 80 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| worker/zombie_sweeper.py | SQLAlchemy statement formatting adjustments. |
| worker/retry_scheduler.py | Small retry policy expression simplification + formatting. |
| worker/processor.py | SQLAlchemy update formatting tweaks. |
| worker/outbox_relay.py | SQLAlchemy query formatting + logger arg trailing comma. |
| worker/main.py | SQLAlchemy select formatting + logger arg trailing commas. |
| worker/job_executor.py | Add Phase 2 executor routing + browser executor dispatch branch. |
| worker/job_claimer.py | SQLAlchemy execution_options formatting + value formatting. |
| worker/health.py | Add configurable health bind host env var for worker. |
| worker/dlq_manager.py | SQLAlchemy execution_options formatting. |
| worker/browser_executor.py | New browser-downloader HTTP client, routing, breaker + error mapping. |
| tests/test_worker/test_job_executor_routing.py | New routing tests for executor selection + execute() dispatch. |
| tests/test_worker/test_browser_executor.py | New tests for HTTP/error-mapping matrix + breaker integration. |
| tests/test_story_8_6_accessibility_audit.py | Update auth cookie name to __Host-* in tests. |
| tests/test_story_8_5_missing_ui_states.py | Update auth cookie name to __Host-* in tests. |
| tests/test_story_8_3_javascript_bugs_performance.py | Update auth cookie name to __Host-* in tests. |
| tests/test_story_3_6_main_decomposition.py | Update auth cookie name to __Host-* in tests. |
| tests/test_story_3_3_web_remaining_extraction.py | Update auth cookie name to __Host-* in tests. |
| tests/test_story_3_2_web_downloads_extraction.py | Update auth cookie name to __Host-* in tests. |
| tests/test_story_3_1_web_auth_extraction.py | Assert __Host-* cookies are set on login/register. |
| tests/test_services/test_yt_dlp.py | Add platform detection + platform-specific script assertions. |
| tests/test_env_contract.py | Adjust token creation contract (remove email claim expectation). |
| tests/test_browser_downloader_config.py | New tests for browser-downloader settings defaults + validation. |
| tests/test_auth_module.py | Update cookie key assertions to __Host-* in cookie helpers. |
| tests/test_api/test_web_routes.py | Update web auth cookie handling to __Host-* throughout. |
| tests/test_api/test_demo_login.py | Update demo-login cookie handling to __Host-*. |
| tests/test_api/test_auth.py | Update cookie-auth test to use __Host-access_token. |
| pyproject.toml | Promote httpx runtime dep, bump bcrypt/pytest-cov, enable ruff S/ASYNC, mypy strict, formatting line endings. |
| infra/nginx/nginx.ssl.local.conf | Remove nginx-level CSP header (CSP now set in app middleware). |
| infra/nginx/nginx.production.conf | Remove nginx-level CSP header (CSP now set in app middleware). |
| entrypoint.sh | Harden shell settings (set -euo pipefail). |
| Dockerfile | Pin base images by digest; verify NodeSource GPG key via sha256. |
| core/utils/security.py | Add trailing commas in multi-line ValueErrors (formatting). |
| core/redis_client.py | Add typing + explicit return annotations + casts; minor doc formatting. |
| core/queue.py | Add typing for lazy redis client wrapper. |
| core/models/user.py | Improve typing for not_deleted() and add trailing commas. |
| core/models/outbox.py | Add trailing comma in mapped_column args. |
| core/models/failed_job.py | Add trailing comma in mapped_column args. |
| core/models/download_job.py | Add trailing commas for relationship/mapped_column args. |
| core/logging_config.py | Docstring spacing tweaks. |
| core/database.py | Add typing/annotations for engine + session factory. |
| core/config.py | Add browser-downloader settings + validation hook. |
| app/utils/validators.py | Add typing to redirect handler; minor coercion fix. |
| app/utils/demo_urls.py | Fix module docstring formatting. |
| app/templates/slides/presentation.html | Move inline styles to CSS + add CSP nonce to <style>. |
| app/templates/dashboard.html | Replace inline HTMX handlers with nonce’d script listeners. |
| app/services/yt_dlp_service.py | Add platform detection, cookies opts, platform-specific format chains/extractor args. |
| app/services/user_service.py | Add logger typing; raise exception classes (no-arg) consistently. |
| app/services/pubsub_service.py | Docstring spacing + logger formatting. |
| app/services/outbox_service.py | SQLAlchemy where-clause formatting. |
| app/services/job_factory.py | Docstring formatting fix. |
| app/services/error_classifier.py | Use SystemRandom via secrets; regex flag cleanup; typing for Redis. |
| app/services/download_service.py | Reorder expired/missing checks; raise class style changes; minor formatting. |
| app/services/circuit_breaker.py | Docstring formatting + typing for execute(). |
| app/services/auth_service.py | Add trailing comma in run_in_executor call. |
| app/schemas/user.py | Forbid extra fields in UserCreate schema. |
| app/schemas/token.py | Forbid extra fields in TokenRefresh schema. |
| app/schemas/download.py | Forbid extra fields in DownloadCreate schema. |
| app/main.py | Read auth token from __Host-access_token cookie. |
| app/auth.py | Switch to __Host-* cookie keys; remove email claim from access token creation. |
| app/api/startup.py | Type/formatting tweak to lifespan signature. |
| app/api/routes/web/web_settings.py | Add response typing + response_model=None where applicable. |
| app/api/routes/web/web_helpers.py | Use compare_digest for CSRF comparisons; typing tweaks; nonce in template context. |
| app/api/routes/web/web_downloads.py | Add response typing; adjust CSRF rotation behavior; remove rotate import. |
| app/api/routes/web/web_dashboard.py | Add TemplateResponse typing + pass nonce to slides template. |
| app/api/routes/web/web_auth.py | Add response typing + response_model=None; adjust demo-login token creation; __Host cookies on logout blacklisting. |
| app/api/routes/web/web_auth_helpers.py | SQLAlchemy select formatting + error-response formatting. |
| app/api/routes/sse.py | Add typing, rate limit, adjust polling interval, and annotate pubsub/session types. |
| app/api/routes/health.py | Docstring formatting + error_response_doc formatting. |
| app/api/routes/downloads.py | error_response_doc formatting + minor typing tweaks. |
| app/api/routes/chaos.py | Add typing + response_model=None; ignore typing mismatch for zadd. |
| app/api/routes/auth.py | Remove email claim from access token; __Host refresh cookie fallback; blacklist old refresh jti. |
| app/api/rate_limit_config.py | Improve typing and minor string handling improvements. |
| app/api/middleware/security_headers.py | Strengthen CSP with strict-dynamic + nonce-based styles + object-src none. |
| app/api/middleware/request_body_size.py | Add typing for middleware dispatch signature. |
| app/api/middleware/prometheus.py | Add typing for middleware dispatch signature. |
| app/api/exceptions.py | Add trailing commas + cast typing strings. |
| app/api/docs.py | Add return types and ensure nonce injection replacement includes comma. |
| app/api/dependencies/init.py | Switch cookie auth lookup to __Host-access_token. |
| .gitignore | Ignore local scan/audit artifacts. |
| .github/workflows/fastapi-test.yml | Add ShellCheck target + generate secrets dynamically + pass DB_PASS to init script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| safe_path = self._validate_download_path(job.file_path) | ||
| if not os.path.isfile(safe_path): | ||
| safe_job_id = str(job_id).replace("\r", "").replace("\n", "") | ||
| logger.error("file_missing_from_disk", job_id=safe_job_id, file_path=safe_path) | ||
| raise DownloadFileMissingError("File not found on disk", code="missing_on_disk") | ||
|
|
||
| if job.expires_at and self._as_utc(job.expires_at) < datetime.now(UTC): | ||
| raise DownloadFileExpiredError | ||
|
|
| # Subdomain-bypass detection: a hostname like youtube.com.evil.com | ||
| # contains a platform domain but doesn't end with a valid suffix. | ||
| # Truly unknown domains (e.g. example.com) default to "youtube" | ||
| # since yt-dlp handles most URLs. | ||
| all_platform_domains = ( | ||
| youtube_all | ||
| | _VIMEO_HOSTS | ||
| | _DAILYMOTION_HOSTS | ||
| | _TWITCH_HOSTS | ||
| | _TIKTOK_HOSTS | ||
| | _INSTAGRAM_HOSTS | ||
| ) | ||
| for domain in all_platform_domains: | ||
| if domain in hostname: | ||
| return "unknown" |
| def _parse_failure_response(response: httpx.Response) -> tuple[str, str, str | None]: | ||
| """Parse a non-200 response, mapping HTTP status + JSON error code to a category.""" | ||
| signal = f"http_{response.status_code}" | ||
| try: | ||
| payload = response.json() | ||
| except (json.JSONDecodeError, ValueError): | ||
| # Non-JSON error body — treat as transient | ||
| logger.warning( | ||
| "browser_downloader_non_json_error", status=response.status_code, | ||
| ) | ||
| raise BrowserExecutorError( | ||
| category=ErrorCategory.TRANSIENT, signal=signal | ||
| ) |
| category=ErrorCategory.TRANSIENT, signal=signal | ||
| ) | ||
|
|
||
| return _parse_failure_payload(payload) |
| # Generic 4xx (other than the BLOCKED codes above) → BLOCKED, | ||
| # except 429 (rate-limit) which is transient. | ||
| if code.startswith("http_4"): | ||
| return ErrorCategory.BLOCKED | ||
| if code.startswith("http_5"): | ||
| return ErrorCategory.TRANSIENT |
| resp = templates.TemplateResponse( | ||
| request, "partials/_download_item.html", get_template_context(request, job=job) | ||
| request, "partials/_download_item.html", get_template_context(request, job=job), | ||
| ) | ||
| rotate_csrf_token(resp) | ||
| return resp |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| }}, | ||
| }}, | ||
| }} | ||
| for i, format_spec in enumerate(fallback_chain): |
There was a problem hiding this comment.
CRITICAL: The fallback extraction loop is now nested inside _progress_hook()
Because this Python snippet is executed verbatim by python -c, moving the for i, format_spec in enumerate(fallback_chain): block under _progress_hook() leaves no top-level code that ever constructs yt_dlp.YoutubeDL(...). The helper reaches the final All formats failed block immediately, so every extraction through this path can fail before a download even starts.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ) -> None: | ||
| response.set_cookie( | ||
| key="access_token", | ||
| key="__Host-access_token", |
There was a problem hiding this comment.
WARNING: __Host- cookies are rejected when Secure is false
These names require the Secure attribute in every browser. The new key names are used everywhere, but the call sites still pass secure=settings.cookie_secure and cookie_secure defaults to False, so local/default logins will silently fail to persist auth cookies.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| response.headers["Content-Security-Policy"] = ( | ||
| f"default-src 'self'; " | ||
| f"script-src 'self' 'nonce-{nonce}' 'unsafe-hashes' " | ||
| f"script-src 'self' 'nonce-{nonce}' 'strict-dynamic' 'unsafe-hashes' " |
There was a problem hiding this comment.
WARNING: The new nonce-based script-src blocks the slide deck controller
/slides still renders an inline <script> without a nonce, while this policy now only allows inline scripts that carry nonce-{...}. Once this header ships, the presentation's keyboard/click navigation code is blocked and the deck becomes non-interactive in the browser.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| service=exc.service_name, | ||
| reset_timeout=exc.reset_timeout, | ||
| ) | ||
| raise BrowserExecutorError( |
There was a problem hiding this comment.
WARNING: Wrapping CircuitBreakerOpenError here bypasses deferred-job handling
worker.processor._handle_execution_result() only routes raw CircuitBreakerOpenError through _handle_circuit_open(). Converting it into BrowserExecutorError("circuit_open") makes browser-downloader outages go through normal retry scheduling instead of the dedicated deferred-until-recovery path that the rest of the worker uses for open circuits.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (80 files)
Reviewed by gpt-5.4-2026-03-05 · Input: 123.6K · Output: 66K · Cached: 2.3M |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/auth.py (1)
125-149: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
__Host-cookies needSecureenabled
Browsers reject__Host-*cookies withoutSecure, so any call path that passessecure=Falsewill silently drop both auth cookies and break cookie-backed login. Either requireSecurehere or use non-__Host-names when insecure cookies are enabled.🤖 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 `@app/auth.py` around lines 125 - 149, The cookie-setting logic in set_token_cookies currently allows secure=False while still using __Host-access_token and __Host-refresh_token, which browsers will reject. Update set_token_cookies to either always enforce Secure for the __Host-* cookies or switch to non-__Host names when insecure cookies are intentionally supported, and keep clear_token_cookies aligned with the chosen naming scheme.app/api/docs.py (1)
53-69: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd nonces to the self-hosted docs scripts.
app/api/docs.py:53-69,75-83
The middleware now sendsscript-src 'strict-dynamic'with a nonce, but the self-hosted/static/swagger/...jsand/static/redoc/...jstags are still parser-inserted scripts without a nonce. Withstrict-dynamic, host allowlists are ignored, so/docsand/redocwill stop booting when the local assets are present. Add the request nonce to those<script src>tags or emit a docs-specific CSP for the self-hosted branch.🤖 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 `@app/api/docs.py` around lines 53 - 69, The self-hosted docs pages still emit parser-inserted Swagger/ReDoc script tags without a nonce, so they will fail under the nonce-based CSP. Update the HTML rewriting in app/api/docs.py so the self-hosted /static/swagger/...js and /static/redoc/...js tags include the request nonce, or generate a separate CSP for the local-assets branch. Use the existing _inject_inline_script_nonce flow and the docs response handling in the docs route to keep the fix consistent.
🤖 Prompt for all review comments with 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.
Inline comments:
In @.github/workflows/fastapi-test.yml:
- Around line 203-221: The database setup in the workflow is using a generated
DB_PASS in the schema initialization step, but the Postgres service still starts
with a fixed POSTGRES_PASSWORD, so authentication will fail. Update the workflow
so the service and the later schema-init connection in the create database
schema step use the same password source, and reference the existing service
definition and the init_db/create_async_engine logic to keep them aligned.
In `@app/api/middleware/security_headers.py`:
- Around line 21-22: The CSP built in security_headers.py is too strict for the
dashboard because `script-src` in the security header includes both a nonce and
`strict-dynamic`, which prevents parser-inserted bundles like the existing
dashboard JS/SSE scripts from loading. Update the logic that constructs the
header so the dashboard path still allows those existing script tags, either by
removing `strict-dynamic` for this response or by otherwise ensuring the
`/static/js/dashboard.js` and `/static/js/sse.js` loads are compatible with
`build_security_headers` and its `script-src` generation.
In `@app/api/rate_limit_config.py`:
- Line 54: The Retry-After calculation in `_parse_retry_after()` is still using
the request count multiplier, which makes values like “5 per 1 minute” far too
large. Update `_parse_retry_after()` to use the parsed window duration from the
rate-limit config rather than multiplying by the number of requests, and verify
the logic around the `unit = unit.removesuffix("s")` parsing so the computed
`Retry-After` matches the actual window length.
In `@app/api/routes/auth.py`:
- Around line 148-152: The auth cookie writes in the token issuance flow are
incorrectly gated by settings.cookie_secure, which breaks __Host-* cookies when
Secure is false. Update the call site in the auth route that invokes
set_token_cookies so these cookies are always written with secure=True, or
switch away from the __Host- prefix if non-HTTPS support is required. Use the
existing create_access_token, create_refresh_token, and set_token_cookies flow
to locate the fix.
In `@app/api/routes/sse.py`:
- Around line 453-457: The SSE rate limit on download_status_stream is too
restrictive for EventSource auto-reconnects and can block legitimate clients
after brief disconnects. Update the limiter on download_status_stream to use a
much larger burst budget or exempt this endpoint from rate limiting entirely,
while keeping the rest of the route behavior unchanged.
In `@app/api/routes/web/web_auth.py`:
- Around line 7-9: The route handlers in web_auth.py are using Starlette’s
private _TemplateResponse as TemplateResponse in public annotations, which is
brittle. Update the affected handler annotations to use a public response type
instead, and remove the private import from starlette.templating. Keep the fix
localized to the route functions that currently reference TemplateResponse so
the API no longer depends on an internal Starlette symbol.
In `@app/api/routes/web/web_dashboard.py`:
- Line 4: The route handlers in web_dashboard use the private
starlette.templating._TemplateResponse in their annotations, so update the
affected handler signatures to use a public response type instead. Replace the
TemplateResponse alias/import and adjust the return annotations on the relevant
web dashboard endpoints to a public type such as HTMLResponse while keeping the
actual response behavior unchanged.
In `@app/api/routes/web/web_downloads.py`:
- Line 8: Avoid relying on Starlette’s private _TemplateResponse in the
web_downloads route module by switching the import and any related annotations
to a public response type such as HTMLResponse or Response. Update the relevant
handler signatures and references in the web_downloads route so they use the
public type consistently and no longer depend on the internal TemplateResponse
alias.
- Around line 109-112: The HTMX create response in web_downloads no longer
rotates CSRF, which breaks the existing contract expected by
test_create_download_htmx_returns_canonical_row_and_rotates_csrf. Update the
response path around the TemplateResponse in the create download flow to re-add
CSRF rotation by calling rotate_csrf_token before returning, and restore the
missing rotate_csrf_token import at the top of the module so the fresh
csrf_token cookie is set again.
In `@app/api/routes/web/web_settings.py`:
- Line 7: The route annotation is using Starlette’s internal _TemplateResponse
private API, which should be replaced with a public response type. Update the
return annotation in the web settings route to use Response or HTMLResponse
instead, and adjust the import in web_settings.py accordingly so the endpoint no
longer depends on TemplateResponse internals.
In `@app/services/download_service.py`:
- Around line 218-225: The expiration check in download handling is currently
happening after the disk existence probe in DownloadService, which causes
expired downloads whose files were already removed to be treated as missing
instead of expired. Move the `job.expires_at` / `_as_utc(...) <
datetime.now(UTC)` check ahead of the `os.path.isfile(safe_path)` branch in
`download_service.py`, keeping the existing `DownloadFileExpiredError` path in
the `DownloadService` flow so expired links are returned before any file-missing
logic.
In `@app/services/yt_dlp_service.py`:
- Around line 262-263: _keep _get_platform() and the platform host tables in
yt_dlp_service.py aligned with select_executor(), so TikTok and Instagram
aliases don’t fall through to YouTube. Update the host sets used by
_get_platform() to include the missing router aliases such as tiktokv.com and
instagr.am, and make sure any direct callers like resolve_video_title() and
extract_media_url() resolve those hosts to the correct platform before choosing
format chains or extractor args._
- Around line 478-493: The fallback download loop is accidentally nested inside
`_progress_hook`, so the top-level flow never reaches the `for i, format_spec in
enumerate(fallback_chain)` block or constructs `YoutubeDL(...)`. Move that loop
and the `ydl_opts` setup back out to the main download path in
`yt_dlp_service.py`, keeping `_progress_hook` as a separate callback, and ensure
the subsequent `ydl_opts.update(cookies_opts)` and `extractor_args` handling
remain in the outer scope.
In `@core/models/user.py`:
- Around line 43-45: The `created_at` and `updated_at` fields in the user model
are annotated with SQLAlchemy’s `DateTime` type instead of Python’s
`datetime.datetime`, which makes type checking inaccurate. Update the
annotations in the model that defines `created_at`/`updated_at` to use the
Python datetime type while keeping the existing `mapped_column` database
configuration unchanged.
In `@core/queue.py`:
- Around line 38-41: The close() method on _LazyRedisClient is shutting down the
shared Redis instance directly, which leaves the process-wide singleton in
core.redis_client in a closed state. Update _LazyRedisClient.close() to delegate
to the shared Redis shutdown helper used by get_redis_client() instead of
calling self._client.close() directly, and make sure the global client reference
is cleared so future get_redis_client() calls create a fresh client.
In `@core/redis_client.py`:
- Around line 74-76: reset_redis_client() currently clears the singleton without
shutting down the existing Redis connection, which can leak the client’s pool
across test cycles. Update the reset_redis_client helper in core/redis_client.py
to close the active client first by delegating to close_redis_client() (or
otherwise ensuring the async shutdown completes) before setting
_redis_state["client"] back to None.
In `@tests/test_api/test_web_routes.py`:
- Around line 2924-2927: The logout/assertion in this test only checks
__Host-access_token, so it can miss a regression where __Host-refresh_token is
not cleared. Update the assertion near the existing response.cookies check to
also verify __Host-refresh_token is absent or empty, matching the success path
behavior that clears both auth cookies.
In `@worker/browser_executor.py`:
- Around line 301-324: The HTTP-status fallback is being lost in
_parse_failure_response because it computes signal as http_<status> but then
calls _parse_failure_payload without passing that fallback, so JSON bodies
without an error field are misclassified. Update _parse_failure_response and the
helper _parse_failure_payload to accept and use the status-derived
signal/category fallback when the payload is JSON but missing error, ensuring
400/404-style responses still map based on HTTP status instead of defaulting to
unknown_error/TRANSIENT.
- Around line 211-221: The browser executor is swallowing the dedicated
circuit-open signal by catching CircuitBreakerOpenError in browser_executor.py
and re-wrapping it as BrowserExecutorError, which prevents worker/processor.py
from marking the job deferred. Update the breaker.execute path in
browser_executor.py so CircuitBreakerOpenError is not converted here; keep the
warning log if needed, but re-raise the original exception so the processor’s
circuit-open deferral logic can handle it. Use the CircuitBreakerOpenError
handling block and BrowserExecutorError creation as the key locations to adjust.
In `@worker/retry_scheduler.py`:
- Around line 58-59: The retry limit calculation in retry_scheduler.py is
treating an explicit max_retries=0 as if it were missing, because the retry
logic uses job.max_retries or 3. Update the logic in the scheduler path that
computes job_max_retries and effective_max so the default is only applied when
the value is None, not when it is zero. Keep the
CATEGORY_POLICIES[category].max_retries cap unchanged, and make sure the code
path in the retry scheduler respects a deliberate “no retries” setting on the
job model.
---
Outside diff comments:
In `@app/api/docs.py`:
- Around line 53-69: The self-hosted docs pages still emit parser-inserted
Swagger/ReDoc script tags without a nonce, so they will fail under the
nonce-based CSP. Update the HTML rewriting in app/api/docs.py so the self-hosted
/static/swagger/...js and /static/redoc/...js tags include the request nonce, or
generate a separate CSP for the local-assets branch. Use the existing
_inject_inline_script_nonce flow and the docs response handling in the docs
route to keep the fix consistent.
In `@app/auth.py`:
- Around line 125-149: The cookie-setting logic in set_token_cookies currently
allows secure=False while still using __Host-access_token and
__Host-refresh_token, which browsers will reject. Update set_token_cookies to
either always enforce Secure for the __Host-* cookies or switch to non-__Host
names when insecure cookies are intentionally supported, and keep
clear_token_cookies aligned with the chosen naming scheme.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8bbc856-697c-4a59-8321-16241fb345ea
📒 Files selected for processing (80)
.github/workflows/fastapi-test.yml.gitignoreDockerfileapp/api/dependencies/__init__.pyapp/api/docs.pyapp/api/exceptions.pyapp/api/middleware/prometheus.pyapp/api/middleware/request_body_size.pyapp/api/middleware/security_headers.pyapp/api/rate_limit_config.pyapp/api/routes/auth.pyapp/api/routes/chaos.pyapp/api/routes/downloads.pyapp/api/routes/health.pyapp/api/routes/sse.pyapp/api/routes/web/web_auth.pyapp/api/routes/web/web_auth_helpers.pyapp/api/routes/web/web_dashboard.pyapp/api/routes/web/web_downloads.pyapp/api/routes/web/web_helpers.pyapp/api/routes/web/web_settings.pyapp/api/startup.pyapp/auth.pyapp/main.pyapp/schemas/download.pyapp/schemas/token.pyapp/schemas/user.pyapp/services/auth_service.pyapp/services/circuit_breaker.pyapp/services/download_service.pyapp/services/error_classifier.pyapp/services/job_factory.pyapp/services/outbox_service.pyapp/services/pubsub_service.pyapp/services/user_service.pyapp/services/yt_dlp_service.pyapp/templates/dashboard.htmlapp/templates/slides/presentation.htmlapp/utils/demo_urls.pyapp/utils/validators.pycore/config.pycore/database.pycore/logging_config.pycore/models/download_job.pycore/models/failed_job.pycore/models/outbox.pycore/models/user.pycore/queue.pycore/redis_client.pycore/utils/security.pyentrypoint.shinfra/nginx/nginx.production.confinfra/nginx/nginx.ssl.local.confpyproject.tomltests/test_api/test_auth.pytests/test_api/test_demo_login.pytests/test_api/test_web_routes.pytests/test_auth_module.pytests/test_browser_downloader_config.pytests/test_env_contract.pytests/test_services/test_yt_dlp.pytests/test_story_3_1_web_auth_extraction.pytests/test_story_3_2_web_downloads_extraction.pytests/test_story_3_3_web_remaining_extraction.pytests/test_story_3_6_main_decomposition.pytests/test_story_8_3_javascript_bugs_performance.pytests/test_story_8_5_missing_ui_states.pytests/test_story_8_6_accessibility_audit.pytests/test_worker/test_browser_executor.pytests/test_worker/test_job_executor_routing.pyworker/browser_executor.pyworker/dlq_manager.pyworker/health.pyworker/job_claimer.pyworker/job_executor.pyworker/main.pyworker/outbox_relay.pyworker/processor.pyworker/retry_scheduler.pyworker/zombie_sweeper.py
💤 Files with no reviewable changes (2)
- infra/nginx/nginx.production.conf
- infra/nginx/nginx.ssl.local.conf
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Test Docker Compose Stack
- GitHub Check: Kilo Code Review
⚠️ CI failures not shown inline (2)
GitHub Actions: FastAPI REST API Tests / 5_Lint (Python + JS + CSS + Markdown + YAML).txt: feat(worker): Phase 2 hybrid routing — TikTok/Instagram/X via browser-downloader
Conclusion: failure
##[group]Run hatch run lint:format-check
�[36;1mhatch run lint:format-check�[0m
shell: /usr/bin/bash -e {0}
env:
PYTHON_VERSION: 3.12
HATCH_VERSION: 1.16.5
UV_PYTHON_INSTALL_DIR: /home/runner/work/_temp/uv-python-dir
UV_PYTHON: 3.12
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
Would reformat: app/api/docs.py
Would reformat: app/api/exceptions.py
Would reformat: app/api/middleware/prometheus.py
Would reformat: app/api/middleware/request_body_size.py
Would reformat: app/api/rate_limit_config.py
Would reformat: app/api/routes/auth.py
Would reformat: app/api/routes/downloads.py
Would reformat: app/api/routes/health.py
Would reformat: app/api/routes/sse.py
Would reformat: app/api/routes/web/web_auth.py
Would reformat: app/api/routes/web/web_auth_helpers.py
Would reformat: app/api/routes/web/web_downloads.py
Would reformat: app/api/routes/web/web_helpers.py
Would reformat: app/api/routes/web/web_settings.py
Would reformat: app/api/startup.py
Would reformat: app/auth.py
Would reformat: app/services/download_service.py
Would reformat: app/services/pubsub_service.py
Would reformat: app/services/user_service.py
Would reformat: app/services/yt_dlp_service.py
Would reformat: core/database.py
Would reformat: core/models/download_job.py
Would reformat: core/models/failed_job.py
Would reformat: core/models/outbox.py
Would reformat: core/models/user.py
25 files would be reformatted, 140 files already formatted
##[error]Process completed with exit code 1.
GitHub Actions: FastAPI REST API Tests / Lint (Python + JS + CSS + Markdown + YAML): feat(worker): Phase 2 hybrid routing — TikTok/Instagram/X via browser-downloader
Conclusion: failure
##[group]Run hatch run lint:format-check
�[36;1mhatch run lint:format-check�[0m
shell: /usr/bin/bash -e {0}
env:
PYTHON_VERSION: 3.12
HATCH_VERSION: 1.16.5
UV_PYTHON_INSTALL_DIR: /home/runner/work/_temp/uv-python-dir
UV_PYTHON: 3.12
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
Would reformat: app/api/docs.py
Would reformat: app/api/exceptions.py
Would reformat: app/api/middleware/prometheus.py
Would reformat: app/api/middleware/request_body_size.py
Would reformat: app/api/rate_limit_config.py
Would reformat: app/api/routes/auth.py
Would reformat: app/api/routes/downloads.py
Would reformat: app/api/routes/health.py
Would reformat: app/api/routes/sse.py
Would reformat: app/api/routes/web/web_auth.py
Would reformat: app/api/routes/web/web_auth_helpers.py
Would reformat: app/api/routes/web/web_downloads.py
Would reformat: app/api/routes/web/web_helpers.py
Would reformat: app/api/routes/web/web_settings.py
Would reformat: app/api/startup.py
Would reformat: app/auth.py
Would reformat: app/services/download_service.py
Would reformat: app/services/pubsub_service.py
Would reformat: app/services/user_service.py
Would reformat: app/services/yt_dlp_service.py
Would reformat: core/database.py
Would reformat: core/models/download_job.py
Would reformat: core/models/failed_job.py
Would reformat: core/models/outbox.py
Would reformat: core/models/user.py
25 files would be reformatted, 140 files already formatted
##[error]Process completed with exit code 1.
🧰 Additional context used
🪛 ast-grep (0.44.0)
tests/test_browser_downloader_config.py
[warning] 27-27: Do not make http calls without encryption
Context: "http://browser-downloader:3000"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
core/config.py
[warning] 79-79: Do not make http calls without encryption
Context: "http://browser-downloader:3000"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
core/models/user.py
[warning] 41-41: Do not use text() as it leads to SQL injection
Context: text("1")
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(disable-sqlalchemy-text)
tests/test_services/test_yt_dlp.py
[info] 736-736: Do not hardcode temporary file or directory names
Context: "/tmp/out"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 758-758: Do not hardcode temporary file or directory names
Context: "/tmp/out"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
worker/job_executor.py
[info] 102-107: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"retry_count": 0,
"next_retry_at": datetime.now(UTC).isoformat(),
},
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
worker/retry_scheduler.py
[info] 155-161: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"retry_count": job.retry_count + 1,
"category": decision.category.value,
"next_retry_at": decision.next_retry_at.isoformat(),
},
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
app/services/yt_dlp_service.py
[info] 91-91: use jsonify instead of json.dumps for JSON output
Context: json.dumps(cookies_opts)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 411-411: use jsonify instead of json.dumps for JSON output
Context: json.dumps(platform)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 413-413: use jsonify instead of json.dumps for JSON output
Context: json.dumps(cookies_opts)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 415-415: use jsonify instead of json.dumps for JSON output
Context: json.dumps(fallback_chain)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 417-417: use jsonify instead of json.dumps for JSON output
Context: json.dumps(extractor_args)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 427-427: use jsonify instead of json.dumps for JSON output
Context: json.dumps(list(_OUTPUT_FIELDS))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 GitHub Actions: FastAPI REST API Tests / 5_Lint (Python + JS + CSS + Markdown + YAML).txt
app/api/startup.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/api/middleware/prometheus.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
core/models/failed_job.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
core/models/outbox.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/api/middleware/request_body_size.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
core/models/download_job.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/api/exceptions.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/api/routes/web/web_auth_helpers.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/api/routes/health.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/api/routes/web/web_settings.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/api/routes/downloads.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
core/models/user.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/auth.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/services/pubsub_service.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/api/rate_limit_config.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/services/user_service.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
core/database.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/api/docs.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/api/routes/web/web_auth.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/api/routes/web/web_downloads.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/api/routes/sse.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/api/routes/auth.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/services/download_service.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/api/routes/web/web_helpers.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
app/services/yt_dlp_service.py
[error] 1-1: lint:format-check reported formatting differences. Run the formatter (e.g., hatch format) to update this file.
🪛 GitHub Actions: FastAPI REST API Tests / Lint (Python + JS + CSS + Markdown + YAML)
app/api/startup.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/api/middleware/prometheus.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
core/models/failed_job.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
core/models/outbox.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/api/middleware/request_body_size.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
core/models/download_job.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/api/exceptions.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/api/routes/web/web_auth_helpers.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/api/routes/health.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/api/routes/web/web_settings.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/api/routes/downloads.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
core/models/user.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/auth.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/services/pubsub_service.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/api/rate_limit_config.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/services/user_service.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
core/database.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/api/docs.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/api/routes/web/web_auth.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/api/routes/web/web_downloads.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/api/routes/sse.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/api/routes/auth.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/services/download_service.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/api/routes/web/web_helpers.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
app/services/yt_dlp_service.py
[error] 1-1: lint:format-check failed (hatch). File would be reformatted.
🔇 Additional comments (45)
app/schemas/download.py (1)
5-11: LGTM!app/schemas/token.py (1)
3-3: LGTM!Also applies to: 13-14
app/schemas/user.py (1)
3-10: LGTM!app/services/auth_service.py (1)
23-23: LGTM!app/utils/validators.py (1)
6-6: LGTM!Also applies to: 151-163, 179-179, 240-240
app/services/error_classifier.py (1)
19-29: LGTM!Also applies to: 133-204, 309-309, 332-332
pyproject.toml (2)
20-20: 🩺 Stability & AvailabilityRe-check the removed upper bound on
bcrypt.This repo still carries
passlib.*compatibility handling in the same file, so changing from a capped range to bare>=4.0means futurebcryptreleases can land without another review. Please verify that the old<4.1guard is no longer needed before merging.Also applies to: 108-108
30-30: LGTM!Also applies to: 76-76, 488-489, 506-615
app/api/rate_limit_config.py (1)
5-6: LGTM!Also applies to: 24-35, 69-90
app/api/routes/downloads.py (1)
83-83: LGTM!Also applies to: 145-145, 204-204, 239-242, 251-251, 296-296
core/models/download_job.py (1)
25-32: LGTM!Also applies to: 53-53
core/models/failed_job.py (1)
50-50: 🚀 Performance & ScalabilityShip the matching migration for this new index.
Adding
index=Trueon the ORM model will not create the index on existing databases by itself. If DLQ expiry queries are expected to benefit from this, please make sure the corresponding migration is included as well.core/models/user.py (1)
7-18: LGTM!worker/dlq_manager.py (1)
86-90: LGTM!Also applies to: 139-142
app/api/middleware/prometheus.py (1)
4-19: LGTM!app/api/middleware/request_body_size.py (1)
3-20: LGTM!app/api/routes/health.py (1)
77-82: LGTM!Also applies to: 124-129
app/services/circuit_breaker.py (1)
428-467: LGTM!Also applies to: 503-537
worker/job_claimer.py (1)
52-94: LGTM!worker/outbox_relay.py (1)
27-102: LGTM!worker/retry_scheduler.py (1)
139-162: LGTM!app/api/exceptions.py (1)
58-60: LGTM!Also applies to: 109-116
core/database.py (1)
8-16: LGTM!Also applies to: 31-34, 58-65, 70-80
core/utils/security.py (1)
15-23: LGTM!app/utils/demo_urls.py (1)
1-1: LGTM!worker/processor.py (1)
79-79: LGTM!Also applies to: 199-199
worker/zombie_sweeper.py (1)
56-56: LGTM!Also applies to: 90-90, 108-108
worker/main.py (1)
96-97: LGTM!Also applies to: 118-124, 135-136, 159-159, 303-303
app/services/pubsub_service.py (1)
40-41: LGTM!Also applies to: 55-56, 67-69, 80-81, 106-108, 165-166, 222-223, 242-245, 261-263
app/services/user_service.py (1)
10-10: LGTM!Also applies to: 97-99, 141-159, 175-175, 195-195, 214-214, 236-236
core/logging_config.py (1)
79-79: LGTM!Also applies to: 176-176
entrypoint.sh (1)
2-2: LGTM!.github/workflows/fastapi-test.yml (1)
52-52: LGTM!Also applies to: 79-79
.gitignore (1)
184-194: LGTM!tests/test_story_3_6_main_decomposition.py (1)
311-312: LGTM!tests/test_story_8_3_javascript_bugs_performance.py (1)
168-168: LGTM!Also applies to: 181-181, 201-201
tests/test_story_8_5_missing_ui_states.py (1)
67-67: LGTM!Also applies to: 96-96
worker/health.py (1)
236-249: LGTM!app/main.py (1)
136-136: LGTM!app/api/routes/auth.py (1)
207-207: LGTM!Also applies to: 263-270, 319-335
tests/test_story_3_3_web_remaining_extraction.py (1)
160-183: LGTM!Also applies to: 205-216, 235-263
app/api/routes/web/web_helpers.py (1)
6-6: LGTM!Also applies to: 49-56, 139-170, 202-202, 235-235
app/templates/dashboard.html (1)
47-55: LGTM!Also applies to: 166-188
app/templates/slides/presentation.html (1)
12-12: LGTM!Also applies to: 253-257, 289-289, 331-339
app/api/routes/web/web_auth_helpers.py (1)
37-37: LGTM!Also applies to: 83-83, 140-147
| @limiter.limit("5/minute") | ||
| async def download_status_stream( | ||
| request: Request, | ||
| current_user: CurrentUserFromCookie, | ||
| ): | ||
| ) -> EventSourceResponse: |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The new SSE limit will lock out legitimate auto-reconnects.
EventSource reconnects automatically on transient disconnects. At 5/minute, a short deploy or flaky network can exhaust the bucket and leave the page without live status/progress updates for the rest of the window. This stream needs a much larger burst budget than normal GET routes, or an exemption entirely.
🤖 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 `@app/api/routes/sse.py` around lines 453 - 457, The SSE rate limit on
download_status_stream is too restrictive for EventSource auto-reconnects and
can block legitimate clients after brief disconnects. Update the limiter on
download_status_stream to use a much larger burst budget or exempt this endpoint
from rate limiting entirely, while keeping the rest of the route behavior
unchanged.
| def reset_redis_client() -> None: | ||
| """Reset the singleton (for testing only).""" | ||
| _redis_state["client"] = None |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
reset_redis_client() drops a live client without closing it.
This helper clears the singleton slot but never closes the existing async Redis client first. In repeated test/setup cycles that leaks the old connection pool. Either make reset delegate to close_redis_client() or require callers to await shutdown before resetting.
🤖 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 `@core/redis_client.py` around lines 74 - 76, reset_redis_client() currently
clears the singleton without shutting down the existing Redis connection, which
can leak the client’s pool across test cycles. Update the reset_redis_client
helper in core/redis_client.py to close the active client first by delegating to
close_redis_client() (or otherwise ensuring the async shutdown completes) before
setting _redis_state["client"] back to None.
| assert ( | ||
| "__Host-access_token" not in response.cookies | ||
| or response.cookies.get("__Host-access_token") == "" | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Assert the refresh cookie is cleared here too.
Line 2924 only verifies __Host-access_token, but the success path clears both auth cookies. This test will miss a regression that leaves __Host-refresh_token behind.
Suggested test update
assert (
"__Host-access_token" not in response.cookies
or response.cookies.get("__Host-access_token") == ""
)
+ assert (
+ "__Host-refresh_token" not in response.cookies
+ or response.cookies.get("__Host-refresh_token") == ""
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert ( | |
| "__Host-access_token" not in response.cookies | |
| or response.cookies.get("__Host-access_token") == "" | |
| ) | |
| assert ( | |
| "__Host-access_token" not in response.cookies | |
| or response.cookies.get("__Host-access_token") == "" | |
| ) | |
| assert ( | |
| "__Host-refresh_token" not in response.cookies | |
| or response.cookies.get("__Host-refresh_token") == "" | |
| ) |
🤖 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/test_api/test_web_routes.py` around lines 2924 - 2927, The
logout/assertion in this test only checks __Host-access_token, so it can miss a
regression where __Host-refresh_token is not cleared. Update the assertion near
the existing response.cookies check to also verify __Host-refresh_token is
absent or empty, matching the success path behavior that clears both auth
cookies.
CRITICAL fixes: - yt_dlp_service.py: move fallback loop out of _progress_hook so extraction actually runs (was nested, causing immediate failure) - yt_dlp_service.py: add missing tiktokv.com/instagr.am to platform hosts - yt_dlp_service.py: tighten subdomain-bypass detection to trigger on domain.label patterns only (not substring matches like myyoutube.com) - browser_executor.py: let CircuitBreakerOpenError propagate raw so processor's deferred-job path handles it correctly - browser_executor.py: use HTTP status signal for non-JSON error bodies so 404/403/429 categorize correctly instead of all being TRANSIENT - browser_executor.py: http_404 → NOT_FOUND (was BLOCKED) - browser_executor.py: preserve HTTP status fallback when JSON body lacks explicit error field HIGH fixes: - download_service.py: expiry check before disk probe (was returning 404 instead of 410 for expired files already cleaned from disk) - retry_scheduler.py: use 'is not None' check for max_retries instead of 'or 3' which overrides explicit 0 - rate_limit_config.py: Retry-After now uses window multiplier instead of request count (5 per 1 minute → 60s, not 300s) - core/queue.py: use shared close_redis_client() instead of closing singleton directly (prevent poisoned singleton) - auth.py: force secure=True for __Host- prefixed cookies regardless of settings.cookie_secure (browsers reject __Host- without Secure) - models/user.py: use datetime.datetime type annotation instead of SQLAlchemy DateTime - web_downloads.py: restore CSRF token rotation on HTMX download create - job_executor.py: remove unused BrowserExecutorError import Updated affected tests to match new behavior.
Carried over from Video DownloadHelper and FlowPick analysis: DRM manifest heuristics (tier1-cdp.js): - Scan HLS manifests for SAMPLE-AES* methods and non-identity KEYFORMAT - Scan DASH manifests for ContentProtection elements - DRM detected at manifest-parse time (milliseconds) instead of waiting for EME API polling - Plain AES-128 with identity keyformat passes through Auth header capture & replay: - CDP Network.requestWillBeSent captures Referer/Origin for segment auth - Injected page script monkey-patches fetch/XHR to capture auth headers - Headers forwarded to streamlink-backend via --http-header flags Streamlink backend improvements (streamlink-backend.js): - Per-resource-kind size caps: manifest=8MiB, key=64KiB, segment=256MiB - Context fallback: same-origin CORS → include CORS with dedup - Segment-level retry with exponential backoff + jitter (3 retries) - DRM detection uses precise regexp (SAMPLE-AES*/non-identity KEYFORMAT) - HLS encrypted stream detection now throws drm_detected (not network_error) 108 tests pass across 8 test files.
…nt cleanup - Outbox: add partial unique index on (job_id, status='pending') for real idempotency under concurrent writers; handle IntegrityError in outbox_service without aborting the outer transaction. - Worker chaos helpers: log swallowed exceptions instead of bare try-except-pass (S110); annotate random.uniform with noqa (S311). - BrowserExecutorError: annotate _CATEGORY_MARKERS with ClassVar (RUF012); add exception chaining via 'from err' (B904). - Browser-downloader JS: fix useOptionalChain, noUnnecessaryContinue, noUnusedVariables (biome). - Outbox relay: structured cleanup of stale entries and relay loop. - Tier1 CDP: capture fetch headers for auth replay in streamlink; add referrer/origin spoofing for video segment requests.
Signed-off-by: tomkabel <[email protected]>
- S110: Replace bare try-except-pass with logger.debug in _update_circuit_deferred_depth - ASYNC240: Suppress os.path.exists warning (local filesystem, negligible latency; project does not use anyio) - Fix missing trailing newline in pnpm-workspace.yaml
Signed-off-by: tomkabel <[email protected]>
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
There was a problem hiding this comment.
CVE-2025-59288 in playwright - high severity
Improper verification of cryptographic signature in Github: Playwright allows an unauthorized attacker to perform spoofing over an adjacent network.
Details
Remediation Aikido suggests bumping this package to version 1.55.1 to resolve this issue
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| let resolved; | ||
| try { | ||
| resolved = await realpath(dir); |
| if (cap != null && buf.length > cap) { | ||
| throw new DownloaderError('network_error', `${resourceKind} body exceeds size cap`); | ||
| } | ||
| await writeFile(destPath, buf); |
There was a problem hiding this comment.
Actionable comments posted: 44
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/auth.py (1)
153-155: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSet
Secure=Truewhen deleting__Host-cookies.
Response.delete_cookiedefaults tosecure=False, so the logout cookies omit the requiredSecureattribute for__Host-cookies. Keeppath="/"and omitdomain, but passsecure=Truefor both deletions to avoid browsers rejecting the delete response and leaving the cookies set.🤖 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 `@app/auth.py` around lines 153 - 155, Update clear_token_cookies so both response.delete_cookie calls for __Host-access_token and __Host-refresh_token pass secure=True, while retaining path="/" and omitting domain.worker/job_executor.py (1)
171-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse snake_case event names for these structlog calls.
Lines 171, 184, and 192 pass a full sentence as the structlog event key, for example
"zombie_sweep_recovery skipped (non-critical)". Every other call in this file uses a snake_case event name, such as"chaos_slow_processing"at Line 189. Sentences as event keys break grouping and alerting on theeventfield.♻️ Proposed change
- logger.debug("zombie_sweep_recovery skipped (non-critical)", exc_info=True) + logger.debug("chaos_zombie_check_skipped", exc_info=True) @@ - logger.debug("chaos_db_failover check skipped (non-critical)", exc_info=True) + logger.debug("chaos_db_failover_check_skipped", exc_info=True) @@ - logger.debug("chaos_slow_processing skipped (non-critical)", exc_info=True) + logger.debug("chaos_slow_processing_check_skipped", exc_info=True)Line 341 has the same pattern:
"extract_task cancel cleanup (non-critical)".🤖 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 `@worker/job_executor.py` around lines 171 - 192, Update the structlog calls in the relevant exception handlers to use concise snake_case event keys instead of sentence-like messages, including the calls near zombie_sweep_recovery, chaos_db_failover, chaos_slow_processing, and extract_task cancellation cleanup. Preserve the existing non-critical context through structured fields or an appropriate snake_case event name, and ensure all affected event values support consistent grouping and alerting.app/services/yt_dlp_service.py (1)
434-443: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPass the YouTube-only options as data instead of pre-indented source text.
youtube_optsembeds literal 12-space indentation inside a string, and Line 493 relies on that string ending with a newline so the following}}closes the dict. Any change to the indentation of the generatedydl_optsblock silently produces anIndentationErroror aSyntaxErrorin the subprocess, which surfaces only at runtime as a failed extraction.Serialize the platform options as JSON and merge them in the script, the same way
cookies_optsandextractor_argsalready work.♻️ Proposed change
- # Only inject youtube-specific options when building the script for YouTube. - # Tests assert that non-YouTube platform scripts do not contain YouTube-only keys. - # Embedding them as dict entries keeps the options inside the ydl_opts dict definition. - if platform == "youtube": - youtube_opts = ( - ' "prefer_free_formats": True,\n "check_formats": "missable",\n' - ) - else: - youtube_opts = "" + # Only inject youtube-specific options when building the script for YouTube. + # Tests assert that non-YouTube platform scripts do not contain YouTube-only keys. + platform_opts = ( + {"prefer_free_formats": True, "check_formats": "missable"} + if platform == "youtube" + else {} + ) + platform_opts_json = json.dumps(platform_opts)In the generated script, declare
platform_opts = {platform_opts_json}next tocookies_opts, replace{youtube_opts} }}with}}, and addydl_opts.update(platform_opts)beside the existingydl_opts.update(cookies_opts).🤖 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 `@app/services/yt_dlp_service.py` around lines 434 - 443, Update the YouTube-specific option handling in the script-generation flow to serialize platform options as JSON data rather than embedding pre-indented source text. In the code that builds the generated script and its ydl_opts block, declare platform_opts alongside cookies_opts, remove the youtube_opts interpolation from the dictionary literal, and merge platform_opts with ydl_opts.update next to the existing cookies_opts update.worker/browser_executor.py (1)
353-368: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
RATE_LIMITEDforhttp_429and handlehttp_408/http_425without blocking retries.
RATE_LIMITEDgets 5 retries with a 60s base delay, whileTRANSIENTgets 3 retries with a 10s base delay.http_429is rate limiting, so mapping it toTRANSIENTskips the dedicated retry policy and rate-limit metrics. Also move the generic 4xx fallback after explicit rate limit and timeout handling;http_408andhttp_425currently matchhttp_4*and are treated non-retryable.🤖 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 `@worker/browser_executor.py` around lines 353 - 368, Update the error-category mapping to return ErrorCategory.RATE_LIMITED for http_429, and explicitly classify http_408 and http_425 as retryable before the generic http_4 fallback. Preserve the existing NOT_FOUND mappings and ensure the generic 4xx handling remains after these explicit cases so they are not marked BLOCKED.
♻️ Duplicate comments (2)
app/api/routes/web/web_downloads.py (2)
110-114: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep CSRF rotation consistent across successful HTMX mutations.
The create response reuses the current token, and the delete response omits token updates. Restore one fresh-token path for both successful mutations.
app/api/routes/web/web_downloads.py#L110-L114: replace the current-token cookie write withrotate_csrf_token(resp)or an equivalent fresh-token helper. Restore the removed import.app/api/routes/web/web_downloads.py#L193-L194: apply the same rotation before returning the successful empty response if deletion shares this contract.🤖 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 `@app/api/routes/web/web_downloads.py` around lines 110 - 114, In app/api/routes/web/web_downloads.py lines 110-114, update the successful create response to use rotate_csrf_token(resp) or the equivalent fresh-token helper instead of set_csrf_token_cookie with the current token, and restore its import. In lines 193-194, apply the same CSRF rotation before returning the successful deletion response.
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a public response type for annotations.
Line 8 aliases Starlette’s private
_TemplateResponseand exposes it through the route annotations. UseHTMLResponseorResponseafter confirming the project’s declared Starlette version.This avoids coupling the route module to a private library API.
#!/usr/bin/env bash set -euo pipefail rg -n 'starlette|fastapi' pyproject.toml rg -n '_TemplateResponse|TemplateResponse' app/api/routes/web/web_downloads.pyAlso applies to: 67-67
🤖 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 `@app/api/routes/web/web_downloads.py` at line 8, Replace the private _TemplateResponse alias used by the route annotations with a public Starlette response type, preferably HTMLResponse or Response, based on the project’s declared Starlette version. Update both TemplateResponse annotation usages in the web downloads route module while preserving the existing response behavior.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@app/services/download_service.py`:
- Around line 389-399: In the fast path around enqueue_job, capture and retain
the original pending Outbox.id before delivery, then update only that row by ID
while preserving the status == "pending" guard; do not continue matching by
job_id alone. Add a regression test that inserts a second pending Outbox row
between queue delivery and the update and verifies the later row remains
pending.
In `@app/services/outbox_service.py`:
- Around line 51-59: Update the outbox insert flow around the db.flush()
IntegrityError handler to execute the add/flush inside db.begin_nested(), so
duplicate conflicts roll back only the savepoint and leave the outer transaction
active. On conflict, confirm the existing pending outbox row before returning
None, and re-raise IntegrityError instances that are not confirmed duplicates;
do not rely on db.expunge(outbox_entry) for recovery.
In `@core/models/outbox.py`:
- Around line 61-67: Add an Alembic migration for the model’s
uq_outbox_pending_job_id index, creating a unique job_id index filtered by
status = 'pending' for both PostgreSQL and SQLite, and include the corresponding
downgrade removal. Ensure the migration is linked to the current revision chain.
- Around line 61-67: Update write_job_to_outbox to execute the outbox insert and
flush inside an await db.begin_nested() savepoint, preserving its existing
concurrent-conflict handling so failures do not invalidate the enclosing
transaction. Add a concurrency test using two sessions that exercises the
partial unique index and verifies only one pending job is retained while both
sessions remain usable.
In `@packages/browser-downloader/Dockerfile`:
- Around line 13-30: Update the Dockerfile dependency installation to pin
streamlink to a fixed version, copy the root pnpm-lock.yaml alongside
package.json before installation, and run pnpm install --prod with
--frozen-lockfile while preserving --ignore-scripts and cleanup behavior.
In `@packages/browser-downloader/src/downloader.js`:
- Around line 96-109: Update the failure path in the downloader’s catch block to
remove the partially created outPath before returning the failed result,
covering both DownloaderError and classified errors while preserving existing
error classification and cleanup behavior.
- Around line 28-38: Replace the boolean stealthApplied state used by
launchStealthBrowser with a shared setup promise, assigning it before awaiting
the stealth plugin import so concurrent calls reuse the same initialization.
Ensure chromium.use(StealthPlugin()) executes only once, then await the memoized
setup before launching the browser.
In `@packages/browser-downloader/src/errors.js`:
- Around line 112-135: Move the TypeError-with-cause recursion in classifyError
outside the typeof code === 'string' block, while keeping direct string-code
timeout and network checks unchanged. Ensure fetch failures without their own
code recurse into err.cause so a cause such as ETIMEDOUT is classified as
ERROR_CODES.TIMEOUT.
In `@packages/browser-downloader/src/server.js`:
- Around line 65-72: Remove the duplicate full validation from the request
handler around validateUrl and validateOutputDir, while preserving download() as
the security boundary and retaining the 400 response mapping for invalid client
input. Either replace this block with a cheap pre-check or pass its normalized
URL and resolved directory into download() so downloader.js can reuse them
without revalidation; ensure downloadStream’s derived-URL validation remains
unchanged.
- Around line 88-116: Propagate an AbortSignal from the request handler through
download() and both tier functions into downloadStream, reusing its existing
opts.signal support. In the request timeout callback, abort the signal before
rejecting so active streamlink/CDP work stops; ensure the timeout path also
closes the Chromium instance before release(), keeping browser and semaphore
cleanup synchronized.
- Around line 21-29: Update the MAX_CONCURRENCY configuration to read from the
appropriate environment variable using the existing parseTimeout helper, while
retaining 2 as the safe default when the value is absent or invalid. Pass the
resulting configurable limit to createSemaphore.
- Around line 100-110: Update the request timeout rejection in the Promise race
to use the exported TIMEOUT error code instead of the plain "request_timeout"
message, ensuring safeClassify returns timeout for hung requests while
preserving the existing catch response flow.
In `@packages/browser-downloader/src/streamlink-backend.js`:
- Around line 446-465: Update the segment-validation loop in the playlist
processing flow to cache successful validateUrl results by hostname for the
duration of this call. Reuse the cached result for consecutive or repeated
segment hosts, and invoke validateUrl with { lookup } only when a hostname has
not yet been validated; continue pushing every resolved href into segUrls.
- Around line 133-211: The retry flow in fetchResWithRetry retries terminal
failures and fetchResOne drops supported context options. In
packages/browser-downloader/src/streamlink-backend.js lines 133-211, reduce
contextCandidates usage to header variants that actually differ, remove the
unused init object in fetchResOne, and immediately propagate non-retryable 4xx
responses, validateUrl rejections, and size-cap errors. In
packages/browser-downloader/tests/streamlink-backend.test.js lines 284-315,
update the redirect-SSRF and size-cap tests to confirm fail-fast behavior and
raise their testTimeout until the source fix eliminates the timing risk.
- Around line 317-337: Update fetchText and fetchToFile to consume res.body as a
stream with a running byte count, aborting or cancelling immediately when the
applicable cap is exceeded instead of calling res.text() or res.arrayBuffer().
Preserve the existing DownloaderError and resource-specific cap behavior; have
fetchText assemble only the permitted text and have fetchToFile write chunks
incrementally so the full segment is never materialized in memory.
In `@packages/browser-downloader/src/tier1-cdp.js`:
- Around line 163-165: Update requestHeaders handling in onRequestWillBeSent and
the line-256 read path to delete each requestId entry immediately after reading
it, then add a fixed capacity and oldest-entry eviction comparable to
tier2-dom.js’s BLOB_CAP behavior. Preserve candidate processing while ensuring
requestHeaders cannot retain unbounded request data or credentials.
- Around line 224-236: In the response-candidate handling around
Network.responseReceived and the Network.getResponseBody calls, reject
candidates using encodedDataLength or the Content-Length header before
materializing the body, while retaining the existing post-read caps as
backstops. Update the manifest check to use Buffer.byteLength(body) instead of
body.length so MANIFEST_CAP is enforced in bytes; apply the analogous pre-read
guard to the bodyCap path.
- Around line 49-50: Prevent credential leakage from the auth-header replay
path: update AUTH_HEADER_NAMES and the authHeaders flow used by interceptMedia,
downloader.js, and downloadStream so cookie and authorization values are not
passed as streamlink command-line arguments. Either restrict capture to referer
and origin when credentials are unnecessary, or pass required credentials
through a file or environment-based mechanism, and verify authHeaders are never
logged or included in HTTP response bodies.
- Around line 347-359: Fix block-page detection in the onDomReady page.evaluate
callback and the corresponding tier2-dom evaluation callbacks by passing the
block-pattern value into the browser context or defining an equivalent
page-local pattern, rather than referencing module-scoped BLOCK_RE. Preserve the
existing document text collection, blocked-result handling, and onTerminal(new
DownloaderError('anti_bot_block')) behavior, and update tests so the evaluate
callbacks are actually executed or otherwise verify the page-context pattern is
available.
- Around line 46-47: Update the DASH_DRM_RE pattern to detect ContentProtection
elements when followed by whitespace, a closing angle bracket, or a self-closing
slash, including both <ContentProtection> and <ContentProtection/> forms.
Preserve its use for identifying DRM-protected DASH manifests before routing to
streamlink.
- Around line 96-112: Normalize header names to lowercase in the object and
array branches of the init.headers handling near the existing auth extraction,
matching the Headers branch and the XHR hook. Ensure the subsequent referer,
origin, cookie, and authorization lookups in the auth object capture headers
regardless of caller casing.
In `@packages/browser-downloader/src/tier2-dom.js`:
- Around line 79-92: Update tryClickPlay so it only returns after a selector’s
click succeeds; when handle.click rejects, swallow the error and continue trying
later selectors. Replace the broad 'button' fallback in the selectors list with
a play-specific button selector such as button[aria-label*="play" i], while
preserving the existing selector iteration and error handling.
- Around line 168-173: Update the in-page media body handling around the
arrayBuffer callback to avoid serializing bytes with Array.from(new
Uint8Array(ab)); after checking ab.byteLength against cap, return a compact
base64 representation and decode it with Buffer.from(..., 'base64') in the
Node-side flow before constructing the result. Ensure the ab.byteLength cap
check occurs before any encoding or conversion, including the corresponding
handling around lines 186-195.
- Around line 25-34: Unify the blob-cap implementation so the browser-injected
HOOK_SRC is the tested source of truth instead of maintaining separate
pushBlobUrl and record logic. Make HOOK_SRC accept the cap argument, pass
BLOB_CAP through page.evaluate, remove the local CAP constant, and keep the
non-Blob guard and oldest-entry eviction behavior inline in the injected
function; update tests to exercise this injected path rather than only
pushBlobUrl.
- Around line 117-128: Update both page.evaluate callbacks in the block-page
checks to make the block pattern available inside the browser context, rather
than referencing the Node-scoped BLOCK_RE directly. Pass or construct the
pattern within each callback while preserving the existing text extraction and
anti-bot detection behavior.
- Around line 157-181: Update the page.evaluate invocation and its evaluated
callback so the URL, body cap, and timeout are passed as one object and
destructured inside the callback; ensure the abort timer uses bodyCap and
timeout, allowing fetch size checks to remain active and the request to receive
the intended bounded timeout.
In `@packages/browser-downloader/src/validate.js`:
- Around line 87-108: Extend isPrivateIp to reject the remaining reserved IPv4
ranges: 100.64.0.0/10, 192.0.0.0/24, 198.18.0.0/15, and multicast/broadcast
addresses (224.0.0.0/4 and 255.255.255.255). Preserve the existing checks for
RFC 1918, loopback, link-local, and 0.0.0.0/8.
In `@packages/browser-downloader/tests/downloader.test.js`:
- Around line 111-119: Update the download test around download() to avoid
dependence on inherited BD_DOWNLOAD_TIMEOUT_MS: either pass downloadTimeout:
120_000 explicitly in the options or remove and restore that environment
variable in setup. Preserve the assertion that streamlink receives a 120_000
timeout.
- Around line 168-189: Update the downloader test setup to stub
BD_DOWNLOAD_TIMEOUT_MS with Vitest’s vi.stubEnv('BD_DOWNLOAD_TIMEOUT_MS',
'60000') in beforeEach, and remove the manual env snapshot and process.env
replacement. Restore environment stubs in afterEach using vi.unstubAllEnvs(),
while preserving the existing mock setup and teardown.
In `@packages/browser-downloader/tests/errors.test.js`:
- Around line 36-41: Update classifyError so cause handling is evaluated
independently of the err.code branch, allowing fetch TypeError errors with an
ETIMEDOUT cause to classify as TIMEOUT while preserving existing network-cause
behavior. Extend the error classification tests with a TypeError carrying an
ETIMEDOUT cause and assert ERROR_CODES.TIMEOUT.
In `@packages/browser-downloader/tests/server.test.js`:
- Around line 169-179: Replace the fixed 30 ms sleep in the semaphore
concurrency test around the download mock with an awaitable barrier that becomes
ready after two download invocations, then await that barrier before issuing the
third request. In packages/browser-downloader/tests/server.test.js lines
169-179, update the download mock and test flow accordingly; in lines 189-221,
replace the 50 ms and 30 ms sleeps with await once(second, 'error') and await
once(server, 'error') respectively, preserving the existing assertions.
- Around line 169-179: Replace the fixed timeout in the concurrency test with a
deterministic barrier: have the mocks.download implementation signal each
invocation, await signals for both initial requests, then submit the third
request and retain the existing 503/concurrency_limit assertions.
In `@packages/browser-downloader/tests/streamlink-backend.test.js`:
- Around line 284-315: Update the retry classification in fetchResWithRetry so
terminal validation failures from validateUrl and
DownloaderError('network_error') caused by the body size cap are not retried.
Preserve retries for genuinely transient fetch failures, allowing the
redirect-SSRF and Content-Length rejection tests to fail immediately.
- Around line 90-106: Move vi.unstubAllGlobals() into the test suite’s afterEach
cleanup hook so global fetch stubs are removed even when assertions fail. Delete
the trailing vi.unstubAllGlobals() calls from each test body, including the test
around downloadManifestFallback.
In `@packages/browser-downloader/tests/tier1-cdp.test.js`:
- Around line 34-46: The page.evaluate fakes must fail loudly on unrecognized
probes instead of silently returning undefined. In
packages/browser-downloader/tests/tier1-cdp.test.js, update the evaluate mock’s
final fallback to throw an error containing the unmatched source; make the same
change in packages/browser-downloader/tests/tier2-dom.test.js while preserving
its explicit undefined return for the createObjectURL hook patch.
In `@packages/browser-downloader/tests/validate.test.js`:
- Around line 134-137: Update the non-existent-directory test around
validateOutputDir to pass the temporary base directory explicitly and enable
realpath resolution, then assert the rejection message for the missing
subdirectory specifically. Keep the test focused on the child path being absent
rather than allowing the default output-base lookup to trigger a different
validation branch.
- Around line 128-132: Update the test named “rejects a path outside the base
(path traversal)” so its mocked realpath resolves an input containing a
parent-directory segment or symlink-like path from within base to an outside
location, such as /etc. Ensure validateOutputDir receives that traversal path
and still rejects it, rather than testing an already absolute path unrelated to
base.
In `@tests/test_story_3_5_processor_retry_dlq_extraction.py`:
- Around line 403-404: Update the assertion in the pending-status test to
require an actual relay predicate matching Outbox.status == _PENDING_STATUS or
Outbox.status == "pending", rather than accepting only the _PENDING_STATUS
declaration. Preserve the source-based validation while ensuring the relay query
itself includes the pending-status filter.
In `@tests/test_story_5_6_model_index_consistency.py`:
- Around line 112-118: Update the outbox index assertions in the test covering
model index consistency to inspect the metadata for uq_outbox_pending_job_id,
verifying it is unique and has the status = 'pending' predicate through both the
model metadata and SQL inspection paths, rather than asserting only its name.
In `@tests/test_worker/test_browser_executor.py`:
- Around line 290-296: Rename
test_open_circuit_raises_transient_without_http_call to
test_open_circuit_propagates_circuit_breaker_open_without_http_call to reflect
the asserted exception, and move the CircuitBreakerOpenError import from inside
the test to the module-level imports.
In `@worker/browser_executor.py`:
- Around line 323-341: Update _parse_failure_payload to return NoReturn and
import NoReturn from typing, reflecting that it always raises
BrowserExecutorError. Remove the type: ignore by retrieving payload.get("error")
without an incorrect str annotation, then validate it with the existing
isinstance/non-empty check before applying fallback_code. Simplify the
_parse_success failure path from returning _parse_failure_payload(payload) to
calling it directly.
In `@worker/main.py`:
- Around line 271-273: Validate the new OUTBOX_SYNC_INTERVAL_SECONDS default and
configured interval against production worker counts in the outbox polling flow
around sync_outbox_to_queue(). Confirm the pending-outbox query and database
capacity support the increased polling frequency, and adjust the interval or
query/database configuration as needed while preserving the existing fallback
behavior for invalid values.
In `@worker/outbox_relay.py`:
- Around line 54-59: Update the pending-age calculation in the outbox relay to
fetch the minimum pending Outbox.created_at value and compute elapsed seconds in
Python, avoiding SQLite datetime subtraction. Preserve the existing zero value
when no pending record exists, and ensure the OUTBOX_OLDEST_PENDING_SECONDS
metric receives the computed age without relying on the exception-reset path.
In `@worker/retry_scheduler.py`:
- Around line 172-179: In the retry scheduling flow around the Outbox status
update and db.commit, ensure the exception handler calls await db.rollback()
when either operation fails after Redis accepts the retry. Keep the outbox row
pending and allow the subsequent select(DownloadJob) to execute without a failed
transaction.
---
Outside diff comments:
In `@app/auth.py`:
- Around line 153-155: Update clear_token_cookies so both response.delete_cookie
calls for __Host-access_token and __Host-refresh_token pass secure=True, while
retaining path="/" and omitting domain.
In `@app/services/yt_dlp_service.py`:
- Around line 434-443: Update the YouTube-specific option handling in the
script-generation flow to serialize platform options as JSON data rather than
embedding pre-indented source text. In the code that builds the generated script
and its ydl_opts block, declare platform_opts alongside cookies_opts, remove the
youtube_opts interpolation from the dictionary literal, and merge platform_opts
with ydl_opts.update next to the existing cookies_opts update.
In `@worker/browser_executor.py`:
- Around line 353-368: Update the error-category mapping to return
ErrorCategory.RATE_LIMITED for http_429, and explicitly classify http_408 and
http_425 as retryable before the generic http_4 fallback. Preserve the existing
NOT_FOUND mappings and ensure the generic 4xx handling remains after these
explicit cases so they are not marked BLOCKED.
In `@worker/job_executor.py`:
- Around line 171-192: Update the structlog calls in the relevant exception
handlers to use concise snake_case event keys instead of sentence-like messages,
including the calls near zombie_sweep_recovery, chaos_db_failover,
chaos_slow_processing, and extract_task cancellation cleanup. Preserve the
existing non-critical context through structured fields or an appropriate
snake_case event name, and ensure all affected event values support consistent
grouping and alerting.
---
Duplicate comments:
In `@app/api/routes/web/web_downloads.py`:
- Around line 110-114: In app/api/routes/web/web_downloads.py lines 110-114,
update the successful create response to use rotate_csrf_token(resp) or the
equivalent fresh-token helper instead of set_csrf_token_cookie with the current
token, and restore its import. In lines 193-194, apply the same CSRF rotation
before returning the successful deletion response.
- Line 8: Replace the private _TemplateResponse alias used by the route
annotations with a public Starlette response type, preferably HTMLResponse or
Response, based on the project’s declared Starlette version. Update both
TemplateResponse annotation usages in the web downloads route module while
preserving the existing response behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5b1993d4-0468-446b-a0d8-e4f36c513516
📒 Files selected for processing (46)
app/api/rate_limit_config.pyapp/api/routes/downloads.pyapp/api/routes/web/web_downloads.pyapp/auth.pyapp/services/download_service.pyapp/services/outbox_service.pyapp/services/yt_dlp_service.pycore/metrics.pycore/models/outbox.pycore/models/user.pycore/queue.pypackages/browser-downloader/Dockerfilepackages/browser-downloader/package.jsonpackages/browser-downloader/src/concurrency.jspackages/browser-downloader/src/downloader.jspackages/browser-downloader/src/errors.jspackages/browser-downloader/src/server.jspackages/browser-downloader/src/streamlink-backend.jspackages/browser-downloader/src/tier1-cdp.jspackages/browser-downloader/src/tier2-dom.jspackages/browser-downloader/src/validate.jspackages/browser-downloader/tests/concurrency.test.jspackages/browser-downloader/tests/downloader.test.jspackages/browser-downloader/tests/errors.test.jspackages/browser-downloader/tests/server.test.jspackages/browser-downloader/tests/streamlink-backend.test.jspackages/browser-downloader/tests/tier1-cdp.test.jspackages/browser-downloader/tests/tier2-dom.test.jspackages/browser-downloader/tests/validate.test.jspackages/browser-downloader/vitest.config.jspnpm-workspace.yamltests/test_services/test_download_service.pytests/test_story_1_2_config_extraction.pytests/test_story_1_3_database_metrics_extraction.pytests/test_story_1_4_redis_logging_queue_extraction.pytests/test_story_2_4_web_html_consolidation.pytests/test_story_3_1_web_auth_extraction.pytests/test_story_3_5_processor_retry_dlq_extraction.pytests/test_story_5_6_model_index_consistency.pytests/test_worker/test_browser_executor.pytests/test_worker/test_outbox_recovery.pyworker/browser_executor.pyworker/job_executor.pyworker/main.pyworker/outbox_relay.pyworker/retry_scheduler.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Test Docker Compose Stack
⚠️ CI failures not shown inline (2)
GitHub Actions: FastAPI REST API Tests / 5_Lint (Python + JS + CSS + Markdown + YAML).txt: feat(worker): Phase 2 hybrid routing — TikTok/Instagram/X via browser-downloader
Conclusion: failure
##[group]Run hatch run lint:check
�[36;1mhatch run lint:check�[0m
shell: /usr/bin/bash -e {0}
env:
PYTHON_VERSION: 3.12
HATCH_VERSION: 1.16.5
UV_PYTHON_INSTALL_DIR: /home/runner/work/_temp/uv-python-dir
UV_PYTHON: 3.12
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
Creating environment: lint
Checking dependencies
Syncing dependencies
##[error]app/api/routes/web/web_auth.py:93:11: PLR0917 Too many positional arguments (6 > 5)
GitHub Actions: FastAPI REST API Tests / Lint (Python + JS + CSS + Markdown + YAML): feat(worker): Phase 2 hybrid routing — TikTok/Instagram/X via browser-downloader
Conclusion: failure
##[group]Run hatch run lint:check
�[36;1mhatch run lint:check�[0m
shell: /usr/bin/bash -e {0}
env:
PYTHON_VERSION: 3.12
HATCH_VERSION: 1.16.5
UV_PYTHON_INSTALL_DIR: /home/runner/work/_temp/uv-python-dir
UV_PYTHON: 3.12
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
Creating environment: lint
Checking dependencies
Syncing dependencies
##[error]app/api/routes/web/web_auth.py:93:11: PLR0917 Too many positional arguments (6 > 5)
🧰 Additional context used
🪛 ast-grep (0.45.0)
packages/browser-downloader/tests/server.test.js
[warning] 175-175: Avoid using the initial state variable in setState
Context: setTimeout(r, 30)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 197-197: Avoid using the initial state variable in setState
Context: setTimeout(r, 50)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 213-213: Avoid using the initial state variable in setState
Context: setTimeout(r, 30)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
core/models/user.py
[warning] 43-43: Do not use text() as it leads to SQL injection
Context: text("1")
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(disable-sqlalchemy-text)
packages/browser-downloader/src/tier2-dom.js
[warning] 94-94: Avoid using the initial state variable in setState
Context: setTimeout(resolve, ms)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 159-159: Avoid using the initial state variable in setState
Context: setTimeout(() => ac.abort(), timeoutMs)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
worker/job_executor.py
[info] 187-187: use secrets package over random package
Context: random.uniform(5.0, 20.0)
Note: [CWE-330] Use of Insufficiently Random Values.
(avoid-random-python)
packages/browser-downloader/tests/tier1-cdp.test.js
[warning] 60-60: Avoid using the initial state variable in setState
Context: setTimeout(r, 0)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
packages/browser-downloader/src/server.js
[warning] 100-100: Avoid using the initial state variable in setState
Context: setTimeout(() => reject(new Error('request_timeout')), requestTimeout)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
packages/browser-downloader/src/tier1-cdp.js
[warning] 194-194: Avoid using the initial state variable in setState
Context: setTimeout(() => onFound(null), timeout)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 368-368: Avoid using the initial state variable in setState
Context: setInterval(checkDrm, 500)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
app/services/yt_dlp_service.py
[warning] 217-217: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(children_path)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
packages/browser-downloader/src/streamlink-backend.js
[warning] 26-26: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
[error] 108-108: React's useState should not be directly called
Context: setTimeout(() => ac.abort(), timeout)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
[error] 400-400: React's useState should not be directly called
Context: setTimeout(() => ac.abort(), dlTimeout)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
[warning] 71-71: Avoid using the initial state variable in setState
Context: setTimeout(resolve, ms)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 108-108: Avoid using the initial state variable in setState
Context: setTimeout(() => ac.abort(), timeout)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 274-274: Avoid using the initial state variable in setState
Context: setTimeout(killNow, 2000)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 286-292: Avoid using the initial state variable in setState
Context: setTimeout(() => {
if (!killed) {
killed = true;
killGroup('SIGTERM');
killTimer = setTimeout(killNow, 2000);
}
}, timeout)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 290-290: Avoid using the initial state variable in setState
Context: setTimeout(killNow, 2000)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 400-400: Avoid using the initial state variable in setState
Context: setTimeout(() => ac.abort(), dlTimeout)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🪛 GitHub Check: CodeQL
packages/browser-downloader/src/validate.js
[failure] 159-159: Uncontrolled data used in path expression
This path depends on a user-provided value.
packages/browser-downloader/src/streamlink-backend.js
[warning] 336-336: Network data written to file
Write to file system depends on Untrusted data.
🪛 Hadolint (2.14.0)
packages/browser-downloader/Dockerfile
[warning] 13-13: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
[warning] 13-13: Pin versions in pip. Instead of pip install <package> use pip install <package>==<version> or pip install --requirement <requirements file>
(DL3013)
🔇 Additional comments (48)
app/auth.py (1)
40-42: LGTM!Also applies to: 124-150
tests/test_story_3_1_web_auth_extraction.py (1)
82-82: LGTM!Also applies to: 117-123
app/api/rate_limit_config.py (1)
5-6: LGTM!Also applies to: 24-36, 53-57, 68-73, 93-95
app/api/routes/web/web_downloads.py (1)
85-92: LGTM!Also applies to: 120-140, 167-167, 203-203
core/queue.py (1)
8-17: LGTM!Also applies to: 30-43
worker/main.py (1)
83-83: LGTM!Also applies to: 96-98, 115-129, 141-144, 167-171, 315-320
packages/browser-downloader/tests/errors.test.js (1)
1-34: LGTM!Also applies to: 44-68, 83-86
tests/test_story_2_4_web_html_consolidation.py (1)
149-149: LGTM!core/models/user.py (1)
40-51: LGTM!app/api/routes/downloads.py (1)
83-85: LGTM!Also applies to: 118-120, 149-151, 210-212, 247-254, 263-265, 310-312
packages/browser-downloader/package.json (1)
4-4:playwright1.49.1 carries a known advisory, and the base image is coupled to it.A previous review flagged CVE-2025-59288 for
playwrightat this version. One additional constraint applies here:packages/browser-downloader/Dockerfileline 4 pinsmcr.microsoft.com/playwright:v1.49.1-focal. The npm version and the base image tag must move together, otherwise the browser binaries shipped in the image will not match the client library.packages/browser-downloader/Dockerfile (1)
34-48: LGTM!packages/browser-downloader/src/concurrency.js (1)
7-43: LGTM!packages/browser-downloader/src/streamlink-backend.js (1)
217-304: LGTM!packages/browser-downloader/tests/concurrency.test.js (1)
5-52: LGTM!packages/browser-downloader/tests/streamlink-backend.test.js (1)
40-87: LGTM!pnpm-workspace.yaml (1)
1-2: LGTM!worker/browser_executor.py (3)
211-223: LGTM!
293-321: LGTM!
49-75: LGTM!Also applies to: 229-255, 258-290
app/services/yt_dlp_service.py (2)
264-267: LGTM!Also applies to: 481-496
90-100: LGTM!Also applies to: 270-313, 415-432
worker/job_executor.py (2)
224-256: LGTM!
309-341: 🩺 Stability & AvailabilityNo change needed for worker timeout cancellation.
The download timeout is already longer than the default browser-downloader timeout; a worker retry does not stack abandoned microservice work by exceeding its timeout, so the saturated 503 failure mode described here does not follow from this code path.
> Likely an incorrect or invalid review comment.tests/test_worker/test_browser_executor.py (1)
297-329: LGTM!packages/browser-downloader/src/server.js (3)
43-63: LGTM!
119-139: LGTM!
144-162: LGTM!packages/browser-downloader/src/tier2-dom.js (1)
39-77: LGTM!core/models/outbox.py (1)
18-53: LGTM!core/metrics.py (1)
35-40: LGTM!Also applies to: 173-173
worker/outbox_relay.py (1)
1-30: LGTM!Also applies to: 64-91, 120-141, 150-165
worker/retry_scheduler.py (1)
8-8: LGTM!Also applies to: 58-58, 142-142, 161-161
tests/test_services/test_download_service.py (1)
79-80: LGTM!Also applies to: 100-102, 144-144
tests/test_worker/test_outbox_recovery.py (1)
95-106: LGTM!Also applies to: 255-264, 301-303, 341-349, 385-394
tests/test_story_3_5_processor_retry_dlq_extraction.py (1)
149-152: LGTM!Also applies to: 297-297, 341-344, 350-350, 389-391
app/services/download_service.py (1)
160-168: LGTM!Also applies to: 184-188, 217-219, 256-266, 278-293, 348-348, 413-425, 435-435, 445-445
tests/test_story_1_2_config_extraction.py (1)
23-23: LGTM!tests/test_story_1_3_database_metrics_extraction.py (1)
24-24: LGTM!tests/test_story_1_4_redis_logging_queue_extraction.py (1)
22-22: LGTM!packages/browser-downloader/src/validate.js (1)
111-142: LGTM!Also applies to: 144-177, 179-222
packages/browser-downloader/src/downloader.js (1)
40-56: LGTM!Also applies to: 64-95
packages/browser-downloader/tests/downloader.test.js (1)
191-213: LGTM!packages/browser-downloader/tests/server.test.js (1)
223-254: LGTM!packages/browser-downloader/tests/tier1-cdp.test.js (1)
63-216: LGTM!Also applies to: 218-298, 300-350
packages/browser-downloader/tests/tier2-dom.test.js (1)
36-129: LGTM!packages/browser-downloader/tests/validate.test.js (1)
16-58: LGTM!Also applies to: 60-116, 139-154, 156-204
packages/browser-downloader/vitest.config.js (1)
1-10: LGTM!
| await enqueue_job(job_id) | ||
| cleanup_started = True | ||
| from sqlalchemy import update as sqlalchemy_update | ||
|
|
||
| await self.db.execute( | ||
| sqlalchemy_delete(Outbox).where( | ||
| sqlalchemy_update(Outbox) | ||
| .where( | ||
| Outbox.job_id == job_id, | ||
| Outbox.status == "pending", | ||
| ) | ||
| .values(status="processed", processed_at=datetime.now(UTC)), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Update only the outbox row that started the fast path.
The update matches every pending row for job_id. After enqueue_job() succeeds, the relay or worker can process the original row and create a later pending retry event. This update can then mark that later event as processed without queueing it.
Select and retain the original Outbox.id before enqueue_job(). Update by that ID and status == "pending". Add a regression test that inserts a second pending outbox row between queue delivery and the update.
🤖 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 `@app/services/download_service.py` around lines 389 - 399, In the fast path
around enqueue_job, capture and retain the original pending Outbox.id before
delivery, then update only that row by ID while preserving the status ==
"pending" guard; do not continue matching by job_id alone. Add a regression test
that inserts a second pending Outbox row between queue delivery and the update
and verifies the later row remains pending.
| try: | ||
| await db.flush() | ||
| except IntegrityError: | ||
| # Concurrent writer beat us to the insert via the partial unique | ||
| # index. Roll back the just-flushed attempt only; the outer | ||
| # transaction (which may include a DownloadJob insert) must still | ||
| # commit. Detach our object so subsequent add/commit is clean. | ||
| db.expunge(outbox_entry) | ||
| return None |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
SQLAlchemy 2.x documentation: After an IntegrityError from Session.flush(), does the Session require rollback before commit? What is the documented Session.begin_nested() pattern for handling duplicate-key insert races?
💡 Result:
In SQLAlchemy 2.x, if a Session.flush raises an IntegrityError (or any other exception), the underlying transaction is rolled back by the database, but the Session itself enters an inactive state [1][2][3]. You must explicitly call Session.rollback to reset the Session before it can be used for further operations [1][2][3]. Attempting to proceed without this explicit rollback will result in an error indicating the transaction has been rolled back due to a previous exception [3][4]. To handle scenarios like duplicate-key insert races without invalidating the entire transaction, the recommended pattern is to use Session.begin_nested [1][5][6]. This creates a SAVEPOINT, allowing you to isolate the operation that might fail [1][5]. If an IntegrityError occurs, you can catch it, roll back only to that savepoint, and continue the transaction [5][6]. The following pattern is documented for handling such races: with session.begin_nested: try: session.add(obj) session.flush except IntegrityError: # Handle the race condition (e.g., skip the insert, # or perform a query to get the existing object) print("Duplicate entry detected, skipping.") # The outer transaction remains active and can be committed or further modified. When Session.begin_nested is called, the Session performs an unconditional flush to ensure the database state is synchronized with the Session's in-memory state before the savepoint is established [5][6].
Citations:
- 1: https://docs.sqlalchemy.org/en/21/faq/sessions.html
- 2: http://docs.sqlalchemy.org/en/latest/orm/session_basics.html
- 3: https://docs.sqlalchemy.org/en/20/errors.html
- 4: https://docs.sqlalchemy.org/en/21/errors.html
- 5: https://docs.sqlalchemy.org/en/21/orm/session_transaction.html
- 6: https://docs.sqlalchemy.org/en/20/orm/session%5Ftransaction.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)outbox_service\.py$|DownloadService|requirements|pyproject|poetry.lock|uv.lock|Pipfile.lock' || true
echo
echo "== outbox_service outline/contents =="
fd -a 'outbox_service.py$' . | while read -r f; do
echo "--- $f"
wc -l "$f"
cat -n "$f"
done
echo
echo "== usages of add_outbox_entry_outbox_entry or AsyncSession rollback/commit =="
rg -n "add_outbox_entry_outbox_entry|--outbox|download|commit\(|rollback\(|begin_nested|IntegrityError" app . -g '!*.pyc' || trueRepository: tomkabel/vooglaadija
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status/stat =="
git status --short
git diff --stat -- app || true
echo
echo "== candidate outbox/download files =="
git ls-files 'app/**' | rg 'outbox|download|service' | sort | sed -n '1,120p'
echo
echo "== focused usages =="
git ls-files 'app/**' | xargs rg -n "add_outbox_entry_outbox_entry|--outbox|create\(|commit\(|rollback\(|begin_nested|IntegrityError|DownloadService" -g '*.py' || trueRepository: tomkabel/vooglaadija
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository entries excluding lock/html =="
git ls-files | awk '!/node_modules|dist|build|.*\.html$|.*\.min\.js$|.*\.json$/ {print}' | sed -n '1,160p'
echo
echo "== app python files =="
git ls-files 'app/**/*.py' | sed -n '1,160p'
echo
echo "== focused service searches =="
((rg -n "add_outbox_entry_outbox_entry|--outbox|Outbox|DownloadService|commit\(|rollback\(|begin_nested|IntegrityError" app --glob '*.py' || true) | sed -n '1,200p')Repository: tomkabel/vooglaadija
Length of output: 10981
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== outbox_service.py =="
cat -n app/services/outbox_service.py
echo
echo "== core/models/outbox.py =="
cat -n core/models/outbox.py
echo
echo "== relevant download_service.py sections =="
sed -n '120,160p' app/services/download_service.py | cat -n
sed -n '180,215p' app/services/download_service.py | cat -n
sed -n '380,408p' app/services/download_service.py | cat -n
echo
echo "== db/session config =="
cat -n core/database.pyRepository: tomkabel/vooglaadija
Length of output: 14169
Recover duplicate outbox inserts inside a savepoint.
await db.flush() leaves the outer AsyncSession inactive after an IntegrityError; db.expunge(outbox_entry) does not make the transaction committable. DownloadService.create() and DownloadService.retry() then fail at commit() instead of handling the concurrent insert idempotently. Wrap the insert and flush in db.begin_nested(), return None only after confirming the pending outbox row exists, and re-raise other integrity failures.
🤖 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 `@app/services/outbox_service.py` around lines 51 - 59, Update the outbox
insert flow around the db.flush() IntegrityError handler to execute the
add/flush inside db.begin_nested(), so duplicate conflicts roll back only the
savepoint and leave the outer transaction active. On conflict, confirm the
existing pending outbox row before returning None, and re-raise IntegrityError
instances that are not confirmed duplicates; do not rely on
db.expunge(outbox_entry) for recovery.
| Index( | ||
| "uq_outbox_pending_job_id", | ||
| "job_id", | ||
| unique=True, | ||
| postgresql_where=(status == _PENDING_STATUS), | ||
| sqlite_where=(status == _PENDING_STATUS), | ||
| ), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -HI -t f '.*(migration|alembic).*' .
rg -n -i -C4 'uq_outbox_pending_job_id|CREATE.*INDEX|outbox.*pending' \
-g '*.py' -g '*.sql' .Repository: tomkabel/vooglaadija
Length of output: 49317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## alembic versions"
fd -HI -t f '.*\.py$' alembic/versions | sort
echo "## candidate migration file"
cat -n alembic/versions/009_add_outbox_pending_unique_index.py
echo "## model context"
sed -n '1,90p' core/models/outbox.py | cat -n
echo "## migration chain references"
rg -n -C3 '00[0-9]|revision|down_revision|alembic|ensure_migration_chain' alembic/versions scripts/ensure_migration_chain.py tests/test_ensure_migration_chain.pyRepository: tomkabel/vooglaadija
Length of output: 703
Add the uq_outbox_pending_job_id migration.
The model declares the partial unique index, but alembic/versions does not contain a migration for uq_outbox_pending_job_id. Existing deployed databases will miss this constraint unless a PostgreSQL and SQLite migration is added for the WHERE status = 'pending' DDL.
🤖 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 `@core/models/outbox.py` around lines 61 - 67, Add an Alembic migration for the
model’s uq_outbox_pending_job_id index, creating a unique job_id index filtered
by status = 'pending' for both PostgreSQL and SQLite, and include the
corresponding downgrade removal. Ensure the migration is linked to the current
revision chain.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline app/services/outbox_service.py --items all
rg -n -C5 'async def write_job_to_outbox|begin_nested|await db\.flush|except IntegrityError' \
app/services/outbox_service.py
rg -n -C4 'write_job_to_outbox|concurr|IntegrityError' testsRepository: tomkabel/vooglaadija
Length of output: 18539
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- app/services/outbox_service.py ---\n'
cat -n app/services/outbox_service.py
printf '\n--- core/models/outbox.py relevant lines ---\n'
sed -n '1,120p' core/models/outbox.py | cat -n
printf '\n--- download_service references ---\n'
rg -n -C6 'write_job_to_outbox|class DownloadService|async def create|DownloadJob' app/services/download_service.py app/services || true
printf '\n--- tests mentioning outbox_service/create + integrity race ---\n'
rg -n -C5 'test_write_job_to_outbox_idempotent_skips_duplicate|IntegrityError|begin_nested|write_job_to_outbox' tests/test_services/test_outbox_service.py tests/app app || true
printf '\n--- deterministic SQLAlchemy session state probe (if installed/readable) ---\n'
python3 - <<'PY'
try:
import sqlalchemy
print("SQLAlchemy available:", sqlalchemy.__version__)
except Exception as e:
print("SQLAlchemy unavailable:", repr(e))
PYRepository: tomkabel/vooglaadija
Length of output: 249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- app/services/outbox_service.py ---'
cat -n app/services/outbox_service.py
printf '%s\n' ''
printf '%s\n' '--- core/models/outbox.py relevant lines ---'
sed -n '1,140p' core/models/outbox.py | cat -n
printf '%s\n' ''
printf '%s\n' '--- download_service references ---'
rg -n -C6 'write_job_to_outbox|class DownloadService|async def create|DownloadJob' app/services/download_service.py app/services || true
printf '%s\n' ''
printf '%s股份有限公司' '--- tests mentioning outbox_service/create + integrity race ---'
rg -n -C5 'test_write_job_to_outbox_idempotent_skips_duplicate|IntegrityError|begin_nested|write_job_to_outbox' tests/test_services/test_outbox_service.py tests/app app || true
printf '%s\n' ''
printf '%s\n' '--- deterministic SQLAlchemy session state probe (if installed/readable) ---'
python3 - <<'PY'
try:
import sqlalchemy
print("SQLAlchemy version:", sqlalchemy.__version__)
except Exception as e:
print("SQLAlchemy unavailable:", repr(e))
PYRepository: tomkabel/vooglaadija
Length of output: 50376
🌐 Web query:
SQLAlchemy asyncio AsyncSession begin_nested rollback_savepoint flush after session failed state after IntegrityError
💡 Result:
In SQLAlchemy, when using AsyncSession.begin_nested(), the session creates a SAVEPOINT in the database, allowing you to perform operations that may fail—such as those triggering an IntegrityError—without rolling back the entire outer transaction [1][2][3]. When an IntegrityError occurs within the async with session.begin_nested(): context manager, the following behavior is standard: 1. Automatic Savepoint Rollback: The context manager automatically issues a ROLLBACK TO SAVEPOINT command to the database when an exception is raised within its block [1][2][4]. This ensures that only the operations within the nested scope are reverted [5][3]. 2. Session State: While the database SAVEPOINT is rolled back, the AsyncSession object itself remains in a usable state [3]. Unlike a standard flush failure on a top-level transaction, which invalidates the entire session and requires an explicit session.rollback() on the session itself, begin_nested() handles the internal session state specifically for the nested block [1][3][6]. 3. Correct Usage Pattern: You do not need to call await session.rollback() on the main AsyncSession if you are using begin_nested() to isolate the error [6]. In fact, calling await session.rollback() on the outer session will roll back the entire transaction, which is usually not the intended behavior when using nested transactions [3][6]. Example of the correct pattern: async def safe_operation(session: AsyncSession): try: async with session.begin_nested: session.add(some_object) await session.flush # IntegrityError might be raised here except IntegrityError: # The savepoint is already rolled back by the context manager. # The outer transaction is still active and safe to continue. pass Key Considerations: - Flush Behavior: When session.begin_nested() is called, SQLAlchemy unconditionally flushes all pending changes to the database [1][7]. - Invalidation: If you experience an InvalidRequestError stating "This Session's transaction has been rolled back due to a previous exception during flush," it typically indicates that an exception occurred during an operation outside of a begin_nested() block or that you manually rolled back the outer transaction [7][8]. Always ensure that potentially failing operations are wrapped in begin_nested() if you intend to recover from them without invalidating the session [3][6].
Citations:
- 1: https://docs.sqlalchemy.org/en/20/orm/session%5Ftransaction.html
- 2: http://docs.sqlalchemy.org/en/latest/orm/session_transaction.html
- 3: https://async-workflows.com/mastering-sqlalchemy-20-core-and-orm-architecture/session-lifecycle-and-scope-management/
- 4: https://docs.sqlalchemy.org/en/20/orm/session_transaction.html
- 5: https://stackoverflow.com/questions/72649194/how-to-rollback-only-the-last-session-add-action-in-sqlalchemy
- 6: https://stackoverflow.com/questions/77390826/sqlalchemy-error-invalidrequesterror-cant-operate-on-closed-transaction-insi
- 7: http://docs.sqlalchemy.org/en/latest/orm/session_basics.html
- 8: https://stackoverflow.com/questions/24522290/cannot-catch-sqlalchemy-integrityerror
Wrap the outbox insert in a savepoint.
write_job_to_outbox catches concurrent flush() failures and calls db.expunge(), but the exception still leaves the top-level transaction unreadable for the enclosing DownloadService.create path. Use await db.begin_nested() around the insert/flush and add a two-session concurrency test for the partial unique index.
🤖 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 `@core/models/outbox.py` around lines 61 - 67, Update write_job_to_outbox to
execute the outbox insert and flush inside an await db.begin_nested() savepoint,
preserving its existing concurrent-conflict handling so failures do not
invalidate the enclosing transaction. Add a concurrency test using two sessions
that exercises the partial unique index and verifies only one pending job is
retained while both sessions remain usable.
| RUN apt-get update \ | ||
| && apt-get install -y --no-install-recommends \ | ||
| ffmpeg \ | ||
| curl \ | ||
| ca-certificates \ | ||
| python3 \ | ||
| python3-pip \ | ||
| && rm -rf /var/lib/apt/lists/* \ | ||
| && pip3 install --no-cache-dir streamlink \ | ||
| && npm install -g [email protected] | ||
|
|
||
| WORKDIR /app | ||
|
|
||
| # Install production deps only. Skip scripts so the `playwright` package does | ||
| # not re-download browsers that the base image already ships. | ||
| COPY package.json ./ | ||
| RUN pnpm install --prod --ignore-scripts \ | ||
| && rm -rf /root/.local/share/pnpm/store /tmp/pnpm/store |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate lockfiles and any compose/CI definition that builds this image.
fd -H -t f 'pnpm-lock.yaml|package-lock.json|yarn.lock'
rg -n -C4 'browser-downloader' --iglob '*compose*' --iglob '*.yml' --iglob '*.yaml' --iglob 'Makefile' | head -60Repository: tomkabel/vooglaadija
Length of output: 224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo files of interest:\n'
for f in pnpm-workspace.yaml pnpm-lock.yaml packages/browser-downloader/package.json packages/browser-downloader/Dockerfile README.md; do
if [ -f "$f" ]; then
printf '\n=== %s ===\n' "$f"
wc -l "$f"
sed -n '1,220p' "$f"
else
printf '%s missing\n' "$f"
fi
done
printf '\nBuild references for browser-downloader:\n'
rg -n -C4 'browser-downloader|packages/browser-downloader|context:|docker build|DOCKERFILE|BUILDKIT|github\.com/actions/docker-build|buildkit' --iglob '*.yml' --iglob '*.yaml' --iglob 'Makefile' --iglob '*Dockerfile*' --iglob '*.md' | head -200Repository: tomkabel/vooglaadija
Length of output: 18768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read-only parse the Dockerfile instructions relevant to the comment:
# base image context, copied package JSON, lockfile presence in context, frozen-lock flag.
python3 - <<'PY'
from pathlib import Path
dockerfile = Path('packages/browser-downloader/Dockerfile').read_text()
lines = dockerfile.splitlines()
found = {
'apt_unpinned': False,
'streamlink_unpinned': False,
'pnpm_10_33_2': False,
'copy_package_json_only': False,
'copy_copy_files': None,
'pnpm_install': False,
'has_frozen_lockfile': False,
'packages_dir_exists': False,
'root_lockfile_exists': False,
}
for line in lines:
stripped = line.strip()
if stripped.startswith('#'):
continue
if stripped.lower() == 'from ' and 'mcr.microsoft.com/playwright:v1.49.1-focal' in stripped:
found['apt_unpinned'] = True
found['streamlink_unpinned'] = False
continue
if 'apt-get install' in stripped:
found['apt_unpinned'] = True
if 'pip3 install --no-cache-dir streamlink' in stripped:
found['streamlink_unpinned'] = True
if 'npm install -g [email protected]' in stripped:
found['pnpm_10_33_2'] = True
if stripped.lower().startswith('copy ') and 'package.json' in stripped:
found['copy_copy_files'] = stripped.split(None, 1)[1]
found['copy_package_json_only'] = stripped.lower() == 'copy package.json ./'.replace('\t', ' ') or stripped.endswith('package.json ./')
if stripped.lower().startswith('run pnpm install'):
found['pnpm_install'] = True
found['has_frozen_lockfile'] = '--frozen-lockfile' in stripped or '--frozen-lockfile' in stripped.lower()
found['packages_dir_exists'] = Path('packages/browser-downloader').is_dir()
found['root_lockfile_exists'] = Path('pnpm-lock.yaml').is_file()
for k, v in found.items():
print(f'{k}={v}')
if found['copy_copy_files']:
print(f'copy_line={found["copy_copy_files"]}')
PYRepository: tomkabel/vooglaadija
Length of output: 400
Pin the build inputs for reproducible images.
pip3 install streamlink resolves to the available streamlink version at build time. pnpm install --prod has no lockfile in the Docker build step because only package.json is copied. Pin streamlink to a fixed version, and add the root pnpm-lock.yaml into the Docker build so pnpm install --prod can run with --frozen-lockfile.
🧰 Tools
🪛 Hadolint (2.14.0)
[warning] 13-13: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
[warning] 13-13: Pin versions in pip. Instead of pip install <package> use pip install <package>==<version> or pip install --requirement <requirements file>
(DL3013)
🤖 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 `@packages/browser-downloader/Dockerfile` around lines 13 - 30, Update the
Dockerfile dependency installation to pin streamlink to a fixed version, copy
the root pnpm-lock.yaml alongside package.json before installation, and run pnpm
install --prod with --frozen-lockfile while preserving --ignore-scripts and
cleanup behavior.
Source: Linters/SAST tools
| async def test_open_circuit_raises_transient_without_http_call(self) -> None: | ||
| # When the breaker is OPEN, no HTTP call should be made. We use a | ||
| # transport that would raise if invoked, proving the call was skipped. | ||
| # Phase 2 fix: CircuitBreakerOpenError propagates raw so the processor's | ||
| # deferred-job path handles it (worker/processor.py:_handle_circuit_open). | ||
| from app.services.circuit_breaker import CircuitBreakerOpenError | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Rename the test to match its new expectation.
The test now asserts CircuitBreakerOpenError, not a transient error, so test_open_circuit_raises_transient_without_http_call is misleading. Rename it to test_open_circuit_propagates_circuit_breaker_open_without_http_call.
Also move the CircuitBreakerOpenError import to the module top level, next to the other imports.
🤖 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/test_worker/test_browser_executor.py` around lines 290 - 296, Rename
test_open_circuit_raises_transient_without_http_call to
test_open_circuit_propagates_circuit_breaker_open_without_http_call to reflect
the asserted exception, and move the CircuitBreakerOpenError import from inside
the test to the module-level imports.
| def _parse_failure_payload( | ||
| payload: dict[str, Any], | ||
| *, | ||
| fallback_code: str = "unknown_error", | ||
| ) -> tuple[str, str, str | None]: | ||
| """Map a structured failure payload to an error category. | ||
|
|
||
| Accepts both 200-with-failed-status and non-200 responses. | ||
| Raises BrowserExecutorError so the circuit breaker records a failure. | ||
|
|
||
| When the JSON body lacks an explicit ``error`` field, ``fallback_code`` | ||
| (typically the synthesized ``http_<status>`` signal) is used so HTTP | ||
| 404/403/429 still map to their correct categories. | ||
| """ | ||
| code: str = payload.get("error") # type: ignore[assignment] | ||
| if not isinstance(code, str) or not code: | ||
| code = fallback_code | ||
| category = _map_response_to_category(code, payload) | ||
| raise BrowserExecutorError(category=category, signal=code) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Drop the type: ignore and use NoReturn.
Line 337 annotates code: str while payload.get("error") can return None or a non-string, which forces the type: ignore. The function also never returns; it always raises. Both can be expressed accurately.
♻️ Proposed change
def _parse_failure_payload(
payload: dict[str, Any],
*,
fallback_code: str = "unknown_error",
-) -> tuple[str, str, str | None]:
+) -> NoReturn:
@@
- code: str = payload.get("error") # type: ignore[assignment]
- if not isinstance(code, str) or not code:
- code = fallback_code
+ raw_code = payload.get("error")
+ code = raw_code if isinstance(raw_code, str) and raw_code else fallback_code
category = _map_response_to_category(code, payload)
raise BrowserExecutorError(category=category, signal=code)Import NoReturn from typing. Note that _parse_success at Line 280 uses return _parse_failure_payload(payload); with NoReturn that line still type-checks, and you can simplify it to a bare call.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _parse_failure_payload( | |
| payload: dict[str, Any], | |
| *, | |
| fallback_code: str = "unknown_error", | |
| ) -> tuple[str, str, str | None]: | |
| """Map a structured failure payload to an error category. | |
| Accepts both 200-with-failed-status and non-200 responses. | |
| Raises BrowserExecutorError so the circuit breaker records a failure. | |
| When the JSON body lacks an explicit ``error`` field, ``fallback_code`` | |
| (typically the synthesized ``http_<status>`` signal) is used so HTTP | |
| 404/403/429 still map to their correct categories. | |
| """ | |
| code: str = payload.get("error") # type: ignore[assignment] | |
| if not isinstance(code, str) or not code: | |
| code = fallback_code | |
| category = _map_response_to_category(code, payload) | |
| raise BrowserExecutorError(category=category, signal=code) | |
| def _parse_failure_payload( | |
| payload: dict[str, Any], | |
| *, | |
| fallback_code: str = "unknown_error", | |
| ) -> NoReturn: | |
| """Map a structured failure payload to an error category. | |
| Accepts both 200-with-failed-status and non-200 responses. | |
| Raises BrowserExecutorError so the circuit breaker records a failure. | |
| When the JSON body lacks an explicit ``error`` field, ``fallback_code`` | |
| (typically the synthesized ``http_<status>`` signal) is used so HTTP | |
| 404/403/429 still map to their correct categories. | |
| """ | |
| raw_code = payload.get("error") | |
| code = raw_code if isinstance(raw_code, str) and raw_code else fallback_code | |
| category = _map_response_to_category(code, payload) | |
| raise BrowserExecutorError(category=category, signal=code) |
🤖 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 `@worker/browser_executor.py` around lines 323 - 341, Update
_parse_failure_payload to return NoReturn and import NoReturn from typing,
reflecting that it always raises BrowserExecutorError. Remove the type: ignore
by retrieving payload.get("error") without an incorrect str annotation, then
validate it with the existing isinstance/non-empty check before applying
fallback_code. Simplify the _parse_success failure path from returning
_parse_failure_payload(payload) to calling it directly.
| outbox_sync_interval_seconds = int(os.environ.get("OUTBOX_SYNC_INTERVAL_SECONDS", "2")) | ||
| except (ValueError, TypeError): | ||
| outbox_sync_interval_seconds = 30 | ||
| outbox_sync_interval_seconds = 2 |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
Validate the 2-second outbox polling interval under production load.
The new default can call sync_outbox_to_queue() up to 15 times more often per worker. Verify that the pending-outbox query and database capacity support the configured worker count.
🤖 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 `@worker/main.py` around lines 271 - 273, Validate the new
OUTBOX_SYNC_INTERVAL_SECONDS default and configured interval against production
worker counts in the outbox polling flow around sync_outbox_to_queue(). Confirm
the pending-outbox query and database capacity support the increased polling
frequency, and adjust the interval or query/database configuration as needed
while preserving the existing fallback behavior for invalid values.
| oldest = await db.scalar( | ||
| select(func.extract("epoch", func.now() - func.min(Outbox.created_at))).where( | ||
| Outbox.status == _PENDING_STATUS, | ||
| ), | ||
| ) | ||
| OUTBOX_OLDEST_PENDING_SECONDS.set(float(oldest or 0)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'sqlite|sqlite_where|TestingSessionLocal|DATABASE_URL' \
core/models/outbox.py core/database.py tests/conftest.py pyproject.toml
rg -n -C4 'extract\("epoch"|OUTBOX_OLDEST_PENDING_SECONDS|func\.min\(Outbox\.created_at\)' \
worker/outbox_relay.pyRepository: tomkabel/vooglaadija
Length of output: 5813
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Outbox model relevant fields:"
sed -n '1,120p' core/models/outbox.py
echo
echo "Relay staleness metrics section:"
sed -n '1,100p' worker/outbox_relay.py
echo
echo "SQLAlchemy expressions and dependencies:"
rg -n 'from sqlalchemy import|import sqlalchemy|sqlalchemy' pyproject.toml worker core tests | head -80
echo
echo "Inspect raw SQL if available:"
rg -n 'def render_bindparam|sqlite|dialect' core | head -80Repository: tomkabel/vooglaadija
Length of output: 13613
🌐 Web query:
SQLAlchemy func now sqlite expression func.extract epoch datetime columns documentation SQLite timestamp arithmetic
💡 Result:
To perform date and time operations with SQLite in SQLAlchemy, it is important to understand that SQLite lacks native DATE, TIME, and DATETIME types, storing them instead as strings [1][2][3]. SQLAlchemy handles this by formatting and parsing these values [2][3]. Using func.now and Time To use the current timestamp, use func.current_timestamp [4][5]. While SQLAlchemy provides a generic func.now that often compiles to CURRENT_TIMESTAMP, explicitly using func.current_timestamp is recommended for clarity [4][6][5]. Note that standard SQLite CURRENT_TIMESTAMP does not include fractional seconds [7]. To achieve higher precision, you may need to use custom logic or SQLite's unixepoch function (available in newer SQLite versions) with appropriate modifiers [8]. Extracting Components (e.g., Epoch) To extract parts of a datetime (such as the epoch/seconds since 1970) in SQLAlchemy with SQLite, use the extract construct [9]. The SQLite dialect implements this by casting the result of the strftime function [1]: from sqlalchemy import extract # Example: extracting year stmt = select(extract("year", MyTable.datetime_column)) The SQLite dialect supports mapping several fields to strftime formats, including "epoch" (which maps to %s) [1]. Timestamp Arithmetic Because SQLite stores dates as strings, standard arithmetic (like adding a timedelta) requires using SQLite's built-in date/time functions [8][10]. You can use func.datetime to apply modifiers, such as adding or subtracting seconds [10]: from sqlalchemy import func, cast, String # Example: adding seconds to a datetime column stmt = select(MyTable).where( func.datetime( MyTable.datetime_column, '+' + cast(MyTable.some_integer_column, String) + ' seconds') < func.now) For complex operations or when using newer SQLite versions (3.38.0+), the unixepoch function can be used for more straightforward arithmetic by converting timestamps to integers [8][10]. Always consult the official SQLite date and time function documentation when dealing with specific formatting or modifier requirements [8][10].
Citations:
- 1: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/dialects/sqlite/base.py
- 2: http://docs.sqlalchemy.org/en/latest/dialects/sqlite.html
- 3: https://docs.sqlalchemy.org/en/20/dialects/sqlite.html
- 4: Missing func.now() in SQLite sqlalchemy/sqlalchemy#708
- 5: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/sql/functions.py
- 6: http://docs.sqlalchemy.org/en/latest/core/functions.html
- 7: https://stackoverflow.com/questions/76473668/how-do-i-get-fractional-seconds-in-func-now-with-sqlalchemy-orm-and-sqlite
- 8: https://sqlite.org/lang_datefunc.html
- 9: http://docs.sqlalchemy.org/en/latest/core/sqlelement.html
- 10: https://stackoverflow.com/questions/71332395/python-sqlalchemy-sqlite-how-to-add-datetime-field-and-integer-seconds-timede
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("worker/outbox_relay.py")
tree = ast.parse(path.read_text(), filename=str(path))
print(path, "exists")
for node in ast.walk(tree):
if isinstance(node, ast.AsyncFunctionDef) and node.name == "_update_staleness_metrics":
print("found _update_staleness_metrics")
print("line", node.lineno)
for child in node.body:
print(type(child).__name__, getattr(child, "lineno", None))
if isinstance(child, ast.Try):
try_body = ast.get_source_segment(path.read_text(), child.body[0])
print("try first stmt:")
print(try_body)
PYRepository: tomkabel/vooglaadija
Length of output: 375
🌐 Web query:
SQLAlchemy func.extract epoch sqlite dialect strftime compile current_timestamp datetime column sub function __new__ source
💡 Result:
In SQLAlchemy, the func.extract(field, column) construct provides a dialect-agnostic way to retrieve components of date/time objects [1]. When using the SQLite dialect, SQLAlchemy compiles this extract call into a SQLite strftime function [2][3]. Specifically, the SQLite compiler (SQLiteCompiler in sqlalchemy.dialects.sqlite.base.py) maintains an extract_map that translates standard fields to SQLite strftime format strings [3]. For example, "year" becomes "%Y", "month" becomes "%m", and "epoch" maps to "%s" [3]. The compilation logic for extract is implemented in the visit_extract method, which wraps the strftime call in a CAST(... AS INTEGER) to ensure the result is returned as an integer, as shown here [3]: return "CAST(STRFTIME('%s', %s) AS INTEGER)" % (self.extract_map[extract.field], self.process(extract.expr, kw)) Regarding the internal source implementation: 1. Function Accessor: func is a FunctionGenerator instance [4]. When you access func.extract, it does not create a standard SQL function call [4]. Instead, it redirects to the extract() expression construct, which is defined in sqlalchemy.sql.expression and returns an Extract object [1][4][5]. 2. FunctionGenerator.getattr: The getattr method in FunctionGenerator handles dynamic attribute access, but extract is explicitly defined as a property on the FunctionGenerator class to facilitate this redirection [4][6]. 3. Current Timestamp: func.current_timestamp() is handled as a generic function or specific dialect-level translation [4][3]. In the SQLite dialect, the visit_now_func method compiles current_timestamp directly to the CURRENT_TIMESTAMP SQL keyword [3]. Note that SQLite does not have native DATETIME types; it typically stores them as strings [7][8][9]. While func.current_timestamp() is recognized by SQLAlchemy, it may require explicit type handling if you need it to return Python datetime objects rather than strings, as SQLAlchemy’s SQLite dialect does not automatically perform this conversion for all freestanding date functions [8][9].
Citations:
- 1: http://docs.sqlalchemy.org/en/latest/core/sqlelement.html
- 2: http://ominian.com/2022/05/08/filtering-and-ordering-by-date-with-sqlalchemy/
- 3: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/dialects/sqlite/base.py
- 4: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/sql/functions.py
- 5: https://github.com/zzzeek/sqlalchemy/blob/master/lib/sqlalchemy/sql/expression.py
- 6: https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/sql/functions.py
- 7: http://docs.sqlalchemy.org/en/latest/dialects/sqlite.html
- 8: https://docs.sqlalchemy.org/en/21/dialects/sqlite.html
- 9: https://docs.sqlalchemy.org/en/20/dialects/sqlite.html
🌐 Web query:
SQLAlchemy SQLite extract_map epoch strftime CURRENT_TIMESTAMP datetime column SQLiteCompiler add_type_expression_engine type_annotation
💡 Result:
In SQLAlchemy, the SQLite dialect handles datetime expressions and extraction via the SQLiteCompiler, which relies on SQLite's built-in strftime function [1][2]. EXTRACT AND STRFTIME The SQLiteCompiler maintains an extract_map that defines how SQL extract expressions are translated into SQLite's strftime syntax [1][2]. For example, "month" maps to "%m" and "year" to "%Y" [1]. When using func.extract(field, column), SQLAlchemy renders this as CAST(STRFTIME(format, column) AS INTEGER), where the format is retrieved from the extract_map [1][2]. The "epoch" field is specifically mapped to "%s" [1][2]. CURRENT_TIMESTAMP AND DATETIME COLUMNS SQLite does not have native DATETIME types [3][4][5]. SQLAlchemy's DATETIME, DATE, and TIME types represent values as ISO-formatted strings [3][5]. The SQLiteCompiler renders func.now as CURRENT_TIMESTAMP [1][2]. Because pysqlite (the standard SQLite driver) does not provide native type information in cursor.description, SQLAlchemy cannot automatically detect and cast columns to Python datetime objects [3][4]. Users requiring native datetime objects from SQLite are generally advised to rely on SQLAlchemy's built-in result processing rather than relying on driver-level type detection [3][4]. TYPE ANNOTATION AND CUSTOM COMPILATION Modern SQLAlchemy (2.0+) uses type annotations with Mapped and mapped_column to associate Python types with SQL types [6][7]. The registry's type_annotation_map allows you to map specific Python types to custom SQL type implementations [7][8]. If you need to force specific DDL rendering for types across dialects (e.g., using a custom DATETIME format), you can use the @compiles decorator to intercept type compilation [9]. SUMMARY OF MECHANISMS - extract_map: Defines the mapping between SQL extract fields and strftime format strings for SQLite [1]. - SQLiteCompiler: The component responsible for translating SQLAlchemy's generic SQL constructs into SQLite-specific syntax (e.g., visit_extract, visit_now_func) [1][2]. - Type Annotation: Used via DeclarativeBase to configure type-to-SQL mapping, which simplifies defining columns that use custom types or specific configurations [7][8]. - Custom Compilation: Use the @compiles decorator for advanced customization of how SQL constructs or types are rendered for the SQLite dialect [10][9].
Citations:
- 1: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/dialects/sqlite/base.py
- 2: https://github.com/sqlalchemy/sqlalchemy/blob/rel%5F1%5F4%5F41/lib/sqlalchemy/dialects/sqlite/base.py
- 3: http://docs.sqlalchemy.org/en/latest/dialects/sqlite.html
- 4: https://docs.sqlalchemy.org/en/21/dialects/sqlite.html
- 5: https://docs.sqlalchemy.org/en/20/dialects/sqlite.html
- 6: https://docs.sqlalchemy.org/orm/quickstart.html
- 7: https://github.com/sqlalchemy/sqlalchemy/blob/d3a85fbd/doc/build/changelog/whatsnew_20.rst
- 8: http://docs.sqlalchemy.org/en/latest/core/custom_types.html
- 9: https://docs.sqlalchemy.org/en/20/core/custom%5Ftypes.html
- 10: https://docs.sqlalchemy.org/en/20/core/compiler.html
🌐 Web query:
SQLite strftime %s CURRENT_TIMESTAMP datetime column subtraction SQLite
💡 Result:
In SQLite, you can calculate the difference between the current timestamp and a datetime column in seconds by converting both values into Unix timestamps using the %s format specifier with the strftime function [1][2][3]. To perform this subtraction, use the following syntax [2][3]: SELECT strftime('%s', 'now') - strftime('%s', your_column_name) FROM your_table_name; When using this method, ensure your datetime column contains values in a format natively supported by SQLite (e.g., 'YYYY-MM-DD HH:MM:SS') [1][3]. If the column uses a different format, the function may return NULL [3]. Alternative Methods: 1. Using Julian Day: This approach is often considered more precise or flexible for differences involving days or larger units, as it returns a floating-point value [4]. You can calculate the difference in days and multiply by 86400 (the number of seconds in a day) to get the result in seconds [5][4]: SELECT (julianday('now') - julianday(your_column_name)) * 86400 FROM your_table_name; 2. Timezone Considerations: If you are calculating the difference between 'now' and a stored column that does not include timezone information, the result may be affected by the system's local time setting [6]. To ensure consistent calculation, you can force the use of local time by adding the 'localtime' modifier [6]: SELECT strftime('%s', 'now', 'localtime') - strftime('%s', your_column_name) FROM your_table_name; It is generally recommended to store dates and times as ISO-8601 strings, Julian day numbers, or Unix integers to facilitate these calculations [7][8]. If you frequently need to perform arithmetic, storing timestamps as Unix integers (the number of seconds since 1970-01-01) is the most performant approach, as it allows for direct integer subtraction without calling conversion functions [7].
Citations:
- 1: https://stackoverflow.com/questions/64867957/how-do-i-get-a-the-time-difference-in-seconds-in-sqlite
- 2: https://stackoverflow.com/questions/289680/difference-between-2-dates-in-sqlite
- 3: https://stackoverflow.com/questions/71685872/calculating-difference-between-datetime-stamp-in-sqlite
- 4: https://learnsql.com/cookbook/how-to-calculate-the-difference-between-two-timestamps-in-sqlite/
- 5: https://stackoverflow.com/questions/17708167/difference-in-seconds-between-timestamps-in-sqlite3
- 6: https://stackoverflow.com/questions/16277366/sqlite-subtracting-a-date-entity-from-current-time
- 7: https://runebook.dev/en/docs/sqlite/lang_datefunc
- 8: https://sqlite.org/draft/lang_datefunc.html
🌐 Web query:
sqlalchemy.dialects.sqlite.base SQLiteCompiler visit_extract extraction_map source __init__
💡 Result:
In SQLAlchemy, the SQLiteCompiler class, located in lib/sqlalchemy/dialects/sqlite/base.py, handles the translation of SQLAlchemy extract expressions into SQLite-specific SQL [1][2]. The implementation relies on an extract_map dictionary, which is initialized by extending the base SQLCompiler.extract_map with SQLite-specific format strings [1][2]. The SQLiteCompiler.extract_map is defined as follows [1]: extract_map = util.update_copy( compiler.SQLCompiler.extract_map, { "month": "%m", "day": "%d", "year": "%Y", "second": "%S", "hour": "%H", "doy": "%j", "minute": "%M", "epoch": "%s", "dow": "%w", "week": "%W", },) The visit_extract method is responsible for generating the SQL string. It uses the provided extract_map to look up the correct format string for the field requested in the extract operation [1][2]. The implementation looks like this [1]: def visit_extract(self, extract, kw): try: return "CAST(STRFTIME('%s', %s) AS INTEGER)" % ( self.extract_map[extract.field], self.process(extract.expr, kw),) except KeyError as err: raise exc.CompileError( "%s is not a valid extract argument." % extract.field) from err This implementation renders the SQL as a CAST to INTEGER of a STRFTIME function call, which is the standard way to perform date/time extraction in SQLite dialects within SQLAlchemy [1][3]. If a field is requested that is not present in the extract_map, the method raises a CompileError [1][2].
Citations:
- 1: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/dialects/sqlite/base.py
- 2: https://github.com/sqlalchemy/sqlalchemy/blob/rel%5F1%5F4%5F41/lib/sqlalchemy/dialects/sqlite/base.py
- 3: https://fossies.org/linux/sqlalchemy/test/dialect/sqlite/test_compiler.py
🌐 Web query:
SQLite documentation SUBTRACT current_timestamp strftime datetime subtraction CURRENT_TIMESTAMP datetime literal syntax
💡 Result:
In SQLite, date and time arithmetic is performed using specific built-in functions rather than standard arithmetic operators [1][2]. The approach depends on whether you are adjusting a timestamp by a duration or calculating the difference between two timestamps [1][2]. To add or subtract a fixed duration from a timestamp, use the datetime (or date/time) function with modifiers [3][4][1]. SQLite does not support direct subtraction syntax like 'now' - '1 day' [1]. Instead, use the negative sign within the modifier string [4][1]. Example: Subtracting 1 day and 2 hours from the current time [3][1]: SELECT datetime('now', '-1 day', '-2 hours'); To calculate the difference between two timestamps, the most common and reliable method is to convert both values to Julian day numbers using the julianday function and then subtract them [5][6][2]. This returns the difference in days [5][6]. To get the difference in other units, you can multiply the result [5]. Example: Calculating the difference in days between two columns [5][6]: SELECT julianday(end_column) - julianday(start_column) AS difference_in_days FROM your_table; Example: Calculating the difference in hours [5]: SELECT (julianday(end_column) - julianday(start_column)) * 24 AS difference_in_hours FROM your_table; Alternatively, for differences in seconds, you can use strftime('%s',...) to convert timestamps to Unix epoch seconds and subtract them [7][8]. Note: All date/time functions expect input strings to be in the format 'YYYY-MM-DD HH:MM:SS' [3][7]. Other formats may return NULL [7]. Using CURRENT_TIMESTAMP is equivalent to using the literal string 'now' in these functions [3][8].
Citations:
- 1: https://sqldocs.org/sqlite-datetime/
- 2: https://www.slingacademy.com/article/how-to-work-with-date-arithmetic-using-sqlite-functions/
- 3: https://www.devkuma.com/en/docs/sqlite/date-time-functions/
- 4: https://www.techonthenet.com/sqlite/functions/datetime.php
- 5: https://stackoverflow.com/questions/50117301/how-do-i-subtract-two-columns-with-datetime-in-sqlite
- 6: https://stackoverflow.com/questions/48920688/subtracting-dates-from-column-in-sqlite
- 7: https://stackoverflow.com/questions/71685872/calculating-difference-between-datetime-stamp-in-sqlite
- 8: https://stackoverflow.com/questions/5956215/how-can-i-store-the-current-timestamp-in-sqlite-as-ticks
🌐 Web query:
GitHub SQLAlchemy lib sqlalchemy dialects sqlite base SQLiteCompiler visit_now_func CURRENT_TIMESTAMP SQLite source
💡 Result:
In SQLAlchemy, the SQLite dialect's compiler handles the now() SQL function by mapping it to the standard SQL CURRENT_TIMESTAMP expression. This is implemented in the SQLiteCompiler class within the lib/sqlalchemy/dialects/sqlite/base.py source file [1][2][3]. Specifically, the SQLiteCompiler overrides the visit_now_func method, which is invoked when a SQLAlchemy func.now() construct is compiled for a SQLite backend [1][2]. The implementation of this method is as follows: def visit_now_func(self, fn, kw): return "CURRENT_TIMESTAMP" This design ensures that func.now() produces valid SQL for SQLite [1][4]. Similarly, the visit_localtimestamp_func method is implemented to return DATETIME(CURRENT_TIMESTAMP, 'localtime') [1][2][3]. While func.now() is commonly used, SQLAlchemy also provides func.current_timestamp(), which is a dialect-agnostic way to generate the CURRENT_TIMESTAMP expression across different database backends [5]. Developers often use func.now() for convenience, but for cross-dialect migration compatibility, func.current_timestamp() is often recommended to ensure consistent SQL generation [5].
Citations:
- 1: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/dialects/sqlite/base.py
- 2: https://github.com/sqlalchemy/sqlalchemy/blob/rel%5F1%5F4%5F41/lib/sqlalchemy/dialects/sqlite/base.py
- 3: https://github.com/zzzeek/sqlalchemy/blob/rel_0_9_4/lib/sqlalchemy/dialects/sqlite/base.py
- 4: Missing func.now() in SQLite sqlalchemy/sqlalchemy#708
- 5: Does sqlalchemy.sql.functions really need now() and current_timestamp()? sqlalchemy/sqlalchemy#7253
🌐 Web query:
SQLAlchemy 2 datetime literal bind expression TIMESTAMP bind_processor SQLite DateTime source
💡 Result:
In SQLAlchemy 2, the interaction between DateTime types and SQLite is managed by dialect-specific implementation classes, most notably sqlalchemy.dialects.sqlite.DATETIME [1][2]. Because SQLite lacks a native DATETIME type, SQLAlchemy handles this by converting Python datetime objects into ISO-formatted strings [1][3]. For bind processing, the DATETIME class implements a bind_processor method [2][4]. This method returns a callable that accepts a Python datetime or date object and formats it into the specific string representation (e.g., YYYY-MM-DD HH:MM:SS.ffffff) that SQLite understands [2]. Regarding literal bind expressions (rendering values directly into SQL strings for debugging or logging via compile_kwargs={"literal_binds": True}): 1. Literal Rendering: If you encounter issues where DateTime values cannot be rendered as literals, it is often because a specific literal_processor has not been defined or invoked for that type in the current context [5][6]. 2. Customizing Behavior: To customize how DateTime values are processed—whether for bind parameters or literal rendering—the recommended approach is to use TypeDecorator [7][5]. This allows you to override process_bind_param for standard parameter binding and process_literal_param for literal rendering [7]. If you need to ensure consistent behavior across SQLite and other databases, TypeDecorator provides a centralized way to handle both the conversion of input values and the serialization of literals, bypassing limitations of the underlying dialect-specific implementation [7][5][8]. Note that the DATETIME and TIME types in the SQLite dialect function differently regarding automatic processing than the DATE or TIMESTAMP types, especially when certain sqlite3 driver flags are enabled [1][3][9].
Citations:
- 1: https://docs.sqlalchemy.org/en/21/dialects/sqlite.html
- 2: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/dialects/sqlite/base.py
- 3: https://docs.sqlalchemy.org/en/20/dialects/sqlite.html
- 4: https://pydoc.dev/sqlalchemy/latest/sqlalchemy.dialects.sqlite.DATETIME.html
- 5: Unable to create custom literal_processor for Date and DateTime sqlalchemy/sqlalchemy#7002
- 6: https://groups.google.com/g/sqlalchemy-devel/c/X15m5Nw8Ino
- 7: https://docs.sqlalchemy.org/en/21/core/custom_types.html
- 8: https://stackoverflow.com/questions/75087464/how-to-use-the-same-code-for-both-sqlite-and-postgres
- 9: http://docs.sqlalchemy.org/en/latest/dialects/sqlite.html
🌐 Web query:
SQLAlchemy SQLite DateTime bind literal processor render datetime object as string source
💡 Result:
In SQLAlchemy, the conversion of Python datetime objects into string literals for SQLite (when using literal binds) is handled by the literal_processor method of the dialect-specific type class [1][2]. For SQLite, SQLAlchemy provides specialized implementation classes: sqlalchemy.dialects.sqlite.DATETIME, DATE, and TIME [3][4]. These types are designed to store and render dates and times as ISO-formatted strings [3]. The literal_processor(self, dialect) method is responsible for returning a callable that accepts a Python value and converts it into the string representation required by the database [1][2]. Within the SQLite dialect's DATETIME implementation, the bind_processor and literal_processor methods use an internal formatting string to convert datetime.datetime or datetime.date objects into the format: 'YYYY-MM-DD HH:MM:SS.ffffff' (or a truncated version depending on the configuration) [5]. If you are implementing a custom type and need to support literal rendering, you should generally inherit from TypeDecorator [6][7]. Instead of overriding literal_processor directly, you should override process_literal_param(self, value, dialect) [6][7]. This allows you to define how your custom type should be rendered as a literal string without breaking the internal processing logic of the underlying implementation type [6][7]. Key technical details: - SQLite lacks native date/time types, so SQLAlchemy treats these values as strings [3][1]. - The literal_processor method is specifically invoked when the "literal_binds" flag is enabled (e.g., during DDL generation or when logging the full SQL string for debugging) [1][2][8]. - For custom types, TypeDecorator.process_literal_param is the recommended hook to ensure that the literal rendering is consistent with your type's process_bind_param and process_result_value logic [6][7].
Citations:
- 1: https://docs.sqlalchemy.org/en/20/core/type%5Fbasics.html
- 2: https://docs.sqlalchemy.org/en/20/core/type%5Fapi.html
- 3: https://docs.sqlalchemy.org/en/20/dialects/sqlite.html
- 4: http://docs.sqlalchemy.org/en/latest/dialects/sqlite.html
- 5: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/dialects/sqlite/base.py
- 6: https://docs.sqlalchemy.org/en/20/core/custom%5Ftypes.html
- 7: http://docs.sqlalchemy.org/en/latest/core/custom_types.html
- 8: https://groups.google.com/g/sqlalchemy-devel/c/X15m5Nw8Ino
🌐 Web query:
site:github.com/sqlalchemy/sqlalchemy SQLAlchemy func.now CURRENT_TIMESTAMP SQLite datetime column subtraction
💡 Result:
SQLAlchemy does not provide built-in, database-agnostic operators for datetime arithmetic [1][2]. Because SQLite stores dates as strings and lacks native date subtraction operators that return a standard numeric interval, you must use SQLite's native date and time functions directly via SQLAlchemy's func construct [3][2]. To calculate the difference between CURRENT_TIMESTAMP and a column (e.g., my_table.c.datetime_col), you can use the SQLite julianday() function, which converts dates to a fractional day count, making them simple to subtract [2]. Example using SQLAlchemy: from sqlalchemy import func, select from your_model import MyTable # This generates: # SELECT julianday('now') - julianday(datetime_col) FROM my_table stmt = select(func.julianday('now') - func.julianday(MyTable.datetime_col)) In this expression: 1. func.julianday('now') acts as the current time. 2. func.julianday(MyTable.datetime_col) converts your stored column value. 3. The result is the difference in days. To get seconds, you would multiply the result by 86400 (24 * 60 * 60). Note that func.now() in SQLAlchemy renders as CURRENT_TIMESTAMP in SQLite, which returns a string [4][5]. Since SQLite arithmetic requires specific functions like julianday() or datetime(), you should explicitly use these via func rather than attempting direct subtraction of column objects [3][2].
Citations:
- 1: filter by date for an datetime field does not work with SQLite, but works with PostgreSQL sqlalchemy/sqlalchemy#5104
- 2: Incorrect results or errors when adding a datetime.timedelta() value to a column sqlalchemy/sqlalchemy#5672
- 3: Sqlalchemy + Sqlite filtering by datetime range is allowed, but returns nonsensical results. sqlalchemy/sqlalchemy#4490
- 4: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/sql/functions.py
- 5: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/dialects/sqlite/base.py
🌐 Web query:
SQLite CURRENT_TIMESTAMP subtraction strftime %s CURRENT_TIMESTAMP column ISO datetime NULL
💡 Result:
In SQLite, you cannot perform direct arithmetic on the CURRENT_TIMESTAMP keyword or columns stored as text, as they are treated as strings [1][2][3]. To perform time calculations, you must convert the values into a numeric format [1][4]. Subtraction using strftime('%s') To calculate the difference between the current time and a timestamp column in seconds, convert both to Unix time (seconds since 1970-01-01) using strftime('%s',...) [5][1][3]: SELECT strftime('%s', 'now') - strftime('%s', my_timestamp_column) FROM my_table; Note: strftime('%s', 'now') returns the time in UTC [5][6]. If your column contains local time, you may need to apply the 'localtime' modifier to your 'now' call or ensure consistency in stored timezone data [7][6]. Alternative using julianday You can also use the julianday function, which returns the fractional number of days since November 24, 4714 B.C. [1][8]. To get the difference in seconds, multiply the difference in days by 86400 (the number of seconds in a day) [1][8]: SELECT (julianday('now') - julianday(my_timestamp_column)) * 86400 FROM my_table; Column Definitions When creating a table, CURRENT_TIMESTAMP can be used as a default value [2][9]. It stores the current UTC time as a text string in the format YYYY-MM-DD HH:MM:SS [2]. CREATE TABLE example ( created_at DATETIME DEFAULT CURRENT_TIMESTAMP); If you require the default value to be an expression (such as forcing local time or storing a Unix timestamp), you must enclose it in parentheses [2][9]. For example, to store the Unix timestamp as an integer: CREATE TABLE example ( created_at INTEGER DEFAULT (strftime('%s', 'now'))); If you encounter issues where CURRENT_TIMESTAMP is being treated as a literal string "CURRENT_TIMESTAMP" rather than a function, ensure your syntax uses the keyword directly in the DEFAULT clause without quotes, or use the parentheses syntax for expressions [2][9][10].
Citations:
- 1: https://stackoverflow.com/questions/17708167/difference-in-seconds-between-timestamps-in-sqlite3
- 2: https://sqlite.org/lang_createtable.html
- 3: https://stackoverflow.com/questions/64867957/how-do-i-get-a-the-time-difference-in-seconds-in-sqlite
- 4: https://stackoverflow.com/questions/289680/difference-between-2-dates-in-sqlite
- 5: https://sqlite.org/lang_datefunc.html
- 6: https://stackoverflow.com/questions/381371/sqlite-current-timestamp-is-in-gmt-not-the-timezone-of-the-machine
- 7: https://stackoverflow.com/questions/16277366/sqlite-subtracting-a-date-entity-from-current-time
- 8: https://learnsql.com/cookbook/how-to-calculate-the-difference-between-two-timestamps-in-sqlite/
- 9: https://stackoverflow.com/questions/200309/how-to-create-timestamp-column-with-default-value-now
- 10: https://stackoverflow.com/questions/78374066/current-timestamp-in-sqlite-ends-up-as-string-literal
Calculate pending age outside SQLite datetime arithmetic.
func.now() - func.min(Outbox.created_at) does not produce a timestamp interval on SQLite, and the except path resets OUTBOX_OLDEST_PENDING_SECONDS to 0. Fetch the oldest created_at and compute elapsed seconds in Python, or use dialect-specific datetime arithmetic.
🤖 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 `@worker/outbox_relay.py` around lines 54 - 59, Update the pending-age
calculation in the outbox relay to fetch the minimum pending Outbox.created_at
value and compute elapsed seconds in Python, avoiding SQLite datetime
subtraction. Preserve the existing zero value when no pending record exists, and
ensure the OUTBOX_OLDEST_PENDING_SECONDS metric receives the computed age
without relying on the exception-reset path.
| beforeEach(async () => { | ||
| base = await mkdtemp(join(tmpdir(), 'bd-dl-')); | ||
| env = { ...process.env }; | ||
| for (const m of Object.values(mocks)) { | ||
| m.mockReset(); | ||
| } | ||
| mocks.newPage.mockResolvedValue({}); | ||
| mocks.newContext.mockResolvedValue({ | ||
| newPage: (...a) => mocks.newPage(...a), | ||
| close: (...a) => mocks.contextClose(...a), | ||
| }); | ||
| mocks.launch.mockResolvedValue({ | ||
| newContext: (...a) => mocks.newContext(...a), | ||
| close: (...a) => mocks.browserClose(...a), | ||
| }); | ||
| mocks.contextClose.mockResolvedValue(); | ||
| mocks.browserClose.mockResolvedValue(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| process.env = env; | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Vitest 2.1 vi.stubEnv vi.unstubAllEnvs API
💡 Result:
In Vitest, vi.stubEnv and vi.unstubAllEnvs are utilities designed to manage environment variables during tests [1][2]. vi.stubEnv(name, value) This method updates an environment variable on both process.env and import.meta.env [1][3]. When you call this for the first time for a specific variable, Vitest stores the original value, allowing you to restore it later [1][2]. - Name: The name of the environment variable (string) [1][2]. - Value: The new value to set [1][2]. If set to undefined, the environment variable is deleted [1][3]. Note that if the name corresponds to specific built-in environment variables (like PROD, DEV, or SSR), the value can be a boolean [1][4]. vi.unstubAllEnvs This method restores all environment variables to their original values as they existed before any vi.stubEnv calls were made [1][2]. It is intended to clean up the environment after tests to prevent state leakage [1][2]. Configuration Option Alternatively, you can enable the unstubEnvs configuration option in your vitest.config.ts file to have Vitest automatically call vi.unstubAllEnvs before each test [5][6][7]: // vitest.config.ts export default defineConfig({ test: { unstubEnvs: true, }, }) Warning: Using the unstubEnvs configuration option may cause issues with concurrent tests, as the completion of one test might restore environment variables that are still in use by other tests running in parallel [7]. In such cases, calling vi.unstubAllEnvs manually in a beforeEach hook is recommended [5][6].
Citations:
- 1: https://vitest.dev/api/vi
- 2: https://github.com/vitest-dev/vitest/blob/main/packages/vitest/src/integrations/vi.ts
- 3: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/api/vi.md
- 4: vitest-dev/vitest@v2.1.0-beta.6...v2.1.0-beta.7
- 5: https://github.com/vitest-dev/vitest/blob/1a4705da/docs/guide/mocking.md
- 6: https://vitest.dev/guide/mocking
- 7: https://vitest.dev/config/unstubenvs.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching downloader.test.js:\n'
fd -a 'downloader\.test\.js$' . | sed 's#^\./##'
FILE="$(fd 'downloader\.test\.js$' . | head -n 1)"
if [ -n "${FILE:-}" ]; then
printf '\nFile: %s\n' "$FILE"
wc -l "$FILE"
printf '\nRelevant lines 1-230:\n'
sed -n '1,230p' "$FILE" | nl -ba
fi
printf '\nvitest version references:\n'
if [ -f package.json ]; then cat package.json | sed -n '1,220p'; fi
fd -a 'package\.json$' . | sort | while read -r f; do
if rg -q '"vitest"|"`@vitest/`' "$f"; then
echo "--- ${f#./}"
rg -n '"vitest"|"`@vitest/`' "$f"
fi
done
printf '\nUsage of vi.stubEnv/unstubAllEnvs/process.env snapshots:\n'
rg -n "vi\.stubEnv|unstubAllEnvs|process\.env\s*=|env\s*=.*process\.env" . -g '!node_modules' -g '!dist' -g '!build'Repository: tomkabel/vooglaadija
Length of output: 450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILES="$(fd 'downloader\.test\.js$' . | head -n 1)"
if [ -n "${FILES:-}" ]; then
printf 'File: %s\n' "$FILES"
wc -l "$FILES"
printf '\nRelevant lines 1-240:\n'
awk '{printf "%5d\t%s\n", NR, $0}' "$FILES" | sed -n '1,240p'
fi
printf '\nRelevant version/config/package files:\n'
fd 'package\.json$' . | sort | while read -r f; do
if rg -q '"vitest"|"`@vitest/`' "$f" || [[ "$f" =~ vitest\.config\.(ts|js|mjs|cjs)$ ]]; then
echo "--- ${f#./}"
rg -n '"vitest"|"`@vitest/`' 'vitest' "$f" || true
fi
done
printf '\nUsage of vi.stubEnv/unstubAllEnvs/process.env snapshots:\n'
rg -n "vi\.stubEnv|unstubAllEnvs|process\.env\s*=|env\s*=.*process\.env" .Repository: tomkabel/vooglaadija
Length of output: 9865
Use Vitest environment stubs for BD_DOWNLOAD_TIMEOUT_MS.
Set this fixture with vi.stubEnv('BD_DOWNLOAD_TIMEOUT_MS', '60000') and restore it in afterEach with vi.unstubAllEnvs() instead of storing/replacing process.env. This keeps environment changes scoped and uses Vitest’s supported restore API.
♻️ Proposed refactor
- let env;
-
beforeEach(async () => {
base = await mkdtemp(join(tmpdir(), 'bd-dl-'));
- env = { ...process.env };- afterEach(() => {
- process.env = env;
- });
+ afterEach(() => {
+ vi.unstubAllEnvs();
+ });Then set the variable with vi.stubEnv('BD_DOWNLOAD_TIMEOUT_MS', '60000') on line 200.
🤖 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 `@packages/browser-downloader/tests/downloader.test.js` around lines 168 - 189,
Update the downloader test setup to stub BD_DOWNLOAD_TIMEOUT_MS with Vitest’s
vi.stubEnv('BD_DOWNLOAD_TIMEOUT_MS', '60000') in beforeEach, and remove the
manual env snapshot and process.env replacement. Restore environment stubs in
afterEach using vi.unstubAllEnvs(), while preserving the existing mock setup and
teardown.
| it('rejects a path outside the base (path traversal)', async () => { | ||
| const base = await mkdtemp(join(tmpdir(), 'bd-out-')); | ||
| const realpath = async (p) => p.replace(`${base}/../`, '/etc/'); | ||
| await expect(validateOutputDir('/etc/passwd', { base, realpath })).rejects.toThrow(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
This test does not exercise path traversal.
The realpath fake replaces `${base}/../` with /etc/. The input is /etc/passwd, which does not contain that substring, so the fake behaves as the identity function. The test only proves that /etc/passwd is not under a temporary base. It does not cover a .. segment or a symlink that resolves outside the base, which is the case the containment check in validateOutputDir defends against.
💚 Proposed test that models a symlink escape
it('rejects a path outside the base (path traversal)', async () => {
const base = await mkdtemp(join(tmpdir(), 'bd-out-'));
- const realpath = async (p) => p.replace(`${base}/../`, '/etc/');
- await expect(validateOutputDir('/etc/passwd', { base, realpath })).rejects.toThrow();
+ // `evil` looks like a child of the base but resolves outside it.
+ const inside = join(base, 'evil');
+ const realpath = async (p) => (p === inside ? '/etc' : p);
+ await expect(validateOutputDir(inside, { base, realpath })).rejects.toThrow(/under/);
+ // A `..` segment must also be rejected after resolution.
+ await expect(
+ validateOutputDir(join(base, '..', 'elsewhere'), { base, realpath: async (p) => p }),
+ ).rejects.toThrow(/under/);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('rejects a path outside the base (path traversal)', async () => { | |
| const base = await mkdtemp(join(tmpdir(), 'bd-out-')); | |
| const realpath = async (p) => p.replace(`${base}/../`, '/etc/'); | |
| await expect(validateOutputDir('/etc/passwd', { base, realpath })).rejects.toThrow(); | |
| }); | |
| it('rejects a path outside the base (path traversal)', async () => { | |
| const base = await mkdtemp(join(tmpdir(), 'bd-out-')); | |
| // `evil` looks like a child of the base but resolves outside it. | |
| const inside = join(base, 'evil'); | |
| const realpath = async (p) => (p === inside ? '/etc' : p); | |
| await expect(validateOutputDir(inside, { base, realpath })).rejects.toThrow(/under/); | |
| // A `..` segment must also be rejected after resolution. | |
| await expect( | |
| validateOutputDir(join(base, '..', 'elsewhere'), { base, realpath: async (p) => p }), | |
| ).rejects.toThrow(/under/); | |
| }); |
🤖 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 `@packages/browser-downloader/tests/validate.test.js` around lines 128 - 132,
Update the test named “rejects a path outside the base (path traversal)” so its
mocked realpath resolves an input containing a parent-directory segment or
symlink-like path from within base to an outside location, such as /etc. Ensure
validateOutputDir receives that traversal path and still rejects it, rather than
testing an already absolute path unrelated to base.
- errors.js: Move TypeError cause recursion outside err.code guard so
fetch timeouts with ETIMEDOUT cause are correctly classified (was
unreachable inside the typeof-code branch, always returned network_error).
- downloader.js: Fix stealth plugin race — two concurrent download()
calls could both see stealthApplied=false and register the plugin twice.
Replace boolean flag with a stable promise (??= pattern).
- tier1-cdp.js: Normalize header keys to lowercase for array/object
forms — Headers.entries() already lowercases, but array and plain-object
header forms preserved caller casing, so capitalized Authorization/
Referer headers were silently missed.
- tier2-dom.js: Fix page.evaluate argument binding — pass a single
object {u, cap, timeoutMs} and destructure in the callback instead
of passing three positional args (only the first was received).
- tier2-dom.js: Serialize BLOCK_RE as {source, flags} for page.evaluate
instead of referencing the Node module-scope constant (was undefined).
- security_headers.py: Remove 'strict-dynamic' from script-src CSP —
strict-dynamic overrides 'self' and blocks parser-inserted external
scripts (sse.js, dashboard.js) that lack nonces.
- yt_dlp_service.py: Tighten subdomain-bypass detection — use
hostname.endswith('.' + domain) instead of substring match which
falsely flagged myyoutube.com.
- retry_scheduler.py: Add db.rollback() after Redis-accepted but
DB-failed retry enqueue to keep the outbox row pending.
- presentation.html: Add nonce attribute to slide deck controller
script so it executes under the nonce-based CSP.
- server.js: Use DownloaderError('timeout') for request timeouts so
classifyError maps them correctly (was generic Error).
- validate.js: Extend IPv4 blocklist with CGNAT (100.64/10),
192.0.0.0/24, 198.18/15, and multicast (>=224) ranges.
- CI: Align DB password between service and test env (test_pass
instead of random hex the Postgres container can never see).
Signed-off-by: tomkabel <[email protected]>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/services/yt_dlp_service.py (1)
270-313: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep browser-downloader routes and
_get_platform()aligned on TikTok and Instagram host aliases.
select_executor()returns"browser"for thevm.tiktok.comsubdomain, but_TIKTOK_HOSTShas novm.tiktok.comentry, so_get_platform()falls back to"youtube". Addvm.tiktok.comtoyt_dlp_service.pyor use the exact same alias set fromworker/browser_executor.pyso TikTok and Instagram URLs route to the same backend and get the same extractor/cookie behavior 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 `@app/services/yt_dlp_service.py` around lines 270 - 313, Update the platform alias definitions used by _get_platform so TikTok and Instagram host recognition matches the aliases handled by select_executor(), including vm.tiktok.com. Prefer reusing the canonical alias set from browser_executor.py when available; otherwise add the missing alias to _TIKTOK_HOSTS and verify Instagram aliases remain aligned, preserving consistent backend, extractor, and cookie behavior.
🤖 Prompt for all review comments with 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.
Outside diff comments:
In `@app/services/yt_dlp_service.py`:
- Around line 270-313: Update the platform alias definitions used by
_get_platform so TikTok and Instagram host recognition matches the aliases
handled by select_executor(), including vm.tiktok.com. Prefer reusing the
canonical alias set from browser_executor.py when available; otherwise add the
missing alias to _TIKTOK_HOSTS and verify Instagram aliases remain aligned,
preserving consistent backend, extractor, and cookie behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1e006bc4-7aff-4dac-a230-9cf27400368b
📒 Files selected for processing (12)
.github/workflows/fastapi-test.yml.secrets.baselineapp/api/middleware/security_headers.pyapp/services/yt_dlp_service.pyapp/templates/slides/presentation.htmlpackages/browser-downloader/src/downloader.jspackages/browser-downloader/src/errors.jspackages/browser-downloader/src/server.jspackages/browser-downloader/src/tier1-cdp.jspackages/browser-downloader/src/tier2-dom.jspackages/browser-downloader/src/validate.jsworker/retry_scheduler.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Test Docker Compose Stack
⚠️ CI failures not shown inline (2)
GitHub Actions: FastAPI REST API Tests / 5_Lint (Python + JS + CSS + Markdown + YAML).txt: feat(worker): Phase 2 hybrid routing — TikTok/Instagram/X via browser-downloader
Conclusion: failure
##[group]Run hatch run lint:check
�[36;1mhatch run lint:check�[0m
shell: /usr/bin/bash -e {0}
env:
PYTHON_VERSION: 3.12
HATCH_VERSION: 1.16.5
UV_PYTHON_INSTALL_DIR: /home/runner/work/_temp/uv-python-dir
UV_PYTHON: 3.12
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
Creating environment: lint
Checking dependencies
Syncing dependencies
##[error]app/api/routes/web/web_auth.py:93:11: PLR0917 Too many positional arguments (6 > 5)
GitHub Actions: FastAPI REST API Tests / Lint (Python + JS + CSS + Markdown + YAML): feat(worker): Phase 2 hybrid routing — TikTok/Instagram/X via browser-downloader
Conclusion: failure
##[group]Run hatch run lint:check
�[36;1mhatch run lint:check�[0m
shell: /usr/bin/bash -e {0}
env:
PYTHON_VERSION: 3.12
HATCH_VERSION: 1.16.5
UV_PYTHON_INSTALL_DIR: /home/runner/work/_temp/uv-python-dir
UV_PYTHON: 3.12
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
Creating environment: lint
Checking dependencies
Syncing dependencies
##[error]app/api/routes/web/web_auth.py:93:11: PLR0917 Too many positional arguments (6 > 5)
🧰 Additional context used
🪛 ast-grep (0.45.0)
packages/browser-downloader/src/tier2-dom.js
[warning] 121-121: Detects non-literal values in regular expressions
Context: new RegExp(source, flags)
Note: [CWE-1333] Inefficient Regular Expression Complexity (ReDoS via non-literal RegExp).
(detect-non-literal-regexp)
[warning] 210-210: Detects non-literal values in regular expressions
Context: new RegExp(source, flags)
Note: [CWE-1333] Inefficient Regular Expression Complexity (ReDoS via non-literal RegExp).
(detect-non-literal-regexp)
packages/browser-downloader/src/tier1-cdp.js
[warning] 357-357: Detects non-literal values in regular expressions
Context: new RegExp(source, flags)
Note: [CWE-1333] Inefficient Regular Expression Complexity (ReDoS via non-literal RegExp).
(detect-non-literal-regexp)
🪛 Betterleaks (1.7.0)
.secrets.baseline
[high] 149-149: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 156-156: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (17)
app/api/middleware/security_headers.py (1)
21-30: LGTM!app/templates/slides/presentation.html (1)
12-12: LGTM!Also applies to: 253-257, 289-289, 331-339, 532-532
.secrets.baseline (1)
149-149: LGTM!Also applies to: 156-156
.github/workflows/fastapi-test.yml (2)
52-52: LGTM!Also applies to: 79-79
167-167: LGTM!Also applies to: 203-220
worker/retry_scheduler.py (1)
8-8: LGTM!Also applies to: 58-58, 142-142, 161-161, 172-184
packages/browser-downloader/src/server.js (1)
88-119: Request timeout still does not stop the in-flight download.The timeout rejection now correctly builds a
DownloaderError('timeout', ...)(Line 102), soclassifyErrorreportstimeoutinstead ofnetwork_error. That part is fixed.The underlying concurrency-leak issue from the previous review remains: when the timeout wins the
Promise.race,finallyonly callsrelease()(Line 118). Nothing aborts thedownload()call, so the Chromium instance, CDP session, and anystreamlinksubprocess keep running past the freed slot. Repeated timeouts let live browsers exceedMAX_CONCURRENCY.packages/browser-downloader/src/tier1-cdp.js (5)
169-171:requestHeadersstill grows unbounded and is never evicted.
onRequestWillBeSentinserts an entry per request carrying anyAUTH_HEADER_NAMESvalue (Lines 204-215). The read at Line 262 never deletes the entry, unlikecandidates, which is deleted at Line 223. A page issuing many requests during the interception window accumulates unbounded entries, some holdingcookieorauthorizationvalues in memory longer than needed.Also applies to: 204-215, 249-265
203-215: Capturedcookie/authorizationheaders still flow towardstreamlinkargv.
AUTH_HEADER_NAMEScapturescookieandauthorization(Lines 204-215), andonLoadingFinishedmerges them intoauthHeaders(Lines 261-265), returned todownloader.jsand forwarded todownloadStream. Process arguments are world-readable through/proc/<pid>/cmdline, so any other process in the container can read a replayed session cookie or bearer token ifdownloadStreamstill passes these tostreamlinkas CLI arguments.Also applies to: 249-271
47-47: LGTM!
95-124: LGTM!
353-368: LGTM!packages/browser-downloader/src/tier2-dom.js (2)
160-182: Blob body is still serialized as a JSON number array.Passing
{ u, cap, timeoutMs }as one object (Line 181) fixes the previous bug wherecapandtimeoutMswereundefined; thecl > capandab.byteLength > capguards now work as intended.The byte-transfer method itself is unchanged: Line 175 still returns
Array.from(new Uint8Array(ab)), and Line 192 rebuilds it withBuffer.from(payload.bytes). Each byte crosses CDP as a JSON decimal number plus a separator, several times the media size, and both the page and Node process hold the expanded form at once. With the defaultbodyCap, a large blob can still produce a multi-gigabyte JSON transfer before the post-fetch size checks apply.Also applies to: 187-196
119-125: LGTM!Also applies to: 208-214
packages/browser-downloader/src/errors.js (1)
99-136: LGTM!packages/browser-downloader/src/validate.js (1)
57-121: LGTM!packages/browser-downloader/src/downloader.js (1)
13-13: LGTM!Also applies to: 28-38, 64-64, 82-82, 97-100
|
Note Docstrings generation - SKIPPED |
|
✅ CI fix PR created: Follow
1 failing check(s)
5 file(s) modified
View agent analysis |
Docstrings generation was requested by @tomkabel. * #141 (comment) The following files were modified: * `app/api/dependencies/__init__.py` * `app/api/docs.py` * `app/api/middleware/prometheus.py` * `app/api/middleware/request_body_size.py` * `app/api/middleware/security_headers.py` * `app/api/rate_limit_config.py` * `app/api/routes/auth.py` * `app/api/routes/chaos.py` * `app/api/routes/health.py` * `app/api/routes/sse.py` * `app/api/routes/web/web_auth.py` * `app/api/routes/web/web_auth_helpers.py` * `app/api/routes/web/web_dashboard.py` * `app/api/routes/web/web_downloads.py` * `app/api/routes/web/web_helpers.py` * `app/api/routes/web/web_settings.py` * `app/api/startup.py` * `app/auth.py` * `app/main.py` * `app/services/auth_service.py` * `app/services/circuit_breaker.py` * `app/services/download_service.py` * `app/services/error_classifier.py` * `app/services/job_factory.py` * `app/services/outbox_service.py` * `app/services/pubsub_service.py` * `app/services/user_service.py` * `app/services/yt_dlp_service.py` * `app/utils/validators.py` * `core/config.py` * `core/database.py` * `core/logging_config.py` * `core/models/user.py` * `core/queue.py` * `core/redis_client.py` * `core/utils/security.py` * `entrypoint.sh` * `packages/browser-downloader/src/concurrency.js` * `packages/browser-downloader/src/downloader.js` * `packages/browser-downloader/src/errors.js` * `packages/browser-downloader/src/server.js` * `packages/browser-downloader/src/streamlink-backend.js` * `packages/browser-downloader/src/tier1-cdp.js` * `packages/browser-downloader/src/tier2-dom.js` * `packages/browser-downloader/src/validate.js` * `tests/test_services/test_yt_dlp.py` * `tests/test_worker/test_browser_executor.py` * `tests/test_worker/test_outbox_recovery.py` * `worker/browser_executor.py` * `worker/dlq_manager.py` * `worker/health.py` * `worker/job_claimer.py` * `worker/job_executor.py` * `worker/main.py` * `worker/outbox_relay.py` * `worker/processor.py` * `worker/retry_scheduler.py` * `worker/zombie_sweeper.py`
Summary
Phase 2 of issue #140. Wires the existing Phase 0 browser-downloader microservice (
packages/browser-downloader/) into the worker so/api/v1/downloadscan serve TikTok/Instagram/Twitter-X URLs end-to-end.Before: every job goes to yt-dlp; TikTok/Instagram/X fail (yt-dlp cannot construct
blob:media).After: platform-aware dispatch — TikTok/Instagram/X go to the microservice, YouTube stays on yt-dlp.
Behavior
browser_downloader_enabled=False. The worker behaves byte-for-byte identical to main until an operator setsBROWSER_DOWNLOADER_ENABLED=true. DefaultBROWSER_DOWNLOADER_ENDPOINT=http://browser-downloader:3000resolves in-cluster.tiktok.com/tiktokv.com/instagram.com/instagr.am/twitter.com/x.com/t.co(withwww.-stripping and FQDN-trailing-dot handling). YouTube and unknown hosts continue to yt-dlp.ErrorCategorycodes so the existing retry/DLQ pipeline handles them:drm_detected/anti_bot_block→ BLOCKEDnetwork_error/ ConnectError / 5xx / non-JSON → TRANSIENTtimeout→ TIMEOUTno_media_found/ 404 → NOT_FOUNDbrowser_downloadercircuit breaker (Redis-distributed opt-in viaBROWSER_DOWNLOADER_CB_USE_REDIS).Files
worker/browser_executor.pyworker/job_executor.pyexecute()core/config.pypyproject.tomlhttpx>=0.27promoted from test extras to runtimetests/test_worker/test_browser_executor.pytests/test_worker/test_job_executor_routing.pyexecute()tests/test_browser_downloader_config.pyVerification
hatch run lint:check→ exit 0hatch run type:check→ no issueshatch run test:unit→ 875 passed, 6 skipped, 0 regressionsDeferred to P4/P5 (documented in
_bmad-output/implementation-artifacts/deferred-work.md)aclose()on the httpx client at worker shutdownt.coredirect resolution (currently routes to browser even for YouTube targets)Out of scope (P1, P3, P4)
packages/browser-downloader/unchanged — Phase 0 contract frozenSpec
Full design + I/O matrix + review order:
_bmad-output/implementation-artifacts/spec-gh-140-p2-worker-integration.md