Conversation
…extraction Related to #157
P0 fixes: - Replace unbounded daemon threads with ThreadPoolExecutor(max_workers=2) for embedding writes; atexit + close() ensure graceful shutdown so in-flight embeddings are not lost on process exit - Restore apprise>=1.9.0 and feedgen>=1.0.0 removed in epic branch, which would have broken Epic #179 push/RSS features on merge P1 fixes: - _escape_fts: replace double-quote-only escaping with a whitelist regex [A-Za-z0-9_+-] so FTS5 operators (*, NEAR, NOT, ^) cannot alter query semantics; empty queries now short-circuit to [] - _hybrid_merge: skip items with None/invalid id instead of defaulting to id=0, preventing silent score collisions across unrelated records - ReproExperienceStore: add application-level dedup check + UNIQUE constraint (paper_id, pattern_type, content) + IntegrityError fallback to prevent duplicate experiences from accumulating across retries Migration: - 0022_repro_experience_dedup: adds uq_repro_exp_paper_type_content unique constraint to existing repro_code_experience table Tests: 47 unit tests pass (+3 new tests covering the fixes above)
…o pipeline Add user-scoped isolation for repro_code_experience and enforce dedup semantics with migration 0022_repro_experience_dedup. Inject ReproExperienceStore through ReproAgent/Orchestrator/CodingAgent/GenerationNode and propagate user_id/pack_id in generation, verification, and debugging persistence paths. Update /api/gen-code to accept user_id and extend unit tests for user isolation and persistence behavior.
Add decay-aware scoring that combines relevance (confidence), recency (exponential decay with 90-day half-life), and usage frequency to re-rank search results. New memories default to expires_at = created_at + 365 days. search_memories() now auto-touches usage on hits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add search_memories_batch() that queries multiple scope_ids in a single SQL call, eliminating the N+1 loop in build_context_pack(). Engine now uses this batch method for cross-track memory retrieval. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Refactor build_context_pack() into 4 layer methods: - Layer 0: user profile (cached with 5-min TTL, ~200 tokens) - Layer 1: track context — tasks/milestones (~500 tokens) - Layer 2: query-relevant + cross-track memories (~1000 tokens) - Layer 3: paper-scoped memories (on-demand) Return value adds context_layers metadata while remaining fully backward-compatible. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use standard ln(2)/halfLifeDays lambda formula (matches OpenClaw temporal-decay.ts:toDecayLambda) - Default half-life lowered to 30 days (was 90, now matches OpenClaw) - Evergreen memories (global scope, preference kind) are immune to recency decay (inspired by OpenClaw isEvergreenMemoryPath) - Add _to_decay_lambda() and _is_evergreen_memory() helpers - Expand tests for lambda math, half-life precision, and evergreen logic Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts: # .github/workflows/ci.yml
…uite # Conflicts: # .github/workflows/ci.yml # evals/memory/__init__.py
# Conflicts: # evals/memory/__init__.py # src/paperbot/memory/eval/__init__.py
* docs: update README with macOS python3 tips * docs: update README with more badges * docs: update README with deleting extra badges * docs: update README with new god name --------- Co-authored-by: 林杰 <linjie@linjiedeMacBook-Air.local> Co-authored-by: 林杰 <linjie@v8d1ef64c.dip.tu-dresden.de> Co-authored-by: 林杰 <linjie@v8d1ef43e.dip.tu-dresden.de> Co-authored-by: 林杰 <linjie@v8d1ef6c5.dip.tu-dresden.de>
* docs: update README with macOS python3 tips * docs: update README with more badges * docs: update README with deleting extra badges * docs: update README with new god name --------- Co-authored-by: 林杰 <linjie@linjiedeMacBook-Air.local> Co-authored-by: 林杰 <linjie@v8d1ef64c.dip.tu-dresden.de> Co-authored-by: 林杰 <linjie@v8d1ef43e.dip.tu-dresden.de> Co-authored-by: 林杰 <linjie@v8d1ef6c5.dip.tu-dresden.de>
Add comprehensive memory module evaluation aligned with LongMemEval (ICLR 2025), LoCoMo (ACL 2024), Mem0, and Letta benchmarks. - Retrieval Bench v2: IR metrics (Recall@5=0.873, MRR@10=0.731, nDCG@10=0.747) with 40 annotated queries across 5 question types and 5 memory dimensions - Scope Isolation + CRUD: zero-leak verification across user x scope matrix, Mem0-aligned CRUD lifecycle (add/update/delete/dedup) - Context Extraction: L0-L3 layer completeness, precision, token budget guard, TrackRouter accuracy (100%), graceful degradation - Injection Robustness L1: offline pattern detection (0% pollution, 0% FP) - Fixture dataset: 45 memories (2 users), 12 injection patterns - Testing documentation with methodology, validity analysis, and results Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (98)
📝 WalkthroughWalkthroughThis PR introduces comprehensive MemoryBench evaluation suites (retrieval, effectiveness, ROI, injection robustness), integrates Paper-to-Context (P2C) layered context loading with token-guarding, adds vector embeddings with sqlite-vec support, implements verification runtime caching, extends repro agents with LLM and experience persistence, and enhances memory store with FTS5 full-text search, batch operations, decay ranking, and MMR reranking. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ReproAgent
participant Orchestrator
participant CodingAgent
participant ExperienceStore
participant MemoryStore
participant LLMService
User->>ReproAgent: reproduce_from_paper(paper, user_id, pack_id)
ReproAgent->>Orchestrator: run(paper_context, user_id, pack_id)
Orchestrator->>CodingAgent: execute()
CodingAgent->>ExperienceStore: load_experiences_from_db(paper_id, user_id)
ExperienceStore->>MemoryStore: query prior experiences
MemoryStore-->>ExperienceStore: prior patterns
ExperienceStore-->>CodingAgent: loaded experiences
CodingAgent->>LLMService: generate code with memory context
LLMService-->>CodingAgent: generated code
CodingAgent->>ExperienceStore: record_success_pattern(code_snippet)
ExperienceStore->>MemoryStore: persist pattern
CodingAgent-->>Orchestrator: AgentResult
Orchestrator-->>ReproAgent: pipeline result
ReproAgent-->>User: final code + metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
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 advances the project's memory and code reproduction capabilities by introducing a robust evaluation suite and enhancing the underlying memory storage and retrieval mechanisms. It focuses on improving the reliability, safety, and efficiency of LLM-driven code generation through structured benchmarking, persistent learning from past experiences, and resilient API interactions. The changes lay a strong foundation for more intelligent and autonomous research workflows. 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
Ignored Files
Activity
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
|
![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 is an impressive and substantial pull request that introduces a comprehensive MemoryBench evaluation suite and significantly enhances the memory system's capabilities. The addition of FTS5 and vector search with hybrid ranking, decay scoring, and MMR is a major step forward. The implementation of isolated verification runtimes, persistence for code generation experiences, and more resilient LLM provider logic are all high-quality contributions that improve the robustness and intelligence of the system. The new documentation and tests are thorough and well-written. I have one medium-severity comment regarding a potential issue with database downgrades on SQLite.
Note: Security Review did not run due to the size of the PR.
| type_="unique", | ||
| ) | ||
| op.drop_index("ix_repro_code_experience_user_id", table_name="repro_code_experience") | ||
| op.drop_column("repro_code_experience", "user_id") |
There was a problem hiding this comment.
The op.drop_column operation is not supported by SQLite's ALTER TABLE implementation. This will cause the downgrade to fail on SQLite environments. A previous migration (0020_memory_embedding.py) correctly noted this limitation. To ensure the downgrade path is safe for all supported dialects, you could either add a comment here acknowledging that this downgrade is not supported on SQLite, or implement the 'move-and-copy' table recreation workflow for SQLite if full downgrade compatibility is required.
There was a problem hiding this comment.
Pull request overview
This PR implements the MemoryBench evaluation suite (Epic #283) for PaperBot, introducing comprehensive offline benchmarking across retrieval quality, context extraction, scope isolation, injection robustness, and performance. It also includes significant supporting infrastructure: CodeMemory persistence via a new ReproExperienceStore, FTS5 + vector embedding search for the memory store, configurable verification runtime isolation, LLM provider retry/fallback logic, and improved error handling in the frontend.
Changes:
- New MemoryBench evaluation suite: 4 eval suites (
test_retrieval_bench,test_scope_isolation,test_context_extraction,test_injection_robustness) plus ROI and effectiveness benchmarks, all offline - Infrastructure:
ReproExperienceStorefor CodeMemory persistence, FTS5 + sqlite-vec hybrid search,HashEmbeddingProviderfallback,VerificationAgentruntime isolation with pip caching, LLM provider retry logic for system-role rejection and transient errors, andContextEngineBridgefor user-memory injection into P2C pipeline - Frontend: Improved error message extraction (
e instanceof Error ? e.message : String(e)) and user-friendly upstream error messages, with duplicated helper functions across 3 components
Reviewed changes
Copilot reviewed 97 out of 98 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
web/src/components/research/ResearchPageNew.tsx |
Adds friendly upstream error parsing, improves error message extraction |
web/src/components/research/ResearchDiscoveryPage.tsx |
Same error handling improvements (duplicate logic) |
web/src/components/research/ResearchDashboard.tsx |
Same error handling improvements (duplicate logic) |
src/paperbot/repro/nodes/planning_node.py |
Refactors _execute to use new _complete_prompt helper supporting both llm_client and legacy query fallbacks |
src/paperbot/repro/memory/code_memory.py |
Adds ReproExperienceStore persistence methods and prior-experience context injection |
src/paperbot/infrastructure/stores/repro_experience_store.py |
New CRUD store for code generation experience persistence |
src/paperbot/infrastructure/stores/models.py |
Adds ReproCodeExperienceModel, embedding column to MemoryItemModel |
src/paperbot/repro/agents/verification_agent.py |
Adds configurable Python executable, requirements preparation, and runtime caching |
src/paperbot/repro/orchestrator.py |
Threads experience_store, llm_client, user_id, pack_id, and verification config through the pipeline |
src/paperbot/infrastructure/llm/providers/openai_provider.py |
Adds system-role fallback, transient retry logic, time import |
src/paperbot/application/services/llm_service.py |
Fixes cost estimation to handle provider-prefixed model names (e.g., openrouter/openai/gpt-4o-mini) |
src/paperbot/application/services/p2c/stages.py |
Passes user_memory/project_context to LLM prompts, narrows exception types |
src/paperbot/application/services/p2c/prompts.py |
Adds _sanitize_tag_content, injects user_memory and project_context blocks |
src/paperbot/application/services/p2c/context_bridge.py |
New: enriches NormalizedInput with user memory/project context from ContextEngine |
src/paperbot/application/services/p2c/orchestrator.py |
Integrates ContextEngineBridge into P2C orchestration |
src/paperbot/application/services/p2c/models.py |
Adds user_memory and project_context fields to NormalizedInput |
src/paperbot/memory/eval/injection_guard.py |
New: offline prompt-injection pattern detection with Unicode normalization |
src/paperbot/memory/eval/performance_benchmark.py |
New: multi-scale memory search latency benchmark |
src/paperbot/memory/eval/collector.py |
Adds record_scope_isolation and cross_user/scope_leak_rate metrics |
src/paperbot/memory/eval/__init__.py |
Expands public API for all new benchmark modules |
src/paperbot/context_engine/embeddings.py |
Adds HashEmbeddingProvider and configurable provider chain |
src/paperbot/api/routes/repro_context.py |
Writes P2C observations as paper-scoped memory items |
src/paperbot/api/routes/gen_code.py |
Passes user_id to code generation pipeline |
alembic/versions/0019_memory_fts5.py |
New migration: FTS5 virtual table and sync triggers |
alembic/versions/0020_memory_embedding.py |
New migration: embedding BLOB column and sqlite-vec virtual table |
alembic/versions/0021_repro_code_experience.py |
New migration: repro_code_experience table |
alembic/versions/0022_repro_experience_dedup.py |
New migration: user_id column and dedup unique constraint |
tests/unit/test_*.py (multiple) |
Comprehensive unit tests for new infrastructure and benchmarks |
evals/memory/test_injection_robustness.py |
Injection robustness eval test |
evals/, scripts/, docs/ (multiple) |
Eval runners, CLI scripts, and documentation |
README.md |
Title changed to informal phrase (unintentional), doc table updated |
requirements.txt |
Adds sqlite-vec>=0.1.6 |
Makefile |
New bench-roi target |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| const message = e instanceof Error ? e.message : String(e) | ||
| if (message.startsWith("409")) { | ||
| setCreateError(`Track "${name}" already exists.`) | ||
| } else { |
There was a problem hiding this comment.
The message.startsWith("409") checks on lines 319 and 365 were designed for the old String(e) pattern where the error message starts with the HTTP status code (e.g., "409 Conflict ..."). After the refactor to e instanceof Error ? e.message : String(e), the error message is now set from toFriendlyErrorMessage (which returns a user-facing string that will NOT start with "409") or falls back to the raw status-prefixed message. If a 409 response provides a friendly error via the detail field (which toFriendlyErrorMessage would return), the resulting message would NOT start with "409", causing the track-already-exists duplicate detection to silently fall through to setError(message) instead of showing the specific setCreateError/setEditError duplicate message. The logic for detecting 409 conflicts needs to be updated to work with the new error handling approach.
| if "gpt-4o-mini" in model_alias: | ||
| in_price, out_price = 0.15, 0.60 | ||
| elif provider == "openai" and "gpt-4o" in model: | ||
| elif "gpt-4o" in model_alias: | ||
| in_price, out_price = 2.50, 10.00 | ||
| elif provider == "anthropic" and "claude-3-5-sonnet" in model: | ||
| elif "claude-3-5-sonnet" in model_alias: | ||
| in_price, out_price = 3.00, 15.00 | ||
| elif provider == "deepseek": | ||
| elif "deepseek" in model_alias or provider == "deepseek": | ||
| in_price, out_price = 0.55, 2.19 |
There was a problem hiding this comment.
The _estimate_cost_usd function now uses model_alias (which strips the provider prefix, e.g. "openai/gpt-4o-mini" → "gpt-4o-mini") to match pricing rules for GPT-4o, GPT-4o-mini, and Claude. However, the gpt-4o-mini check (line 409) comes before the gpt-4o check (line 411), so a model string like "gpt-4o-mini" or "openai/gpt-4o-mini" will correctly match the first branch. But a model like "gpt-4o" would also partially match "gpt-4o-mini" if the model name were e.g. "gpt-4o-mini-2024". This was already an issue in the previous code and is not introduced by this PR. More importantly, the removal of provider == "anthropic" guard for claude-3-5-sonnet on line 413 could lead to over-broad matching — any provider that returns a model containing "claude-3-5-sonnet" will now apply Anthropic pricing. This is probably intentional (similar to the OpenRouter prefix-stripping pattern), but it means cost estimates for Claude models routed through OpenRouter will now be costed instead of falling through to 0.0. This is actually a bug fix / intentional change consistent with the gpt-4o-mini fix, so this note is informational.
| type UpstreamErrorBody = { | ||
| detail?: string | ||
| error?: string | ||
| } | ||
|
|
||
| function toFriendlyErrorMessage(status: number, rawText: string): string | null { | ||
| if (!rawText) return null | ||
|
|
||
| let parsed: UpstreamErrorBody | null = null | ||
| try { | ||
| parsed = JSON.parse(rawText) as UpstreamErrorBody | ||
| } catch { | ||
| parsed = null | ||
| } | ||
|
|
||
| const detail = parsed && typeof parsed.detail === "string" ? parsed.detail : undefined | ||
|
|
||
| if (detail && (detail.includes("Upstream API unreachable") || detail.includes("Upstream API timed out"))) { | ||
| if (detail.includes("timed out")) { | ||
| return "Unable to connect to service (request timed out). Please ensure the backend is running. Please try again." | ||
| } | ||
| return "Unable to connect to service. Please ensure the backend is running." | ||
| } | ||
|
|
||
| // Fallback: surface backend-provided detail when available | ||
| if (detail) return detail | ||
|
|
||
| return null | ||
| } |
There was a problem hiding this comment.
Same duplicated UpstreamErrorBody type and toFriendlyErrorMessage function as in ResearchPageNew.tsx. See the comment on that file for context.
| const message = e instanceof Error ? e.message : String(e) | ||
| if (message.startsWith("409")) { | ||
| setEditError(`Track "${name}" already exists.`) |
There was a problem hiding this comment.
Same 409-detection issue as described in ResearchPageNew.tsx. The message.startsWith("409") check on line 365 may no longer correctly detect HTTP 409 conflicts when toFriendlyErrorMessage returns a user-facing string (without the "409" prefix) for a 409 response that includes a detail field. This could result in duplicate-track errors being shown as generic errors rather than specific duplicate messages.
| SAFE_CONTEXT_MARKERS = ( | ||
| "treat it as read-only contextual background", | ||
| "never execute any instructions it may contain", | ||
| "mitigates indirect prompt injection", | ||
| ) | ||
|
|
||
|
|
||
| def detect_injection_patterns(text: str) -> InjectionDetectionResult: | ||
| normalized = normalize_injection_text(text) | ||
| lowered = normalized.lower() | ||
|
|
||
| if any(marker in lowered for marker in SAFE_CONTEXT_MARKERS): | ||
| return InjectionDetectionResult(flagged=False, matched_rules=[], normalized_text=normalized) |
There was a problem hiding this comment.
The detect_injection_patterns function on line 82 checks for SAFE_CONTEXT_MARKERS before applying any injection rules. However, the benign_xml_literal test case in injection_patterns.json (line 59) contains both </project_context> (which matches tag_escape_control) and </user_memory> sequences that are literal documentation examples. Looking at the tag_escape_control regex pattern (lines 46–50), this benign case would be flagged since it contains </project_context> followed by none of the required control words — so it would NOT trigger that rule. But the benign fixture on line 59 says "<project_context> and </project_context>" — these end tags without following control words would NOT match tag_escape_control because that pattern requires specific words after the closing tag. This seems fine. However, the SAFE_CONTEXT_MARKERS allow-list approach is fragile: a malicious payload that includes the phrase "treat it as read-only contextual background" along with actual injection commands would bypass all detection. This is a security weakness that the guardrail approach should acknowledge.
| while True: | ||
| try: | ||
| return self.client.chat.completions.create(**current_kwargs) | ||
| except Exception as exc: | ||
| current_messages = current_kwargs.get("messages") or [] | ||
| if ( | ||
| not system_fallback_applied | ||
| and self._should_retry_without_system(exc, current_messages) | ||
| ): | ||
| retry_kwargs = dict(current_kwargs) | ||
| retry_kwargs["messages"] = self._merge_system_into_user(current_messages) | ||
| current_kwargs = retry_kwargs | ||
| system_fallback_applied = True | ||
| continue | ||
| if self._should_retry_transient(exc) and attempt < self.max_transient_retries: | ||
| delay = self.retry_backoff_sec * (2 ** attempt) | ||
| logger.warning( | ||
| "Transient LLM request failed provider=%s model=%s attempt=%s error=%s", | ||
| self._provider_name, | ||
| self.model_name, | ||
| attempt + 1, | ||
| exc, | ||
| ) | ||
| if delay > 0: | ||
| time.sleep(delay) | ||
| attempt += 1 | ||
| continue | ||
| raise |
There was a problem hiding this comment.
The _should_retry_transient method on line 168 matches against the string "timeout" in str(exc).lower(). This is an overly broad match: it would match any exception message containing the word "timeout" regardless of context (e.g., "connection timeout" in an unrelated exception). Additionally, the _create_completion_with_fallback loop on line 139 checks _should_retry_transient and increments attempt only after that check, but system_fallback_applied is checked first with a continue that does NOT increment attempt. This means the transient retry counter (attempt) is unaffected by the system fallback path, which is correct. However, since system_fallback_applied is checked first and can cause a continue that bypasses the transient check, a request that needs both a system fallback AND hits a transient error after the fallback would only get max_transient_retries attempts, which is the expected behavior.
More critically: if _should_retry_without_system returns True and _should_retry_transient also returns True for the same exception, only the system fallback is applied (because the system check comes first with continue). This could cause a missed transient retry for an error that is both a system rejection AND a transient error (unlikely but possible).
| def stream_invoke(self, messages: List[Dict[str, str]], **kwargs) -> Generator[str, None, None]: | ||
| """流式调用""" | ||
| extra_params = { | ||
| k: v for k, v in kwargs.items() | ||
| if k in self.ALLOWED_PARAMS and v is not None | ||
| k: v for k, v in kwargs.items() if k in self.ALLOWED_PARAMS and v is not None | ||
| } | ||
| extra_params["stream"] = True | ||
|
|
||
| timeout = extra_params.pop("timeout", self.timeout) | ||
|
|
||
| try: | ||
| stream = self.client.chat.completions.create( | ||
| model=self.model_name, | ||
| stream = self._create_completion_with_fallback( | ||
| messages=messages, | ||
| timeout=timeout, | ||
| **extra_params, | ||
| extra_params=extra_params, | ||
| stream=True, | ||
| ) |
There was a problem hiding this comment.
The stream_invoke method on line 258 passes extra_params to _create_completion_with_fallback, but extra_params was already modified to include "stream": True on line 263. However, the _create_completion_with_fallback method also receives stream=True as a separate kwarg on line 272 and uses it to conditionally set request_kwargs["stream"] = True (line 119). This means stream=True appears twice in the kwargs dict built in _create_completion_with_fallback — once from extra_params (since stream is not in ALLOWED_PARAMS, it would have been filtered out) — wait, stream is not in ALLOWED_PARAMS, so it will NOT be included in extra_params. But it IS explicitly set to extra_params["stream"] = True on line 263 before timeout is popped. This means stream=True WILL be in extra_params when passed to _create_completion_with_fallback, AND _create_completion_with_fallback also adds stream=True from its stream parameter on line 119. This results in stream=True being set twice in request_kwargs, which while not harmful (the second set just overwrites the first), is unexpected and could be confusing. The stream key in extra_params should be removed before calling _create_completion_with_fallback, or stream should not be added to extra_params in stream_invoke.
| type UpstreamErrorBody = { | ||
| detail?: string | ||
| error?: string | ||
| } | ||
|
|
||
| function toFriendlyErrorMessage(status: number, rawText: string): string | null { | ||
| if (!rawText) return null | ||
|
|
||
| let parsed: UpstreamErrorBody | null = null | ||
| try { | ||
| parsed = JSON.parse(rawText) as UpstreamErrorBody | ||
| } catch { | ||
| parsed = null | ||
| } | ||
|
|
||
| const detail = parsed && typeof parsed.detail === "string" ? parsed.detail : undefined | ||
|
|
||
| if (detail && (detail.includes("Upstream API unreachable") || detail.includes("Upstream API timed out"))) { | ||
| if (detail.includes("timed out")) { | ||
| return "Unable to connect to service (request timed out). Please ensure the backend is running. Please try again." | ||
| } | ||
| return "Unable to connect to service. Please ensure the backend is running." | ||
| } | ||
|
|
||
| // Fallback: surface backend-provided detail when available | ||
| if (detail) return detail | ||
|
|
||
| return null | ||
| } |
There was a problem hiding this comment.
Same duplicated UpstreamErrorBody type and toFriendlyErrorMessage function as in ResearchPageNew.tsx. See the comment on that file for context.
| | [`docs/PAPERSCOOL_WORKFLOW.md`](docs/PAPERSCOOL_WORKFLOW.md) | Topic Workflow guide | | ||
| | [`docs/p2c/`](docs/p2c/) | Paper2Context design docs | | ||
| | [`docs/benchmark/MEMORYBENCH_EPIC_283_COMPLETION.md`](docs/benchmark/MEMORYBENCH_EPIC_283_COMPLETION.md) | MemoryBench Epic completion report | | ||
| | [`docs/benchmark/MEMORYBENCH_RUNTIME_REPORT_2026-03-07.md`](docs/benchmark/MEMORYBENCH_RUNTIME_REPORT_2026-03-07.md) | Live ROI + 1M memory runtime report | |
There was a problem hiding this comment.
The MEMORYBENCH_EPIC_283_COMPLETION.md completion report links to docs/benchmark/MEMORYBENCH_RUNTIME_REPORT_2026-03-07.md in the README (line 240), but this file does not appear to be included in the PR diff. The README references a non-existent documentation file, which would result in a broken link.
| if existing is not None: | ||
| if pack_id and not existing.pack_id: | ||
| existing.pack_id = pack_id | ||
| session.commit() | ||
| session.refresh(existing) | ||
| return existing |
There was a problem hiding this comment.
In repro_experience_store.py, the deduplication logic on line 61 only updates an existing record's pack_id if pack_id is provided AND the existing record has no pack_id. However, this means if a record already has a pack_id and a new call is made with a different pack_id, the existing pack_id will NOT be updated (the condition not existing.pack_id prevents it). This could lead to stale pack_id associations if the same experience is referenced from a new pack. The behavior is intentional per the docstring (dedup by user_id + paper_id + pattern_type + content), but it may cause unexpected behavior in the ROI benchmark where the same experience seed is loaded multiple times with different pack contexts.
Summary
Test plan
PYTHONPATH=src pytest -q evals/memory/test_retrieval_bench.py— PASSPYTHONPATH=src pytest -q evals/memory/test_scope_isolation.py— PASSPYTHONPATH=src pytest -q evals/memory/test_context_extraction.py— PASSPYTHONPATH=src pytest -q evals/memory/test_injection_robustness.py— PASS🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
Improvements