Conversation
📝 WalkthroughWalkthroughThis PR adds source-filtered search, chunk lifecycle management (archive/supersede), person-context retrieval, and enrichment statistics to BrainDatabase, exposes these via four new MCP tools ( ChangesSearch, Chunk Lifecycle, and Enrichment Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR introduces multiple interconnected features: schema modifications, source-based filtering affecting FTS query construction and result mapping, four new MCP tool handlers with distinct business logic (archive, supersede, enrichment, person retrieval), and comprehensive test coverage. The changes span diverse areas (database schema, search logic, tool dispatch, enrichment backfill) requiring separate reasoning for each, though patterns repeat within feature areas. FTS column reordering in particular demands careful index verification. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing [Slack Agent](https://www.coderabbit.ai/agent): Turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
@coderabbitai review |
|
@codex review |
✅ Actions performedReview triggered.
|
|
Now I'll perform a thorough code review focused on finding bugs: Bug Review SummaryAfter reviewing the PR changes, I've identified several potential issues: 🔴 CRITICAL BUG: SQL Injection Vulnerability in
|
|
@greptileai review |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| private func handleBrainArchive(_ args: [String: Any]) throws -> ToolOutput { | ||
| guard let chunkID = args["chunk_id"] as? String else { | ||
| throw ToolError.missingParameter("chunk_id") | ||
| } | ||
| let reason = args["reason"] as? String | ||
| guard let db = database else { throw ToolError.noDatabase } | ||
| guard try db.archiveChunk(id: chunkID, reason: reason) else { |
There was a problem hiding this comment.
🟡 Medium BrainBar/MCPRouter.swift:493
handleBrainArchive accepts a reason parameter and echoes it back in the JSON response at lines 506–508, but BrainDatabase.archiveChunk() declares the parameter as reason _: String? = nil and never writes it to the database. This causes user-provided archive reasons to be silently discarded while the response misleadingly implies they were stored.
private func handleBrainArchive(_ args: [String: Any]) throws -> ToolOutput {
guard let chunkID = args["chunk_id"] as? String else {
throw ToolError.missingParameter("chunk_id")
}
let reason = args["reason"] as? String
guard let db = database else { throw ToolError.noDatabase }
- guard try db.archiveChunk(id: chunkID, reason: reason) else {
+ guard try db.archiveChunk(id: chunkID, reason: reason) else {Also found in 1 other location(s)
brain-bar/Sources/BrainBar/BrainDatabase.swift:3265
The
skippedvalue in the returned dictionary at line 3265 will always be 0 because the loop at lines 3244-3257 increments eitherenrichedorfailedfor every target—there is no code path that skips a target. The calculationmax(0, targets.count - enriched - failed)is therefore always 0, making this statistic meaningless and potentially misleading to callers who expect it to reflect actually skipped chunks.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file brain-bar/Sources/BrainBar/MCPRouter.swift around lines 493-499:
`handleBrainArchive` accepts a `reason` parameter and echoes it back in the JSON response at lines 506–508, but `BrainDatabase.archiveChunk()` declares the parameter as `reason _: String? = nil` and never writes it to the database. This causes user-provided archive reasons to be silently discarded while the response misleadingly implies they were stored.
Evidence trail:
brain-bar/Sources/BrainBar/MCPRouter.swift lines 493-510 (REVIEWED_COMMIT): handleBrainArchive accepts reason, passes it to archiveChunk, and echoes it in response payload.
brain-bar/Sources/BrainBar/BrainDatabase.swift line 3046 (REVIEWED_COMMIT): `func archiveChunk(id: String, reason _: String? = nil)` — underscore discards the parameter; SQL UPDATE on lines 3048-3055 only sets `archived=1` and `archived_at`, no reason column.
Also found in 1 other location(s):
- brain-bar/Sources/BrainBar/BrainDatabase.swift:3265 -- The `skipped` value in the returned dictionary at line 3265 will always be 0 because the loop at lines 3244-3257 increments either `enriched` or `failed` for every target—there is no code path that skips a target. The calculation `max(0, targets.count - enriched - failed)` is therefore always 0, making this statistic meaningless and potentially misleading to callers who expect it to reflect actually skipped chunks.
There was a problem hiding this comment.
Investigated, but intentionally not changing this in the Swift parity PR.
Python currently has the same behavior in src/brainlayer/mcp/store_handler.py:_brain_archive(...): it echoes reason in the MCP response without persisting it to the DB. I kept Swift aligned with the existing cross-runtime contract here. Persisting archive reasons would be a broader schema/contract change, not a Swift-only parity port.
There was a problem hiding this comment.
Understood — maintaining consistency with the Python implementation makes sense for a parity port. Thanks for the clarification.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c70dfddc2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| private func handleBrainEnrich(_ args: [String: Any]) throws -> ToolOutput { | ||
| let mode = args["mode"] as? String ?? "realtime" | ||
| let limit = min(args["limit"] as? Int ?? 25, 5_000) |
There was a problem hiding this comment.
handleBrainEnrich only caps limit at 5000, so callers can pass 0 or a negative value. In SQLite, a negative LIMIT removes the row cap, which means a malformed or adversarial request like {"limit": -1} can cause brain_enrich to update every eligible chunk (summary + enriched_at) instead of a bounded batch. This is a write-safety regression for a destructive tool and should enforce a lower bound (for example max(1, ...)) before calling the DB layer.
Useful? React with 👍 / 👎.
| if chunkIDs == nil { | ||
| conditions.append("enriched_at IS NULL") | ||
| conditions.append("char_count >= 50") | ||
| } |
There was a problem hiding this comment.
When chunk_ids is an empty array, enrichChunks takes the chunkIDs != nil path, so it skips the default enriched_at IS NULL/char_count >= 50 guards and also skips the id IN (...) clause (because the array is empty). That broadens the query to unrelated chunks and enriches arbitrary rows, which violates expected tool contract for explicit IDs and can silently mutate memory state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on 92df6d94.
Swift brain_enrich now treats chunk_ids: [] as an explicit empty target set and returns attempted=0 instead of broadening to unrelated eligible chunks. I also added a regression test for that path in MCPRouterTests.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
@codex review |
|
Addressing the Cursor review on the latest push (
Not changing in this PR:
|
|
Based on the PR comments, the user mentioned pushing commit 🔍 Comprehensive Bug Re-Review🔴 CRITICAL: Empty chunk_ids array bypasses enrichment eligibility filtersLocation: 3198| var conditions = ["superseded_by IS NULL", "archived_at IS NULL"]
3199| if chunkIDs == nil {
3200| conditions.append("enriched_at IS NULL")
3201| conditions.append("char_count >= 50")
3202| }
...
3206| if let chunkIDs, !chunkIDs.isEmpty {
3207| let placeholders = Array(repeating: "?", count: chunkIDs.count).joined(separator: ", ")
3208| conditions.append("id IN (\(placeholders))")
3209| }Issue: When
Fix Required: if chunkIDs == nil || chunkIDs?.isEmpty == true {
conditions.append("enriched_at IS NULL")
conditions.append("char_count >= 50")
}Or return early: if let chunkIDs, chunkIDs.isEmpty {
return [
"mode": mode,
"attempted": 0,
"enriched": 0,
"skipped": 0,
"failed": 0,
]
}🔴 CRITICAL: brain_enrich limit not clamped to positive valuesLocation: 514| let limit = min(args["limit"] as? Int ?? 25, 5_000)Issue: Negative limits pass through unchecked. SQLite interprets negative LIMIT as "no limit", causing Test case: Fix Required: let limit = max(1, min(args["limit"] as? Int ?? 25, 5_000))Status: User claimed this was fixed in 🔴 CRITICAL: source="all" disables KG entity detectionLocation: 239| let hasActiveFilters = project != nil || source != nil || tag != nil || subscriberID != nil || importanceMin != nilIssue: When user passes Evidence: Fix Required: let hasActiveFilters = project != nil || (source != nil && source != "all") || tag != nil || subscriberID != nil || importanceMin != nilOr normalize earlier: let normalizedSource = source == "all" ? nil : source
let hasActiveFilters = project != nil || normalizedSource != nil || tag != nil || subscriberID != nil || importanceMin != nil
|
| Severity | Issue | Fixed? |
|---|---|---|
| 🔴 CRITICAL | Empty chunk_ids array bypasses filters |
❌ No |
| 🔴 CRITICAL | Negative limit not clamped | ❌ No |
| 🔴 CRITICAL | source="all" disables KG detection |
❌ No |
| Archive reason silently discarded | ❌ No | |
| Silent error swallowing | ❌ No | |
| 🟡 LOW | Misleading "skipped" count | ❌ No |
Recommendation: The user mentioned fixing some issues in commit 7a1eb90c, but that commit is not present in the current branch. The PR should not be merged until these critical issues are addressed:
- Empty array handling in
enrichChunks - Negative limit clamping in
handleBrainEnrich - KG detection logic for
source="all"
The medium/low issues can be addressed in follow-up work if documented properly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a1eb90c9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if chunkIDs == nil { | ||
| conditions.append("enriched_at IS NULL") | ||
| conditions.append("char_count >= 50") | ||
| } |
There was a problem hiding this comment.
In enrichChunks, an explicitly empty chunkIDs array skips the default eligibility filters because they are gated on chunkIDs == nil, but it also skips the id IN (...) clause because that is only added when the array is non-empty. That means brain_enrich calls with chunk_ids: [] can unexpectedly target arbitrary non-archived/non-superseded chunks (up to limit) instead of zero chunks, which is a risky write-path expansion for lifecycle tooling.
Useful? React with 👍 / 👎.
| // Entity detection → KG fact lookup | ||
| var kgSection = "" | ||
| let hasActiveFilters = project != nil || tag != nil || subscriberID != nil || importanceMin != nil | ||
| let hasActiveFilters = project != nil || source != nil || tag != nil || subscriberID != nil || importanceMin != nil |
There was a problem hiding this comment.
handleBrainSearch now includes source != nil in hasActiveFilters, so passing source: "all" disables entity/KG augmentation even though normalizedSourceFilter("all") removes the DB filter and search scope is unchanged. This creates a behavior regression where explicit all returns less context than omitting source, despite both representing the same query scope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
@codex review |
|
Follow-up push on Changes:
Verification:
Left intentionally unchanged in this PR:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Now let me create a comprehensive summary of the current state: ✅ Re-Review Complete: All Critical Issues ResolvedSummary of Fixes AppliedBased on my review of commits ✅ FIXED: Empty chunk_ids array edge caseCommit: if let chunkIDs, chunkIDs.isEmpty {
return [
"mode": mode,
"attempted": 0,
"enriched": 0,
"skipped": 0,
"failed": 0,
]
}Status: ✅ Now correctly returns early with zero counts instead of processing arbitrary chunks. ✅ FIXED: Negative limit bypassing bounds checkCommit: let limit = max(1, min(args["limit"] as? Int ?? 25, 5_000))Status: ✅ Now clamps to range ✅ FIXED: source="all" disables KG entity detectionCommit: let sourceCountsAsFilter: Bool
if let source {
let trimmed = source.trimmingCharacters(in: .whitespacesAndNewlines)
sourceCountsAsFilter = !trimmed.isEmpty && trimmed != "all"
} else {
sourceCountsAsFilter = false
}
let hasActiveFilters = project != nil || sourceCountsAsFilter || tag != nil || subscriberID != nil || importanceMin != nilStatus: ✅ Now treats ✅ FIXED: Silent error swallowing in enrichChunksCommit: } catch {
failed += 1
NSLog(
"[BrainBar] Enrichment summary backfill failed for chunk %@: %@",
target.id,
String(describing: error)
)
}Status: ✅ Now logs enrichment failures for debugging. ✅ DOCUMENTED: archiveChunk reason parameterCommit: // Keep the reason parameter for Python MCP contract parity; BrainBar
// does not persist archive reasons yet.Status: ✅ Clearly documents that 🟡 Remaining Low-Priority Issue: Misleading schema descriptionLocation: "source": [..., "description": "Filter by data source (default: claude_code). Use 'all' to search everything."]Issue: The description says Suggested Fix: "description": "Filter by data source. Omit to search all sources, or use 'all' explicitly. Options: claude_code, whatsapp, youtube."Impact: Low - This is a documentation inconsistency rather than a behavior bug. The actual behavior is correct; only the description is misleading. Final Verdict✅ Safe to Merge with one optional documentation improvement
Recommendation:
Excellent work addressing all the critical findings! The code is now production-ready. 🎉 |
| return [ | ||
| "mode": mode, | ||
| "attempted": targets.count, | ||
| "enriched": enriched, | ||
| "skipped": max(0, targets.count - enriched - failed), | ||
| "failed": failed, | ||
| ] |
There was a problem hiding this comment.
🟢 Low BrainBar/BrainDatabase.swift:3277
The skipped count in the returned dictionary is always 0 because the loop increments either enriched or failed for every target, so enriched + failed == targets.count always holds. If tracking skipped chunks is intentional, the logic needs to identify and count actually-skipped cases; otherwise consider removing the field to avoid misleading consumers.
return [
"mode": mode,
"attempted": targets.count,
"enriched": enriched,
- "skipped": max(0, targets.count - enriched - failed),
"failed": failed,
]🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file brain-bar/Sources/BrainBar/BrainDatabase.swift around lines 3277-3283:
The `skipped` count in the returned dictionary is always 0 because the loop increments either `enriched` or `failed` for every target, so `enriched + failed == targets.count` always holds. If tracking skipped chunks is intentional, the logic needs to identify and count actually-skipped cases; otherwise consider removing the field to avoid misleading consumers.
Evidence trail:
brain-bar/Sources/BrainBar/BrainDatabase.swift lines 3254-3282 at REVIEWED_COMMIT: The for loop over `targets` increments `enriched` in the do block or `failed` in the catch block for every element. The return dictionary at line ~3277 computes `skipped` as `max(0, targets.count - enriched - failed)` which is always 0.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92df6d9484
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| throw ToolError.missingParameter("name") | ||
| } | ||
| let context = args["context"] as? String | ||
| let numMemories = min(args["num_memories"] as? Int ?? 10, 50) |
There was a problem hiding this comment.
handleBrainGetPerson only applies an upper bound, so a caller can pass num_memories <= -1, which is then bound directly into LIMIT ? in fetchEntityMemories; in SQLite, a negative LIMIT removes the cap and returns all rows. This makes the tool unexpectedly unbounded for malformed or adversarial inputs, increasing latency and potentially returning far more personal memory content than requested.
Useful? React with 👍 / 👎.
| enriched: try queryInt("SELECT COUNT(*) FROM chunks WHERE enriched_at IS NOT NULL"), | ||
| unenrichedEligible: try queryInt("SELECT COUNT(*) FROM chunks WHERE enriched_at IS NULL AND char_count >= 50"), | ||
| skippedTooShort: try queryInt("SELECT COUNT(*) FROM chunks WHERE enriched_at IS NULL AND char_count < 50"), | ||
| enrichedLast24Hours: try queryInt("SELECT COUNT(*) FROM chunks WHERE enriched_at > datetime('now', '-24 hours')") |
There was a problem hiding this comment.
enrichedLast24Hours compares enriched_at text directly to datetime('now', '-24 hours'), but enriched_at is written via ISO8601DateFormatter (YYYY-MM-DDTHH:MM:SSZ). Lexicographic comparison against SQLite's space-separated datetime format overcounts rows (for example, same-date timestamps earlier than the cutoff still compare greater due to 'T' vs ' '). This makes enrichment progress stats inaccurate.
Useful? React with 👍 / 👎.
| totalChunks: try queryInt("SELECT COUNT(*) FROM chunks"), | ||
| enriched: try queryInt("SELECT COUNT(*) FROM chunks WHERE enriched_at IS NOT NULL"), | ||
| unenrichedEligible: try queryInt("SELECT COUNT(*) FROM chunks WHERE enriched_at IS NULL AND char_count >= 50"), | ||
| skippedTooShort: try queryInt("SELECT COUNT(*) FROM chunks WHERE enriched_at IS NULL AND char_count < 50"), |
There was a problem hiding this comment.
Enrichment stats overcount eligible chunks by including archived/superseded
Medium Severity
enrichmentStats() counts unenrichedEligible and skippedTooShort without filtering out archived or superseded chunks, while enrichChunks() explicitly adds superseded_by IS NULL and archived_at IS NULL conditions. This causes the stats to report more eligible chunks than will actually be processed during enrichment, making progress reporting inaccurate.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 92df6d9. Configure here.
There was a problem hiding this comment.
Leaving this unchanged in the parity PR. Swift is currently matching Python here: src/brainlayer/mcp/enrich_handler.py:_enrich_stats() also counts unenriched_eligible / skipped_too_short without lifecycle filters. I agree the metric semantics could be tightened, but that would be a cross-server behavior change rather than a Swift-port fix, so I’d treat it as follow-up work if we want both implementations updated together.
| "new_chunk_id": newChunkID, | ||
| "instruction": "Re-call with confirm=true to proceed", | ||
| ] as [String: String])) | ||
| } |
There was a problem hiding this comment.
Supersede safety gate bypassed with direct confirm parameter
Medium Severity
The personal-content safety gate in handleBrainSupersede only triggers when safety_check is "auto". A caller can completely bypass the personal-content detection by passing safety_check="confirm" and confirm=true in a single initial call, since the requiresSupersedeConfirmation check at line 470 is never evaluated when safetyCheck != "auto". The confirmation gate provides no actual protection when a caller opts into "confirm" mode directly.
Reviewed by Cursor Bugbot for commit 92df6d9. Configure here.
There was a problem hiding this comment.
Leaving this unchanged for parity with the existing Python MCP contract. src/brainlayer/mcp/store_handler.py:_brain_supersede(...) has the same semantics today: safety_check="confirm" + confirm=True is treated as the explicit operator override, not as a two-step challenge token. If we want to harden that flow, I’d prefer to do it as a deliberate cross-server behavior change rather than quietly diverging Swift from Python in the parity port.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
@codex review |
|
Follow-up push on Changes on this head:
Verification on this head:
Reviewer notes:
|
|
Tip For best results, initiate chat on the files or code changes.
[review] |
|
|
||
| guard try db.supersedeChunk(oldChunkID: oldChunkID, newChunkID: newChunkID) else { | ||
| throw ToolError.notFound("Supersede failed for: \(oldChunkID)") |
There was a problem hiding this comment.
🟡 Medium BrainBar/MCPRouter.swift:489
handleBrainSupersede does not validate that old_chunk_id and new_chunk_id are different. When both parameters have the same value, the chunk's superseded_by field is set to itself, creating a self-referential record that excludes the chunk from search without providing a valid replacement. Code that follows superseded_by chains could also loop infinitely. Consider rejecting the request with an error when the IDs are equal.
- guard try db.getChunk(id: newChunkID) != nil else {
+ guard oldChunkID != newChunkID else {
+ throw ToolError.schemaValidation("old_chunk_id and new_chunk_id must be different")
+ }
+ guard try db.getChunk(id: newChunkID) != nil else {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file brain-bar/Sources/BrainBar/MCPRouter.swift around lines 489-491:
`handleBrainSupersede` does not validate that `old_chunk_id` and `new_chunk_id` are different. When both parameters have the same value, the chunk's `superseded_by` field is set to itself, creating a self-referential record that excludes the chunk from search without providing a valid replacement. Code that follows `superseded_by` chains could also loop infinitely. Consider rejecting the request with an error when the IDs are equal.
Evidence trail:
brain-bar/Sources/BrainBar/MCPRouter.swift lines 452-497 (handleBrainSupersede — no equality check between oldChunkID and newChunkID); brain-bar/Sources/BrainBar/BrainDatabase.swift lines 3065-3077 (supersedeChunk — no equality check, directly executes UPDATE); brain-bar/Sources/BrainBar/BrainDatabase.swift lines 576, 831, 1731 (search queries filter on `c.superseded_by IS NULL`, confirming self-referential record would exclude chunk from all searches)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 04c35b7. Configure here.
| "old_chunk_id": oldChunkID, | ||
| "new_chunk_id": newChunkID, | ||
| ] as [String: String])) | ||
| } |
There was a problem hiding this comment.
Self-supersede permanently hides chunk from search
Medium Severity
handleBrainSupersede never validates that old_chunk_id and new_chunk_id are different. If both are the same, supersedeChunk sets superseded_by to the chunk's own ID, permanently hiding it from all default search paths (which filter on superseded_by IS NULL) with no recovery mechanism through the MCP surface.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 04c35b7. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
brain-bar/Sources/BrainBar/MCPRouter.swift (1)
784-798:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAllow arbitrary
sourcevalues in the MCP schema.
chunks.sourceis free-form and the backend already writes values outside this list (for example,brain_digeststores"digest"), but the schema only permitsclaude_code,youtube,mcp, andall. That makes valid source filters fail schema validation before they ever reach the database.Suggested fix
- "source": ["type": "string", "enum": ["claude_code", "whatsapp", "youtube", "mcp", "all"], "description": "Filter by data source. Omit or use 'all' to search everything."], + "source": ["type": "string", "description": "Filter by data source. Omit or use 'all' to search everything."],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brain-bar/Sources/BrainBar/MCPRouter.swift` around lines 784 - 798, The "source" field in the MCPRouter.limitedInputSchema currently restricts values to a fixed enum which blocks valid free-form sources (e.g. "digest"); update the "source" property in the MCPRouter.limitedInputSchema to accept arbitrary strings instead of the enum — replace the enum constraint with a simple "type": "string" (or add a permissive pattern) and keep the description, so valid backend-written chunk.source values pass validation before DB filtering logic in MCPRouter runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@brain-bar/Sources/BrainBar/BrainDatabase.swift`:
- Around line 3065-3077: supersedeChunk currently does a separate
read-then-write and allows oldChunkID == newChunkID; make it atomic by
performing the validation inside the update (or a transaction) and return false
if the replacement is invalid or not active. Specifically, in supersedeChunk
replace the guard + executeUpdate flow with a single transactional update (or a
single SQL UPDATE) that: requires oldChunkID != newChunkID, requires the old
chunk to still be unsuperseded/active (e.g. superseded_by IS NULL) and requires
the new chunk to be active/not archived/not already superseded (check the same
column or state used by getChunk), and only sets superseded_by when those
conditions hold; if the update affects 0 rows return false, otherwise call
refreshSearchStatisticsBestEffort() and return true. Use the existing
executeUpdate, getChunk semantics, and refreshSearchStatisticsBestEffort
references to locate and modify the code.
In `@brain-bar/Sources/BrainBar/MCPRouter.swift`:
- Around line 220-225: The current computation of limit allows 0 or negative
values; change the calculation of limit so it is clamped to the range 1...100
(e.g. compute an Int from args["num_results"], then apply max(1, min(value,
100))) so BrainDatabase.search() always receives at least 1 row requested;
update the line that sets limit (the variable named limit built from
args["num_results"]) to enforce this min of 1 while preserving the existing 100
upper bound and existing fallback when the arg is missing or not an Int.
---
Outside diff comments:
In `@brain-bar/Sources/BrainBar/MCPRouter.swift`:
- Around line 784-798: The "source" field in the MCPRouter.limitedInputSchema
currently restricts values to a fixed enum which blocks valid free-form sources
(e.g. "digest"); update the "source" property in the
MCPRouter.limitedInputSchema to accept arbitrary strings instead of the enum —
replace the enum constraint with a simple "type": "string" (or add a permissive
pattern) and keep the description, so valid backend-written chunk.source values
pass validation before DB filtering logic in MCPRouter runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 225e7490-1b78-4168-bf76-e183bc888f66
📒 Files selected for processing (4)
brain-bar/Sources/BrainBar/BrainDatabase.swiftbrain-bar/Sources/BrainBar/MCPRouter.swiftbrain-bar/Tests/BrainBarTests/MCPRouterTests.swiftbrain-bar/Tests/BrainBarTests/SocketIntegrationTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: Cursor Bugbot
- GitHub Check: Macroscope - Correctness Check
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-03-18T00:12:08.774Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 87
File: brain-bar/Sources/BrainBar/BrainBarServer.swift:118-129
Timestamp: 2026-03-18T00:12:08.774Z
Learning: In Swift files under brain-bar/Sources/BrainBar, enforce that when a critical dependency like the database is nil due to startup ordering (socket before DB), any tool handler that accesses the database must throw an explicit error (e.g., ToolError.noDatabase) instead of returning a default/empty value. Do not allow silent defaults (e.g., guard let db else { return ... }). Flag patterns that silently return defaults when db is nil, as this masks startup timing issues. This guidance applies broadly to similar Swift files in the BrainBar module, not just this one location.
Applied to files:
brain-bar/Sources/BrainBar/MCPRouter.swiftbrain-bar/Sources/BrainBar/BrainDatabase.swift
📚 Learning: 2026-03-29T18:45:40.988Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 133
File: brain-bar/Sources/BrainBar/BrainDatabase.swift:0-0
Timestamp: 2026-03-29T18:45:40.988Z
Learning: In the BrainBar module’s Swift database layer (notably BrainDatabase.swift), ensure that the `search()` function’s `unreadOnly=true` path orders results by the delivery frontier cursor so the watermark `maxRowID` stays contiguous. Specifically, when `unreadOnly` is enabled, the query must include `ORDER BY c.rowid ASC` (e.g., via `let orderByClause = unreadOnly ? "c.rowid ASC" : "f.rank"`). Do not replace the unread-only ordering with relevance-based sorting (e.g., `f.rank`) unconditionally or for the unread-only path, as it can introduce gaps in the watermark and incorrectly mark unseen rows as delivered. Flag any future change to the `ORDER BY` clause in this function that makes relevance sorting apply to the unread-only case.
Applied to files:
brain-bar/Sources/BrainBar/MCPRouter.swiftbrain-bar/Sources/BrainBar/BrainDatabase.swift
🪛 SwiftLint (0.63.2)
brain-bar/Sources/BrainBar/BrainDatabase.swift
[Warning] 2707-2707: Prefer empty collection over optional collection
(discouraged_optional_collection)
[Warning] 3019-3019: Prefer empty collection over optional collection
(discouraged_optional_collection)
[Warning] 3079-3079: Prefer empty collection over optional collection
(discouraged_optional_collection)
[Warning] 3196-3196: Prefer empty collection over optional collection
(discouraged_optional_collection)
| func supersedeChunk(oldChunkID: String, newChunkID: String) throws -> Bool { | ||
| guard try getChunk(id: oldChunkID) != nil, try getChunk(id: newChunkID) != nil else { | ||
| return false | ||
| } | ||
| try executeUpdate( | ||
| "UPDATE chunks SET superseded_by = ? WHERE id = ?" | ||
| ) { stmt in | ||
| bindText(newChunkID, to: stmt, index: 1) | ||
| bindText(oldChunkID, to: stmt, index: 2) | ||
| } | ||
| refreshSearchStatisticsBestEffort() | ||
| return true | ||
| } |
There was a problem hiding this comment.
Make supersedeChunk atomic and reject invalid replacements.
This does a read-then-write on a shared database, so newChunkID can become archived/superseded after the guards pass and before the update runs. It also currently accepts oldChunkID == newChunkID, which hides the chunk with no visible replacement. Fold the validation into the update (or a transaction) and return false unless the replacement is distinct and still active.
Suggested direction
func supersedeChunk(oldChunkID: String, newChunkID: String) throws -> Bool {
- guard try getChunk(id: oldChunkID) != nil, try getChunk(id: newChunkID) != nil else {
+ guard let db else { throw DBError.notOpen }
+ guard oldChunkID != newChunkID else {
return false
}
try executeUpdate(
- "UPDATE chunks SET superseded_by = ? WHERE id = ?"
+ """
+ UPDATE chunks
+ SET superseded_by = ?
+ WHERE id = ?
+ AND EXISTS (
+ SELECT 1
+ FROM chunks replacement
+ WHERE replacement.id = ?
+ AND replacement.archived_at IS NULL
+ AND replacement.superseded_by IS NULL
+ )
+ """
) { stmt in
bindText(newChunkID, to: stmt, index: 1)
bindText(oldChunkID, to: stmt, index: 2)
+ bindText(newChunkID, to: stmt, index: 3)
}
- refreshSearchStatisticsBestEffort()
- return true
+ let updated = sqlite3_changes(db) == 1
+ if updated {
+ refreshSearchStatisticsBestEffort()
+ }
+ return updated
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@brain-bar/Sources/BrainBar/BrainDatabase.swift` around lines 3065 - 3077,
supersedeChunk currently does a separate read-then-write and allows oldChunkID
== newChunkID; make it atomic by performing the validation inside the update (or
a transaction) and return false if the replacement is invalid or not active.
Specifically, in supersedeChunk replace the guard + executeUpdate flow with a
single transactional update (or a single SQL UPDATE) that: requires oldChunkID
!= newChunkID, requires the old chunk to still be unsuperseded/active (e.g.
superseded_by IS NULL) and requires the new chunk to be active/not archived/not
already superseded (check the same column or state used by getChunk), and only
sets superseded_by when those conditions hold; if the update affects 0 rows
return false, otherwise call refreshSearchStatisticsBestEffort() and return
true. Use the existing executeUpdate, getChunk semantics, and
refreshSearchStatisticsBestEffort references to locate and modify the code.
| let limit = min(args["num_results"] as? Int ?? 5, 100) | ||
| let project = args["project"] as? String | ||
| let source = args["source"] as? String | ||
| let tag = args["tag"] as? String | ||
| let subscriberID = (args["agent_id"] as? String) ?? (args["subscriber_id"] as? String) | ||
| let unreadOnly = args["unread_only"] as? Bool ?? false |
There was a problem hiding this comment.
Clamp num_results to at least 1.
min(..., 100) still accepts 0 and negative values. BrainDatabase.search() normalizes the SQL LIMIT back up to 1 internally, so in unread_only mode the delivery cursor can advance even though the caller receives no rows.
Suggested fix
- let limit = min(args["num_results"] as? Int ?? 5, 100)
+ let limit = max(1, min(args["num_results"] as? Int ?? 5, 100))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@brain-bar/Sources/BrainBar/MCPRouter.swift` around lines 220 - 225, The
current computation of limit allows 0 or negative values; change the calculation
of limit so it is clamped to the range 1...100 (e.g. compute an Int from
args["num_results"], then apply max(1, min(value, 100))) so
BrainDatabase.search() always receives at least 1 row requested; update the line
that sets limit (the variable named limit built from args["num_results"]) to
enforce this min of 1 while preserving the existing 100 upper bound and existing
fallback when the arg is missing or not an Int.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04c35b73d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard try db.supersedeChunk(oldChunkID: oldChunkID, newChunkID: newChunkID) else { | ||
| throw ToolError.notFound("Supersede failed for: \(oldChunkID)") | ||
| } |
There was a problem hiding this comment.
brain_supersede currently allows old_chunk_id == new_chunk_id; in that case supersedeChunk writes superseded_by to the same chunk ID, which immediately removes that chunk from default search and effectively self-hides data after a simple caller typo. Add an explicit equality check before calling db.supersedeChunk and return a validation error for identical IDs.
Useful? React with 👍 / 👎.
| throw ToolError.missingParameter("name") | ||
| } | ||
| let context = args["context"] as? String | ||
| let numMemories = min(args["num_memories"] as? Int ?? 10, 50) |
There was a problem hiding this comment.
num_memories is only capped with min(..., 50) and can still be negative; that negative value is passed to SQL LIMIT in fetchEntityMemories, and in SQLite a negative LIMIT means no upper bound, so a malformed request can return an unbounded number of memories and blow up response size/latency. Clamp with max(1, min(..., 50)) before calling the DB layer.
Useful? React with 👍 / 👎.
|
|
||
| var bindIndex: Int32 = 1 | ||
| if mode == "realtime", chunkIDs == nil { | ||
| bindText("-\(sinceHours) hours", to: stmt, index: bindIndex) |
There was a problem hiding this comment.
since_hours is accepted as any integer and then interpolated as "-\(sinceHours) hours"; negative input produces an invalid modifier like --5 hours, causing datetime('now', ?) to evaluate to NULL and silently filter out all realtime enrichment candidates. Enforce since_hours >= 1 (or clamp) before building the modifier string.
Useful? React with 👍 / 👎.
1 participant
Summary
brain_get_person,brain_supersede,brain_archive, andbrain_enrichto the Swift BrainBar MCP surfacesourceparameter to Swiftbrain_searchand thread it through search filteringTest plan
swift test --package-path brain-barinitialize,tools/list,brain_store,brain_searchpytestunit suite:1819 passed, 9 skipped, 75 deselected, 1 xfailedpytestMCP tool registrationpytestisolated eval and hook routingbun testtests/regression/test_fts5_determinism.shNotes
#268is still open/dirty, so this branch is based on currentmain, not post-#268main.cr review --plainwas attempted pre-commit but blocked by the org hourly cap, not by code findings.Note
Medium Risk
Adds new MCP tools that mutate chunk state (archive/supersede) and changes default search to exclude archived/superseded rows, plus schema migrations; mistakes could hide data unexpectedly or break search queries.
Overview
Expands the Swift BrainBar MCP surface from 11 to 15 tools by adding
brain_get_person,brain_supersede,brain_archive, andbrain_enrich, including input validation bounds, new handler implementations, and updated tool annotations.Search behavior changes:
brain_searchgains an optionalsourcefilter (withalltreated as no filter), and all FTS/exact-ID/candidate search paths now exclude chunks that are archived or superseded (archived_at IS NULLandsuperseded_by IS NULL).Database + lifecycle additions:
chunksmigrations addsource,enriched_at,archived,archived_at, andsuperseded_by; new DB APIs supportgetChunk,archiveChunk,supersedeChunk, person context assembly (entity-type filtered lookup + related memories), and summary backfill enrichment with stats. Tests are updated/extended to cover the new tools,sourcefiltering, and archive/supersede/enrich behavior (including socket tool listing count).Reviewed by Cursor Bugbot for commit 04c35b7. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add brain_get_person, brain_supersede, brain_archive, and brain_enrich MCP tools to BrainBar
brain_get_person,brain_supersede,brain_archive,brain_enrich) to MCPRouter.swift, routing through new handlers in the dispatcher.brain_searchto accept asourceparameter; KG augmentation is suppressed only when source acts as an active filter (notallor empty).source,enriched_at,archived,superseded_by, andarchived_atcolumns to thechunkstable.content_typeor keyword) requires an explicit confirmation step before the operation proceeds.source = 'claude_code',archived = 0), which may affect source-filtered queries on older data.Macroscope summarized 04c35b7.
Summary by CodeRabbit