fix(gbrain): cap fallback enrichment graph size (BLO-21635) - #1070
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
🔗 Paperclip issue: BLO-21635 |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 311dcec
Critical Issues (0)
Important Issues (1)
- [pr-review-toolkit/gstack]
packages/plugins/paperclip-plugin-gbrain/src/recall.ts:198— The fallback payload is not fully bounded across graph shapes that this module accepts.capEnrichmentGraphleaves an accepted edges-only graph unchanged, and for{ nodes, edges }records it spreads every other top-level property unchanged while retaining every edge between kept nodes. An oversized{ edges: [...] },pathsfield, or dense edge set can therefore still be merged and persisted at multi-megabyte size despite the stated guarantee that fallback payload size is bounded.- Normalize supported fallback records to a bounded shape and cap/reject edges-only and auxiliary collections (ideally with a serialized-byte budget). Add regressions for an oversized edges-only graph and a dense-edge or oversized auxiliary-property graph.
Suggestions (0)
Strengths
- The cap applies only to opportunistic fallback enrichment, preserving the full issue-scoped traversal.
- Array fallbacks are depth-prioritized without mutating the source response.
- The standard
{ nodes, edges }test verifies that recognized dangling edges are removed after trimming.
Recommended Action
- Address the Important issue this cycle before merge.
…635) The gbrain-context recall prefetch falls back to the agent's or project's hub node when the issue-scoped traversal comes back thin (empty or a single-node island). That hub node can be linked to thousands of unrelated fact-* nodes — observed up to ~23,000 nodes / ~5.8MB decoded JSON on a single "ok" recall, ~96% of ok recalls affected. Cap the fallback graph's contribution to MAX_ENRICHMENT_NODES (50) nodes before merging, preferring nodes structurally closest to the hub (lowest traversal depth) when available, and drop edges that would dangle to trimmed nodes. The issue-scoped traversal is left untouched since it's already relevance-scoped and safe to attach whole. https://paperclip.blockcast.net/BLO/issues/BLO-21635 Co-Authored-By: Paperclip <noreply@paperclip.ing>
311dcec to
0573880
Compare
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 0573880
Looks good. The fallback contribution is now normalized and bounded across the accepted traversal graph shapes.
Prior Findings Dispositioned (1)
- prior:311dcec important 1 — fixed —
packages/plugins/paperclip-plugin-gbrain/src/recall.ts:196—capEnrichmentGraphnow normalizes arrays, node/edge records, and edges-only records; it drops auxiliary collections, limits nodes and edges, enforces the serialized-byte budget, and removes edges to trimmed nodes.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [native-codex]
packages/plugins/paperclip-plugin-gbrain/src/recall.ts:203— Consider collecting eligible edges incrementally and stopping atMAX_ENRICHMENT_EDGESinstead of filtering the full edge array; this would reduce temporary allocation for unusually dense hub traversals.
Strengths
- The full issue-scoped traversal remains intact while only opportunistic fallback enrichment is capped.
- Regression coverage now includes array, nodes/edges, edges-only, dense-edge, auxiliary-field, and serialized-byte cases.
- The byte cap uses UTF-8 byte length and revalidates the combined normalized fallback record.
Recommended Action
- Merge after the repository's required checks are green.
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 0573880
Looks good. The fallback contribution is now normalized and bounded across the accepted traversal graph shapes.
Prior Findings Dispositioned (1)
- prior:311dcec important 1 — fixed —
packages/plugins/paperclip-plugin-gbrain/src/recall.ts:196—capEnrichmentGraphnow normalizes arrays, node/edge records, and edges-only records; it drops auxiliary collections, limits nodes and edges, enforces the serialized-byte budget, and removes edges to trimmed nodes.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [native-codex]
packages/plugins/paperclip-plugin-gbrain/src/recall.ts:203— Consider collecting eligible edges incrementally and stopping atMAX_ENRICHMENT_EDGESinstead of filtering the full edge array; this would reduce temporary allocation for unusually dense hub traversals.
Strengths
- The full issue-scoped traversal remains intact while only opportunistic fallback enrichment is capped.
- Regression coverage now includes array, nodes/edges, edges-only, dense-edge, auxiliary-field, and serialized-byte cases.
- The byte cap uses UTF-8 byte length and revalidates the combined normalized fallback record.
Recommended Action
- Merge after the repository's required checks are green.
Pull request was closed
Held out of the merge queue — it cannot pass as-is, and it was evicting other PRsI've dequeued this PR. Not a judgement on the change; it is structurally unable to pass the merge queue in its current form, and each attempt takes unrelated PRs down with it. Reversible — re-queue as soon as the one-line fix below is in. What fails
Why you had no way to know
So the PR reads The collateral8 merge-group failures across 08-09/08-10 — the highest of any PR by 4× — and a failing entry evicts everything batched behind it. #1186 (a To re-queueRe-attribute git rebase -i 05738808^^ --exec 'git commit --amend --no-edit --author="Omar Ramadan <you@…>"'
# or, since it is the older of two commits:
git rebase origin/master -x 'git commit --amend --no-edit --reset-author'Then re-enable auto-merge. Happy to do the re-attribution myself if you'd rather — say the word. Sorry for the interruption to your PR; the eviction of a critical fix forced the call. |
Thinking Path
Linked Issues or Issue Description
Refs: https://paperclip.blockcast.net/BLO/issues/BLO-21635
Supersedes: #1068
Bug/perf issue: Agent/project fallback enrichment could merge very large hub traversals into otherwise issue-scoped recall payloads. Field evidence in BLO-21635 showed many
okrecalls carrying tens of thousands of nodes and large JSON payloads dominated by unrelated hub facts.What Changed
MAX_ENRICHMENT_NODESbefore merging.Verification
Original app-authored PR #1068 reported:
pnpm testinpackages/plugins/paperclip-plugin-gbrain(99/99 passing)pnpm typecheck(clean)I did not rerun those locally while converting the exact head to this human-authored replacement PR. CI is the verification signal for this replacement branch.
Risks
Low to moderate. The change deliberately drops fallback-only graph nodes beyond the cap, so some broad hub context may no longer appear in recall. The targeted issue traversal remains intact, and the cap applies only to the fallback enrichment path that was producing oversized unrelated payloads.
Model Used
Original implementation:
allyblockcast[bot]/Paperclip agent. Replacement PR body and branch conversion: Codex GPT-5 coding agent with GitHub CLI.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template