feat: Git-backed transaction notes and core helpers #62
Conversation
This PR introduces a transaction core module and structured Git note support for every write operation in ChronDB, fulfilling architectural requirements for chronological traceability and external correlation/audit. Highlights: - Adds `chrondb.transaction.core` namespace. Provides a shared transaction context, unique `tx_id` generation, and helpers for capturing metadata (origin, user, flags) across all entrypoints (REST, SQL, Redis). - Implements a new `chrondb.storage.git.notes` module. Every commit now stores a JSON note under the `chrondb` namespace, containing: `tx_id`, origin (protocol/source), optional `user`, semantic flags (`bulk-load`, `migration`, `rollback`, etc.), and client/request metadata (e.g. `X-Request-Id`). - Refactors all transaction/write paths in the Git-backed storage engine to invoke these helpers. Enforces that missing notes are treated as a regression (test coverage included). - Documents new transaction block (`with-transaction`) usage, and how to pass metadata via HTTP headers or protocol attributes to propagate context into Git notes. See `docs/api.md`, "Transaction Operations" and "Transaction Metadata and Git Notes". - Adds and updates unit/integration tests for: correct note attachment, lookup with `git notes`, failure modes (e.g., repository contention, branch mutation), and regression coverage (notes always present after a commit). - Updates AGENT.md: All write paths _must_ call transaction helpers to guarantee Git note context. No destructive mutations; always append-only. - No backwards-incompatible API changes. Protocols remain consistent across REST, SQL (Postgres), and Redis wire formats. Rationale: ChronDB's Git-based architecture enables full auditability and time-travel. Storing rich transaction metadata as Git notes ensures every operation is contextualized—supporting debugging, lineage analysis, external audit, and advanced correlation across distributed systems. This change centralizes transaction and commit metadata handling, eliminating per-protocol ad-hoc logic and reducing the risk of silent context loss. See also: - `src/chrondb/storage/git/notes.clj` (implementation) - `src/chrondb/transaction/core.clj` (transaction core logic) - New/updated tests in `test/chrondb/storage/git/notes_test.clj`, `test/chrondb/transaction/core_test.clj` - Expanded documentation in `docs/api.md` and process requirements in AGENT.md. BREAKING: None, but all future write code must route through the transaction context to guarantee note coverage and protocol metadata flow. fixed #43 fixed #44 Signed-off-by: Avelino <[email protected]>
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds an end-to-end transaction context system that attaches structured Git notes to commits for all write paths (REST, CLI, Redis, SQL), including note read/merge utilities, commit integration, request-context propagation across routes/handlers, and tests validating note payloads and lifecycle. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Router as Routes
participant API as APIv1
participant TxCore as TxCore
participant Storage as GitStorage
participant Notes as GitNotes
Client->>Router: Send request (headers + body)
Router->>Router: build-request-context()
Router->>API: handle-*(context)
API->>TxCore: with-transaction(tx-opts)
activate TxCore
TxCore->>TxCore: bind/extend *transaction-context*
TxCore->>Storage: perform write (commit-virtual / save / delete / restore)
activate Storage
Storage->>Storage: create commit (git)
Storage-->>TxCore: return commit-id
Storage->>Notes: add-git-note(commit-id, tx-note) %% attach structured note after commit
Notes-->>Storage: merged-note
deactivate Storage
TxCore->>TxCore: run on-complete callbacks (success / rollback)
deactivate TxCore
Note right of Notes: "notes visible via git log --show-notes"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Comment |
![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.
ℹ️ 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".
- Update `.github/workflows/publish-docker.yml` to also trigger on pull_request events, not just pushes to main. - Enables CI/CD Docker build checks for PRs, improving contributor feedback and pre-merge validation. Signed-off-by: Avelino <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/chrondb/storage/git/history.clj (1)
135-213: Fix unused locals that are breaking lint.
data-dirandid-onlyare no longer used after the refactor, and clj-kondo flags both (see pipeline warning). Please drop the binding and ignore the second return value so the lint job goes green again.- ; Get data directory from config - data-dir (get-in config-map [:storage :data-dir]) - ; Split logic - extract table if ID has table prefix (e.g., "user:1") - [table-hint id-only] (path/extract-table-and-id id) + ; Split logic - extract table if ID has table prefix (e.g., "user:1") + [table-hint _] (path/extract-table-and-id id)
🧹 Nitpick comments (3)
src/chrondb/transaction/core.clj (1)
45-45: Silence the new clj-kondo warnings.Lint is flagging
opts,overrides, andstorageas unused. Dropping the unused destructuring and renaming the ignored argument keeps CI clean without affecting behavior.-(defn- base-context - [{:keys [tx-id origin user timestamp flags metadata status] :as opts}] +(defn- base-context + [{:keys [tx-id origin user timestamp flags metadata status]}] @@ - ([context {:keys [commit-id commit-message branch path document-id operation flags metadata timestamp] - :as overrides}] + ([context {:keys [commit-id commit-message branch path document-id operation flags metadata timestamp]}] @@ - [storage {:keys [on-complete] :as opts} f] + [_storage {:keys [on-complete] :as opts} f]Also applies to: 127-127, 168-168
docs/architecture.md (1)
59-62: Add language tag to Git CLI example.
markdownlint (MD040) flags the new fenced block because it lacks a language. Tagging it asbashkeeps tooling green and matches the rest of the docs.-``` +```bash git log --show-notes=chrondb git notes --ref=chrondb show <commit></blockquote></details> <details> <summary>docs/api.md (1)</summary><blockquote> `195-198`: **Add language tag to Git commands block.** The new fenced example lacks a language hint, tripping markdownlint (MD040). Mark it as `bash` so tooling stays happy. ```diff -``` +```bash git log --show-notes=chrondb git notes --ref=chrondb show <commit-hash></blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 9c46bdccb6fb7ea6fc30d16be03f86a2103aec1f and 706eda39f594a3359e23b5f610587b641b01b944. </details> <details> <summary>📒 Files selected for processing (19)</summary> * `AGENT.md` (1 hunks) * `docs/api.md` (2 hunks) * `docs/architecture.md` (1 hunks) * `docs/examples-clojure.md` (1 hunks) * `src/chrondb/api/redis/core.clj` (15 hunks) * `src/chrondb/api/sql/execution/query.clj` (4 hunks) * `src/chrondb/api/v1.clj` (4 hunks) * `src/chrondb/api/v1/routes.clj` (3 hunks) * `src/chrondb/cli/http.clj` (1 hunks) * `src/chrondb/storage/git/commit.clj` (2 hunks) * `src/chrondb/storage/git/core.clj` (1 hunks) * `src/chrondb/storage/git/document.clj` (2 hunks) * `src/chrondb/storage/git/history.clj` (3 hunks) * `src/chrondb/storage/git/notes.clj` (1 hunks) * `src/chrondb/transaction/core.clj` (1 hunks) * `test/chrondb/storage/git/document_test.clj` (3 hunks) * `test/chrondb/storage/git/history_test.clj` (2 hunks) * `test/chrondb/storage/git/notes_test.clj` (1 hunks) * `test/chrondb/transaction/core_test.clj` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (1)</summary> <details> <summary>📚 Learning: 2025-10-12T09:40:13.442Z</summary>Learnt from: avelino
Repo: moclojer/chrondb PR: 59
File: src/chrondb/core.clj:158-171
Timestamp: 2025-10-12T09:40:13.442Z
Learning: ChronDB's GitStorage only implements the internal protocol chrondb.storage.protocol/Storage and is not a Closeable. There is no simple way to use with-open or try-with-resources patterns without changing the storage's public API. The storage remains open and in use by the server throughout the process lifetime.**Applied to files:** - `src/chrondb/api/v1.clj` - `test/chrondb/storage/git/history_test.clj` - `test/chrondb/storage/git/notes_test.clj` - `docs/api.md` - `src/chrondb/transaction/core.clj` - `src/chrondb/storage/git/commit.clj` </details> </details><details> <summary>🪛 GitHub Actions: Lint project with clj-kondo</summary> <details> <summary>src/chrondb/api/v1/routes.clj</summary> [warning] 132-132: clj-kondo: Redundant nested call: merge. </details> <details> <summary>src/chrondb/api/v1.clj</summary> [warning] 81-81: clj-kondo: unused binding ctx. --- [warning] 106-106: clj-kondo: unused binding ctx. --- [warning] 178-178: clj-kondo: unused binding ctx. --- [warning] 215-215: clj-kondo: unused binding ctx. </details> <details> <summary>src/chrondb/transaction/core.clj</summary> [warning] 46-46: clj-kondo: unused binding opts. --- [warning] 128-128: clj-kondo: unused binding overrides. --- [warning] 168-168: clj-kondo: unused binding storage. </details> <details> <summary>src/chrondb/storage/git/history.clj</summary> [warning] 207-207: clj-kondo: unused binding data-dir. --- [warning] 210-210: clj-kondo: unused binding id-only. </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/architecture.md</summary> 59-59: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>docs/api.md</summary> 195-195: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ 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). (3)</summary> * GitHub Check: test * GitHub Check: Run SQL Protocol Benchmark * GitHub Check: test </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| (defn execute-redis-write | ||
| ([storage command args f] | ||
| (execute-redis-write storage command args {} f)) | ||
| ([storage command args opts f] | ||
| (tx/with-transaction [storage (redis-tx-options command args opts)] | ||
| (f)))) |
There was a problem hiding this comment.
Redis transaction callback never sees the transactional storage
tx/with-transaction rebinding is purely lexical. Because we pass a zero-arity closure, it closes over the pre-transaction storage. Every (storage/...) call inside Redis handlers is still hitting the original handle, so the transaction context (including git notes) never runs. Please thread the transaction-scoped storage into the callback (and update the call sites) so writes execute under the new binding.
(defn execute-redis-write
([storage command args f]
(execute-redis-write storage command args {} f))
([storage command args opts f]
- (tx/with-transaction [storage (redis-tx-options command args opts)]
- (f))))
+ (tx/with-transaction [tx-storage (redis-tx-options command args opts)]
+ (f tx-storage))))After this change, each caller should accept the transaction storage (e.g. (fn [tx-storage] (storage/save-document tx-storage doc) ...)).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/chrondb/api/redis/core.clj around lines 57 to 62, the transaction
callback is a zero-arity closure that closes over the pre-transaction storage so
transactional operations never see the rebinding; change execute-redis-write to
invoke the callback with the transaction-scoped storage (inside
tx/with-transaction call pass the rebounded storage into f), and update all
callers to accept and use that tx-storage (e.g. change (fn [] ...) to (fn
[tx-storage] (storage/save-document tx-storage ...))).
| (let [context (build-request-context request {:metadata {:endpoint "/api/v1/restore"}}) | ||
| params* (keywordize-params params) | ||
| backup-file (:tempfile params) | ||
| result (handlers/handle-restore storage backup-file (merge params* context))] | ||
| (-> (response/response (:body result)) |
There was a problem hiding this comment.
Restore endpoint never gets the uploaded file
Ring’s :params map keeps string keys, so (:tempfile params) is always nil. The handler therefore receives backup-file nil and responds with “file upload is required” even when a file was posted. Pull the upload from the keywordized params (and plumb the filename) before invoking handle-restore.
- (let [context (build-request-context request {:metadata {:endpoint "/api/v1/restore"}})
- params* (keywordize-params params)
- backup-file (:tempfile params)
- result (handlers/handle-restore storage backup-file (merge params* context))]
+ (let [context (build-request-context request {:metadata {:endpoint "/api/v1/restore"}})
+ params* (keywordize-params params)
+ upload (:tempfile params*)
+ backup-file (:tempfile upload)
+ filename (:filename upload)
+ result (handlers/handle-restore storage backup-file
+ (cond-> (merge params* context)
+ filename (assoc :filename filename)))]With this change the uploaded archive reaches handle-restore, and transaction metadata can record the filename.
🤖 Prompt for AI Agents
In src/chrondb/api/v1/routes.clj around lines 316 to 320, the code reads the
uploaded file from the non-keywordized params so backup-file is nil; change it
to pull the upload from the already keywordized params* and extract both its
:tempfile and :filename before calling handle-restore (for example, let [upload
(:file params*) backup-file (:tempfile upload) filename (:filename upload)] and
then call (handlers/handle-restore storage backup-file (merge params* context
{:filename filename})) so the handler receives the actual tempfile and the
original filename).
| request-headers (cond-> {"Accept" "application/json" | ||
| "X-ChronDB-Origin" "cli"} | ||
| (:token cfg) (assoc "Authorization" (str "Bearer " (:token cfg))) | ||
| headers (merge headers)) |
There was a problem hiding this comment.
Ensure the CLI origin header remains trustworthy.
Because the user-provided headers map is merged after the defaults, anyone calling this helper can override "X-ChronDB-Origin" and spoof a different origin. That defeats the point of emitting a stable origin for transaction metadata and lets clients mis-tag commits. Please set the origin header after merging user headers so the CLI always identifies itself as "cli".
- request-headers (cond-> {"Accept" "application/json"
- "X-ChronDB-Origin" "cli"}
- (:token cfg) (assoc "Authorization" (str "Bearer " (:token cfg)))
- headers (merge headers))
+ request-headers (-> {"Accept" "application/json"}
+ (cond-> (:token cfg)
+ (assoc "Authorization" (str "Bearer " (:token cfg))))
+ (merge headers)
+ (assoc "X-ChronDB-Origin" "cli"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| request-headers (cond-> {"Accept" "application/json" | |
| "X-ChronDB-Origin" "cli"} | |
| (:token cfg) (assoc "Authorization" (str "Bearer " (:token cfg))) | |
| headers (merge headers)) | |
| request-headers (-> {"Accept" "application/json"} | |
| (cond-> (:token cfg) | |
| (assoc "Authorization" (str "Bearer " (:token cfg)))) | |
| (merge headers) | |
| (assoc "X-ChronDB-Origin" "cli")) |
🤖 Prompt for AI Agents
In src/chrondb/cli/http.clj around lines 45 to 48, the user-supplied headers are
currently merged after the defaults which allows callers to override
"X-ChronDB-Origin"; change the build order so user headers are merged into the
defaults first and then explicitly set "X-ChronDB-Origin" to "cli" (and finally
assoc the Authorization header when :token is present) so callers cannot spoof
the CLI origin.
| {:note (cond-> {:document-id (:id document) | ||
| :operation "save-document"} | ||
| table-name (assoc :metadata {:table table-name}))}) | ||
|
|
There was a problem hiding this comment.
Document note keys should use snake_case.
The options map hands commit-virtual a note with :document-id, which serializes to document-id. Consumers (and the new tests) look for document_id, so the field comes back nil. Switch both save/delete paths to :document_id to align with the documented schema.
- {:note (cond-> {:document-id (:id document)
+ {:note (cond-> {:document_id (:id document)
:operation "save-document"}
table-name (assoc :metadata {:table table-name}))})
...
- {:note (cond-> {:document-id id
+ {:note (cond-> {:document_id id
:operation "delete-document"
:flags ["delete"]}
table-name (assoc :metadata {:table table-name}))})Also applies to: 340-343
🤖 Prompt for AI Agents
In src/chrondb/storage/git/document.clj around lines 201-204 (and also adjust
the identical case at lines 340-343), the note map keys use :document-id which
serializes to document-id but consumers expect snake_case document_id; change
both save and delete note constructions to use :document_id instead of
:document-id, and preserve the existing :operation and optional :metadata/:table
behavior so the serialized note uses document_id and aligns with the documented
schema.
…lways record commit notes - Add regression test coverage to assert that every mutation (insert/update/delete) via SQL records a commit context note with unique `tx_id` and correct `origin` - Validate commit note metadata for inserts (document-count), updates (updated doc IDs), and deletes (delete flags, target doc ID) - Cover concurrent insert scenarios for unique transaction IDs and context isolation - Ensure every path leverages the shared `tx/context-for-commit` helper for structured note payloads - Prevents silent note omissions and guards chronological auditability invariants Signed-off-by: Avelino <[email protected]>
- Refactor `commit-virtual` so all Git write operations add a structured transaction note (with `commit-id`, `commit-message`, and branch, plus propagated metadata if available) via `notes/add-git-note`. - New note payload is built by merging intrinsic commit info, optional path, and the active transaction metadata using `tx/context-for-commit`. - Any failure to attach a git note now aborts the commit and logs an error, ensuring no commits are created without contextual notes. - Unit/integration test coverage added (`test-commit-virtual-note-failure-does-not-advance-ref`) to assert that commit refs are rolled back if note attachment fails, preventing orphaned commits without notes. See: #62 (comment) Signed-off-by: Avelino <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
test/chrondb/storage/git/notes_test.clj (3)
55-59: Clarify assertion targets for consistency.The test checks
tx_idon themergedreturn value (line 57) but verifiesoriginandflagson the separately-readnote(lines 58-59). While this may intentionally validate both the return value and the persisted note, the inconsistency can be confusing.Consider checking all fields on the same object for clarity, or add a comment explaining why both objects are being validated:
- (let [merged (notes/add-git-note git commit-id payload) - note (notes/read-note git commit-id)] - (is (= "tx-123" (:tx_id merged))) - (is (= "rest" (:origin note))) - (is (= ["bulk-load"] (:flags note)))) + (let [merged (notes/add-git-note git commit-id payload) + note (notes/read-note git commit-id)] + ;; Verify both return value and persisted note + (is (= "tx-123" (:tx_id merged))) + (is (= "rest" (:origin merged))) + (is (= ["bulk-load"] (:flags merged))) + (is (= note merged)))
38-61: Consider adding edge case test coverage.The current test validates the happy path successfully. Consider adding tests for edge cases such as:
- Reading a note that doesn't exist
- Invalid or nil commit-id
- Malformed note payload
- Large note payloads
63-86: LGTM with a suggestion for enhanced coverage.The test correctly validates flag deduplication and metadata merging. The use of set comparison for flags (lines 82-83) appropriately handles the unordered nature of the collection. The cleanup pattern with
protocol/closeis correct. Based on learnings.Consider adding a test case where both note additions include metadata to verify complete metadata merging behavior:
(deftest merge-metadata (testing "merges metadata from multiple notes" (let [storage (create-test-storage) repository (:repository storage) git (Git/wrap repository) commit-id (commit/commit-virtual git "main" "baz.txt" "data" "Third commit" "Test User" "[email protected]")] (try (notes/add-git-note git commit-id {:tx_id "tx-789" :metadata {:key1 "value1"}}) (let [merged (notes/add-git-note git commit-id {:metadata {:key2 "value2"}}) note (notes/read-note git commit-id)] (is (= {:key1 "value1" :key2 "value2"} (:metadata note)))) (finally (protocol/close storage))))))test/chrondb/api/sql/execution/transaction_test.clj (1)
76-102: Consider verifying storage state in addition to transaction contexts.The test correctly validates transaction-context isolation (distinct
tx_idvalues, correct origin, and metadata). However, it doesn't assert that the documents were actually persisted in the mock storage (@docs) or that the final document count matches the thread count. Adding these checks would strengthen confidence that concurrent writes don't corrupt storage state.Consider adding assertions like:
(is (every? #(= "sql" (:origin %)) @contexts) "Origin should be sql for all contexts") (is (every? #(= 1 (get-in % [:metadata :document-count])) @contexts) - "Each insert metadata should record a single document")))) + "Each insert metadata should record a single document") + (is (= thread-count (count @docs)) "All documents should be persisted in storage"))))src/chrondb/api/sql/execution/query.clj (1)
698-711: Indexing inside transaction may impact performance.The batch insert correctly wraps all document saves in a single transaction for atomicity. However, index operations (lines 703-709) execute within the transaction block. If indexing is I/O-intensive or slow, it could hold the transaction open longer than necessary. Consider whether indexing should be deferred to after transaction commit, or verify that the performance impact is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/publish-docker.yml(1 hunks)src/chrondb/api/sql/execution/query.clj(7 hunks)test/chrondb/api/sql/execution/transaction_test.clj(1 hunks)test/chrondb/storage/git/notes_test.clj(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-12T09:40:13.442Z
Learnt from: avelino
Repo: moclojer/chrondb PR: 59
File: src/chrondb/core.clj:158-171
Timestamp: 2025-10-12T09:40:13.442Z
Learning: ChronDB's GitStorage only implements the internal protocol chrondb.storage.protocol/Storage and is not a Closeable. There is no simple way to use with-open or try-with-resources patterns without changing the storage's public API. The storage remains open and in use by the server throughout the process lifetime.
Applied to files:
test/chrondb/storage/git/notes_test.clj
⏰ 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). (4)
- GitHub Check: test
- GitHub Check: build-and-push
- GitHub Check: Run SQL Protocol Benchmark
- GitHub Check: test
🔇 Additional comments (19)
.github/workflows/publish-docker.yml (2)
3-7: Add pull_request trigger for CI validation.Well done restructuring the workflow triggers to explicitly include both
push(to main) andpull_requestevents. This enables validation builds on PRs before merge—a best practice for CI/CD.
26-32: Conditionals correctly gate credentials and registry pushes to main.The conditionals on the login (line 28) and build-push (line 44) steps correctly restrict actual registry operations to
pushevents on the main branch, while permitting validation builds on PR events. This is secure and follows GitHub Actions best practices.Also applies to: 44-44
test/chrondb/storage/git/notes_test.clj (5)
1-10: LGTM!The namespace declaration and imports are well-organized and appropriate for testing Git note functionality.
12-21: LGTM!The test configuration properly isolates tests by disabling push and using a dedicated test directory.
23-26: LGTM!The recursive directory deletion helper follows the correct pattern (deleting from leaves to root).
28-33: LGTM!The fixture properly ensures test isolation by cleaning the repository before each test and overriding configuration.
35-36: LGTM!Clean test helper for creating storage instances.
test/chrondb/api/sql/execution/transaction_test.clj (4)
69-74: LGTM!Helper functions are straightforward and correctly implemented.
104-118: LGTM!The test correctly validates that UPDATE operations produce transaction contexts with the "update" flag, proper origin, and document metadata.
120-135: Review comment is incorrect. No changes needed.The code uses
:document_id(underscore) on line 133 becausetx/context-for-commitexplicitly transforms the input parameter:document-id(hyphen) into:document_id(underscore) when constructing the returned context payload. This transformation is defined at src/chrondb/transaction/core.clj:145 withdocument-id (assoc :document_id document-id). The test correctly reads the transformed key from the context object.Likely an incorrect or invalid review comment.
10-67: The mock captures transaction contexts at the wrong layer; clarify whether these tests should verify git integration or only SQL layer behavior.The mock storage calls
tx/context-for-commitdirectly insave-documentanddelete-documentmethods. However, in production code (src/chrondb/storage/git/commit.clj:197),context-for-commitis called in the Git commit handler after storage operations complete, not within the storage layer itself.The tests as written verify that SQL query handlers (
query/handle-insert-case, etc.) properly wrap operations intx/with-transactionand thatcontext-for-commitworks when invoked within that context. However, they do not test the actual Git note attachment flow where the final payload is built.This is acceptable if the tests are scoped to SQL layer behavior only. But if the intent is to verify end-to-end transaction context flow including Git integration, the mock should either:
- Remove direct
context-for-commitcalls from storage methods, or- Be paired with integration tests that exercise the full git commit layer
src/chrondb/api/sql/execution/query.clj (8)
1-14: LGTM!The namespace imports are appropriate for integrating transaction context and index operations.
18-30: LGTM!The flag normalization logic correctly flattens nested collections, removes blanks, deduplicates, and returns
nilfor empty results (which is properly handled by the caller at line 41).
32-41: LGTM!Transaction options builder correctly constructs metadata from operation parameters, merges additional metadata, normalizes flags, and returns a well-formed options map with origin "sql".
476-487: LGTM!Single-document insert correctly wrapped in transaction with appropriate metadata (operation, id, table, branch).
489-511: LGTM!Document update correctly wrapped in transaction with proper metadata.
513-520: LGTM!Document deletion correctly wrapped in transaction with "delete" flag.
736-786: LGTM!Batch UPDATE correctly wrapped in transaction with "update" flag and document metadata. All matching documents are updated atomically.
788-856: LGTM!DELETE operation correctly wrapped in transaction with "delete" flag, including both storage and index removal.
…structuring - Remove unused `:as ctx` destructurings that trigger linter warnings - No logic/functionality changed, only signature cleanup for commit standards Signed-off-by: Avelino <[email protected]>
- Refactor `commit-virtual` so all Git write operations add a structured transaction note (with `commit-id`, `commit-message`, and branch, plus propagated metadata if available) via `notes/add-git-note`. - New note payload is built by merging intrinsic commit info, optional path, and the active transaction metadata using `tx/context-for-commit`. - Any failure to attach a git note now aborts the commit and logs an error, ensuring no commits are created without contextual notes. - Unit/integration test coverage added (`test-commit-virtual-note-failure-does-not-advance-ref`) to assert that commit refs are rolled back if note attachment fails, preventing orphaned commits without notes. See: #62 (comment) Signed-off-by: Avelino <[email protected]>
This PR introduces a transaction core module and structured Git note support for every write operation in ChronDB, fulfilling architectural requirements for chronological traceability and external correlation/audit.
Highlights:
chrondb.transaction.corenamespace. Provides a shared transaction context, uniquetx_idgeneration, and helpers for capturing metadata (origin, user, flags) across all entrypoints (REST, SQL, Redis).chrondb.storage.git.notesmodule. Every commit now stores a JSON note under thechrondbnamespace, containing:tx_id, origin (protocol/source), optionaluser, semantic flags (bulk-load,migration,rollback, etc.), and client/request metadata (e.g.X-Request-Id).with-transaction) usage, and how to pass metadata via HTTP headers or protocol attributes to propagate context into Git notes. Seedocs/api.md, "Transaction Operations" and "Transaction Metadata and Git Notes".git notes, failure modes (e.g., repository contention, branch mutation), and regression coverage (notes always present after a commit).Rationale:
ChronDB's Git-based architecture enables full auditability and time-travel. Storing rich transaction metadata as Git notes ensures every operation is contextualized—supporting debugging, lineage analysis, external audit, and advanced correlation across distributed systems. This change centralizes transaction and commit metadata handling, eliminating per-protocol ad-hoc logic and reducing the risk of silent context loss.
See also:
src/chrondb/storage/git/notes.clj(implementation)src/chrondb/transaction/core.clj(transaction core logic)test/chrondb/storage/git/notes_test.clj,test/chrondb/transaction/core_test.cljdocs/api.mdand process requirements in AGENT.md.BREAKING: None, but all future write code must route through the transaction context to guarantee note coverage and protocol metadata flow.
fixed #43
fixed #44
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests