Fix handle_before_tool dict-truthy bug + MCP response size limit - #2
Merged
Conversation
Three fixes for Zero integration:
1. handle_before_tool: brain.recall() returns a dict, which is ALWAYS
truthy. Changed to check result.get('merged', []) so the hook only
reports when there are actual recalled items. Also fixed len() to
count merged results instead of dict keys (always 3).
2. handle_after_tool: Consolidated to create a single Brain() instance
instead of two (one for file edits, one for tool observations).
3. as_brain MCP server: Added 50KB response size limit to ok() helper.
Without this, brain_recall returns the full per_layer dict which can
grow very large with accumulated facts, causing provider context
overflow and 'provider agent crashes' in Zero.
Fixes: https://github.com/PatrickNoFilter/eling/issues
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe MCP server truncates oversized JSON tool results. The hook reads recalled items from ChangesRuntime behavior updates
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
notion.available property and _has_httpx() both called _require_httpx() which raises RuntimeError when httpx is not installed. Default recall layers now include 'notion' (main branch changed from ['builtin', 'facts', 'kb', 'code'] to include 'notion'). This means any call to brain.recall() without explicit layers triggers notion.available, which crashed the benchmark job when httpx is absent. Fix: wrap _require_httpx() in try/except so available=property returns False instead of raising. Same fix for _has_httpx(). Fixes benchmark CI failure for PR #2.
The try/except around _require_httpx() assigns _HAS_HTTPX, but without a 'global' declaration Python treats it as a local variable, causing UnboundLocalError. Add global declarations to available and _has_httpx.
The 'Comment benchmark on PR' step uses GITHUB_TOKEN to call issues.createComment, which requires pull-requests: write scope. Without it, runs fail with 'HttpError: Resource not accessible by integration'.
⏱ Eling Benchmark
|
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
1 participant
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
Three fixes found while diagnosing a Zero+Eling crash where
search facts(viabrain_recallMCP tool) caused the provider agent to crash in Zero.Fix 1 —
handle_before_toolfalse positive (eling-hook.py)brain.recall()returns adict(always truthy), soif results:was always True. The hook reportedlen(results)= 3 (always, because that's the number of dict keys), even when zero facts matched. Fixed to checkresults.get("merged", [])and report correct count.Fix 2 —
handle_after_tooldouble Brain init (eling-hook.py)Created two separate
Brain()instances per hook call (file edits + observations). Consolidated to a single shared instance.Fix 3 — MCP response size limit (as_brain/mcp_server.py)
brain_recallreturns the entireper_layerdict with raw results from every layer. With accumulated facts this JSON response can grow very large, causing provider context overflow and agent crash. Added a 50KB cap to theok()serializer.Found while debugging Zero + eling integration: https://github.com/PatrickNoFilter/eling
Summary by CodeRabbit