Repository navigation
feat/next hex task - #174
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a new unit test for cache strict equality, updates an existing journal test to capture and validate edges, registers the new test in the build, and updates multiple documentation files to reflect OID-first terminology and progress notes. No public APIs or runtime behavior changed. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
docs/cli/gitmind-cache-rebuild.md (2)
17-24: Make ToC a level-2 heading to satisfy docs rules and markdownlint.Change “Table of Contents” to an H2.
-Table of Contents +## Table of Contents
1-12: Move SPDX and copyright blocks after the YAML front matterRemove the first two lines (the SPDX header and copyright notice) and reinsert them immediately after the closing
---of the front matter in docs/cli/gitmind-cache-rebuild.md, then runmake docs-verify.docs/architecture/deprecations/legacy-sha-fields.md (2)
13-21: Promote ToC to H2 heading.-Table of Contents +## Table of Contents
1-12: Insert SPDX/license after the front matter (currently missing).--- title: Deprecation Notice: Legacy SHA-1 Byte Arrays in Edges description: Plan to remove SHA-1 specific arrays in favor of OID-only APIs and storage. audience: [developers] domain: [architecture] tags: [deprecation, oid] status: planned last_updated: 2025-09-29 --- + +<!-- SPDX-License-Identifier: LicenseRef-MIND-UCAL-1.0 --> +<!-- © 2025 J. Kirby Ross / Neuroglyph Collective -->docs/cli/gitmind-link.md (2)
17-24: Make ToC an H2 to pass docs checks.-Table of Contents +## Table of Contents
1-12: Move SPDX/license after the front matter-<!-- SPDX-License-Identifier: LicenseRef-MIND-UCAL-1.0 --> -<!-- © 2025 J. Kirby Ross / Neuroglyph Collective --> --- title: git-mind link description: Create a semantic connection between two paths. audience: [users, developers] domain: [cli] tags: [cli, link] status: stable last_updated: 2025-09-15 --- +<!-- SPDX-License-Identifier: LicenseRef-MIND-UCAL-1.0 --> +<!-- © 2025 J. Kirby Ross / Neuroglyph Collective -->Run
make docs-verifyto ensure markdownlint and TOC checks pass.
🧹 Nitpick comments (12)
docs/cli/gitmind-cache-rebuild.md (1)
51-55: OID-first terminology LGTM; consider cross-linking the deprecation note.The forward/reverse index wording and the temporary “SHA” note read well. Optional: add a “See also” link to docs/architecture/deprecations/legacy-sha-fields.md to anchor the rename rationale.
AGENTS.md (1)
418-425: Good activity log; mirror migration notes into dated activity doc.Add a brief entry under docs/activity/2025-09-29_hexagonal.md per policy so the migration log isn’t only in AGENTS.md. I can draft it from this block if helpful.
Based on learnings
docs/architecture/deprecations/legacy-sha-fields.md (1)
47-49: Clarify fallback trigger.Wording is good; add “when either side lacks a binary OID” to avoid implying a two-sided absence is required.
-Consumers should use `gm_oid_t` exclusively and compare via `gm_oid_equal`, - falling back to legacy SHA bytes only when a binary OID is unavailable. +Consumers should use `gm_oid_t` exclusively and compare via `gm_oid_equal`, + falling back to legacy SHA bytes only when either side lacks a binary OID.docs/cli/gitmind-link.md (1)
156-164: OID wording LGTM; tiny UX tweak optional.Consider adding “(aka content hashes)” after first “object IDs (OIDs)” to help users mapping old mental model.
docs/cli/gitmind.md (3)
114-116: Terminology note: mention the legacy flag for clarity.Good callout about legacy “SHA” labels. Consider adding “see --show-sha (legacy name)” to tie this to the flag users will encounter.
191-191: Consistency nit: align with list docs phrasing.To reduce confusion, mirror the list-page phrasing: “labels retain the legacy ‘SHA’ name; values are OIDs.”
1-12: Fix doc structure: SPDX after front matter; add proper ToC heading.
- Move SPDX/license comments to immediately after the front matter.
- Change the literal “Table of Contents” line to an H2 “## Table of Contents” to satisfy guidance and markdownlint.
Apply:
-<!-- SPDX-License-Identifier: LicenseRef-MIND-UCAL-1.0 --> -<!-- © 2025 J. Kirby Ross / Neuroglyph Collective --> - ---- +--- title: git-mind (command) @@ last_updated: 2025-09-15 --- + +<!-- SPDX-License-Identifier: LicenseRef-MIND-UCAL-1.0 --> +<!-- © 2025 J. Kirby Ross / Neuroglyph Collective --> @@ -Table of Contents +## Table of Contentsdocs/cli/gitmind-list.md (1)
18-24: Make ToC a proper H2 heading.Change plain text “Table of Contents” to “## Table of Contents” for consistency with docs rules.
-Table of Contents +## Table of Contentsdocs/cli/gitmind-install-hooks.md (1)
18-25: Promote ToC to H2 for consistency.Use “## Table of Contents” to align with docs rules.
-Table of Contents +## Table of Contentsdocs/code-reviews/PR174/bc1fc319303bf1a00b43b03a40992ba788996b1a.md (1)
1-12: Bring file into docs standard: title/H1, SPDX, ToC.
- Title/H1: align by setting title to “Code Review Feedback” or change H1 to match current title.
- Add SPDX/license comments after front matter.
- Add “## Table of Contents”.
--- -title: bc1fc319303bf1a00b43b03a40992ba788996b1a.md +title: Code Review Feedback description: Preserved review artifacts and rationale. @@ --- -# Code Review Feedback +# Code Review Feedback + +## Table of Contents + +<!-- SPDX-License-Identifier: LicenseRef-MIND-UCAL-1.0 --> +<!-- © 2025 J. Kirby Ross / Neuroglyph Collective -->Based on learnings
docs/TECHNICAL.md (2)
19-21: Clarify OID-first fallback condition.Consider tightening the phrasing: “falling back to SHA hex only if a binary OID is unavailable in legacy paths.” Helps avoid implying mixed-mode by default.
-- OID‑first APIs: new interfaces use `gm_oid_t` and compare via `gm_oid_equal`, - falling back to SHA hex only when a binary OID is unavailable. +- OID‑first APIs: new interfaces use `gm_oid_t` and compare via `gm_oid_equal`, + falling back to SHA hex only if a binary OID is unavailable in legacy paths.
79-80: Align with helper usage: prefer gm_ formatting helpers over direct libgit2.*Docs currently say to use
git_oid_fmt/git_oid_tostr, which conflicts with guidance to prefergm_oid_to_hex/gm_bytes_to_hex.-- OID formatting/prefixing now uses libgit2 (`git_oid_fmt`, `git_oid_tostr`). +- OID formatting/prefixing should use project helpers (`gm_oid_to_hex`, `gm_bytes_to_hex`); + avoid direct `git_oid_*` formatting in new code.Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
AGENTS.md(1 hunks)core/tests/unit/test_cache_strict_equality.c(1 hunks)core/tests/unit/test_journal_mixed_cbor.c(3 hunks)docs/TECHNICAL.md(4 hunks)docs/activity/2025-09.md(1 hunks)docs/architecture/deprecations/legacy-sha-fields.md(3 hunks)docs/cli/combined_markdown.md(10 hunks)docs/cli/gitmind-cache-rebuild.md(2 hunks)docs/cli/gitmind-install-hooks.md(3 hunks)docs/cli/gitmind-link.md(1 hunks)docs/cli/gitmind-list.md(2 hunks)docs/cli/gitmind.md(2 hunks)docs/code-reviews/PR174/bc1fc319303bf1a00b43b03a40992ba788996b1a.md(1 hunks)docs/features/Features_Ledger.md(2 hunks)meson.build(2 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
{docs/**/*.md,README.md}
📄 CodeRabbit inference engine (AGENTS.md)
{docs/**/*.md,README.md}: Docs must have front matter first, a single H1, a '## Table of Contents', and H1/title alignment; API docs include api_version; SPDX/license after front matter
Markdown style should satisfy markdownlint configuration (.markdownlint.jsonc)
Files:
docs/cli/gitmind-cache-rebuild.mddocs/cli/gitmind-install-hooks.mddocs/cli/gitmind.mddocs/TECHNICAL.mddocs/cli/gitmind-link.mddocs/cli/combined_markdown.mddocs/cli/gitmind-list.mddocs/code-reviews/PR174/bc1fc319303bf1a00b43b03a40992ba788996b1a.mddocs/architecture/deprecations/legacy-sha-fields.mddocs/activity/2025-09.mddocs/features/Features_Ledger.md
meson.build
📄 CodeRabbit inference engine (AGENTS.md)
Meson build targets should keep c_std set to c2x (targeting C23) and options defined in meson_options.txt
Files:
meson.build
docs/activity/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
After each migration, document lessons/status in docs/activity/_hexagonal.md
Files:
docs/activity/2025-09.md
**/*.{c,cc,cpp,cxx,h,hpp,hxx,m,mm}
📄 CodeRabbit inference engine (CLAUDE.md)
NEVER use
NOLINTto suppress clang-tidy warnings; ALWAYS fix the underlying issue
Files:
core/tests/unit/test_cache_strict_equality.ccore/tests/unit/test_journal_mixed_cbor.c
core/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
core/**/*.{c,h}: Use safe ops: gm_snprintf, gm_strcpy_safe, gm_memcpy_span; check return codes; treat truncation as error
Zero output buffers on failure paths before formatting or copying
Do not use GM_PATH_MAX*2 buffers; prefer GM_PATH_MAX or allocate when larger needed
Language: C23; forbid VLAs and shadowing; require explicit prototypes; warnings as errors
Includes: prefer specific headers; order/regroup per .clang-format
Naming: functions/vars lower_snake_case prefixed gm_; macros UPPER_SNAKE; types end with t; header guards start with GITMIND
Enforce OID-first behavior for identity/equality; use git_oid (gm_oid_t) in new interfaces and compare via git_oid_cmp or gm-level helpers
One-Thing rule: when touching multi-concern files, extract the edited type/function into its own file (one enum/struct/module per TU where practical)
Keep clang-tidy at zero warnings for touched files; do not introduce new diagnostics
Files:
core/tests/unit/test_cache_strict_equality.ccore/tests/unit/test_journal_mixed_cbor.c
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Formatting via clang-format (LLVM style, 4 spaces, 80 cols, right-aligned pointers); pre-commit enforces
Files:
core/tests/unit/test_cache_strict_equality.ccore/tests/unit/test_journal_mixed_cbor.c
core/tests/unit/test_*.c
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests live in core/tests/unit/ and are named test_.c; keep deterministic and isolated
Files:
core/tests/unit/test_cache_strict_equality.ccore/tests/unit/test_journal_mixed_cbor.c
core/tests/!(fakes)/**.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Test code (non-fakes) should not include libgit2 headers; prefer ports and fakes
Files:
core/tests/unit/test_cache_strict_equality.ccore/tests/unit/test_journal_mixed_cbor.c
core/tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests should rely on fakes only; integration tests may use real adapters within Docker
Files:
core/tests/unit/test_cache_strict_equality.ccore/tests/unit/test_journal_mixed_cbor.c
🧠 Learnings (9)
📓 Common learnings
Learnt from: flyingrobots
PR: neuroglyph/git-mind#169
File: docs/templates/code-review-feedback.md:26-33
Timestamp: 2025-09-16T17:10:27.477Z
Learning: The user (flyingrobots) uses Obsidian to view/edit Markdown files and prefers to keep Obsidian-specific syntax like collapsible callout markers (e.g., `[!NOTE]-` for collapsible callouts) even if they're not GitHub-compatible.
📚 Learning: 2025-09-29T12:20:59.326Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T12:20:59.326Z
Learning: Applies to docs/activity/**/*.md : After each migration, document lessons/status in docs/activity/<date>_hexagonal.md
Applied to files:
AGENTS.md
📚 Learning: 2025-09-14T09:27:33.283Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-14T09:27:33.283Z
Learning: Tag all memories saved to MEMORY BANKS with at least #git-mind
Applied to files:
docs/cli/gitmind.md
📚 Learning: 2025-09-29T12:20:59.326Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T12:20:59.326Z
Learning: Applies to core/**/*.{c,h} : Enforce OID-first behavior for identity/equality; use git_oid (gm_oid_t) in new interfaces and compare via git_oid_cmp or gm-level helpers
Applied to files:
docs/TECHNICAL.mddocs/architecture/deprecations/legacy-sha-fields.mddocs/activity/2025-09.mddocs/features/Features_Ledger.mdcore/tests/unit/test_journal_mixed_cbor.c
📚 Learning: 2025-09-29T12:20:59.326Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T12:20:59.326Z
Learning: Applies to core/src/**/{*.c,*.h} : Prefer gm_oid_to_hex and gm_bytes_to_hex helpers over direct git_oid_* formatting utilities
Applied to files:
docs/TECHNICAL.mddocs/cli/gitmind-link.mddocs/architecture/deprecations/legacy-sha-fields.mddocs/activity/2025-09.mdcore/tests/unit/test_journal_mixed_cbor.c
📚 Learning: 2025-09-13T03:13:54.145Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-13T03:13:54.145Z
Learning: Applies to core/tests/unit/test_*.c : Unit tests live in core/tests/unit/ and are named test_<module>.c; keep tests deterministic and isolated
Applied to files:
core/tests/unit/test_cache_strict_equality.c
📚 Learning: 2025-09-29T12:20:59.326Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T12:20:59.326Z
Learning: Applies to core/tests/unit/test_*.c : Unit tests live in core/tests/unit/ and are named test_<module>.c; keep deterministic and isolated
Applied to files:
core/tests/unit/test_cache_strict_equality.c
📚 Learning: 2025-09-29T12:20:59.326Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T12:20:59.326Z
Learning: Applies to core/tests/!(fakes)/**.{c,h} : Test code (non-fakes) should not include libgit2 headers; prefer ports and fakes
Applied to files:
core/tests/unit/test_cache_strict_equality.c
📚 Learning: 2025-09-29T12:20:59.326Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T12:20:59.326Z
Learning: Applies to core/tests/fakes/**/*.c : Deterministic fakes/mocks/stubs must exist for each port under core/tests/fakes/**
Applied to files:
core/tests/unit/test_cache_strict_equality.c
🧬 Code graph analysis (2)
core/tests/unit/test_cache_strict_equality.c (2)
core/include/gitmind/security/memory.h (1)
gm_memset_safe(47-59)core/src/cache/query.c (1)
gm_cache_is_stale(157-208)
core/tests/unit/test_journal_mixed_cbor.c (3)
core/include/gitmind/util/memory.h (1)
gm_memcpy_span(32-47)core/src/edge/edge.c (1)
gm_edge_encode_cbor(260-361)core/src/journal/reader.c (2)
gm_journal_read(372-376)gm_journal_read_attributed(379-383)
🔇 Additional comments (31)
docs/activity/2025-09.md (1)
28-36: Strict-equality test is wired correctly; please verify docs build and ledger entry
test_cache_strict_equalityis declared and linked in meson.build (lines 321 & 393).- Cannot run
make docs-verifyhere; confirm your local docs build passes and thatGM.MVP.T001appears in docs/activity/2025-09.md.docs/cli/gitmind.md (1)
120-126: LGTM: OID-first wording.The AUGMENTS/OID explanation is clear and consistent with the PR’s shift to OIDs.
docs/cli/gitmind-list.md (2)
93-95: LGTM: flag description matches OID-first migration.Clear that the flag name is legacy while values are OIDs.
269-274: LGTM: output metadata now references OIDs.Matches the updated flag semantics.
docs/cli/gitmind-install-hooks.md (3)
44-50: LGTM: OID terminology in evolution example.Example correctly switches from SHA to OID and is easy to follow.
121-128: LGTM: step uses “old blob OID”.Accurately reflects the OID-first behavior.
198-205: LGTM: edge format fields renamed to src_oid/tgt_oid.Field names and comments align with migration.
docs/TECHNICAL.md (2)
59-60: LGTM: test focus items.The CBOR round‑trip and
gm_snprintftruncation paths callout is helpful and concrete.
71-76: LGTM: recent changes capture strict-equality guardrails.The gm_oid_equal‑before‑hex fallback sequencing is documented clearly.
core/tests/unit/test_cache_strict_equality.c (8)
1-18: LGTM: Clean headers and includesThe SPDX license header and include structure are well-organized and appropriate for this unit test.
19-26: Well-designed test harness structureThe
test_git_repo_port_tstructure effectively extends the base port type and provides the necessary state management for simulating different reference scenarios in the tests.
28-196: Comprehensive and well-structured stub implementationsThe stub functions properly implement the full vtable interface while providing realistic behavior for the test scenarios. The
reference_tipfunction contains the core logic needed to test different OID scenarios, and all stubs handle parameters appropriately.
55-88: Excellent implementation of reference tip simulationThe
reference_tipfunction effectively simulates different reference scenarios with proper prefix matching, bounds checking, and stateful sequence management. The use ofgm_memset_safeand careful index management shows good attention to safety.
208-254: Well-organized helper functions and vtableThe vtable setup and helper functions are cleanly implemented. The
set_tipfunction's flexibility to handle both binary OID and hex scenarios is particularly well-suited for the test scenarios.
273-302: Excellent OID-first equality test scenariosThese scenarios effectively test the core OID-first behavior requirement. Scenario 1 validates that OID matches ignore hex mismatches (correct behavior), while scenario 2 confirms OID mismatches are properly detected. This aligns perfectly with the coding guidelines for OID-first equality enforcement.
303-329: Comprehensive coverage of hex fallback scenariosThese scenarios provide excellent coverage of the fallback behavior when binary OIDs are missing. The test design effectively validates both the positive case (hex match when OID missing) and negative case (hex mismatch detection), ensuring robust fallback handling.
267-267: Clean test structure and outputThe test main function has good organization with clear scenario separation and appropriate success messaging for CI integration.
Also applies to: 330-332
meson.build (2)
321-325: LGTM: Standard test executable definitionThe new
test_cache_strict_equalityexecutable follows the established pattern with correct source path, include directories, dependencies, and linking. The configuration matches other unit tests in the file.
393-393: Correct test registrationThe test registration follows the established pattern and naming convention, properly integrating the new test into the build system.
docs/cli/combined_markdown.md (4)
143-144: Good terminology update to OID-first languageThe update from "source SHA" to "source object ID (OID)" and similar for target aligns well with the codebase's OID-first approach while maintaining clear functional descriptions.
Based on coding guidelines
146-146: Thoughtful handling of terminology transitionThe approach of updating core terminology to "OID" while noting that "legacy CLI output may still print 'SHA'" provides a good balance during the transition period. The technical accuracy is improved while acknowledging user-facing compatibility.
Also applies to: 234-234
318-318: Comprehensive and consistent terminology updatesThe updates throughout the document consistently replace "blob SHA" with "blob object ID" and related terminology. The changes maintain technical accuracy while providing a cohesive OID-first presentation across different sections.
Based on coding guidelinesAlso applies to: 659-659, 663-663, 1260-1260, 1265-1265, 1336-1336
882-882: Clear documentation of flag transitionThe documentation for the
--show-shaflag effectively communicates that while the flag name retains the legacy "SHA" label, it now displays blob object IDs (OIDs). This provides good user guidance during the terminology transition.Also applies to: 1060-1060
docs/features/Features_Ledger.md (2)
8-8: Accurate task completion trackingThe update to GM.MVP.T001 "Finalize strict equality" from incomplete (0/4) to complete (4/4) with 100% progress bar accurately reflects the implementation of strict equality regression tests and documentation updates in this PR. The timestamp update to 2025-09-29 properly tracks the completion date.
Also applies to: 140-144
141-144: Well-aligned task completion detailsThe completion of all four sub-tasks (audit equality helpers, add regression tests, update docs, sweep references) accurately reflects the scope of work completed in this PR, including the new strict equality test and documentation updates.
core/tests/unit/test_journal_mixed_cbor.c (6)
22-30: Enhanced context structures for better validationThe addition of
lastfields to store complete edge data enables more thorough validation of journal read operations beyond simple counting. The newattr_ctx_tstructure maintains consistency with the pattern.
32-58: Improved callback implementations with proper validationThe updated callbacks now include proper parameter validation, safe memory copying with
gm_memcpy_span, and clear error handling. The rename fromcount_*tocapture_*better reflects their enhanced functionality.
Based on coding guidelines
82-87: Clean test data setup with safe operationsThe upfront creation of
rawAandrawBbuffers with immediate population of edge SHA fields usinggm_memcpy_spanprovides clean test data organization and follows safe coding practices.
Based on coding guidelines
125-128: Proper setup for OID validationThe creation of expected OID values using
gm_oid_from_rawprovides a solid foundation for validating that the journal read operations preserve OID data correctly.
Based on coding guidelines
130-135: Comprehensive OID validation for basic edgesThe enhanced validation now verifies that captured edges have non-zero OIDs and that they match expected values. This provides much stronger testing of OID-first behavior compared to simple counting.
Based on coding guidelines
137-142: Consistent validation pattern for attributed edgesThe attributed edge validation follows the same comprehensive pattern as basic edges, ensuring that both edge types properly preserve OID data through the journal read process.
Based on coding guidelines
Docstrings generation was requested by @flyingrobots. * #174 (comment) The following files were modified: * `core/tests/unit/test_cache_strict_equality.c` * `core/tests/unit/test_journal_mixed_cbor.c`
|
Note Generated docstrings for this pull request at #175 |
📝 Add docstrings to `feat/next-hex-task`
1 participant
Summary by CodeRabbit
Documentation
Tests