Feat/daily push epic 179#194
Merged
Merged
Conversation
Multi-channel push infrastructure with structured digest cards: - #187: Integrate HF upvotes into Judge scoring prompt - #180: Add digest_card extraction (highlight/method/finding/tags) with new LLM prompt, email template rendering, markdown output - #181: MinerU Cloud API client for PDF figure extraction - #182: Apprise multi-channel push layer with YAML config - #183-185: Channel formatters (Telegram MarkdownV2, Discord Rich Embed, WeCom markdown+news, Feishu/Lark interactive card) - #186: RSS 2.0 + Atom feed endpoints (/api/feed/daily.xml, .atom) Also creates follow-up issues #190 (Push Channel Settings UI) and #191 (Dashboard Digest Card Enhancement). 56 new tests, all passing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…y-paper workflow Refs #179
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces a comprehensive daily paper push notification system with multi-channel support (Telegram, Discord, WeCom, Feishu), RSS/Atom feed generation, PDF figure extraction via MinerU Cloud API, and digest card enrichment using LLM prompts. New API endpoints, infrastructure components, and configuration management enable daily digest distribution across diverse platforms. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Schedule
participant API as API Server<br/>(Feed Routes)
participant Feed as Feed Service<br/>(ScholarFeedService)
participant LLM as LLM Service<br/>(Digest Card)
participant Notifier as AppriseNotifier<br/>(Multi-Channel)
participant Channels as External<br/>Channels
Client->>API: GET /api/feed/daily.xml
API->>Feed: export_feed_entries(limit)
Feed->>LLM: extract_daily_digest_card(title, abstract)
LLM-->>Feed: {highlight, method, finding, tags}
Feed-->>API: [feed_entry{...digest_card}]
API-->>Client: RSS/Atom XML
Note over Client,Channels: Parallel: Daily Push Workflow
Client->>LLM: enrich_dailypaper(enable_digest_card=true)
LLM->>LLM: extract_daily_digest_card (batch)
LLM-->>Client: enriched_report{items[...digest_card]}
Client->>Notifier: push_daily_digest(report, markdown, html, tag="daily")
Notifier->>Notifier: load_yaml_config (channels + tags)
loop For each channel with "daily" tag
Notifier->>Notifier: format_digest (channel-specific formatter)
Notifier->>Channels: POST with formatted payload
Channels-->>Notifier: 200 OK / error
Notifier->>Notifier: retry_with_backoff (if retryable)
Notifier->>Notifier: mark_sent (idempotency)
end
Notifier-->>Client: {success: true, channels_notified: 4}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the daily paper digest functionality by introducing AI-powered digest card generation, PDF figure extraction, and robust multi-channel push notification capabilities. It also adds RSS/Atom feed endpoints for broader content distribution and Telegram bot command support for interactive subscription management. Architectural updates and documentation changes reflect these new features and future directions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR expands PaperBot’s DailyPaper pipeline and delivery surface by adding digest-card enrichment, optional figure extraction, multi-channel push formatting via Apprise, Telegram subscription commands, and RSS/Atom feed exports—plus strengthening HF Daily sourcing and dedup behavior.
Changes:
- Add
digest_cardextraction + rendering, and include HF community upvotes in judge prompts. - Add MinerU-based PDF figure extraction (with optional CLI + ARQ enablement) and expose RSS/Atom feed endpoints.
- Introduce Apprise-based multi-channel push with channel formatters + Telegram command webhook; improve HF Daily connector caching/trending and dedup via stable identities.
Reviewed changes
Copilot reviewed 52 out of 54 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| web/README.md | Rename DeepCode Studio reference to AgentSwarm Studio. |
| tests/unit/test_push_formatters.py | Add formatter registry + channel payload tests (Telegram/Discord/WeCom/Feishu). |
| tests/unit/test_push_commands_route.py | Add tests for Telegram command webhook (subscribe/list/today). |
| tests/unit/test_push_channel_e2e.py | Add mocked Apprise E2E tests for multi-channel digest push + failure injection. |
| tests/unit/test_paper_search_service_rrf.py | Add test ensuring dedup prefers shared arXiv identity over title-hash. |
| tests/unit/test_mineru_client.py | Add MinerU client + figure selection + caching + pipeline integration tests. |
| tests/unit/test_hf_upvote_judge_integration.py | Add tests for including HF upvotes in judge prompt. |
| tests/unit/test_hf_daily_papers_connector.py | Add tests for HF Daily connector caching, trending modes, and graceful degradation. |
| tests/unit/test_hf_daily_adapter.py | Add tests for adapter daily/trending methods and arXiv identity inference. |
| tests/unit/test_feed_routes.py | Add tests for RSS/Atom helper behavior + keyword/track filtering. |
| tests/unit/test_daily_digest_card.py | Add digest-card prompt/registry + pipeline/email/markdown integration tests. |
| tests/unit/test_audit_daily_push_reports.py | Add tests for audit script helpers. |
| tests/unit/test_arq_daily_papers.py | Add test ensuring cron enqueues figure extraction flags. |
| tests/unit/test_apprise_notifier.py | Add unit tests for YAML loading, retry behavior, idempotency, formatter usage. |
| src/paperbot/workflows/feed.py | Add export_feed_entries() for feed entry export. |
| src/paperbot/presentation/cli/main.py | Add CLI flags for MinerU figure extraction and output stats. |
| src/paperbot/infrastructure/queue/arq_worker.py | Add figure extraction flags to cron/job and call extraction in job. |
| src/paperbot/infrastructure/push/telegram_subscription_store.py | Add JSON-backed Telegram subscription store. |
| src/paperbot/infrastructure/push/formatters/wecom.py | Add WeCom digest formatter (markdown + news card). |
| src/paperbot/infrastructure/push/formatters/telegram.py | Add Telegram MarkdownV2 digest formatter (photo + inline keyboard in payload). |
| src/paperbot/infrastructure/push/formatters/feishu.py | Add Feishu/Lark interactive card + post formatter. |
| src/paperbot/infrastructure/push/formatters/discord.py | Add Discord embed formatter with highlights/tags/thumbnail. |
| src/paperbot/infrastructure/push/formatters/base.py | Add formatter base class + shared top-paper collection/sorting. |
| src/paperbot/infrastructure/push/formatters/init.py | Add formatter registry + getters. |
| src/paperbot/infrastructure/push/apprise_notifier.py | Add AppriseNotifier with YAML config, retry, idempotency, formatter integration. |
| src/paperbot/infrastructure/push/init.py | Export push utilities. |
| src/paperbot/infrastructure/extractors/mineru_client.py | Add MinerU Cloud client with TTL cache and main-figure heuristics. |
| src/paperbot/infrastructure/extractors/init.py | Export MineruClient/Figure. |
| src/paperbot/infrastructure/connectors/hf_daily_papers_connector.py | Add in-memory TTL cache, metrics, trending modes, graceful degradation, improved sorting. |
| src/paperbot/infrastructure/adapters/hf_daily_adapter.py | Add daily/trending methods and infer arXiv identity from external_url. |
| src/paperbot/application/workflows/dailypaper.py | Add digest_card feature and extract_figures_for_report(); render digest_card in markdown. |
| src/paperbot/application/workflows/analysis/judge_prompts.py | Include HF community upvotes in judge prompt when present. |
| src/paperbot/application/services/paper_search_service.py | Prefer stable identity key for dedup when available. |
| src/paperbot/application/services/llm_service.py | Add extract_daily_digest_card() using new prompt template. |
| src/paperbot/application/services/email_template.py | Render digest_card (HTML + text) in email templates. |
| src/paperbot/application/services/daily_push_service.py | Add apprise channel support via AppriseNotifier. |
| src/paperbot/application/prompts/registry.py | Register daily_digest_card prompt template. |
| src/paperbot/application/prompts/paper_analysis.py | Add digest-card system/user prompts. |
| src/paperbot/api/routes/push_commands.py | Add Telegram command webhook for subscriptions and “today” titles. |
| src/paperbot/api/routes/feed.py | Add RSS/Atom feed endpoints + helpers and caching. |
| src/paperbot/api/routes/init.py | Export push_commands router. |
| src/paperbot/api/main.py | Register feed + push routers. |
| scripts/audit_daily_push_reports.py | Add audit script for digest-card/figure coverage sampling + outputs. |
| requirements.txt | Add apprise and feedgen dependencies. |
| docs/runbooks/daily_push_ops.md | Add ops runbook for push + feed delivery and audit procedure. |
| docs/progress/progress.txt | Add progress log entry for epic #179 work. |
| docs/archive/issues/191-dashboard-digest-card-enhancement.md | Archive issue draft doc. |
| docs/archive/issues/190-settings-push-channel-ui.md | Archive issue draft doc. |
| docs/archive/README.md | Add archive index/maintenance notes. |
| docs/archive/PROJECT_ISSUE_BACKLOG_legacy.md | Add archived legacy backlog. |
| docs/HF_DAILY_SOURCE.md | Document HF Daily connector/adapter entry points and metrics. |
| docs/AGENTSWARM_TODO.md | Rename DeepCode TODO to AgentSwarm Studio TODO. |
| config/push_channels.yaml | Add Apprise push channel configuration template. |
| README.md | Update product description and docs links for AgentSwarm + repro context routes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+136
to
+140
| if figures: | ||
| return figures | ||
| except Exception: | ||
| return None | ||
| return None |
There was a problem hiding this comment.
_load_cached_figures() only returns a cached result when it reconstructs a non-empty figures list. Since extract_figures() always writes a cache file (even when the API returns 0 figures), subsequent calls will treat an empty cached response as a cache miss and re-call the API every time. Consider either (a) returning [] when the cache file is valid but contains no figures, or (b) skipping _store_cached_figures() when figures is empty so you don't create unusable cache entries.
Suggested change
| if figures: | |
| return figures | |
| except Exception: | |
| return None | |
| return None | |
| return figures | |
| except Exception: | |
| return None |
| score, record = candidate | ||
| date_key = _record_date(record) | ||
| if mode == "new": | ||
| return (date_key.timestamp(), date_key, record.upvotes) |
There was a problem hiding this comment.
_candidate_sort_key() uses date_key.timestamp() for mode == "new". When a record has no usable date, _record_date() can return datetime.min (UTC), and calling .timestamp() on very old datetimes can raise on some platforms (notably Windows) or produce out-of-range behavior. Prefer sorting directly by the datetime value (or clamp missing dates to a safe epoch) to avoid potential crashes when dates are missing/malformed.
Suggested change
| return (date_key.timestamp(), date_key, record.upvotes) | |
| # Avoid calling .timestamp() on very old datetimes (for example, datetime.min), | |
| # which can raise on some platforms. Clamp to the Unix epoch for such cases. | |
| safe_epoch = datetime(1970, 1, 1, tzinfo=timezone.utc) | |
| if date_key < safe_epoch: | |
| primary = safe_epoch.timestamp() | |
| else: | |
| primary = date_key.timestamp() | |
| return (primary, date_key, record.upvotes) |
Comment on lines
+42
to
+45
| - `/api/feed/daily.rss` | ||
| - `/api/feed/daily.atom` | ||
| - `/api/feed/track/{track}.rss` | ||
| - `/api/feed/keyword/{keyword}.rss` |
There was a problem hiding this comment.
The feed endpoint URLs listed here don’t match the actual routes implemented in paperbot.api.routes.feed (e.g., /api/feed/daily.xml, /api/feed/daily.atom, /api/feed/track/{track_name}.xml, /api/feed/keyword/{keyword}.xml). Please update the runbook to the correct paths so ops validation steps are accurate.
Suggested change
| - `/api/feed/daily.rss` | |
| - `/api/feed/daily.atom` | |
| - `/api/feed/track/{track}.rss` | |
| - `/api/feed/keyword/{keyword}.rss` | |
| - `/api/feed/daily.xml` | |
| - `/api/feed/daily.atom` | |
| - `/api/feed/track/{track_name}.xml` | |
| - `/api/feed/keyword/{keyword}.xml` |
Comment on lines
+401
to
+406
| def _payload_to_body(channel_type: str, payload: Dict[str, Any]) -> tuple[str, str]: | ||
| if channel_type == "telegram": | ||
| return str(payload.get("text") or ""), "markdown" | ||
| if channel_type == "wecom": | ||
| return str((payload.get("markdown") or {}).get("content") or ""), "markdown" | ||
| if channel_type in {"feishu", "lark"}: |
There was a problem hiding this comment.
_payload_to_body() flattens channel-specific formatter payloads into a plain text body and drops Telegram-specific fields like parse_mode, photo_url/photo_caption, and inline_keyboard. As a result, the Telegram formatter’s image + buttons (and MarkdownV2 escaping) won’t actually be used when sending via Apprise, which can lead to confusing output (e.g., literal backslashes) and missing features. Either extend the notifier to pass these fields through in a channel-aware way (if Apprise supports it) or adjust the Telegram formatter/escaping/tests to reflect the plain-markdown-only behavior.
Comment on lines
+35
to
+51
| def _collect_top_papers( | ||
| self, report: Dict[str, Any], max_papers: int = 10 | ||
| ) -> List[Dict[str, Any]]: | ||
| """Helper to collect deduplicated top papers from report.""" | ||
| seen: set = set() | ||
| papers: List[Dict[str, Any]] = [] | ||
| for q in report.get("queries") or []: | ||
| for item in q.get("top_items") or []: | ||
| key = item.get("title") or id(item) | ||
| if key not in seen: | ||
| seen.add(key) | ||
| papers.append(item) | ||
| for item in report.get("global_top") or []: | ||
| key = item.get("title") or id(item) | ||
| if key not in seen: | ||
| seen.add(key) | ||
| papers.append(item) |
There was a problem hiding this comment.
_collect_top_papers() deduplicates papers using item.get("title") as the key. Different papers can legitimately share the same title (or the same paper title can appear with slight formatting differences), so this can incorrectly drop items and affect push/feed output ordering. Consider using a more stable identifier when available (e.g., url, external_id/arXiv id, or a composite like url|title) and fall back to title only as a last resort.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive daily push notification system with multi-channel support (Apprise, RSS/Atom, Telegram), AI-generated digest cards, and PDF figure extraction. However, several critical security vulnerabilities were identified, including a lack of authentication on the Telegram command webhook allowing unauthorized subscription management, a potential memory exhaustion (DoS) in the RSS feed cache, and race conditions in the subscription storage mechanism. Additionally, the review highlights areas for improving overall robustness, particularly concerning asynchronous I/O operations and exception handling, to ensure the new services are reliable and performant.
| # | ||
| # Examples (uncomment and fill in credentials): | ||
|
|
||
| channels: [] |
There was a problem hiding this comment.
The YAML syntax on this line is invalid. channels: [] defines channels as an empty list, but the following indented lines are not valid in this context. This will cause a parsing error when loading the configuration.
channels:
Comment on lines
+85
to
+86
| @router.post("/push/telegram/command", response_model=TelegramCommandResponse) | ||
| async def telegram_command(body: TelegramCommandRequest): |
There was a problem hiding this comment.
The telegram_command webhook endpoint lacks authentication, which is a critical security vulnerability allowing unauthorized subscription management. Telegram recommends using a secret token to verify requests. Additionally, the telegram_command async route, which starts here, calls _latest_digest_titles (a blocking file I/O operation), potentially degrading performance by blocking the server's event loop. Consider implementing a check for the X-Telegram-Bot-Api-Secret-Token header for authentication and running blocking functions like _latest_digest_titles in a separate thread using asyncio.to_thread.
Comment on lines
+317
to
+327
| @router.get("/feed/daily.xml") | ||
| async def daily_rss( | ||
| limit: int = Query(default=20, ge=1, le=100), | ||
| ): | ||
| """RSS 2.0 feed of recent DailyPaper digest papers.""" | ||
| cache_key = f"daily_rss:{limit}" | ||
| xml = _cached_xml(cache_key, _REPORTS_DIR) | ||
| if not xml: | ||
| reports = _load_latest_reports(_REPORTS_DIR, limit=limit) | ||
| xml = _store_cached_xml(cache_key, _REPORTS_DIR, _build_rss_xml(reports)) | ||
| return Response(content=xml, media_type="application/rss+xml; charset=utf-8") |
There was a problem hiding this comment.
The async route daily_rss and other feed routes in this file perform blocking file I/O operations (e.g., in _load_latest_reports and _build_rss_xml) without using an async-compatible approach. This will block the server's event loop, severely impacting its ability to handle concurrent requests. Blocking calls in async functions should be run in a separate thread using asyncio.to_thread.
For example, reports = _load_latest_reports(...) should become reports = await asyncio.to_thread(_load_latest_reports, ...).
Comment on lines
+33
to
+35
| def _save(self, payload: Dict[str, Dict[str, List[str] | str]]) -> None: | ||
| self._path.parent.mkdir(parents=True, exist_ok=True) | ||
| self._path.write_text(json.dumps(payload, ensure_ascii=False, indent=2), encoding="utf-8") |
There was a problem hiding this comment.
The _save method in TelegramSubscriptionStore performs read-modify-write operations on a JSON file without concurrency control, making it susceptible to race conditions and potential data loss during concurrent updates. Additionally, the method lacks proper IOError exception handling, which could lead to crashes if file write operations fail. Implement file locking (e.g., using fcntl or portalocker) to ensure atomic access and wrap file operations in a try...except block to gracefully handle IOError exceptions.
Suggested change
| def _save(self, payload: Dict[str, Dict[str, List[str] | str]]) -> None: | |
| self._path.parent.mkdir(parents=True, exist_ok=True) | |
| self._path.write_text(json.dumps(payload, ensure_ascii=False, indent=2), encoding="utf-8") | |
| def _save(self, payload: Dict[str, Dict[str, List[str] | str]]) -> None: | |
| try: | |
| self._path.parent.mkdir(parents=True, exist_ok=True) | |
| self._path.write_text(json.dumps(payload, ensure_ascii=False, indent=2), encoding="utf-8") | |
| except IOError as e: | |
| # logger should be imported at the top of the file | |
| # import logging | |
| # logger = logging.getLogger(__name__) | |
| logger.error("Failed to save telegram subscriptions to %s: %s", self._path, e) | |
| raise |
| router = APIRouter() | ||
|
|
||
| _REPORTS_DIR = Path("./reports/dailypaper") | ||
| _FEED_CACHE: Dict[str, Dict[str, Any]] = {} |
There was a problem hiding this comment.
The _FEED_CACHE dictionary is an unbounded in-memory cache. Since the cache keys are derived from user-supplied path parameters (track_name and keyword), an attacker can send a large number of requests with unique values to exhaust the server's memory, leading to a Denial of Service (DoS).
Remediation: Use a cache with a maximum size and an eviction policy, such as an LRU cache from the cachetools library.
| for file in sorted(reports_dir.glob("*.json"), reverse=True): | ||
| try: | ||
| payload = json.loads(file.read_text(encoding="utf-8")) | ||
| except Exception: |
There was a problem hiding this comment.
The except Exception: clause is too broad. It can mask unexpected errors during file processing, making debugging more difficult. It's better to catch more specific exceptions.
Suggested change
| except Exception: | |
| except (json.JSONDecodeError, IOError): |
| ) | ||
| if figures: | ||
| return figures | ||
| except Exception: |
There was a problem hiding this comment.
The except Exception: clause in _load_cached_figures is too broad. It can hide unexpected errors beyond file I/O or JSON parsing issues, making debugging harder. It's better to catch specific exceptions like IOError and json.JSONDecodeError.
Suggested change
| except Exception: | |
| except (IOError, json.JSONDecodeError): |
| if formatter is None: | ||
| return {} | ||
| return formatter.format_digest(report, max_papers=10) | ||
| except Exception: |
There was a problem hiding this comment.
The except Exception: clause here silently catches all errors during formatter execution and returns an empty dictionary. This can hide bugs in the formatters and makes it difficult to debug push notification issues. It would be better to log the exception before returning.
Suggested change
| except Exception: | |
| except Exception as exc: | |
| logger.warning("Failed to format payload for %s: %s", channel_type, exc) | |
| return {} |
| payload = json.loads(self._path.read_text(encoding="utf-8")) | ||
| if isinstance(payload, dict): | ||
| return payload | ||
| except Exception: |
There was a problem hiding this comment.
The except Exception: clause in the _load method is too broad. It can mask unexpected errors beyond file I/O or JSON parsing issues. It's better to catch specific exceptions like IOError and json.JSONDecodeError.
Suggested change
| except Exception: | |
| except (IOError, json.JSONDecodeError): |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
src/paperbot/application/services/email_template.py-210-216 (1)
210-216:⚠️ Potential issue | 🟡 MinorNormalize
digest_card.tagsbefore rendering in both HTML and text paths.Line 542 currently assumes
tagsis a list of strings. If upstream passes a scalar/mixed type, text output can misrender (or fail on join), and HTML can render unexpected character-level pills.💡 Suggested hardening diff
- tags = digest_card.get("tags") or [] + raw_tags = digest_card.get("tags") or [] + if not isinstance(raw_tags, list): + raw_tags = [raw_tags] + tags = [str(t).strip() for t in raw_tags if str(t).strip()] if tags: tag_pills = " ".join( f'<span style="background:`#e0e7ff`;color:`#3730a3`;padding:1px 6px;border-radius:3px;' f'font-size:10px;margin-right:3px;">{_esc(t)}</span>' for t in tags[:6] ) parts.append(f'<div style="margin-top:4px;">{tag_pills}</div>') ... - dc_tags = digest_card.get("tags") or [] + dc_tags_raw = digest_card.get("tags") or [] + if not isinstance(dc_tags_raw, list): + dc_tags_raw = [dc_tags_raw] + dc_tags = [str(t).strip() for t in dc_tags_raw if str(t).strip()] if dc_highlight: lines.append(f" 💎 {dc_highlight}") if dc_method: lines.append(f" 🔬 方法: {dc_method}") if dc_finding: lines.append(f" 📌 发现: {dc_finding}") if dc_tags: lines.append(f" 🏷️ {', '.join(dc_tags)}")Also applies to: 536-551
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/application/services/email_template.py` around lines 210 - 216, Normalize digest_card.get("tags") into a safe list of strings before rendering: replace the current tags = digest_card.get("tags") or [] with logic that ensures tags is a list (if a scalar, wrap it), then map each entry to str(...).strip() and filter out empty values, e.g. normalized_tags = [str(t).strip() for t in (digest_card.get("tags") or [])] or if not iterable wrap single values into a list; use normalized_tags[:6] everywhere (the HTML tag_pills generation and the text join) so both the HTML branch that builds tag_pills and the plain-text join path operate on the same validated list of strings.tests/unit/test_daily_digest_card.py-50-63 (1)
50-63:⚠️ Potential issue | 🟡 MinorSilence Ruff ARG002 in fake service method signatures.
The fake methods intentionally ignore some parameters, but Line 50–Line 63 currently trigger unused-argument warnings.
🧹 Suggested lint-only cleanup
-class _FakeLLMService: - def summarize_paper(self, title: str, abstract: str) -> str: +class _FakeLLMService: + def summarize_paper(self, title: str, _abstract: str) -> str: return f"summary:{title}" - def analyze_trends(self, *, topic: str, papers): + def analyze_trends(self, *, topic: str, _papers): return f"trend:{topic}" - def assess_relevance(self, *, paper, query: str): + def assess_relevance(self, *, _paper, _query: str): return {"score": 80, "reason": "relevant"} - def generate_daily_insight(self, report): + def generate_daily_insight(self, _report): return "insight" - def extract_daily_digest_card(self, title: str, abstract: str): + def extract_daily_digest_card(self, title: str, _abstract: str): return { "highlight": f"Key finding from {title}", "method": "Novel approach", "finding": "Significant improvement", "tags": ["LLM", "efficiency"], }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_daily_digest_card.py` around lines 50 - 63, Several fake methods in the test stub trigger Ruff ARG002 because parameters are intentionally unused; rename those unused parameters to start with an underscore to silence the warning. Specifically: in summarize_paper keep title (it’s used) but in analyze_trends rename papers to _papers, in assess_relevance rename paper to _paper, in generate_daily_insight rename report to _report, and in extract_daily_digest_card rename title/abstract to _title/_abstract (or any leading-underscore variants) so the function signatures retain types but Ruff no longer flags unused arguments.src/paperbot/workflows/feed.py-445-466 (1)
445-466:⚠️ Potential issue | 🟡 MinorExpand test coverage for
export_feed_entriesto validate the transformation contract.Tests exist (
test_scholar_feed_service_export), but only verify basic field presence. Add assertions for:
linkprecedence logic (metadata.url takes priority over paper.url)publishedfield uses ISO format fromevent.timestamp.isoformat()categoriesstructure is[event.event_type.value]Per coding guidelines, behavior changes require test coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/workflows/feed.py` around lines 445 - 466, Add assertions to the tests for export_feed_entries in test_scholar_feed_service_export to cover the transformation contract: create events with both paper.url and metadata['url'] to assert metadata['url'] overrides paper.url (link precedence), assert published equals event.timestamp.isoformat() (ISO formatted string), and assert categories is a single-element list containing event.event_type.value; locate behavior in export_feed_entries (uses event.paper, getattr(event.paper, "url"), event.metadata.get("url"), and event.timestamp.isoformat()) and add targeted assertions for those fields.src/paperbot/application/workflows/dailypaper.py-30-31 (1)
30-31:⚠️ Potential issue | 🟡 Minor
extract_figures_for_reporthas inconsistent copy semantics.Line 30 returns the original
reportwhenapi_keyis empty, while the normal path returns a deep-copied object. This can cause aliasing side effects for callers expecting a detached result.💡 Proposed fix
- if not api_key: - return report + if not api_key: + return copy.deepcopy(report)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/application/workflows/dailypaper.py` around lines 30 - 31, The early return in extract_figures_for_report returns the original report when api_key is falsy, causing inconsistent copy semantics versus the normal path which returns a deep-copied object; change the early-return to return a deep copy of report (e.g., use copy.deepcopy(report) or the same deep-copy helper used later) so callers always receive a detached object and avoid aliasing, and ensure the copy import/helper is available in the module.src/paperbot/infrastructure/push/formatters/wecom.py-41-41 (1)
41-41:⚠️ Potential issue | 🟡 MinorRemove the unnecessary
fprefix on the static string.Line 41 has no interpolation and triggers Ruff F541.
Proposed fix
- lines.append(f"**📖 本期导读**") + lines.append("**📖 本期导读**")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/infrastructure/push/formatters/wecom.py` at line 41, Remove the unnecessary f-string prefix on the static string passed to lines.append to fix Ruff F541: locate the lines.append call that adds "**📖 本期导读**" (the lines.append(f"**📖 本期导读**") invocation) and change it to a plain string literal without the leading f so it becomes lines.append("**📖 本期导读**"); no other logic should change.src/paperbot/infrastructure/push/formatters/__init__.py-34-42 (1)
34-42:⚠️ Potential issue | 🟡 MinorSort
__all__to satisfy Ruff RUF022.Line 34-Line 42 are currently unsorted and will keep lint noisy/failing.
Proposed fix
__all__ = [ - "PushFormatter", - "TelegramFormatter", "DiscordFormatter", - "WeComFormatter", "FeishuFormatter", + "PushFormatter", + "TelegramFormatter", + "WeComFormatter", "get_formatter", "list_formatters", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/infrastructure/push/formatters/__init__.py` around lines 34 - 42, The __all__ list is unsorted and triggers Ruff RUF022; open the module containing __all__ and alphabetically sort its entries (the strings: "DiscordFormatter", "FeishuFormatter", "Get_formatter" should be "get_formatter" keep case exact, "List_formatters" likewise preserve names) — specifically reorder the existing entries ("PushFormatter", "TelegramFormatter", "DiscordFormatter", "WeComFormatter", "FeishuFormatter", "get_formatter", "list_formatters") into a stable, alphabetical sequence (e.g., "DiscordFormatter", "FeishuFormatter", "PushFormatter", "TelegramFormatter", "WeComFormatter", "get_formatter", "list_formatters") so Ruff RUF022 is satisfied.src/paperbot/infrastructure/push/formatters/__init__.py-21-24 (1)
21-24:⚠️ Potential issue | 🟡 MinorDefensively handle non-string
channel_typeinget_formatter.Line 23 calls
.lower()unconditionally;None/non-string values will raiseAttributeErrorinstead of returningNone.Proposed fix
-def get_formatter(channel_type: str) -> Optional[PushFormatter]: +def get_formatter(channel_type: str | None) -> Optional[PushFormatter]: """Get a formatter instance for the given channel type.""" + if not isinstance(channel_type, str): + return None cls = _REGISTRY.get(channel_type.lower()) if cls is None: return None return cls()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/infrastructure/push/formatters/__init__.py` around lines 21 - 24, get_formatter currently calls channel_type.lower() unconditionally which raises AttributeError for None or non-string inputs; update get_formatter to defensively validate/coerce channel_type (e.g., check isinstance(channel_type, str) or use str(channel_type).lower()) before looking up _REGISTRY so non-string/None returns None instead of raising; reference the get_formatter function and the _REGISTRY lookup and ensure the lowered key is derived only after the type check.src/paperbot/api/routes/push_commands.py-64-82 (1)
64-82:⚠️ Potential issue | 🟡 MinorNarrow exception handling in
_latest_digest_titlesand log failures.Line 80-Line 81 swallow all errors, making malformed/unreadable report files invisible during incident debugging.
Proposed fix
- except Exception: + except (json.JSONDecodeError, OSError): continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/push_commands.py` around lines 64 - 82, The broad except in _latest_digest_titles is hiding file/read/parse errors; replace the bare except with targeted exception handling for path.read_text and json.loads (e.g., OSError/IOError, json.JSONDecodeError, TypeError) and log failures using the module logger with the offending path and exception details (include path, exception message, and optionally stacktrace) before continuing; keep the existing behavior of skipping that file on error but do not swallow or silence the error silently.src/paperbot/infrastructure/push/telegram_subscription_store.py-25-31 (1)
25-31:⚠️ Potential issue | 🟡 MinorAvoid blind exception handling in
_load.Line 29 swallows all exceptions and returns
{}, which can mask permission/path failures as “no subscriptions”.Proposed fix
+import logging @@ +logger = logging.getLogger(__name__) @@ - except Exception: + except (json.JSONDecodeError, OSError) as exc: + logger.warning("Failed to load Telegram subscriptions from %s: %s", self._path, exc) return {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/infrastructure/push/telegram_subscription_store.py` around lines 25 - 31, The _load method currently swallows all exceptions around json.loads(self._path.read_text(...)) which can hide real filesystem errors; change the broad except Exception to handle only expected cases: catch FileNotFoundError (and PermissionError) when the file is missing/unreadable and return {}, catch json.JSONDecodeError to return {} (and log the parse error with the path), and let any other exceptions propagate (or re-raise) so real issues aren’t masked; include the file path and exception details in the log messages when catching these specific exceptions to aid debugging.src/paperbot/infrastructure/push/apprise_notifier.py-118-120 (1)
118-120:⚠️ Potential issue | 🟡 MinorNormalize scalar
tagsvalues to avoid per-character tags.When config uses
tags: daily, Line 120 iterates characters ("d","a", ...), sotag="daily"matching fails.🛠️ Proposed fix
urls.append(url) - ch_tags = ch.get("tags") or [] - if ch_tags: - tags[url] = [str(t).strip() for t in ch_tags if str(t).strip()] + raw_tags = ch.get("tags") or [] + if isinstance(raw_tags, str): + raw_tags = [raw_tags] + if isinstance(raw_tags, list): + normalized = [str(t).strip() for t in raw_tags if str(t).strip()] + if normalized: + tags[url] = normalized🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/infrastructure/push/apprise_notifier.py` around lines 118 - 120, The code treats ch.get("tags") as iterable and when a scalar string like "daily" is provided it iterates characters; change the normalization before the list comprehension in the block that sets tags[url] so scalar values become a single-element list. Specifically, for the variable ch_tags (and the assignment to tags[url]) detect str and non-iterable scalars (e.g., isinstance(ch_tags, str) or not isinstance(ch_tags, (list, tuple, set))) and wrap them as [ch_tags], then run the existing [str(t).strip() for t in ch_tags if str(t).strip()] to populate tags[url].
🧹 Nitpick comments (9)
src/paperbot/application/workflows/analysis/judge_prompts.py (1)
19-47: Validateupvotestype before prompt interpolation.Current logic accepts any non-
Nonevalue; constraining to non-negative integers avoids malformed prompt context from unexpected payloads.♻️ Proposed hardening
- upvotes = paper.get("upvotes") + upvotes = paper.get("upvotes") + if not isinstance(upvotes, int) or upvotes < 0: + upvotes = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/application/workflows/analysis/judge_prompts.py` around lines 19 - 47, The prompt currently includes Community Upvotes whenever upvotes is not None which can inject malformed values; update the logic around the upvotes variable (in the prompt construction in judge_prompts.py) to only include the upvotes line when upvotes is an integer and >= 0 (or coerce/parse a numeric string to an int first), otherwise omit it; locate the upvotes reference near the prompt return and replace the non-None check with an explicit type-and-range check (e.g., isinstance(upvotes, int) and upvotes >= 0) or a safe parse-and-validate step before interpolating into the prompt.src/paperbot/application/workflows/dailypaper.py (1)
427-429: Harden digest tag rendering against non-string values.If
digest_card.tagscontains non-string values,joincan fail or produce noisy output; normalize before rendering.💡 Proposed fix
- tags = digest_card.get("tags") or [] - if tags: - lines.append(f" - Tags: {', '.join(tags)}") + tags = digest_card.get("tags") or [] + safe_tags = [str(tag).strip() for tag in tags if str(tag).strip()] + if safe_tags: + lines.append(f" - Tags: {', '.join(safe_tags)}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/application/workflows/dailypaper.py` around lines 427 - 429, The digest tag rendering uses tags = digest_card.get("tags") or [] and then ', '.join(tags) which can fail on non-string items; update the logic around the tags variable (in the block that builds lines and calls lines.append) to normalize and filter tags before joining: coerce each tag to string (or skip None/empty) and filter out non-scalar/unexpected types so the join always receives a list of strings, then use ', '.join(normalized_tags) when calling lines.append.config/push_channels.yaml (1)
31-32: 避免在示例里固化user:pass@...形式。即使是注释示例，也会鼓励把 SMTP 凭据直接写进配置文件；建议改为明显的占位符并强调走环境变量注入。
💡 Proposed wording tweak
- # - url: "mailto://user:pass@smtp.example.com?to=recipient@example.com" + # - url: "mailto://<SMTP_USER>:<SMTP_PASS>@smtp.example.com?to=recipient@example.com" + # # 推荐：通过环境变量注入凭据，避免将明文密钥提交到仓库🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/push_channels.yaml` around lines 31 - 32, Replace the hardcoded credential pattern "user:pass@..." in the commented SMTP example in push_channels.yaml with explicit placeholders (e.g., "<SMTP_USER>" and "<SMTP_PASS>") and add a short comment next to the example (or update the example URL) instructing users to inject credentials via environment variables rather than embedding them in the config; modify the commented example URL and adjacent text so it clearly shows placeholders and an env-var usage recommendation.tests/unit/test_hf_daily_adapter.py (1)
25-26: Rename unused fake-connector argument to avoid lint warning.
limitis intentionally unused in the stub, so underscore-prefixing keeps the intent explicit.Suggested cleanup
- def get_daily(self, *, limit: int): + def get_daily(self, *, _limit: int): return [self._record("2602.99999", "Daily Result")]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_hf_daily_adapter.py` around lines 25 - 26, Rename the unused parameter in the test stub get_daily from limit to _limit so the intent that it is unused is explicit and linter warnings are avoided; update the function signature def get_daily(self, *, _limit: int) while keeping the body that returns [self._record("2602.99999", "Daily Result")] unchanged.src/paperbot/infrastructure/extractors/__init__.py (1)
1-3: Sort exported names consistently in import and__all__.This avoids
RUF022and keeps re-exports stable for lint/format tooling.Suggested cleanup
-from paperbot.infrastructure.extractors.mineru_client import MineruClient, Figure +from paperbot.infrastructure.extractors.mineru_client import Figure, MineruClient -__all__ = ["MineruClient", "Figure"] +__all__ = ["Figure", "MineruClient"]As per coding guidelines "Use isort with Black profile for Python import sorting".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/infrastructure/extractors/__init__.py` around lines 1 - 3, The exported names are not consistently sorted: update the import and the __all__ to use a stable, alphabetical order (swap to "Figure", "MineruClient") so both the from-import list and __all__ match and pass import-sorting checks (e.g., RUF022); run isort with the Black profile or apply your project import formatting to ensure the change is canonical across the module.tests/unit/test_hf_daily_papers_connector.py (1)
102-103: Silence unused-argument lint noise in local HTTP stubs.These stub signatures trigger
ARG001. Prefix intentionally unused parameters with_to keep tests clean and avoid warning churn.Suggested cleanup
- def _fake_get(url, params, headers, timeout): + def _fake_get(_url, params, _headers, _timeout): calls.append({"url": url, "params": dict(params)}) return _DummyResponse( @@ - def _fake_get(url, params, headers, timeout): + def _fake_get(_url, params, _headers, _timeout): return _DummyResponse(payload if int(params.get("p", 0)) == 0 else []) @@ - def _fake_get(url, params, headers, timeout): + def _fake_get(_url, _params, _headers, _timeout): raise _Boom("timeout")Also applies to: 148-149, 167-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_hf_daily_papers_connector.py` around lines 102 - 103, The test HTTP stub function _fake_get (and the other local stub functions around the file) declare parameters that remain unused, triggering ARG001; update those signatures to prefix intentionally unused params with an underscore (e.g., change headers -> _headers and timeout -> _timeout) so the callsites and body remain unchanged (keep using url and params as before) and similarly apply the same renaming to the other stub functions referenced in the file.src/paperbot/infrastructure/queue/arq_worker.py (1)
223-224: Redundantint()conversion.
figures_max_itemsis already anintfrom line 205. Theint()call inmax(1, int(figures_max_items))is unnecessary.Simplify to remove redundant conversion
- enable_figures=enable_figures, - figures_max_items=max(1, int(figures_max_items)), + enable_figures=enable_figures, + figures_max_items=max(1, figures_max_items),Similarly at line 236:
- "figures_max_items": max(1, int(figures_max_items)), + "figures_max_items": max(1, figures_max_items),Also applies to: 235-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/infrastructure/queue/arq_worker.py` around lines 223 - 224, The call to int() around figures_max_items is redundant because figures_max_items is already an int; update the two call sites where you pass figures_max_items (the arguments building enable_figures and figures_max_items in arq_worker.py, and the similar call at the other site referenced) to use max(1, figures_max_items) instead of max(1, int(figures_max_items)) so you remove the unnecessary conversion while preserving the minimum-1 guard.tests/unit/test_push_commands_route.py (1)
61-91: Consider asserting deduplication behavior explicitly.The test creates duplicate "FlashKV" entries but only asserts that "FlashKV" appears in the reply. Consider adding an explicit count check to verify deduplication works (e.g., ensuring "FlashKV" appears only once).
Add explicit deduplication assertion
assert "Today's picks:" in reply assert "- FlashKV" in reply assert "- GraphRAG" in reply + # Verify deduplication: FlashKV should appear only once despite duplicates in report + assert reply.count("- FlashKV") == 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_push_commands_route.py` around lines 61 - 91, The test test_telegram_today_with_report_titles should assert deduplication explicitly: after making the request via _post_command against api_main.app and reading reply, add a check that the entry "- FlashKV" appears exactly once (e.g., count occurrences of the substring "- FlashKV" == 1) to ensure push_commands_route's dedup behavior (which reads reports from _REPORTS_DIR) is verified; update the test body to include this additional assertion immediately after the existing presence checks.tests/unit/test_push_formatters.py (1)
86-94: Add regression coverage for thelarkalias.Line 86-Line 94 validate
feishu, but the registry also exposeslarkas a compatibility key. Addassert isinstance(get_formatter("lark"), FeishuFormatter)to prevent alias regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_push_formatters.py` around lines 86 - 94, The test test_get_formatter_returns_correct_type is missing coverage for the compatibility alias "lark"; update that test to assert that get_formatter("lark") returns a FeishuFormatter (i.e., add assert isinstance(get_formatter("lark"), FeishuFormatter)) so the registry's alias mapping for Feishu is protected from regressions and clearly reference get_formatter and FeishuFormatter when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/push_channels.yaml`:
- Around line 9-12: The current "channels: []" inline empty list breaks YAML
when users later uncomment the example "- url: ..." entries; change the
`channels` value from an inline empty list to a block key (replace `channels:
[]` with `channels:`) and keep the example entries as commented block sequence
items (e.g. comment lines beginning with `# - url: "tgram://bot_token/chat_id"`
and `# tags: ["daily", "telegram"]`) indented under `channels:` so
uncommenting will produce a valid YAML list.
In `@docs/archive/issues/190-settings-push-channel-ui.md`:
- Around line 18-33: Update the spec to require redaction/masking of secrets
contained in credential URLs across the UI, API, and logs: when rendering
channel cards, Quick Presets, Add Channel inputs, Test Push payloads, and any
saved channel data, ensure tokens/credentials in URLs (e.g.,
tgram://bot_token/chat_id, discord://webhook_id/webhook_token,
lark://app_id/app_secret, wecom://key, slack://token_a/token_b/token_c/#channel,
mailto://user:pass@smtp/to, dingtalk://token) are masked (partial or replaced
with ****) in all user-facing displays and non-secure logs; specify storage must
encrypt full secrets at rest and only expose masked versions via APIs, and
require explicit developers to use masked fields in UI components "channel
card", "Add Channel", "Quick Presets", "Test Push" and logging paths.
In `@scripts/audit_daily_push_reports.py`:
- Around line 28-31: Loops in scripts/audit_daily_push_reports.py assume nested
entries are dicts and call .get() on them (e.g., when computing query_name from
query.get(...) and digest_card = item.get(...) in the for query/item loops);
guard against malformed non-dict rows by checking isinstance(query, dict) before
accessing query.get and isinstance(item, dict) before accessing item.get (skip
or continue for non-dict entries), and apply the same fix to the similar loop
handling top_items later (the block around digest_card usage).
In `@src/paperbot/api/routes/feed.py`:
- Around line 44-50: The loop in feed processing assumes each report "item" is a
dict and calls item.get(...) which can raise AttributeError for malformed
entries; update the iteration that builds papers (the for q in
report.get("queries") and inner for item in q.get("top_items")) to guard each q
and item with isinstance checks (e.g., ensure q is a dict and item is a dict)
before calling item.get or using id(item), skip or log non-dict entries, and
apply the same validation pattern to the other similar block around lines 77-93
so nested field access is only performed on validated dict objects (referencing
the variables q, item, seen, and papers to locate the code).
- Around line 18-19: _FEED_CACHE is an unbounded dict that can grow indefinitely
because keys include request-derived values (e.g., track/keyword endpoints);
replace the raw dict with a bounded cache and sanitize keys: create a fixed-size
LRU (or TTL) cache instance (e.g., using cachetools.LRUCache or
functools.lru_cache wrapper) to back _FEED_CACHE, ensure cache writes/reads in
the functions that populate it (references to _FEED_CACHE in feed route handlers
for track/keyword endpoints) use sanitized, low-cardinality keys (strip or hash
user-supplied query parameters) and add eviction behavior to prevent unlimited
memory growth; ensure the same change is applied to the other usages mentioned
around the second occurrence (the code that currently mutates _FEED_CACHE at the
other site).
- Around line 276-277: The except ImportError handler currently returns a
non-compliant string; replace it with a valid minimal Atom 1.0 XML response
built in the except ImportError block of the feed route (the except ImportError
handler in src/paperbot/api/routes/feed.py) that includes the required feed root
with xmlns="http://www.w3.org/2005/Atom", a <title> (e.g., "Paperbot Feed
(fallback)"), a unique <id>, an <updated> timestamp (use
datetime.utcnow().isoformat() + "Z"), and no <entry> elements so feed readers
accept it; ensure you return that XML string and the appropriate Atom
content-type if the route normally sets it.
In `@src/paperbot/api/routes/push_commands.py`:
- Around line 85-89: The telegram_command endpoint (function telegram_command)
accepts client-provided chat_id and mutates subscriptions via
TelegramSubscriptionStore without verifying request authenticity; add a
shared-secret header check (e.g. X-Telegram-Bot-Api-Secret-Token) at the start
of telegram_command and any related push/telegram handlers that mutate state
(the handlers using TelegramSubscriptionStore and _parse_telegram_command) by
reading a configured secret (env var), comparing it in constant-time, and
returning an HTTP 401/403 if missing or invalid; make the check a small reusable
helper (e.g. verify_telegram_secret or dependency) so the same validation is
applied to the other routes referenced in the review (the subscribe/unsubscribe
handlers around lines 99–121).
In `@src/paperbot/application/services/paper_search_service.py`:
- Around line 191-224: _stable_identity_key currently returns a single preferred
id which misses overlaps; change PaperSearchService._stable_identity_key to
normalize every identity in PaperCandidate.identities using the existing
_normalized logic (handle arxiv version stripping and doi casing) and produce a
canonical collection of all non-empty ids (format each as
"id:{source}:{normalized}"), then return a deterministic representation (e.g., a
sorted list or a sorted, pipe-joined string) instead of a single id so dedup can
match on any shared normalized identity (ensure callers that expect a string are
updated to handle the collection or the new joined-string format).
In `@src/paperbot/infrastructure/connectors/hf_daily_papers_connector.py`:
- Line 313: The code returns datetime.min.replace(tzinfo=timezone.utc) which can
cause out-of-range errors when calling .timestamp() on some platforms; replace
all occurrences (the return expressions that use
datetime.min.replace(tzinfo=timezone.utc) and any code that calls .timestamp()
on that sentinel) with a safe, platform-independent sentinel such as
datetime.fromtimestamp(0, timezone.utc) or
datetime(1970,1,1,tzinfo=timezone.utc), and update any comparisons/sorting logic
that expects a sentinel accordingly so timestamp conversion cannot overflow.
In `@src/paperbot/infrastructure/push/apprise_notifier.py`:
- Around line 104-108: The YAML loader can return non-mapping roots (list/str)
so calling data.get(...) can raise AttributeError; update the code around data =
yaml.safe_load(fh) or {} to validate the type (e.g., if not isinstance(data,
dict): data = {}) before using data.get("channels"), ensuring channels =
data.get("channels") or [] is safe; keep references to config_path, data,
channels and urls when applying the guard so notifier initialization won't fail
on list/string YAML roots.
In `@src/paperbot/infrastructure/push/formatters/base.py`:
- Around line 53-58: The sort key in the papers.sort call (lambda p:
float((p.get("judge") or {}).get("overall") or p.get("score") or 0)) treats 0 as
falsy and will fall back to score, and float(...) can raise on non-numeric
input; update the sort key logic (the lambda used in papers.sort) to 1) prefer
judge["overall"] when it is not None (use an explicit is not None check instead
of truthiness), 2) then fallback to p.get("score") if overall is None, and 3)
coerce values to a safe numeric using a small helper or inline try/except that
returns 0 for non-numeric/malformed inputs so float conversion cannot raise and
zero scores are preserved. Ensure the updated key returns a numeric (float) for
correct reverse sorting.
In `@src/paperbot/infrastructure/push/telegram_subscription_store.py`:
- Around line 33-36: The current read-modify-write to the JSON file in _save
(and the subscription update methods that call it, e.g.,
add_subscription/remove_subscription) is not concurrency-safe; implement an
inter-process exclusive lock around the entire read-modify-write sequence and
perform atomic replace on write: acquire a file lock (flock or a cross-platform
locker) before reading the file, make your modifications in memory, write the
new JSON to a temp file in the same directory, fsync the temp file and directory
as appropriate, then atomically replace the original with os.replace, and
finally release the lock; update _save (and the methods that mutate state) to
use this lock+atomic-replace pattern and ensure the containing directory is
created before creating the temp file.
---
Minor comments:
In `@src/paperbot/api/routes/push_commands.py`:
- Around line 64-82: The broad except in _latest_digest_titles is hiding
file/read/parse errors; replace the bare except with targeted exception handling
for path.read_text and json.loads (e.g., OSError/IOError, json.JSONDecodeError,
TypeError) and log failures using the module logger with the offending path and
exception details (include path, exception message, and optionally stacktrace)
before continuing; keep the existing behavior of skipping that file on error but
do not swallow or silence the error silently.
In `@src/paperbot/application/services/email_template.py`:
- Around line 210-216: Normalize digest_card.get("tags") into a safe list of
strings before rendering: replace the current tags = digest_card.get("tags") or
[] with logic that ensures tags is a list (if a scalar, wrap it), then map each
entry to str(...).strip() and filter out empty values, e.g. normalized_tags =
[str(t).strip() for t in (digest_card.get("tags") or [])] or if not iterable
wrap single values into a list; use normalized_tags[:6] everywhere (the HTML
tag_pills generation and the text join) so both the HTML branch that builds
tag_pills and the plain-text join path operate on the same validated list of
strings.
In `@src/paperbot/application/workflows/dailypaper.py`:
- Around line 30-31: The early return in extract_figures_for_report returns the
original report when api_key is falsy, causing inconsistent copy semantics
versus the normal path which returns a deep-copied object; change the
early-return to return a deep copy of report (e.g., use copy.deepcopy(report) or
the same deep-copy helper used later) so callers always receive a detached
object and avoid aliasing, and ensure the copy import/helper is available in the
module.
In `@src/paperbot/infrastructure/push/apprise_notifier.py`:
- Around line 118-120: The code treats ch.get("tags") as iterable and when a
scalar string like "daily" is provided it iterates characters; change the
normalization before the list comprehension in the block that sets tags[url] so
scalar values become a single-element list. Specifically, for the variable
ch_tags (and the assignment to tags[url]) detect str and non-iterable scalars
(e.g., isinstance(ch_tags, str) or not isinstance(ch_tags, (list, tuple, set)))
and wrap them as [ch_tags], then run the existing [str(t).strip() for t in
ch_tags if str(t).strip()] to populate tags[url].
In `@src/paperbot/infrastructure/push/formatters/__init__.py`:
- Around line 34-42: The __all__ list is unsorted and triggers Ruff RUF022; open
the module containing __all__ and alphabetically sort its entries (the strings:
"DiscordFormatter", "FeishuFormatter", "Get_formatter" should be "get_formatter"
keep case exact, "List_formatters" likewise preserve names) — specifically
reorder the existing entries ("PushFormatter", "TelegramFormatter",
"DiscordFormatter", "WeComFormatter", "FeishuFormatter", "get_formatter",
"list_formatters") into a stable, alphabetical sequence (e.g.,
"DiscordFormatter", "FeishuFormatter", "PushFormatter", "TelegramFormatter",
"WeComFormatter", "get_formatter", "list_formatters") so Ruff RUF022 is
satisfied.
- Around line 21-24: get_formatter currently calls channel_type.lower()
unconditionally which raises AttributeError for None or non-string inputs;
update get_formatter to defensively validate/coerce channel_type (e.g., check
isinstance(channel_type, str) or use str(channel_type).lower()) before looking
up _REGISTRY so non-string/None returns None instead of raising; reference the
get_formatter function and the _REGISTRY lookup and ensure the lowered key is
derived only after the type check.
In `@src/paperbot/infrastructure/push/formatters/wecom.py`:
- Line 41: Remove the unnecessary f-string prefix on the static string passed to
lines.append to fix Ruff F541: locate the lines.append call that adds "**📖
本期导读**" (the lines.append(f"**📖 本期导读**") invocation) and change it to a plain
string literal without the leading f so it becomes lines.append("**📖 本期导读**");
no other logic should change.
In `@src/paperbot/infrastructure/push/telegram_subscription_store.py`:
- Around line 25-31: The _load method currently swallows all exceptions around
json.loads(self._path.read_text(...)) which can hide real filesystem errors;
change the broad except Exception to handle only expected cases: catch
FileNotFoundError (and PermissionError) when the file is missing/unreadable and
return {}, catch json.JSONDecodeError to return {} (and log the parse error with
the path), and let any other exceptions propagate (or re-raise) so real issues
aren’t masked; include the file path and exception details in the log messages
when catching these specific exceptions to aid debugging.
In `@src/paperbot/workflows/feed.py`:
- Around line 445-466: Add assertions to the tests for export_feed_entries in
test_scholar_feed_service_export to cover the transformation contract: create
events with both paper.url and metadata['url'] to assert metadata['url']
overrides paper.url (link precedence), assert published equals
event.timestamp.isoformat() (ISO formatted string), and assert categories is a
single-element list containing event.event_type.value; locate behavior in
export_feed_entries (uses event.paper, getattr(event.paper, "url"),
event.metadata.get("url"), and event.timestamp.isoformat()) and add targeted
assertions for those fields.
In `@tests/unit/test_daily_digest_card.py`:
- Around line 50-63: Several fake methods in the test stub trigger Ruff ARG002
because parameters are intentionally unused; rename those unused parameters to
start with an underscore to silence the warning. Specifically: in
summarize_paper keep title (it’s used) but in analyze_trends rename papers to
_papers, in assess_relevance rename paper to _paper, in generate_daily_insight
rename report to _report, and in extract_daily_digest_card rename title/abstract
to _title/_abstract (or any leading-underscore variants) so the function
signatures retain types but Ruff no longer flags unused arguments.
---
Nitpick comments:
In `@config/push_channels.yaml`:
- Around line 31-32: Replace the hardcoded credential pattern "user:pass@..." in
the commented SMTP example in push_channels.yaml with explicit placeholders
(e.g., "<SMTP_USER>" and "<SMTP_PASS>") and add a short comment next to the
example (or update the example URL) instructing users to inject credentials via
environment variables rather than embedding them in the config; modify the
commented example URL and adjacent text so it clearly shows placeholders and an
env-var usage recommendation.
In `@src/paperbot/application/workflows/analysis/judge_prompts.py`:
- Around line 19-47: The prompt currently includes Community Upvotes whenever
upvotes is not None which can inject malformed values; update the logic around
the upvotes variable (in the prompt construction in judge_prompts.py) to only
include the upvotes line when upvotes is an integer and >= 0 (or coerce/parse a
numeric string to an int first), otherwise omit it; locate the upvotes reference
near the prompt return and replace the non-None check with an explicit
type-and-range check (e.g., isinstance(upvotes, int) and upvotes >= 0) or a safe
parse-and-validate step before interpolating into the prompt.
In `@src/paperbot/application/workflows/dailypaper.py`:
- Around line 427-429: The digest tag rendering uses tags =
digest_card.get("tags") or [] and then ', '.join(tags) which can fail on
non-string items; update the logic around the tags variable (in the block that
builds lines and calls lines.append) to normalize and filter tags before
joining: coerce each tag to string (or skip None/empty) and filter out
non-scalar/unexpected types so the join always receives a list of strings, then
use ', '.join(normalized_tags) when calling lines.append.
In `@src/paperbot/infrastructure/extractors/__init__.py`:
- Around line 1-3: The exported names are not consistently sorted: update the
import and the __all__ to use a stable, alphabetical order (swap to "Figure",
"MineruClient") so both the from-import list and __all__ match and pass
import-sorting checks (e.g., RUF022); run isort with the Black profile or apply
your project import formatting to ensure the change is canonical across the
module.
In `@src/paperbot/infrastructure/queue/arq_worker.py`:
- Around line 223-224: The call to int() around figures_max_items is redundant
because figures_max_items is already an int; update the two call sites where you
pass figures_max_items (the arguments building enable_figures and
figures_max_items in arq_worker.py, and the similar call at the other site
referenced) to use max(1, figures_max_items) instead of max(1,
int(figures_max_items)) so you remove the unnecessary conversion while
preserving the minimum-1 guard.
In `@tests/unit/test_hf_daily_adapter.py`:
- Around line 25-26: Rename the unused parameter in the test stub get_daily from
limit to _limit so the intent that it is unused is explicit and linter warnings
are avoided; update the function signature def get_daily(self, *, _limit: int)
while keeping the body that returns [self._record("2602.99999", "Daily Result")]
unchanged.
In `@tests/unit/test_hf_daily_papers_connector.py`:
- Around line 102-103: The test HTTP stub function _fake_get (and the other
local stub functions around the file) declare parameters that remain unused,
triggering ARG001; update those signatures to prefix intentionally unused params
with an underscore (e.g., change headers -> _headers and timeout -> _timeout) so
the callsites and body remain unchanged (keep using url and params as before)
and similarly apply the same renaming to the other stub functions referenced in
the file.
In `@tests/unit/test_push_commands_route.py`:
- Around line 61-91: The test test_telegram_today_with_report_titles should
assert deduplication explicitly: after making the request via _post_command
against api_main.app and reading reply, add a check that the entry "- FlashKV"
appears exactly once (e.g., count occurrences of the substring "- FlashKV" == 1)
to ensure push_commands_route's dedup behavior (which reads reports from
_REPORTS_DIR) is verified; update the test body to include this additional
assertion immediately after the existing presence checks.
In `@tests/unit/test_push_formatters.py`:
- Around line 86-94: The test test_get_formatter_returns_correct_type is missing
coverage for the compatibility alias "lark"; update that test to assert that
get_formatter("lark") returns a FeishuFormatter (i.e., add assert
isinstance(get_formatter("lark"), FeishuFormatter)) so the registry's alias
mapping for Feishu is protected from regressions and clearly reference
get_formatter and FeishuFormatter when making the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (54)
README.mdconfig/push_channels.yamldocs/AGENTSWARM_TODO.mddocs/HF_DAILY_SOURCE.mddocs/archive/PROJECT_ISSUE_BACKLOG_legacy.mddocs/archive/README.mddocs/archive/issues/190-settings-push-channel-ui.mddocs/archive/issues/191-dashboard-digest-card-enhancement.mddocs/progress/progress.txtdocs/runbooks/daily_push_ops.mdrequirements.txtscripts/audit_daily_push_reports.pysrc/paperbot/api/main.pysrc/paperbot/api/routes/__init__.pysrc/paperbot/api/routes/feed.pysrc/paperbot/api/routes/push_commands.pysrc/paperbot/application/prompts/paper_analysis.pysrc/paperbot/application/prompts/registry.pysrc/paperbot/application/services/daily_push_service.pysrc/paperbot/application/services/email_template.pysrc/paperbot/application/services/llm_service.pysrc/paperbot/application/services/paper_search_service.pysrc/paperbot/application/workflows/analysis/judge_prompts.pysrc/paperbot/application/workflows/dailypaper.pysrc/paperbot/infrastructure/adapters/hf_daily_adapter.pysrc/paperbot/infrastructure/connectors/hf_daily_papers_connector.pysrc/paperbot/infrastructure/extractors/__init__.pysrc/paperbot/infrastructure/extractors/mineru_client.pysrc/paperbot/infrastructure/push/__init__.pysrc/paperbot/infrastructure/push/apprise_notifier.pysrc/paperbot/infrastructure/push/formatters/__init__.pysrc/paperbot/infrastructure/push/formatters/base.pysrc/paperbot/infrastructure/push/formatters/discord.pysrc/paperbot/infrastructure/push/formatters/feishu.pysrc/paperbot/infrastructure/push/formatters/telegram.pysrc/paperbot/infrastructure/push/formatters/wecom.pysrc/paperbot/infrastructure/push/telegram_subscription_store.pysrc/paperbot/infrastructure/queue/arq_worker.pysrc/paperbot/presentation/cli/main.pysrc/paperbot/workflows/feed.pytests/unit/test_apprise_notifier.pytests/unit/test_arq_daily_papers.pytests/unit/test_audit_daily_push_reports.pytests/unit/test_daily_digest_card.pytests/unit/test_feed_routes.pytests/unit/test_hf_daily_adapter.pytests/unit/test_hf_daily_papers_connector.pytests/unit/test_hf_upvote_judge_integration.pytests/unit/test_mineru_client.pytests/unit/test_paper_search_service_rrf.pytests/unit/test_push_channel_e2e.pytests/unit/test_push_commands_route.pytests/unit/test_push_formatters.pyweb/README.md
Comment on lines
+9
to
+12
| channels: [] | ||
| # ── Telegram ── | ||
| # - url: "tgram://bot_token/chat_id" | ||
| # tags: ["daily", "telegram"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if file exists and read relevant section
if [ -f "config/push_channels.yaml" ]; then
echo "=== File found, reading lines 1-20 ==="
head -20 "config/push_channels.yaml"
else
echo "File not found at config/push_channels.yaml"
find . -name "push_channels.yaml" 2>/dev/null
fiRepository: jerry609/PaperBot
Length of output: 702
🏁 Script executed:
#!/bin/bash
python3 << 'EOF'
import yaml
# Test case 1: Current structure (inline empty list + block items indented)
current_structure = """channels: []
# ── Telegram ──
# - url: "tgram://bot_token/chat_id"
tags: ["daily", "telegram"]
"""
# Test case 2: Proposed structure (block format)
proposed_structure = """channels:
# ── Telegram ──
# - url: "tgram://bot_token/chat_id"
# tags: ["daily", "telegram"]
"""
# Test case 3: What happens when user uncomments under current structure
uncommented_current = """channels: []
- url: "tgram://bot_token/chat_id"
tags: ["daily", "telegram"]
"""
# Test case 4: What happens when user uncomments under proposed structure
uncommented_proposed = """channels:
- url: "tgram://bot_token/chat_id"
tags: ["daily", "telegram"]
"""
print("=" * 50)
print("TEST 1: Current structure (with comments)")
try:
result = yaml.safe_load(current_structure)
print(f"✓ VALID YAML: {result}")
except Exception as e:
print(f"✗ INVALID YAML: {type(e).__name__}: {e}")
print("\n" + "=" * 50)
print("TEST 2: Proposed structure (with comments)")
try:
result = yaml.safe_load(proposed_structure)
print(f"✓ VALID YAML: {result}")
except Exception as e:
print(f"✗ INVALID YAML: {type(e).__name__}: {e}")
print("\n" + "=" * 50)
print("TEST 3: User uncomments under CURRENT structure")
try:
result = yaml.safe_load(uncommented_current)
print(f"✓ VALID YAML: {result}")
except Exception as e:
print(f"✗ INVALID YAML: {type(e).__name__}: {e}")
print("\n" + "=" * 50)
print("TEST 4: User uncomments under PROPOSED structure")
try:
result = yaml.safe_load(uncommented_proposed)
print(f"✓ VALID YAML: {result}")
except Exception as e:
print(f"✗ INVALID YAML: {type(e).__name__}: {e}")
EOFRepository: python_verify
Length of output: 1808
🏁 Script executed:
#!/bin/bash
python3 << 'EOF'
import yaml
# Test case 1: Current structure (inline empty list + block items indented)
current_structure = """channels: []
# ── Telegram ──
# - url: "tgram://bot_token/chat_id"
# tags: ["daily", "telegram"]
"""
# Test case 2: Proposed structure (block format)
proposed_structure = """channels:
# ── Telegram ──
# - url: "tgram://bot_token/chat_id"
# tags: ["daily", "telegram"]
"""
# Test case 3: What happens when user uncomments under current structure
uncommented_current = """channels: []
- url: "tgram://bot_token/chat_id"
tags: ["daily", "telegram"]
"""
# Test case 4: What happens when user uncomments under proposed structure
uncommented_proposed = """channels:
- url: "tgram://bot_token/chat_id"
tags: ["daily", "telegram"]
"""
print("=" * 50)
print("TEST 1: Current structure (with comments)")
try:
result = yaml.safe_load(current_structure)
print(f"✓ VALID YAML: {result}")
except Exception as e:
print(f"✗ INVALID YAML: {type(e).__name__}: {e}")
print("\n" + "=" * 50)
print("TEST 2: Proposed structure (with comments)")
try:
result = yaml.safe_load(proposed_structure)
print(f"✓ VALID YAML: {result}")
except Exception as e:
print(f"✗ INVALID YAML: {type(e).__name__}: {e}")
print("\n" + "=" * 50)
print("TEST 3: User uncomments under CURRENT structure")
try:
result = yaml.safe_load(uncommented_current)
print(f"✓ VALID YAML: {result}")
except Exception as e:
print(f"✗ INVALID YAML: {type(e).__name__}: {e}")
print("\n" + "=" * 50)
print("TEST 4: User uncomments under PROPOSED structure")
try:
result = yaml.safe_load(uncommented_proposed)
print(f"✓ VALID YAML: {result}")
except Exception as e:
print(f"✗ INVALID YAML: {type(e).__name__}: {e}")
EOFRepository: jerry609/PaperBot
Length of output: 885
channels: [] template structure causes invalid YAML when users uncomment list items.
When users uncomment the block sequence items (- url: ...) under channels: [], YAML parsing fails because you cannot mix an inline empty list with indented block sequence markers. The proposed fix changes to block format syntax, which correctly accepts both commented and uncommented entries.
Proposed fix
-channels: []
+channels:
# ── Telegram ──
# - url: "tgram://bot_token/chat_id"
# tags: ["daily", "telegram"]📝 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.
Suggested change
| channels: [] | |
| # ── Telegram ── | |
| # - url: "tgram://bot_token/chat_id" | |
| # tags: ["daily", "telegram"] | |
| channels: | |
| # ── Telegram ── | |
| # - url: "tgram://bot_token/chat_id" | |
| # tags: ["daily", "telegram"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/push_channels.yaml` around lines 9 - 12, The current "channels: []"
inline empty list breaks YAML when users later uncomment the example "- url:
..." entries; change the `channels` value from an inline empty list to a block
key (replace `channels: []` with `channels:`) and keep the example entries as
commented block sequence items (e.g. comment lines beginning with `# - url:
"tgram://bot_token/chat_id"` and `# tags: ["daily", "telegram"]`) indented
under `channels:` so uncommenting will produce a valid YAML list.
Comment on lines
+18
to
+33
| - 每个通道卡片显示： | ||
| - 渠道类型图标（Telegram/Discord/Lark/WeCom/Slack/Email/DingTalk） | ||
| - 连接状态（URL 是否有效） | ||
| - 标签（daily / alert 等） | ||
| - 编辑 / 删除按钮 | ||
| - "Add Channel" 按钮 + Quick Presets（类似 Model Providers 的预设） | ||
| - Telegram Bot → `tgram://bot_token/chat_id` | ||
| - Discord Webhook → `discord://webhook_id/webhook_token` | ||
| - Lark/Feishu Webhook → `lark://app_id/app_secret` | ||
| - WeCom Webhook → `wecom://key` | ||
| - Slack Webhook → `slack://token_a/token_b/token_c/#channel` | ||
| - Email SMTP → `mailto://user:pass@smtp/to` | ||
| - DingTalk Webhook → `dingtalk://token` | ||
| - "Test Push" 按钮：向选定通道发送测试消息 | ||
| - 标签管理：选择该通道接收哪些类型的推送（daily / alert / all） | ||
|
|
There was a problem hiding this comment.
Add explicit secret-masking requirements for credential URLs.
The presets include credential/token-bearing URLs. Without a masking/redaction requirement, UI/API/logging can leak secrets.
🔐 Suggested spec update
- 每个通道卡片显示：
- 渠道类型图标（Telegram/Discord/Lark/WeCom/Slack/Email/DingTalk）
- 连接状态（URL 是否有效）
+ - URL 仅显示脱敏版本（token/password 必须掩码）
- 标签（daily / alert 等）
- 编辑 / 删除按钮
@@
## Acceptance Criteria
@@
- [ ] 用户可以通过 UI 添加/编辑/删除推送通道
+- [ ] 所有包含密钥/口令的 URL 在 UI、日志、错误信息中均脱敏展示
+- [ ] 后端持久化前进行字段级加密或至少 secret 分离存储（避免明文落库/落盘）
- [ ] 测试推送功能正常工作
- [ ] RSS 订阅地址可一键复制
- [ ] 与现有 YAML 配置双向兼容Also applies to: 55-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/archive/issues/190-settings-push-channel-ui.md` around lines 18 - 33,
Update the spec to require redaction/masking of secrets contained in credential
URLs across the UI, API, and logs: when rendering channel cards, Quick Presets,
Add Channel inputs, Test Push payloads, and any saved channel data, ensure
tokens/credentials in URLs (e.g., tgram://bot_token/chat_id,
discord://webhook_id/webhook_token, lark://app_id/app_secret, wecom://key,
slack://token_a/token_b/token_c/#channel, mailto://user:pass@smtp/to,
dingtalk://token) are masked (partial or replaced with ****) in all user-facing
displays and non-secure logs; specify storage must encrypt full secrets at rest
and only expose masked versions via APIs, and require explicit developers to use
masked fields in UI components "channel card", "Add Channel", "Quick Presets",
"Test Push" and logging paths.
Comment on lines
+28
to
+31
| for query in report.get("queries") or []: | ||
| query_name = str(query.get("normalized_query") or query.get("raw_query") or "").strip() | ||
| for item in query.get("top_items") or []: | ||
| digest_card = item.get("digest_card") if isinstance(item, dict) else {} |
There was a problem hiding this comment.
Guard malformed queries/top_items entries before calling .get().
Line 29 and Lines 39-45 assume dict-like rows. One malformed nested row can raise AttributeError and abort the audit run.
🛠️ Proposed fix
def _iter_items(report: Dict[str, Any], report_name: str) -> List[SampleRow]:
rows: List[SampleRow] = []
for query in report.get("queries") or []:
+ if not isinstance(query, dict):
+ continue
query_name = str(query.get("normalized_query") or query.get("raw_query") or "").strip()
for item in query.get("top_items") or []:
- digest_card = item.get("digest_card") if isinstance(item, dict) else {}
+ if not isinstance(item, dict):
+ continue
+ digest_card = item.get("digest_card")
if not isinstance(digest_card, dict):
digest_card = {}Also applies to: 39-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/audit_daily_push_reports.py` around lines 28 - 31, Loops in
scripts/audit_daily_push_reports.py assume nested entries are dicts and call
.get() on them (e.g., when computing query_name from query.get(...) and
digest_card = item.get(...) in the for query/item loops); guard against
malformed non-dict rows by checking isinstance(query, dict) before accessing
query.get and isinstance(item, dict) before accessing item.get (skip or continue
for non-dict entries), and apply the same fix to the similar loop handling
top_items later (the block around digest_card usage).
Comment on lines
+18
to
+19
| _FEED_CACHE: Dict[str, Dict[str, Any]] = {} | ||
|
|
There was a problem hiding this comment.
Bound _FEED_CACHE growth to avoid memory blow-up.
_FEED_CACHE is unbounded, and cache keys include request-derived values (track/keyword endpoints). High-cardinality requests can permanently grow process memory.
🛠️ Proposed fix
+from collections import OrderedDict
@@
-_FEED_CACHE: Dict[str, Dict[str, Any]] = {}
+_FEED_CACHE_MAX_ENTRIES = 256
+_FEED_CACHE: "OrderedDict[str, Dict[str, Any]]" = OrderedDict()
@@
def _store_cached_xml(cache_key: str, reports_dir: Path, xml: str) -> str:
- _FEED_CACHE[cache_key] = {"signature": _reports_signature(reports_dir), "xml": xml}
+ _FEED_CACHE.pop(cache_key, None)
+ _FEED_CACHE[cache_key] = {"signature": _reports_signature(reports_dir), "xml": xml}
+ while len(_FEED_CACHE) > _FEED_CACHE_MAX_ENTRIES:
+ _FEED_CACHE.popitem(last=False)
return xmlAlso applies to: 134-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/api/routes/feed.py` around lines 18 - 19, _FEED_CACHE is an
unbounded dict that can grow indefinitely because keys include request-derived
values (e.g., track/keyword endpoints); replace the raw dict with a bounded
cache and sanitize keys: create a fixed-size LRU (or TTL) cache instance (e.g.,
using cachetools.LRUCache or functools.lru_cache wrapper) to back _FEED_CACHE,
ensure cache writes/reads in the functions that populate it (references to
_FEED_CACHE in feed route handlers for track/keyword endpoints) use sanitized,
low-cardinality keys (strip or hash user-supplied query parameters) and add
eviction behavior to prevent unlimited memory growth; ensure the same change is
applied to the other usages mentioned around the second occurrence (the code
that currently mutates _FEED_CACHE at the other site).
Comment on lines
+44
to
+50
| for q in report.get("queries") or []: | ||
| for item in q.get("top_items") or []: | ||
| key = item.get("title") or id(item) | ||
| if key not in seen: | ||
| seen.add(key) | ||
| papers.append(item) | ||
| return papers |
There was a problem hiding this comment.
Validate report item types before nested field access.
Line 46 and Line 82+ assume item is a dict. Malformed report entries can raise AttributeError and fail feed requests.
🛠️ Proposed fix
def _collect_papers_from_report(report: Dict[str, Any]) -> List[Dict[str, Any]]:
@@
for q in report.get("queries") or []:
+ if not isinstance(q, dict):
+ continue
for item in q.get("top_items") or []:
+ if not isinstance(item, dict):
+ continue
key = item.get("title") or id(item)
@@
def _paper_matches_keyword(item: Dict[str, Any], keyword: str) -> bool:
+ if not isinstance(item, dict):
+ return False
needle = (keyword or "").strip().lower()Also applies to: 77-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/api/routes/feed.py` around lines 44 - 50, The loop in feed
processing assumes each report "item" is a dict and calls item.get(...) which
can raise AttributeError for malformed entries; update the iteration that builds
papers (the for q in report.get("queries") and inner for item in
q.get("top_items")) to guard each q and item with isinstance checks (e.g.,
ensure q is a dict and item is a dict) before calling item.get or using
id(item), skip or log non-dict entries, and apply the same validation pattern to
the other similar block around lines 77-93 so nested field access is only
performed on validated dict objects (referencing the variables q, item, seen,
and papers to locate the code).
Comment on lines
+191
to
+224
| identity_key = PaperSearchService._stable_identity_key(paper) | ||
| if identity_key: | ||
| return identity_key | ||
| if paper.title_hash: | ||
| return paper.title_hash | ||
| normalized = _TOKEN_SEP_RX.sub(" ", (paper.title or "").strip().lower()) | ||
| return hashlib.sha256(normalized.encode("utf-8")).hexdigest() | ||
|
|
||
| @staticmethod | ||
| def _stable_identity_key(paper: PaperCandidate) -> str: | ||
| identities = paper.identities or [] | ||
| if not identities: | ||
| return "" | ||
|
|
||
| def _normalized(source: str, external_id: str) -> str: | ||
| value = (external_id or "").strip().lower() | ||
| if source == "arxiv": | ||
| value = value.removeprefix("arxiv:") | ||
| if "v" in value: | ||
| head, tail = value.rsplit("v", 1) | ||
| if head and tail.isdigit(): | ||
| value = head | ||
| if source == "doi": | ||
| value = value.strip().lower() | ||
| return value | ||
|
|
||
| for source in ("doi", "arxiv", "openalex", "semantic_scholar", "hf_daily", "papers_cool"): | ||
| for ident in identities: | ||
| if ident.source != source: | ||
| continue | ||
| normalized = _normalized(source, ident.external_id) | ||
| if normalized: | ||
| return f"id:{source}:{normalized}" | ||
| return "" |
There was a problem hiding this comment.
Single preferred identity key can miss valid duplicate merges.
Line 217–Line 224 returns only one identity key by source priority. That breaks dedup when papers overlap on a non-selected identity (e.g., one record has doi+arxiv, another has only arxiv), producing different keys for the same paper.
Please switch dedup matching to use any shared normalized identity (alias map / union step) rather than a single preferred identity key.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/application/services/paper_search_service.py` around lines 191 -
224, _stable_identity_key currently returns a single preferred id which misses
overlaps; change PaperSearchService._stable_identity_key to normalize every
identity in PaperCandidate.identities using the existing _normalized logic
(handle arxiv version stripping and doi casing) and produce a canonical
collection of all non-empty ids (format each as "id:{source}:{normalized}"),
then return a deterministic representation (e.g., a sorted list or a sorted,
pipe-joined string) instead of a single id so dedup can match on any shared
normalized identity (ensure callers that expect a string are updated to handle
the collection or the new joined-string format).
| text = (value or "").strip() | ||
| if not text: | ||
| return datetime.min | ||
| return datetime.min.replace(tzinfo=timezone.utc) |
There was a problem hiding this comment.
Avoid datetime.min as a sortable sentinel.
Line 313 and Line 321 return datetime.min, and Line 342 converts it with .timestamp(). That can throw on some platforms for out-of-range epoch conversion, which can crash sorting in "new" mode when dates are missing/invalid.
In Python 3.10, can `datetime.min.replace(tzinfo=timezone.utc).timestamp()` raise range/overflow errors on some platforms (for example Windows)?
🛠️ Proposed fix
_ARXIV_ID_RE = re.compile(r"^(\d{4}\.\d{4,5})(v\d+)?$")
_TRENDING_MODES = {"hot", "rising", "new"}
+_MIN_SORT_DATE = datetime(1970, 1, 1, tzinfo=timezone.utc)
@@
def _date_sort_key(value: str) -> datetime:
text = (value or "").strip()
if not text:
- return datetime.min.replace(tzinfo=timezone.utc)
+ return _MIN_SORT_DATE
@@
except ValueError:
- return datetime.min.replace(tzinfo=timezone.utc)
+ return _MIN_SORT_DATEAlso applies to: 321-322, 342-343
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/infrastructure/connectors/hf_daily_papers_connector.py` at line
313, The code returns datetime.min.replace(tzinfo=timezone.utc) which can cause
out-of-range errors when calling .timestamp() on some platforms; replace all
occurrences (the return expressions that use
datetime.min.replace(tzinfo=timezone.utc) and any code that calls .timestamp()
on that sentinel) with a safe, platform-independent sentinel such as
datetime.fromtimestamp(0, timezone.utc) or
datetime(1970,1,1,tzinfo=timezone.utc), and update any comparisons/sorting logic
that expects a sentinel accordingly so timestamp conversion cannot overflow.
Comment on lines
+104
to
+108
| with open(config_path, encoding="utf-8") as fh: | ||
| data = yaml.safe_load(fh) or {} | ||
|
|
||
| channels = data.get("channels") or [] | ||
| urls: List[str] = [] |
There was a problem hiding this comment.
Handle non-mapping YAML roots before calling .get().
If the YAML root is a list/string, Line 107 raises AttributeError and prevents notifier initialization.
🛠️ Proposed fix
with open(config_path, encoding="utf-8") as fh:
- data = yaml.safe_load(fh) or {}
+ loaded = yaml.safe_load(fh)
+ data = loaded if isinstance(loaded, dict) else {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/infrastructure/push/apprise_notifier.py` around lines 104 - 108,
The YAML loader can return non-mapping roots (list/str) so calling data.get(...)
can raise AttributeError; update the code around data = yaml.safe_load(fh) or {}
to validate the type (e.g., if not isinstance(data, dict): data = {}) before
using data.get("channels"), ensuring channels = data.get("channels") or [] is
safe; keep references to config_path, data, channels and urls when applying the
guard so notifier initialization won't fail on list/string YAML roots.
Comment on lines
+53
to
+58
| papers.sort( | ||
| key=lambda p: float( | ||
| (p.get("judge") or {}).get("overall") or p.get("score") or 0 | ||
| ), | ||
| reverse=True, | ||
| ) |
There was a problem hiding this comment.
Sorting key can mis-rank papers and crash on malformed scores.
Line 55 uses overall or score, so overall == 0 incorrectly falls back to score; float(...) can also raise on non-numeric input and break digest formatting.
Proposed fix
- papers.sort(
- key=lambda p: float(
- (p.get("judge") or {}).get("overall") or p.get("score") or 0
- ),
- reverse=True,
- )
+ def _score(item: Dict[str, Any]) -> float:
+ judge = item.get("judge") or {}
+ raw_score = judge.get("overall")
+ if raw_score is None:
+ raw_score = item.get("score", 0)
+ try:
+ return float(raw_score)
+ except (TypeError, ValueError):
+ return 0.0
+
+ papers.sort(key=_score, reverse=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/infrastructure/push/formatters/base.py` around lines 53 - 58,
The sort key in the papers.sort call (lambda p: float((p.get("judge") or
{}).get("overall") or p.get("score") or 0)) treats 0 as falsy and will fall back
to score, and float(...) can raise on non-numeric input; update the sort key
logic (the lambda used in papers.sort) to 1) prefer judge["overall"] when it is
not None (use an explicit is not None check instead of truthiness), 2) then
fallback to p.get("score") if overall is None, and 3) coerce values to a safe
numeric using a small helper or inline try/except that returns 0 for
non-numeric/malformed inputs so float conversion cannot raise and zero scores
are preserved. Ensure the updated key returns a numeric (float) for correct
reverse sorting.
Comment on lines
+33
to
+36
| def _save(self, payload: Dict[str, Dict[str, List[str] | str]]) -> None: | ||
| self._path.parent.mkdir(parents=True, exist_ok=True) | ||
| self._path.write_text(json.dumps(payload, ensure_ascii=False, indent=2), encoding="utf-8") | ||
|
|
There was a problem hiding this comment.
Read-modify-write is not concurrency-safe; updates can be lost.
Line 55-Line 66 and Line 75-Line 89 mutate a shared JSON file without synchronization, and Line 35 writes in place. Concurrent requests can overwrite each other or leave corrupted state.
Proposed fix (lock + atomic replace)
import json
import os
+import threading
from datetime import datetime, timezone
from pathlib import Path
from typing import Dict, List
@@
class TelegramSubscriptionStore:
@@
def __init__(self, path: str | None = None):
configured = path or os.getenv("PAPERBOT_TELEGRAM_SUBS_PATH", "data/telegram_subscriptions.json")
self._path = Path(configured).expanduser()
+ self._lock = threading.Lock()
@@
def _save(self, payload: Dict[str, Dict[str, List[str] | str]]) -> None:
self._path.parent.mkdir(parents=True, exist_ok=True)
- self._path.write_text(json.dumps(payload, ensure_ascii=False, indent=2), encoding="utf-8")
+ tmp_path = self._path.with_suffix(f"{self._path.suffix}.tmp")
+ tmp_path.write_text(json.dumps(payload, ensure_ascii=False, indent=2), encoding="utf-8")
+ tmp_path.replace(self._path)
@@
def subscribe(self, *, chat_id: str, value: str, scope: str = "keyword") -> Dict[str, List[str]]:
@@
- payload = self._load()
- row = payload.setdefault(
- normalized_chat,
- {"keyword": [], "track": [], "updated_at": _utc_now_iso()},
- )
- values = row.setdefault(normalized_scope, [])
- if isinstance(values, list) and normalized_value not in values:
- values.append(normalized_value)
- values.sort()
- row["updated_at"] = _utc_now_iso()
- self._save(payload)
+ with self._lock:
+ payload = self._load()
+ row = payload.setdefault(
+ normalized_chat,
+ {"keyword": [], "track": [], "updated_at": _utc_now_iso()},
+ )
+ values = row.setdefault(normalized_scope, [])
+ if isinstance(values, list) and normalized_value not in values:
+ values.append(normalized_value)
+ values.sort()
+ row["updated_at"] = _utc_now_iso()
+ self._save(payload)
return self.list_subscriptions(chat_id=normalized_chat)
@@
- payload = self._load()
- row = payload.get(normalized_chat) or {}
- values = row.get(normalized_scope)
- if not isinstance(values, list):
- values = []
-
- if normalized_value:
- values = [item for item in values if item != normalized_value]
- else:
- values = []
- row[normalized_scope] = values
- row["updated_at"] = _utc_now_iso()
- payload[normalized_chat] = row
- self._save(payload)
+ with self._lock:
+ payload = self._load()
+ row = payload.get(normalized_chat) or {}
+ values = row.get(normalized_scope)
+ if not isinstance(values, list):
+ values = []
+
+ if normalized_value:
+ values = [item for item in values if item != normalized_value]
+ else:
+ values = []
+ row[normalized_scope] = values
+ row["updated_at"] = _utc_now_iso()
+ payload[normalized_chat] = row
+ self._save(payload)
return self.list_subscriptions(chat_id=normalized_chat)Also applies to: 55-66, 75-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/infrastructure/push/telegram_subscription_store.py` around lines
33 - 36, The current read-modify-write to the JSON file in _save (and the
subscription update methods that call it, e.g.,
add_subscription/remove_subscription) is not concurrency-safe; implement an
inter-process exclusive lock around the entire read-modify-write sequence and
perform atomic replace on write: acquire a file lock (flock or a cross-platform
locker) before reading the file, make your modifications in memory, write the
new JSON to a temp file in the same directory, fsync the temp file and directory
as appropriate, then atomically replace the original with os.replace, and
finally release the lock; update _save (and the methods that mutate state) to
use this lock+atomic-replace pattern and ensure the containing directory is
created before creating the temp file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation