Skip to content

fix(brainbar): close digest integrity review gaps - #646

Merged
EtanHey merged 1 commit into
mainfrom
fix/brain-digest-stopbleed-review
Aug 4, 2026
Merged

fix(brainbar): close digest integrity review gaps#646
EtanHey merged 1 commit into
mainfrom
fix/brain-digest-stopbleed-review

Conversation

@EtanHey

@EtanHey EtanHey commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Scope

Follow-up to #645 containing only the two CodeRabbit integrity findings plus the minimal current-main constructor compatibility fix. No historical-row repair or backfill.

  • Treat a post-insert digest row that cannot be read as a content-integrity failure and roll back.
  • Compare persisted digest content as length-aware raw SQLite bytes, preventing embedded-NUL suffix corruption from passing verification.
  • Preserve non-digest dedupe behavior with contentIntegrity: nil at the existing constructor added on current main.

Verification

  • RED on current main: Swift compile failed at the newer StoredChunk constructor until compatibility was supplied.
  • Digest-focused Swift tests: 7 passed, including truncation, deletion, embedded-NUL suffix, and multibyte exact persistence.
  • Full current-main Swift suite: 847 passed, 10 skipped, 0 failures.
  • Changed-only pre-push: registration 3 passed; isolated 40 passed; Bun 1 passed; shell determinism passed.
  • Local CodeRabbit had 0 findings on the substantive two-file review delta; a final one-line retry was rate-limited, so merge remains gated on remote review and CI.

Explicit boundary

This PR stops new silent corruption. It does not touch or backfill the 308 historical rows.


Note

Medium Risk
Touches the core chunk store and digest persistence path where failed checks roll back writes; scope is limited to integrity verification and constructor compatibility, not historical data repair.

Overview
Digest stores with verifyContentIntegrity now validate persisted content against the exact UTF-8 bytes read from SQLite (columnData / blob), not a CString-based text round-trip—so suffixes after embedded NUL bytes can’t slip past verification.

If the row can’t be read back after insert while integrity checking is on, the write path throws contentIntegrityCheckFailed instead of a generic noResult. StoredChunkDetails carries optional cached contentUTF8 for that comparison; non-digest dedupe still returns StoredChunk with contentIntegrity: nil.

Tests assert the integrity error message on failed digest checks and add a trigger-driven embedded-NUL corruption case that must roll back and leave search empty.

Reviewed by Cursor Bugbot for commit b33a442. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix digest integrity review gaps by comparing raw UTF-8 bytes in BrainDatabase

  • Content integrity verification now reads stored bytes via a new columnData helper (using sqlite3_column_blob) instead of text, catching mismatches caused by embedded NUL characters that text-based reads would silently mask.
  • When verifyContentIntegrity is enabled and the stored chunk cannot be read after insert, the method now throws DBError.contentIntegrityCheckFailed rather than falling through to a generic noResult error.
  • StoredChunkDetails gains a contentUTF8: Data field populated from the raw blob, and byte counts are used for comparison instead of string-derived lengths.
  • Adds a test covering embedded NUL injection via a SQLite trigger to confirm the integrity check rejects tampered content and surfaces the error text to the caller.

Macroscope summarized b33a442.

Summary by CodeRabbit

  • Bug Fixes

    • Improved data integrity checks for stored content, including embedded null bytes and appended corruption.
    • Preserved raw content bytes during storage and retrieval to detect previously missed inconsistencies.
    • Invalid or unreadable content is now rejected cleanly without leaving partial records behind.
  • Tests

    • Added coverage for corrupted, unreadable, and rolled-back stored content scenarios.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

@cursor

cursor Bot commented Aug 4, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_6e409ed5-3032-4a6f-8a66-e2804fac4608)

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 74d07edf-1816-4011-aeb2-39017f6d40fc

📥 Commits

Reviewing files that changed from the base of the PR and between f9e28a5 and b33a442.

📒 Files selected for processing (2)
  • brain-bar/Sources/BrainBar/BrainDatabase.swift
  • brain-bar/Tests/BrainBarTests/MCPRouterTests.swift
📜 Recent review details
⏰ Context from checks skipped due to timeout. (5)
  • GitHub Check: swift (macos-15)
  • GitHub Check: Macroscope - Correctness Check
  • GitHub Check: test (3.13)
  • GitHub Check: test (3.11)
  • GitHub Check: test (3.12)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-07-20T07:44:40.216Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 606
File: brain-bar/Tests/BrainBarTests/BrainBarDashboardTruthPresentationTests.swift:170-179
Timestamp: 2026-07-20T07:44:40.216Z
Learning: For SwiftPM source-contract-style tests in the `brain-bar` package (e.g., under `brain-bar/Tests/**`), assume tests are executed from a full repo checkout using `swift test --package-path brain-bar`. These tests may rely on `#filePath`-based inspection of production Swift sources as part of that execution contract. Do not suggest copying production source files into test resources (e.g., bundling duplicates under the test target), since it duplicates sources and can cause drift from the real production implementation.

Applied to files:

  • brain-bar/Tests/BrainBarTests/MCPRouterTests.swift
📚 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/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/BrainDatabase.swift
🪛 SwiftLint (0.65.0)
brain-bar/Sources/BrainBar/BrainDatabase.swift

[Warning] 1985-1985: Prefer failable String(bytes:encoding:) initializer when converting Data to String

(optional_data_string_conversion)

🔇 Additional comments (7)
brain-bar/Sources/BrainBar/BrainDatabase.swift (5)

599-608: LGTM!


1391-1408: LGTM!


2940-2947: LGTM!


4722-4726: LGTM!


1984-1990: 🗄️ Data Integrity & Integration

No change needed.

columns’ schema defines chunks.content TEXT NOT NULL, and the Swift code path always inserts content through bindText.

brain-bar/Tests/BrainBarTests/MCPRouterTests.swift (2)

1267-1271: LGTM!


1273-1310: LGTM!


📝 Walkthrough

Walkthrough

The database now preserves raw SQLite blob bytes, decodes content loss-tolerantly, and verifies digest integrity at the byte level. Tests cover unreadable content, embedded NUL bytes, corruption, error reporting, and rollback behavior.

Changes

Digest integrity

Layer / File(s) Summary
Raw content preservation
brain-bar/Sources/BrainBar/BrainDatabase.swift
StoredChunkDetails stores raw UTF-8 bytes. SQLite blob content is converted to Data and decoded with loss-tolerant UTF-8 conversion.
Byte verification and rollback
brain-bar/Sources/BrainBar/BrainDatabase.swift, brain-bar/Tests/BrainBarTests/MCPRouterTests.swift
Integrity checks compare raw byte data and report byte counts. Stored chunk construction sets contentIntegrity explicitly. Tests verify corruption errors and digest rollback.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Poem

I’m a rabbit guarding bytes in a burrow so tight,
NULs and corrupt tails cannot hide from sight.
Raw blobs stay safe, decoded text stays clear,
Failed digests roll back and disappear.
Hop, hop—integrity checks are working right!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the digest integrity fixes in the pull request.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/brain-digest-stopbleed-review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@EtanHey

EtanHey commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@EtanHey

EtanHey commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

@cursor review

@EtanHey

EtanHey commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

@BugBot review

@EtanHey

EtanHey commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@cursor

cursor Bot commented Aug 4, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_1c97bc90-9b69-4422-8e26-e0091646e98f)

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cursor

cursor Bot commented Aug 4, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_aa18e558-080a-4c85-84bf-dd3ce83901e3)

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: 7b7163218f

ℹ️ 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".

@EtanHey

EtanHey commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

Review and baseline receipt:

  • Codex reviewed exact head 7b7163218f and found no major issues.
  • CodeRabbit remote review was rate-limited (its nominal success status is not counted as a review); the substantive delta had a prior local zero-finding review.
  • The lint failure is inherited from current main: both origin/main and this PR report Ruff I001 in untouched tests/test_mcp_palette.py; git diff origin/main -- tests/test_mcp_palette.py is empty.
  • Current-main CI run 30913286858 independently shows the same lint failure plus an unrelated watchdog test failure. This bounded Swift PR will not absorb those baseline repairs.
  • Swift and Python matrices are still running; no merge while a lane-relevant check is pending or failing.

@EtanHey

EtanHey commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

Final CI attribution:

  • swift (macos-15): PASS (4m27s).
  • Python 3.13: failed only at untouched tests/test_throughput_watchdog.py::test_alert_framing_pages_once_per_episode_not_per_kickstart, identical to current-main run 30913286858.
  • Python 3.11 / 3.12: cancelled by fail-fast after that failure; not independent failures.
  • Local reproduction on this current-main branch: the same watchdog test failed in 45.43s with recovery_failed vs expected kickstart:.
  • Ruff: inherited I001 in untouched tests/test_mcp_palette.py, also present on current main.

No #646 file participates in either Python failure. Per the bounded lane, no unrelated Python fix is being added; merge awaits a separate current-main baseline repair.

@EtanHey
EtanHey force-pushed the fix/brain-digest-stopbleed-review branch from 7b71632 to b33a442 Compare August 4, 2026 20:49

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

@cursor

cursor Bot commented Aug 4, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_be022e97-3ff8-47c1-86fd-c8263179cf22)

@EtanHey
EtanHey merged commit 431ed83 into main Aug 4, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant