Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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 introduces tiered context summaries (L0/L1) to workspace documents, enhancing search capabilities and memory tool output. It includes automatic summary generation, schema migrations for database compatibility, and extensive testing to ensure functionality and reliability. The changes aim to improve search result relevance and provide more structured context within the workspace. Highlights
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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces tiered context summaries (L0/L1) for workspace documents, enhancing search functionality and document previews. The database schema is updated to include summary_l0 (one-line abstract) and summary_l1 (structured overview) columns. Document updates now trigger asynchronous background generation of these summaries using an LLM, with a backfill process initiated on application startup. Search results can now specify a detail level (L0, L1, or raw content) for the returned content, defaulting to L1. Review comments suggest improving error handling for prompt injection checks in summary generation, considering a retry mechanism for LLM failures, logging warnings when summaries are unexpectedly missing, and potentially triggering synchronous summary updates in the database layer for immediate consistency.
zmanian
left a comment
There was a problem hiding this comment.
Code Review: feat(workspace): add tiered context summaries
Verdict: REQUEST_CHANGES
Solid feature design with clean L0/L1/L2 tiering model. Dual-backend support is properly implemented (PostgreSQL + libSQL migrations, trait method, both implementations). Prompt injection defense via reject_if_injected is good. Three issues need addressing.
Database Dual-Backend Compliance: PASS
Both backends properly handled -- V14 migration for both, update_document_summaries in trait and both impls, column indices shifted correctly in libSQL, summaries NULLed on content update.
Must Fix
1. No content truncation before LLM call
generate_document_summaries passes entire document content to the LLM with no size limit. A 500KB document will blow the context window or incur massive token costs. with_max_tokens(1024) only limits output tokens.
Fix: Truncate content to a reasonable limit (e.g., first ~32KB) before sending to the LLM.
2. Unbounded backfill on startup
backfill_summaries loads ALL documents into memory and iterates sequentially with LLM calls. For workspaces with hundreds of documents, this hammers the LLM endpoint at startup with no concurrency limit, rate limiting, or batch cap.
Fix: Add a configurable batch limit (e.g., max N documents per startup) and/or a concurrency semaphore.
3. Error variant misuse
Summary generation/persistence/JSON parse failures all map to WorkspaceError::SearchFailed. This produces confusing error messages and logs.
Fix: Add WorkspaceError::SummaryGenerationFailed { reason: String } in src/error.rs following the thiserror convention.
Suggestions (non-blocking)
-
Redundant data in tool output --
memory_searchreturnscontent(tier-selected) plussummary_l0andsummary_l1as separate fields. Consider omitting raw summary fields for the selected tier. -
Double word-count check --
schedule_summary_refreshandrefresh_document_summariesboth checkSUMMARY_MIN_WORDS. Defensive but redundant. -
extract_json_objectfragility -- outer-brace matching (find('{')/rfind('}')) could pick wrong boundaries with nested braces. Acceptable for now given prompt design andserde_jsonvalidation. -
No retry on LLM failure -- summary silently lost until next content update. Consider marking documents as "needs summary" for the next backfill.
30af8d6 to
252d141
Compare
Code Review —
|
|
Thank you for exploring tiered L0/L1 summaries to reduce memory-search context cost.\n\nWe are closing this legacy implementation because Reborn now retrieves bounded, scope-checked, sanitized memory snippets through the MemoryService/native provider boundary. It does not currently persist L0/L1 summaries, and we do not want to carry over the v1 database schema and summary-generation lifecycle without benchmark evidence that the existing Reborn retrieval path needs another persisted tier.\n\nReferences:\n- https://github.com/nearai/ironclaw/blob/main/crates/ironclaw_memory/src/service.rs\n- https://github.com/nearai/ironclaw/blob/main/crates/ironclaw_memory_native/src/service.rs\n- https://github.com/nearai/ironclaw/pull/5327\n\nIf Reborn memory benchmarks later show context-quality or token-cost problems, tiered summaries can be reconsidered as a provider-owned design with explicit invalidation and cost policy. This is an architecture-transition closure, not a reflection on the quality of your work. |
Implements the first slice of #1473.
What changed:
Validation:
Note: