Repository navigation
feat(core/oid): migrate edge/cache to gm helpers - #173
Conversation
WalkthroughAdds a new gm_oid utility API (gm_oid_equal, gm_oid_is_zero, gm_oid_from_raw, gm_oid_from_hex, and related helpers), migrates core modules and many unit tests from libgit2 git_oid_* to the gm_oid_* API, tightens cache/journal OID handling and init, removes multiple archived PR172 code-review artifacts, and adds PR173 review artifacts and activity docs updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as CacheUser
participant CQ as cache/query.c
participant OID as gm_oid utils
participant Meta as CacheMeta
Caller->>CQ: gm_cache_is_stale(repo, branch)
CQ->>Meta: read meta (tip oids)
alt binary tip present
CQ->>OID: gm_oid_is_zero(meta.journal_tip_oid_bin)
alt not zero
CQ->>OID: gm_oid_equal(meta.journal_tip_oid_bin, current_tip.oid)
OID-->>CQ: equal?/not
else zero
CQ->>OID: gm_oid_from_hex(&cached_tip, meta.journal_tip_oid)
OID-->>CQ: GM_OK/ERR
end
else no binary tip
CQ->>OID: gm_oid_from_hex(&cached_tip, meta.journal_tip_oid)
OID-->>CQ: GM_OK/ERR
end
CQ-->>Caller: stale?/fresh status
sequenceDiagram
autonumber
actor Producer as EdgeEncoder
participant EDGE as edge/edge.c
participant OID as gm_oid utils
Producer->>EDGE: encode/decode edge
EDGE->>OID: gm_oid_is_zero(src_oid/tgt_oid)
alt OIDs present
EDGE->>OID: gm_oid_equal(...) for comparisons
OID-->>EDGE: equal?/not
else fallback
EDGE->>OID: gm_oid_from_raw(&oid, sha_raw, GM_OID_RAWSZ)
OID-->>EDGE: GM_OK/ERR
end
EDGE-->>Producer: result (edge state)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
docs/activity/2025-09.md (1)
1-12: Add SPDX header, H1, and Table of ContentsConform this activity log to house style: SPDX after front matter, single H1 matching title, and a ToC.
As per coding guidelines
--- title: Activity 2025-09 description: Archived agent activity logs. audience: [contributors] domain: [quality] tags: [activity] status: archive last_updated: 2025-09-29 --- -## 2025-09-29 +<!-- SPDX-License-Identifier: LicenseRef-MIND-UCAL-1.0 --> + +# Activity 2025-09 + +## Table of Contents +- 2025-09-29 +- 2025-09-20 +- 2025-09-19 +- 2025-09-14 + +## 2025-09-29core/tests/unit/test_edge_oid_fallback.c (1)
23-25: Replacestrcpywith safe gm string API and check return codesTests must use gm_strcpy_safe/gm_snprintf per guidelines; also check return codes.
As per coding guidelines
#include "gitmind/edge.h" #include "gitmind/util/oid.h" #include "gitmind/error.h" +#include "gitmind/util/strings.h" @@ - strcpy(a.src_path, "A"); strcpy(a.tgt_path, "B"); - strcpy(b.src_path, "A"); strcpy(b.tgt_path, "B"); + assert(gm_strcpy_safe(a.src_path, sizeof a.src_path, "A") == 0); + assert(gm_strcpy_safe(a.tgt_path, sizeof a.tgt_path, "B") == 0); + assert(gm_strcpy_safe(b.src_path, sizeof b.src_path, "A") == 0); + assert(gm_strcpy_safe(b.tgt_path, sizeof b.tgt_path, "B") == 0);core/src/domain/cache/edge_map.c (1)
79-99: Disallowbucket_count == 0to prevent OOB access in addWith zero buckets,
gm_edge_map_addcomputes index 0 and dereferencesmap->buckets[0], which is OOB. Reject zero at creation.gm_result_void_t gm_edge_map_create(size_t bucket_count, gm_edge_map_t **out_map) { if (out_map == NULL) { return gm_err_void( GM_ERROR(GM_ERR_INVALID_ARGUMENT, "edge map output pointer required")); } + if (bucket_count == 0U) { + return gm_err_void( + GM_ERROR(GM_ERR_INVALID_ARGUMENT, "bucket_count must be > 0")); + } + gm_edge_map_t *map = calloc(1, sizeof(gm_edge_map_t)); if (map == NULL) { return gm_err_void(GM_ERROR(GM_ERR_OUT_OF_MEMORY, "edge map alloc failed")); }core/src/hooks/augment.c (2)
181-186: Replace magic number with GM_CALLBACK_STOP consistently.Return GM_CALLBACK_STOP from the callback and check for it at the call site; relying on literal 1 is brittle.
- if (ctx->scanned > LOOKBACK_LIMIT) { - return 1; /* Stop iteration */ + if (ctx->scanned > LOOKBACK_LIMIT) { + return GM_CALLBACK_STOP; /* Stop iteration */ } ... - if (error < 0 && error != 1) { /* 1 means we stopped early */ + if (error < 0 && error != GM_CALLBACK_STOP) { /* stopped early */ free(search_ctx.edges); return error; }Also applies to: 223-226
4-18: Missing <stdlib.h> for malloc/realloc/free under C23 (warnings-as-errors).malloc/realloc/free are used below without including <stdlib.h>; this will fail our C23 + Werror build.
#include "gitmind/hooks/augment.h" #include <ctype.h> +#include <stdlib.h> #include <string.h> #include <time.h>core/tests/unit/test_cache_branch_limits.c (1)
82-85: Replace unsafe strcpy with gm_strcpy_safe and check return codes.Tests follow the same safety rules: use gm_strcpy_safe and fail on truncation.
- strcpy(edge.src_path, "A"); - strcpy(edge.tgt_path, "B"); + assert(gm_strcpy_safe(edge.src_path, GM_PATH_MAX, "A") == GM_OK); + assert(gm_strcpy_safe(edge.tgt_path, GM_PATH_MAX, "B") == GM_OK);Add the needed include if not present:
+#include "gitmind/util/memory.h"As per coding guidelines
core/tests/unit/test_edge_cbor_oid.c (1)
24-27: Use gm_strcpy_safe for paths and ULID; fail on truncation.Bring test in line with string safety rules.
- strcpy(e.src_path, "src/A.c"); - strcpy(e.tgt_path, "src/B.c"); - strcpy(e.ulid, "01ARZ3NDEKTSV4RRFFQ69G5FAV"); + assert(gm_strcpy_safe(e.src_path, GM_PATH_MAX, "src/A.c") == GM_OK); + assert(gm_strcpy_safe(e.tgt_path, GM_PATH_MAX, "src/B.c") == GM_OK); + assert(gm_strcpy_safe(e.ulid, sizeof e.ulid, "01ARZ3NDEKTSV4RRFFQ69G5FAV") == GM_OK);Add header if missing:
+#include "gitmind/util/memory.h"As per coding guidelines
core/tests/unit/test_cache_query.c (1)
37-39: Prefer gm_snprintf and check truncationReplace raw snprintf with gm_snprintf and assert the result for determinism.
- char refname[128]; - snprintf(refname, sizeof refname, "refs/heads/%s", branch); + char refname[128]; + int wn = gm_snprintf(refname, sizeof refname, "refs/heads/%s", branch); + assert(wn >= 0 && (size_t)wn < sizeof refname);Include:
#include "gitmind/security/string.h"As per coding guidelines.
core/tests/unit/test_journal_safety.c (1)
4-8: Missing stdlib.h for malloc/free — will fail under C23 with Werror.Add the header to satisfy prototypes and avoid implicit declarations.
#include <assert.h> #include <stdio.h> #include <string.h> #include <sodium.h> +#include <stdlib.h>core/include/gitmind/util/oid.h (1)
4-76: Remove lingering direct libgit2 OID helper calls
- Replace the call to
git_oid_equalat core/src/adapters/git/libgit2_repository_port.c:926 withgm_oid_equal.core/src/cache/query.c (1)
169-172: Fix unsigned underflow in staleness age checkIf meta.journal_tip_time > now, (uint64_t)now - meta.journal_tip_time underflows, marking fresh caches as stale. Guard and only subtract when now_u > journal_tip_time.
- time_t now = time(NULL); - if ((uint64_t)now - meta.journal_tip_time > CACHE_MAX_AGE_SECONDS) { - return true; - } + time_t now = time(NULL); + if (now <= 0) { + return true; + } + uint64_t now_u = (uint64_t)now; + if (now_u > meta.journal_tip_time && + (now_u - meta.journal_tip_time) > CACHE_MAX_AGE_SECONDS) { + return true; + }core/tests/unit/test_journal_mixed_cbor.c (1)
56-57: Replace unsafe strcpy with gm_strcpy_safe and check resultsTests must follow the same safe string/memory rules. Replace strcpy, assert GM_OK, and keep buffers zero‑initialized on failure paths.
- strcpy(e.src_path, "src/A.c"); strcpy(e.tgt_path, "src/B.c"); + assert(gm_strcpy_safe(e.src_path, sizeof e.src_path, "src/A.c") == GM_OK); + assert(gm_strcpy_safe(e.tgt_path, sizeof e.tgt_path, "src/B.c") == GM_OK); @@ - strcpy(ae.src_path, "docs/A.md"); strcpy(ae.tgt_path, "src/C.c"); + assert(gm_strcpy_safe(ae.src_path, sizeof ae.src_path, "docs/A.md") == GM_OK); + assert(gm_strcpy_safe(ae.tgt_path, sizeof ae.tgt_path, "src/C.c") == GM_OK); @@ - strcpy(ae.attribution.author, "claude@local"); - strcpy(ae.attribution.session_id, "s1"); + assert(gm_strcpy_safe(ae.attribution.author, sizeof ae.attribution.author, "claude@local") == GM_OK); + assert(gm_strcpy_safe(ae.attribution.session_id, sizeof ae.attribution.session_id, "s1") == GM_OK);As per coding guidelines
Also applies to: 66-67, 71-73
🧹 Nitpick comments (15)
docs/code-reviews/PR173/7445bcf00c71b6f768e9eb6b1fd99d2b1c60783f.md (1)
82-99: Remove template-only “finishing touches” block from archival artifactThis checkbox block is a workflow scaffold, not archival content. Consider removing to keep the record concise.
core/src/util/oid.c (1)
94-112: Unify error on copy failureIf gm_memcpy_span ever fails here (after length check), treating it as invalid argument is clearer than BUFFER_TOO_SMALL.
- if (gm_memcpy_span(oid->id, GM_OID_RAWSZ, raw, raw_len) != 0) { + if (gm_memcpy_span(oid->id, GM_OID_RAWSZ, raw, raw_len) != 0) { gm_memset_safe(oid, sizeof(*oid), 0, sizeof(*oid)); - return GM_ERR_BUFFER_TOO_SMALL; + return GM_ERR_INVALID_ARGUMENT; }core/tests/unit/test_edge_oid_fallback.c (1)
18-21: Prefer gm_memset_safe for deterministic zero/nonzero fillsFor consistency with security APIs used elsewhere, switch to gm_memset_safe.
As per coding guidelines
- memset(a.src_sha, 0x11, GM_SHA1_SIZE); - memset(a.tgt_sha, 0x22, GM_SHA1_SIZE); - memset(b.src_sha, 0x11, GM_SHA1_SIZE); - memset(b.tgt_sha, 0x22, GM_SHA1_SIZE); + gm_memset_safe(a.src_sha, sizeof a.src_sha, 0x11, GM_SHA1_SIZE); + gm_memset_safe(a.tgt_sha, sizeof a.tgt_sha, 0x22, GM_SHA1_SIZE); + gm_memset_safe(b.src_sha, sizeof b.src_sha, 0x11, GM_SHA1_SIZE); + gm_memset_safe(b.tgt_sha, sizeof b.tgt_sha, 0x22, GM_SHA1_SIZE);Note: add
#include "gitmind/security/memory.h".core/src/hooks/augment.c (1)
191-199: *Guard realloc growth against size_t overflow; prefer sizeof(ptr).Add an overflow check before realloc and use sizeof(*ctx->edges) to avoid type drift.
- size_t new_capacity = ctx->capacity * ARRAY_GROWTH_FACTOR; - gm_edge_t *new_edges = - realloc(ctx->edges, new_capacity * sizeof(gm_edge_t)); + size_t new_capacity = ctx->capacity * ARRAY_GROWTH_FACTOR; + if (new_capacity < ctx->capacity || + new_capacity > SIZE_MAX / sizeof(*ctx->edges)) { + return GM_ERR_OUT_OF_MEMORY; + } + gm_edge_t *new_edges = + realloc(ctx->edges, new_capacity * sizeof(*ctx->edges));core/tests/unit/test_cache_shard_distribution.c (1)
21-24: Minor: prefer size_t for byte loops.Small nit to avoid signed/unsigned mix.
- for (int i = 0; i < GM_OID_RAWSZ; i++) raw[i] = (uint8_t)(lcg_next() & 0xFF); + for (size_t i = 0; i < GM_OID_RAWSZ; i++) raw[i] = (uint8_t)(lcg_next() & 0xFF);core/tests/unit/test_cache_query.c (2)
10-10: Avoid internal libgit2 headers in unit testsUse the public commit header instead of git2/sys/commit.h to reduce brittleness.
-#include <git2/sys/commit.h> +#include <git2/commit.h>
69-73: Use temp FS port for per-test repo dirHardcoding ./.gm_cache_query_tmp can leave artifacts and cause cross-run interference. Prefer a unique temp dir from the fs temp port, then pass it to git_repository_init.
I can sketch a small helper to obtain a temp dir via ctx.fs_temp_port if helpful. Based on learnings.
core/src/edge/attributed.c (1)
155-166: Make OID encoding consistent with edge.c (avoid dead fallback)src_raw is always non-null, so the ternary never picks e->src_sha. Prefer building fixed-size arrays and padding from SHA when OID is zero (mirrors core/src/edge/edge.c).
- const uint8_t *src_raw = (const uint8_t *)e->src_oid.id; - const uint8_t *tgt_raw = (const uint8_t *)e->tgt_oid.id; + uint8_t src_raw[GM_OID_RAWSZ] = {0}; + uint8_t tgt_raw[GM_OID_RAWSZ] = {0}; + if (!gm_oid_is_zero(&e->src_oid)) { + gm_memcpy_span(src_raw, sizeof src_raw, e->src_oid.id, GM_OID_RAWSZ); + } else { + gm_memcpy_span(src_raw, sizeof src_raw, e->src_sha, GM_SHA1_SIZE); + } + if (!gm_oid_is_zero(&e->tgt_oid)) { + gm_memcpy_span(tgt_raw, sizeof tgt_raw, e->tgt_oid.id, GM_OID_RAWSZ); + } else { + gm_memcpy_span(tgt_raw, sizeof tgt_raw, e->tgt_sha, GM_SHA1_SIZE); + } @@ - r = gm_cbor_write_bytes(buffer + offset, avail - offset, src_raw ? src_raw : e->src_sha, GM_OID_RAWSZ); + r = gm_cbor_write_bytes(buffer + offset, avail - offset, src_raw, GM_OID_RAWSZ); @@ - r = gm_cbor_write_bytes(buffer + offset, avail - offset, tgt_raw ? tgt_raw : e->tgt_sha, GM_OID_RAWSZ); + r = gm_cbor_write_bytes(buffer + offset, avail - offset, tgt_raw, GM_OID_RAWSZ);Keeps OID-first while ensuring deterministic encoding.
core/include/gitmind/util/oid.h (1)
57-66: Doc nit: clarify zeroing semantics on failure for gm_oid_from_raw.Implementation clears
*oidbefore validation and leaves it zeroed on error. Mirror that in the doc for consistency withgm_oid_from_hex./** * Populate an OID from raw bytes. * * @param oid Destination OID (must not be NULL) * @param raw Source byte buffer * @param raw_len Number of bytes provided in `raw` - * @return GM_OK on success, GM_ERR_INVALID_ARGUMENT when parameters are invalid + * On entry, the destination is cleared. On failure, it remains all‑zero. + * @return GM_OK on success; GM_ERR_INVALID_ARGUMENT when parameters are invalidcore/tests/unit/test_edge_attributed_cbor.c (1)
23-25: Use gm_snprintf or gm_strcpy_safe instead of strcpy in tests.Tests must use project‑safe string APIs and check truncation. Replace the
strcpycalls and include the right header. As per coding guidelines#include <string.h> #include "gitmind/edge_attributed.h" #include "gitmind/cbor/cbor.h" #include "gitmind/cbor/keys.h" #include "gitmind/types.h" +#include "gitmind/security/string.h" #include "gitmind/util/oid.h" #include "gitmind/error.h" @@ - strcpy(e.src_path, "docs/A.md"); - strcpy(e.tgt_path, "src/A.c"); - strcpy(e.ulid, "01ARZ3NDEKTSV4RRFFQ69G5FAV"); + int n = gm_snprintf(e.src_path, sizeof e.src_path, "%s", "docs/A.md"); + assert(n >= 0 && (size_t)n < sizeof e.src_path); + n = gm_snprintf(e.tgt_path, sizeof e.tgt_path, "%s", "src/A.c"); + assert(n >= 0 && (size_t)n < sizeof e.tgt_path); + n = gm_snprintf(e.ulid, sizeof e.ulid, "%s", "01ARZ3NDEKTSV4RRFFQ69G5FAV"); + assert(n >= 0 && (size_t)n < sizeof e.ulid); @@ - strcpy(e.attribution.author, "claude@local"); - strcpy(e.attribution.session_id, "sess-1"); + n = gm_snprintf(e.attribution.author, sizeof e.attribution.author, "%s", "claude@local"); + assert(n >= 0 && (size_t)n < sizeof e.attribution.author); + n = gm_snprintf(e.attribution.session_id, sizeof e.attribution.session_id, "%s", "sess-1"); + assert(n >= 0 && (size_t)n < sizeof e.attribution.session_id);Also applies to: 31-32
core/src/cache/query.c (2)
163-164: Minor: redundant local zero‑init before gm_cache_load_metagm_cache_load_meta already zero‑initializes meta. Keeping this memset is harmless but redundant. Consider removing for clarity.
- gm_cache_meta_t meta; - gm_memset_safe(&meta, sizeof(meta), 0, sizeof(meta)); + gm_cache_meta_t meta;
211-211: Prefer OID‑sized hex buffer to avoid accidental mismatchUse GM_OID_HEX_CHARS+1 instead of SHA_HEX_SIZE to couple the buffer to the encoded OID length.
- char sha_hex[SHA_HEX_SIZE]; + char sha_hex[GM_OID_HEX_CHARS + 1];core/tests/unit/test_journal_mixed_cbor.c (3)
88-89: Build refs via gm_build_ref instead of hard‑coding "refs/"Avoid double‑prefix and ensure normalization by constructing refs with gm_build_ref.
- rc = gm_journal_create_commit(&ctx, "refs/gitmind/edges/test", payload, len1 + len2); + #include "gitmind/util/ref.h" + char ref_name[REF_NAME_BUFFER_SIZE]; + assert(gm_build_ref(ref_name, sizeof ref_name, GITMIND_EDGES_REF_PREFIX, "test") == GM_OK); + rc = gm_journal_create_commit(&ctx, ref_name, payload, len1 + len2);As per coding guidelines
10-14: Remove duplicate include of gitmind/error.hgitmind/error.h is included twice; drop one to keep includes clean.
-#include "gitmind/error.h"
39-41: Optional: ensure test repo is isolated and cleaned upCreating a fixed ./.gm_journal_tmp can leave artifacts. Prefer a mkdtemp‑style unique dir and delete it after.
Also applies to: 97-104
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
AGENTS.md(1 hunks)core/include/gitmind/util/oid.h(2 hunks)core/src/cache/query.c(4 hunks)core/src/domain/cache/edge_map.c(2 hunks)core/src/edge/attributed.c(3 hunks)core/src/edge/edge.c(7 hunks)core/src/hooks/augment.c(2 hunks)core/src/journal/writer.c(4 hunks)core/src/util/oid.c(2 hunks)core/tests/unit/test_cache_branch_limits.c(2 hunks)core/tests/unit/test_cache_edge_map.c(2 hunks)core/tests/unit/test_cache_query.c(3 hunks)core/tests/unit/test_cache_shard_distribution.c(2 hunks)core/tests/unit/test_edge_attributed_cbor.c(4 hunks)core/tests/unit/test_edge_cbor_oid.c(3 hunks)core/tests/unit/test_edge_equal_semantics.c(2 hunks)core/tests/unit/test_edge_oid_fallback.c(2 hunks)core/tests/unit/test_journal_mixed_cbor.c(2 hunks)core/tests/unit/test_journal_safety.c(2 hunks)docs/activity/2025-09.md(1 hunks)docs/code-reviews/PR172/203f4e3539b1d8fc73b641e94b0a420f2e592a14.md(0 hunks)docs/code-reviews/PR172/5402cadb5e7a3f71e0bee7a1e05a1f5034d4fc3c.md(0 hunks)docs/code-reviews/PR172/5add14fcdabafff59a99ea484d691140afdb68c3.md(0 hunks)docs/code-reviews/PR172/7b7b5f565ef23d959b126b0df50e0a3992c52da2.md(0 hunks)docs/code-reviews/PR172/82dab217893b6ee8bb8dcdca2217f9900a06b76f.md(0 hunks)docs/code-reviews/PR172/8a32c6a810ebf632709cec6c928381c9c47fdee9.md(0 hunks)docs/code-reviews/PR172/b22c22398de39c4ab93e1c747b1122d2241eed22.md(0 hunks)docs/code-reviews/PR172/d11ee830d5615e5582114d625a52283760d61767.md(0 hunks)docs/code-reviews/PR173/7445bcf00c71b6f768e9eb6b1fd99d2b1c60783f.md(1 hunks)
💤 Files with no reviewable changes (8)
- docs/code-reviews/PR172/8a32c6a810ebf632709cec6c928381c9c47fdee9.md
- docs/code-reviews/PR172/5add14fcdabafff59a99ea484d691140afdb68c3.md
- docs/code-reviews/PR172/5402cadb5e7a3f71e0bee7a1e05a1f5034d4fc3c.md
- docs/code-reviews/PR172/203f4e3539b1d8fc73b641e94b0a420f2e592a14.md
- docs/code-reviews/PR172/82dab217893b6ee8bb8dcdca2217f9900a06b76f.md
- docs/code-reviews/PR172/d11ee830d5615e5582114d625a52283760d61767.md
- docs/code-reviews/PR172/7b7b5f565ef23d959b126b0df50e0a3992c52da2.md
- docs/code-reviews/PR172/b22c22398de39c4ab93e1c747b1122d2241eed22.md
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{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/src/edge/edge.ccore/src/edge/attributed.ccore/src/domain/cache/edge_map.ccore/tests/unit/test_cache_shard_distribution.ccore/tests/unit/test_edge_equal_semantics.ccore/include/gitmind/util/oid.hcore/tests/unit/test_edge_cbor_oid.ccore/tests/unit/test_cache_branch_limits.ccore/tests/unit/test_cache_edge_map.ccore/src/hooks/augment.ccore/tests/unit/test_edge_attributed_cbor.ccore/tests/unit/test_cache_query.ccore/src/util/oid.ccore/src/journal/writer.ccore/src/cache/query.ccore/tests/unit/test_journal_mixed_cbor.ccore/tests/unit/test_edge_oid_fallback.ccore/tests/unit/test_journal_safety.c
{core,apps,tests}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
{core,apps,tests}/**/*.{c,h}: Use gm_snprintf, gm_strcpy_safe, and gm_memcpy_span for all string/memory operations; check return codes and treat truncation as an error; zero output buffers on error paths before copying/formatting
C23 only with warnings-as-errors; no VLAs; no shadowing; all functions have explicit prototypes
Equality and identity are OID-first: when OIDs are present, compare via git_oid_cmp (typedef gm_oid_t) and do not fall back to SHA if OIDs exist
Prefer specific headers in includes; follow include order/regrouping enforced by .clang-format
Build refs via gm_build_ref and reject inputs starting with "refs/" to avoid double-prefixing; follow libgit2 ref-name safety via normalization
Files:
core/src/edge/edge.ccore/src/edge/attributed.ccore/src/domain/cache/edge_map.ccore/tests/unit/test_cache_shard_distribution.ccore/tests/unit/test_edge_equal_semantics.ccore/include/gitmind/util/oid.hcore/tests/unit/test_edge_cbor_oid.ccore/tests/unit/test_cache_branch_limits.ccore/tests/unit/test_cache_edge_map.ccore/src/hooks/augment.ccore/tests/unit/test_edge_attributed_cbor.ccore/tests/unit/test_cache_query.ccore/src/util/oid.ccore/src/journal/writer.ccore/src/cache/query.ccore/tests/unit/test_journal_mixed_cbor.ccore/tests/unit/test_edge_oid_fallback.ccore/tests/unit/test_journal_safety.c
{core,apps}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Naming: functions/variables lower_snake_case with gm_ prefix; macros UPPER_SNAKE; typedef’d types end with t; header guards begin with GITMIND
Files:
core/src/edge/edge.ccore/src/edge/attributed.ccore/src/domain/cache/edge_map.ccore/tests/unit/test_cache_shard_distribution.ccore/tests/unit/test_edge_equal_semantics.ccore/include/gitmind/util/oid.hcore/tests/unit/test_edge_cbor_oid.ccore/tests/unit/test_cache_branch_limits.ccore/tests/unit/test_cache_edge_map.ccore/src/hooks/augment.ccore/tests/unit/test_edge_attributed_cbor.ccore/tests/unit/test_cache_query.ccore/src/util/oid.ccore/src/journal/writer.ccore/src/cache/query.ccore/tests/unit/test_journal_mixed_cbor.ccore/tests/unit/test_edge_oid_fallback.ccore/tests/unit/test_journal_safety.c
core/src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
Do not allocate GM_PATH_MAX*2 buffers; prefer GM_PATH_MAX and allocate dynamically if larger is required
Files:
core/src/edge/edge.ccore/src/edge/attributed.ccore/src/domain/cache/edge_map.ccore/src/hooks/augment.ccore/src/util/oid.ccore/src/journal/writer.ccore/src/cache/query.c
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Formatting must follow .clang-format (LLVM style, 4 spaces, 80 cols, pointer alignment right); run clang-format via pre-commit
Files:
core/src/edge/edge.ccore/src/edge/attributed.ccore/src/domain/cache/edge_map.ccore/tests/unit/test_cache_shard_distribution.ccore/tests/unit/test_edge_equal_semantics.ccore/include/gitmind/util/oid.hcore/tests/unit/test_edge_cbor_oid.ccore/tests/unit/test_cache_branch_limits.ccore/tests/unit/test_cache_edge_map.ccore/src/hooks/augment.ccore/tests/unit/test_edge_attributed_cbor.ccore/tests/unit/test_cache_query.ccore/src/util/oid.ccore/src/journal/writer.ccore/src/cache/query.ccore/tests/unit/test_journal_mixed_cbor.ccore/tests/unit/test_edge_oid_fallback.ccore/tests/unit/test_journal_safety.c
core/src/domain/cache/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
Cache lives under refs/gitmind/cache/ and is rebuildable; do not merge cache refs
Files:
core/src/domain/cache/edge_map.c
core/src/domain/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep domain logic pure (no direct IO/libgit2/global state) and one responsibility per translation unit
Files:
core/src/domain/cache/edge_map.c
core/tests/unit/test_*.c
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests live in core/tests/unit/ and must be deterministic and isolated; name files test_.c
Files:
core/tests/unit/test_cache_shard_distribution.ccore/tests/unit/test_edge_equal_semantics.ccore/tests/unit/test_edge_cbor_oid.ccore/tests/unit/test_cache_branch_limits.ccore/tests/unit/test_cache_edge_map.ccore/tests/unit/test_edge_attributed_cbor.ccore/tests/unit/test_cache_query.ccore/tests/unit/test_journal_mixed_cbor.ccore/tests/unit/test_edge_oid_fallback.ccore/tests/unit/test_journal_safety.c
{include,core/include/gitmind}/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
{include,core/include/gitmind}/**/*.h: Public headers must be umbrella-safe, compile standalone, use stable header guards, and include C++ linkage guards (extern "C")
Avoid public ABI breaks: when changing public structs, only append new fields at the end; mark old fields deprecated rather than removing/reordering
Files:
core/include/gitmind/util/oid.h
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Docs: include front matter first, a single H1 matching the title, a '## Table of Contents' section, and SPDX license comments after front matter; API docs must include api_version
Conform to markdownlint rules configured by .markdownlint.jsonc (e.g., underscore emphasis/strong, blockquote spacing, code fence and heading punctuation)
Files:
docs/activity/2025-09.mddocs/code-reviews/PR173/7445bcf00c71b6f768e9eb6b1fd99d2b1c60783f.md
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Applies to {core,apps,tests}/**/*.{c,h} : Equality and identity are OID-first: when OIDs are present, compare via git_oid_cmp (typedef gm_oid_t) and do not fall back to SHA if OIDs exist
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-29T06:07:18.746Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Conventional commits and PR hygiene: type(scope): description, link issues, include test plan, keep clang-tidy at zero for touched files, update docs as needed
Applied to files:
AGENTS.mddocs/code-reviews/PR173/7445bcf00c71b6f768e9eb6b1fd99d2b1c60783f.md
📚 Learning: 2025-09-29T06:07:18.746Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Applies to {core,apps,tests}/**/*.{c,h} : Equality and identity are OID-first: when OIDs are present, compare via git_oid_cmp (typedef gm_oid_t) and do not fall back to SHA if OIDs exist
Applied to files:
core/src/edge/edge.ccore/src/edge/attributed.ccore/src/domain/cache/edge_map.ccore/tests/unit/test_cache_shard_distribution.ccore/tests/unit/test_edge_equal_semantics.ccore/include/gitmind/util/oid.hcore/tests/unit/test_edge_cbor_oid.ccore/tests/unit/test_cache_branch_limits.ccore/tests/unit/test_cache_edge_map.ccore/src/hooks/augment.ccore/tests/unit/test_edge_attributed_cbor.ccore/tests/unit/test_cache_query.ccore/src/util/oid.ccore/src/journal/writer.ccore/tests/unit/test_journal_mixed_cbor.ccore/tests/unit/test_edge_oid_fallback.ccore/tests/unit/test_journal_safety.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 include/gitmind.h : Maintain umbrella API at include/gitmind.h
Applied to files:
core/src/edge/edge.ccore/src/edge/attributed.ccore/include/gitmind/util/oid.hcore/tests/unit/test_cache_branch_limits.ccore/src/hooks/augment.ccore/tests/unit/test_cache_query.ccore/src/journal/writer.ccore/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 {include,core/include}/**/*.h : Header guards use GITMIND_* naming
Applied to files:
core/src/hooks/augment.c
📚 Learning: 2025-09-29T06:07:18.746Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Applies to {core,apps}/**/*.{c,h} : Naming: functions/variables lower_snake_case with gm_ prefix; macros UPPER_SNAKE; typedef’d types end with _t; header guards begin with GITMIND_
Applied to files:
core/src/hooks/augment.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 include/gitmind/**/*.h : Public, namespaced headers live under include/gitmind/
Applied to files:
core/src/hooks/augment.c
📚 Learning: 2025-09-29T06:07:18.746Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Applies to {include,core/include/gitmind}/**/*.h : Public headers must be umbrella-safe, compile standalone, use stable header guards, and include C++ linkage guards (extern "C")
Applied to files:
core/src/hooks/augment.c
📚 Learning: 2025-09-29T06:07:18.746Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Applies to core/src/domain/journal/**/*.c : Journal is append-only under refs/gitmind/edges/<branch>; never mutate history entries
Applied to files:
core/src/journal/writer.c
🧬 Code graph analysis (9)
core/src/edge/edge.c (1)
core/src/cbor/cbor.c (1)
gm_cbor_read_bytes(190-244)
core/src/edge/attributed.c (1)
core/src/cbor/cbor.c (1)
gm_cbor_read_bytes(190-244)
core/include/gitmind/util/oid.h (1)
core/src/util/oid.c (6)
bool(75-80)bool(82-92)int(36-63)int(65-73)int(94-112)int(114-144)
core/tests/unit/test_cache_query.c (1)
core/src/cache/query.c (2)
gm_cache_query_fanout(386-390)gm_cache_result_free(400-406)
core/src/util/oid.c (2)
core/include/gitmind/security/memory.h (1)
gm_memset_safe(47-59)core/include/gitmind/util/memory.h (1)
gm_memcpy_span(32-47)
core/src/journal/writer.c (1)
core/include/gitmind/security/memory.h (1)
gm_memset_safe(47-59)
core/src/cache/query.c (1)
core/include/gitmind/security/memory.h (1)
gm_memset_safe(47-59)
core/tests/unit/test_journal_mixed_cbor.c (2)
core/src/types/ulid.c (1)
gm_ulid_generate(118-126)core/src/edge/edge.c (1)
gm_edge_encode_cbor(260-361)
core/tests/unit/test_journal_safety.c (1)
core/src/edge/edge.c (1)
gm_edge_equal(112-141)
🔇 Additional comments (40)
core/src/util/oid.c (4)
75-80: LGTM: OID-first equality via byte-compareUsing memcmp over GM_OID_RAWSZ aligns with OID-first semantics; no SHA fallback here.
Based on learnings
82-92: Confirm NULL-semantics for gm_oid_is_zeroReturning true for NULL is convenient but can mask bugs if callers mistakenly pass NULL. Confirm all call sites intend this behavior; otherwise, consider returning false on NULL to force explicit zero-init.
Based on learnings
114-144: LGTM: Strict full-length hex parsing with case-insensitive nibblesLength check + nibble validation + zeroing on error is solid and side-channel safe enough for our use.
36-63: LGTM: Safe hex encoding with overflow checks and zeroingArgument validation, required-size computation, and out-buffer zeroing on failure look correct.
core/tests/unit/test_edge_oid_fallback.c (1)
29-36: LGTM: OID-first equality behavior is exercised correctlyZero OIDs → SHA fallback; non-zero OIDs (different) → edges not equal. Matches policy.
Based on learnings
core/src/domain/cache/edge_map.c (1)
129-132: LGTM: Switch to gm_oid_equal for key matchEquality predicate aligns with OID-first rule and avoids libgit2 dependency here.
Based on learnings
core/src/hooks/augment.c (1)
16-16: Good: migrated to public OID utilities header.Using gitmind/util/oid.h aligns this hook with the gm_oid_* API surface. Based on learnings
core/tests/unit/test_cache_edge_map.c (3)
11-11: Header update LGTM.Including gitmind/util/oid.h is correct for gm_oid_* in tests. Based on learnings
15-16: Parsing OIDs via gm_oid_from_hex with GM_OK assert: good.This makes failures explicit and decouples from libgit2.
39-41: OID-first equality via gm_oid_equal: good.Aligns test semantics with production comparison rules. Based on learnings
core/tests/unit/test_cache_shard_distribution.c (1)
10-12: Headers LGTM.Pulling in gitmind/util/oid.h and error definitions matches the new helpers.
core/tests/unit/test_cache_branch_limits.c (2)
18-18: Header update LGTM.Using gitmind/util/oid.h in this test is correct.
77-79: Good: populate OIDs via gm_oid_from_raw with GM_OK asserts.This removes libgit2 coupling for OID population.
core/tests/unit/test_edge_cbor_oid.c (3)
10-12: Headers LGTM.Includes align with gm_oid_* usage and error handling.
31-33: Good: set OIDs via gm_oid_from_raw with explicit length checks.Clear, deterministic setup for encode/decode.
45-50: OID-first equality usage is correct.gm_oid_equal checks plus gm_edge_equal uphold OID-wins semantics. Based on learnings
AGENTS.md (1)
452-455: Docs alignment with OID-first + gm helpers looks goodThe activity log accurately captures the gm_oid_to_hex migration and remaining sweep items. No changes requested. Run docs check to keep guardrails green.
make docs-verify
core/tests/unit/test_cache_query.c (2)
17-17: Good migration to gm_oid_ in tests*Adopting gm_oid_from_raw and passing gm_oid_t into queries is aligned with the OID-first policy.
Also applies to: 91-96, 108-109, 114-115
120-129: Remove incorrect double-free warning The dispose callback only frees the port’s internal state and does not freerepo, so callinggit_repository_free(repo)is correct and does not double-free.Likely an incorrect or invalid review comment.
core/src/edge/attributed.c (1)
241-259: CBOR → raw bytes → gm_oid_from_raw is correctDecoding OIDs via gm_cbor_read_bytes then gm_oid_from_raw with GM_OID_RAWSZ is the right flow.
core/src/journal/writer.c (4)
53-55: gm_oid_from_hex for EMPTY_TREE_SHA — goodParsing the empty tree via gm helper aligns with the migration.
116-121: gm_oid_to_hex with zeroing on failure — nice hardeningZeroing oid_hex on error follows the “zero outputs on error paths” rule.
126-128: Commit OID param narrowed to gm_oid_t*Type narrowing keeps internal surfaces consistent with OID-first design.
374-376: Repeat of EMPTY_TREE_SHA parse via gm helper — consistentGood consistency between writer entry points.
core/src/edge/edge.c (2)
117-131: OID-first equality is implemented correctlyChecks use gm_oid_is_zero + gm_oid_equal with SHA fallback only when OIDs are absent.
Based on learnings.
Also applies to: 129-133
319-346: CBOR OID encoding mirrors policy and pads from SHA when missingLocal fixed-size buffers + padding are correct and deterministic.
Also applies to: 348-357
core/include/gitmind/util/oid.h (2)
40-56: OID equality/zero helpers look correct and align with OID‑first semantics.Null handling is explicit in both docs and implementation; this matches the project rule to prefer OIDs when present. Looks good. Based on learnings
68-76: gm_oid_from_hex API is well‑specified; LGTM.Exact-length requirement and failure clearing behavior are documented and consistent with impl.
core/tests/unit/test_edge_attributed_cbor.c (4)
12-13: Correct include additions for gm OID helpers.Brings in the new public API; good.
28-29: Good: length‑checked OID initialization via gm_oid_from_raw.Passing
sizeof raw*ensures strictGM_OID_RAWSZvalidation.
43-44: Good: compare OIDs with gm_oid_equal (OID‑first).Matches the repo’s equality rule. Based on learnings
110-111: Good: legacy backfill validated via non‑zero OIDs.Assert ensures decode path populates OIDs from legacy SHAs.
core/tests/unit/test_journal_safety.c (2)
60-64: LGTM: OID‑first equality semantics are exercised correctly.Using
gm_oid_from_rawwith explicit sizes and then assertinggm_edge_equalbehavior matches the OID‑first rule even when legacy SHAs differ/match. Based on learningsAlso applies to: 76-77
12-13: Include of gm OID utilities is correct and necessary.No issues.
core/src/cache/query.c (3)
190-201: Good: OID‑first comparison with proper fallbacksUsing gm_oid_is_zero + gm_oid_equal, and falling back to gm_oid_from_hex, matches the OID‑first equality rule. Looks correct and simplifies the old branching. Based on learnings
302-305: Good: zero‑OID fast‑exit for cache tipEarly return when meta.cache_tip_oid is zero avoids unnecessary IO.
429-438: Good: gated tree‑size lookup with OID zero‑check and fallback estimateThis keeps gm_cache_stats robust even when the tip is missing.
core/tests/unit/test_edge_equal_semantics.c (2)
10-11: Headers: correct public OID/error includes for new helpersIncluding gitmind/util/oid.h and gitmind/error.h is appropriate for gm_oid_from_raw and GM_OK.
21-26: Good: OID‑first equality scenario validatedAsserting GM_OK from gm_oid_from_raw and verifying equality with differing SHAs enforces the OID‑first rule the project requires. Based on learnings
core/tests/unit/test_journal_mixed_cbor.c (1)
60-61: Good: gm_oid_from_raw with explicit lengthsCorrectly populates OIDs from raw bytes with GM_OK assertions.
Also applies to: 68-69
| #ifndef GITMIND_UTIL_OID_H | ||
| #define GITMIND_UTIL_OID_H | ||
|
|
||
| #include <stdbool.h> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify umbrella exposure in include/gitmind.h.
Public headers must be umbrella‑safe and reachable via the umbrella. Please confirm this header is re‑exported.
🏁 Script executed:
#!/bin/bash
# Expect: include/gitmind.h exists and references util/oid.h
fd -a gitmind.h include | xargs -I{} rg -n 'gitmind/util/oid\.h' {}
# Also check for duplicates/different paths:
rg -nP --glob 'include/**' '\butil/oid\.h\b'Length of output: 0
🏁 Script executed:
#!/usr/bin/env bash
# Locate all gitmind.h files and check for util/oid.h references
find . -type f -name gitmind.h | while read file; do
echo "---- $file ----"
grep -Hn "util/oid\.h" "$file" || echo "no reference"
doneLength of output: 42
Include util/oid.h in the umbrella header
Add the following to include/gitmind.h so that core/include/gitmind/util/oid.h is re-exported by the umbrella API:
--- a/include/gitmind.h
+++ b/include/gitmind.h
@@ /* somewhere among other includes */
+#include <gitmind/util/oid.h>📝 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.
| #include <stdbool.h> | |
| ++ b/include/gitmind.h | |
| @@ /* somewhere among other umbrella-header includes */ | |
| #include <gitmind/other_header.h> | |
| #include <gitmind/util/oid.h> | |
| #include <gitmind/yet_another_header.h> |
🤖 Prompt for AI Agents
In core/include/gitmind/util/oid.h at line 7, the util/oid header is not being
re-exported by the umbrella API; open include/gitmind.h and add an #include
"gitmind/util/oid.h" (placed with the other util/includes or alphabetical group)
so that core/include/gitmind/util/oid.h is exposed via the umbrella header;
update any include guards or ordering if necessary to preserve compile order.
| if (gm_oid_is_zero(&out.src_oid)) { | ||
| (void)gm_oid_from_raw(&out.src_oid, out.src_sha, GM_OID_RAWSZ); | ||
| } | ||
| if (gm_oid_is_zero(&out.tgt_oid)) { | ||
| (void)gm_oid_from_raw(&out.tgt_oid, out.tgt_sha, GM_OID_RAWSZ); | ||
| } |
There was a problem hiding this comment.
Fix OID backfill length mismatch (potential OOB read)
You pass GM_OID_RAWSZ while sourcing from out.src_sha/out.tgt_sha (GM_SHA1_SIZE). That can read past the SHA buffer or fail. Zero‑pad into a GM_OID_RAWSZ scratch first.
- if (gm_oid_is_zero(&out.src_oid)) {
- (void)gm_oid_from_raw(&out.src_oid, out.src_sha, GM_OID_RAWSZ);
- }
- if (gm_oid_is_zero(&out.tgt_oid)) {
- (void)gm_oid_from_raw(&out.tgt_oid, out.tgt_sha, GM_OID_RAWSZ);
- }
+ if (gm_oid_is_zero(&out.src_oid)) {
+ uint8_t tmp[GM_OID_RAWSZ];
+ gm_memset_safe(tmp, sizeof tmp, 0, sizeof tmp);
+ gm_memcpy_span(tmp, sizeof tmp, out.src_sha, GM_SHA1_SIZE);
+ (void)gm_oid_from_raw(&out.src_oid, tmp, GM_OID_RAWSZ);
+ }
+ if (gm_oid_is_zero(&out.tgt_oid)) {
+ uint8_t tmp[GM_OID_RAWSZ];
+ gm_memset_safe(tmp, sizeof tmp, 0, sizeof tmp);
+ gm_memcpy_span(tmp, sizeof tmp, out.tgt_sha, GM_SHA1_SIZE);
+ (void)gm_oid_from_raw(&out.tgt_oid, tmp, GM_OID_RAWSZ);
+ }As per coding guidelines (zero outputs on error paths; use gm_memcpy_span). Based on learnings.
📝 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.
| if (gm_oid_is_zero(&out.src_oid)) { | |
| (void)gm_oid_from_raw(&out.src_oid, out.src_sha, GM_OID_RAWSZ); | |
| } | |
| if (gm_oid_is_zero(&out.tgt_oid)) { | |
| (void)gm_oid_from_raw(&out.tgt_oid, out.tgt_sha, GM_OID_RAWSZ); | |
| } | |
| if (gm_oid_is_zero(&out.src_oid)) { | |
| uint8_t tmp[GM_OID_RAWSZ]; | |
| gm_memset_safe(tmp, sizeof tmp, 0, sizeof tmp); | |
| gm_memcpy_span(tmp, sizeof tmp, out.src_sha, GM_SHA1_SIZE); | |
| (void)gm_oid_from_raw(&out.src_oid, tmp, GM_OID_RAWSZ); | |
| } | |
| if (gm_oid_is_zero(&out.tgt_oid)) { | |
| uint8_t tmp[GM_OID_RAWSZ]; | |
| gm_memset_safe(tmp, sizeof tmp, 0, sizeof tmp); | |
| gm_memcpy_span(tmp, sizeof tmp, out.tgt_sha, GM_SHA1_SIZE); | |
| (void)gm_oid_from_raw(&out.tgt_oid, tmp, GM_OID_RAWSZ); | |
| } |
| if (gm_oid_is_zero(&edge.src_oid)) { | ||
| (void)gm_oid_from_raw(&edge.src_oid, edge.src_sha, GM_OID_RAWSZ); | ||
| } | ||
| if (git_oid_is_zero(&edge.tgt_oid)) { | ||
| git_oid_fromraw(&edge.tgt_oid, edge.tgt_sha); | ||
| if (gm_oid_is_zero(&edge.tgt_oid)) { | ||
| (void)gm_oid_from_raw(&edge.tgt_oid, edge.tgt_sha, GM_OID_RAWSZ); | ||
| } |
There was a problem hiding this comment.
Fix OID backfill length mismatch (potential OOB read)
As in attributed.c, passing GM_OID_RAWSZ while sourcing from GM_SHA1_SIZE buffers risks OOB. Zero‑pad into a GM_OID_RAWSZ scratch first.
- if (gm_oid_is_zero(&edge.src_oid)) {
- (void)gm_oid_from_raw(&edge.src_oid, edge.src_sha, GM_OID_RAWSZ);
- }
- if (gm_oid_is_zero(&edge.tgt_oid)) {
- (void)gm_oid_from_raw(&edge.tgt_oid, edge.tgt_sha, GM_OID_RAWSZ);
- }
+ if (gm_oid_is_zero(&edge.src_oid)) {
+ uint8_t tmp[GM_OID_RAWSZ];
+ gm_memset_safe(tmp, sizeof tmp, 0, sizeof tmp);
+ gm_memcpy_span(tmp, sizeof tmp, edge.src_sha, GM_SHA1_SIZE);
+ (void)gm_oid_from_raw(&edge.src_oid, tmp, GM_OID_RAWSZ);
+ }
+ if (gm_oid_is_zero(&edge.tgt_oid)) {
+ uint8_t tmp[GM_OID_RAWSZ];
+ gm_memset_safe(tmp, sizeof tmp, 0, sizeof tmp);
+ gm_memcpy_span(tmp, sizeof tmp, edge.tgt_sha, GM_SHA1_SIZE);
+ (void)gm_oid_from_raw(&edge.tgt_oid, tmp, GM_OID_RAWSZ);
+ }As per coding guidelines (safe memory ops; zero on error).
📝 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.
| if (gm_oid_is_zero(&edge.src_oid)) { | |
| (void)gm_oid_from_raw(&edge.src_oid, edge.src_sha, GM_OID_RAWSZ); | |
| } | |
| if (git_oid_is_zero(&edge.tgt_oid)) { | |
| git_oid_fromraw(&edge.tgt_oid, edge.tgt_sha); | |
| if (gm_oid_is_zero(&edge.tgt_oid)) { | |
| (void)gm_oid_from_raw(&edge.tgt_oid, edge.tgt_sha, GM_OID_RAWSZ); | |
| } | |
| if (gm_oid_is_zero(&edge.src_oid)) { | |
| uint8_t tmp[GM_OID_RAWSZ]; | |
| gm_memset_safe(tmp, sizeof tmp, 0, sizeof tmp); | |
| gm_memcpy_span(tmp, sizeof tmp, edge.src_sha, GM_SHA1_SIZE); | |
| (void)gm_oid_from_raw(&edge.src_oid, tmp, GM_OID_RAWSZ); | |
| } | |
| if (gm_oid_is_zero(&edge.tgt_oid)) { | |
| uint8_t tmp[GM_OID_RAWSZ]; | |
| gm_memset_safe(tmp, sizeof tmp, 0, sizeof tmp); | |
| gm_memcpy_span(tmp, sizeof tmp, edge.tgt_sha, GM_SHA1_SIZE); | |
| (void)gm_oid_from_raw(&edge.tgt_oid, tmp, GM_OID_RAWSZ); | |
| } |
🤖 Prompt for AI Agents
In core/src/edge/edge.c around lines 492 to 497, the code calls gm_oid_from_raw
using edge.src_sha/edge.tgt_sha buffers sized GM_SHA1_SIZE but passes
GM_OID_RAWSZ which can read past the source buffer; to fix, allocate a
GM_OID_RAWSZ-byte scratch array, zero it, copy GM_SHA1_SIZE bytes from the SHA
buffer into the scratch, then call gm_oid_from_raw with the scratch and
GM_OID_RAWSZ; ensure the scratch is zeroed before use and that any failure from
gm_oid_from_raw leaves the oid zeroed (follow existing safe-memory/error-zeroing
patterns).
| #include "gitmind/util/oid.h" | ||
|
|
||
| #include <limits.h> | ||
| #include <stdbool.h> | ||
| #include <stddef.h> | ||
| #include <stdint.h> | ||
| #include <string.h> | ||
|
|
||
| #include "gitmind/error.h" | ||
| #include "gitmind/result.h" | ||
| #include "gitmind/security/memory.h" | ||
| #include "gitmind/types.h" | ||
| #include "gitmind/util/memory.h" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Repo-wide check: ensure no straggler git_oid_cmp/raw memcmp remain
Run a quick scan to confirm all equality moves funnel through gm_oid_equal.
🏁 Script executed:
#!/bin/bash
# Find legacy equality patterns that should be gm_oid_equal
rg -nP --type=c --type=h -C2 '\bgit_oid_cmp\s*\(|\bmemcmp\s*\(\s*[^,]*->id\s*,\s*[^,]*->id\s*,\s*GM_OID_RAWSZ\s*\)'Length of output: 201
Replace raw memcmp in OID comparison with gm_oid_equal
- In
core/src/util/oid.c:79, replace
return memcmp(lhs->id, rhs->id, GM_OID_RAWSZ) == 0;
with
return gm_oid_equal(lhs, rhs);
🤖 Prompt for AI Agents
In core/src/util/oid.c around lines 4 to 17 and specifically at line ~79,
replace the raw memcmp comparison `memcmp(lhs->id, rhs->id, GM_OID_RAWSZ) == 0`
with the helper `gm_oid_equal(lhs, rhs)`; update the return to `return
gm_oid_equal(lhs, rhs);` so the OID equality uses the centralized function (no
other changes needed if oid.h is already included).
| --- | ||
| title: 7445bcf00c71b6f768e9eb6b1fd99d2b1c60783f.md | ||
| description: Preserved review artifacts and rationale. | ||
| audience: [contributors] | ||
| domain: [quality] | ||
| tags: [review] | ||
| status: archive | ||
| --- | ||
|
|
||
| # Code Review Feedback |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align title/H1, add ToC, and insert SPDX header
To meet docs guidelines: make H1 match the title, add a "## Table of Contents", and place SPDX after front matter.
As per coding guidelines
---
-title: 7445bcf00c71b6f768e9eb6b1fd99d2b1c60783f.md
+title: Code Review Feedback
description: Preserved review artifacts and rationale.
audience: [contributors]
domain: [quality]
tags: [review]
status: archive
---
-# Code Review Feedback
+<!-- SPDX-License-Identifier: LicenseRef-MIND-UCAL-1.0 -->
+
+# Code Review Feedback
+
+## Table of Contents
+- Overview
+- Commits Reviewed
+- Notes
+- Finishing Touches📝 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.
| --- | |
| title: 7445bcf00c71b6f768e9eb6b1fd99d2b1c60783f.md | |
| description: Preserved review artifacts and rationale. | |
| audience: [contributors] | |
| domain: [quality] | |
| tags: [review] | |
| status: archive | |
| --- | |
| # Code Review Feedback | |
| --- | |
| title: Code Review Feedback | |
| description: Preserved review artifacts and rationale. | |
| audience: [contributors] | |
| domain: [quality] | |
| tags: [review] | |
| status: archive | |
| --- | |
| <!-- SPDX-License-Identifier: LicenseRef-MIND-UCAL-1.0 --> | |
| # Code Review Feedback | |
| ## Table of Contents | |
| - Overview | |
| - Commits Reviewed | |
| - Notes | |
| - Finishing Touches |
🤖 Prompt for AI Agents
In docs/code-reviews/PR173/7445bcf00c71b6f768e9eb6b1fd99d2b1c60783f.md around
lines 1 to 10, the document needs to follow docs guidelines: make the H1 match
the title, add a "## Table of Contents" section, and insert an SPDX header
immediately after the YAML front matter; update the top-level heading to exactly
match the title line, add a "## Table of Contents" line beneath the H1, and add
a standard SPDX license identifier line (e.g., "SPDX-License-Identifier: MIT")
directly after the closing front-matter delimiter.
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 (3)
core/tests/unit/test_cache_branch_limits.c (1)
53-54: Use gm_snprintf and validate bounds (no raw snprintf in tests)Replace raw snprintf with gm_snprintf and assert no truncation, as required by guidelines.
Apply:
- snprintf(refname, sizeof refname, "refs/heads/%s", branch); + int wn = gm_snprintf(refname, sizeof refname, "refs/heads/%s", branch); + assert(wn >= 0 && (size_t)wn < sizeof refname);Also ensure the header is included:
#include "gitmind/edge.h" #include "gitmind/util/memory.h" +#include "gitmind/security/string.h" #include "gitmind/error.h"As per coding guidelines.
core/tests/unit/test_edge_cbor_oid.c (1)
25-27: Remove leftover raw strcpy (violates safety guidelines)These raw strcpy calls precede the safe copies below and should be removed to avoid regressions and to follow project policy.
Apply:
- strcpy(e.src_path, "src/A.c"); - strcpy(e.tgt_path, "src/B.c"); - strcpy(e.ulid, "01ARZ3NDEKTSV4RRFFQ69G5FAV");core/src/cache/query.c (1)
216-225: Fix hex buffer size for OID → hex conversion (risk: truncation/overflow/false error)sha_hex is sized with SHA_HEX_SIZE but you encode GM_OID_RAWSZ bytes. If GM_OID_RAWSZ encodes to GM_OID_HEX_CHARS, a smaller buffer triggers GM_ERR_BUFFER_TOO_SMALL or worse. Use GM_OID_HEX_CHARS + 1.
Apply:
- char sha_hex[SHA_HEX_SIZE]; + char sha_hex[GM_OID_HEX_CHARS + 1];This matches get_sha_prefix’s pattern and keeps encoding consistent with OID size.
Based on learnings
🧹 Nitpick comments (9)
core/src/adapters/git/libgit2_repository_port.c (2)
892-896: Validate target OID isn’t zero before proceeding.Add a fast input guard using gm_oid_is_zero to reject invalid/placeholder targets early. This prevents attempting to create/update refs to a zero OID.
if (spec == NULL || spec->ref_name == NULL || spec->target_oid == NULL) { return gm_err_void(GM_ERROR(GM_ERR_INVALID_ARGUMENT, "reference update spec missing inputs")); } + if (gm_oid_is_zero(spec->target_oid)) { + return gm_err_void(GM_ERROR(GM_ERR_INVALID_ARGUMENT, + "reference update target OID is zero")); + }Based on learnings.
927-946: Early return on equal OIDs to avoid redundant reflog entriesBail out immediately when
current_targetequalsspec->target_oid, then run the ancestry check only otherwise—this skips the extragit_reference_set_targetcall and reflog noise.- if (!gm_oid_equal(current_target, spec->target_oid)) { - int descendant = git_graph_descendant_of(state->repo, spec->target_oid, - current_target); - if (descendant < 0) { - git_reference_free(existing); - return gm_err_void(GM_ERROR(GM_ERR_UNKNOWN, - "failed to check ancestry for %s", spec->ref_name)); - } - if (descendant == 0) { - git_reference_free(existing); - return gm_err_void(GM_ERROR(GM_ERR_ALREADY_EXISTS, - "non-fast-forward update rejected for %s", - spec->ref_name)); - } - } + if (gm_oid_equal(current_target, spec->target_oid)) { + git_reference_free(existing); + return gm_ok_void(); + } + int descendant = + git_graph_descendant_of(state->repo, spec->target_oid, current_target); + if (descendant < 0) { + git_reference_free(existing); + return gm_err_void(GM_ERROR(GM_ERR_UNKNOWN, + "failed to check ancestry for %s", spec->ref_name)); + } + if (descendant == 0) { + git_reference_free(existing); + return gm_err_void(GM_ERROR(GM_ERR_ALREADY_EXISTS, + "non-fast-forward update rejected for %s", + spec->ref_name)); + }Apply the same refactor at lines 951–956.
core/tests/unit/test_cache_branch_limits.c (1)
85-86: Check gm_ulid_generate resultAssert the result to keep tests deterministic and aligned with “check return codes”.
- (void)gm_ulid_generate(edge.ulid); + gm_result_ulid_t ur = gm_ulid_generate(edge.ulid); + assert(ur.ok);core/src/domain/cache/edge_map.c (3)
49-50: Avoid narrowing cast in modulo (handle very large bucket_count)Dropping the uint32_t cast prevents unexpected wrap when bucket_count > UINT32_MAX.
- return (size_t)(hash_value % (uint32_t)bucket_count); + return (size_t)hash_value % bucket_count;
95-99: Null out out_map on error pathsSet *out_map = NULL before returning errors to avoid stale pointers on caller side.
map->buckets = calloc(bucket_count, sizeof(*map->buckets)); if (map->buckets == NULL) { - free(map); - return gm_err_void(GM_ERROR(GM_ERR_OUT_OF_MEMORY, "edge map buckets alloc failed")); + free(map); + *out_map = NULL; + return gm_err_void(GM_ERROR(GM_ERR_OUT_OF_MEMORY, "edge map buckets alloc failed")); }And above:
- if (bucket_count == 0U) { - return gm_err_void(GM_ERROR(GM_ERR_INVALID_ARGUMENT, - "edge map bucket count must be > 0")); - } + if (bucket_count == 0U) { + *out_map = NULL; + return gm_err_void(GM_ERROR(GM_ERR_INVALID_ARGUMENT, + "edge map bucket count must be > 0")); + }And for map alloc failure:
- if (map == NULL) { - return gm_err_void(GM_ERROR(GM_ERR_OUT_OF_MEMORY, "edge map alloc failed")); - } + if (map == NULL) { + *out_map = NULL; + return gm_err_void(GM_ERROR(GM_ERR_OUT_OF_MEMORY, "edge map alloc failed")); + }
10-12: Drop unused includegitmind/util/memory.h isn’t used here. Remove to keep headers minimal.
-#include "gitmind/util/memory.h" #include "gitmind/util/oid.h"core/src/cache/query.c (1)
260-269: Prefer gm_oid_equal over raw memcmp for OID id bytesFor consistency and single‑point semantics, compare OIDs with gm_oid_equal instead of memcmp on ->id. You can pass a gm_oid_t* target (or wrap target_sha into a temporary gm_oid_t) to avoid raw byte compares.
Example:
- const uint8_t *edge_sha = state->check_source - ? (const uint8_t *)edge->src_oid.id - : (const uint8_t *)edge->tgt_oid.id; - if (memcmp(edge_sha, state->target_sha, GM_OID_RAWSZ) == 0) { + const gm_oid_t *edge_oid = state->check_source ? &edge->src_oid : &edge->tgt_oid; + if (gm_oid_equal(edge_oid, (const gm_oid_t *)state->target_sha)) {Or change journal_scan_state_t.target_sha to const gm_oid_t* and adjust callers.
Based on learningsAlso applies to: 333-341, 360-367
docs/code-reviews/PR173/17ddad1aa3747f40ecacc0bd202e7b83edbf1d1d.md (2)
1-11: Align front matter, SPDX, and H1 with docs rulesTitle and H1 must match; add SPDX after front matter; include a Table of Contents.
Apply:
---- -title: 17ddad1aa3747f40ecacc0bd202e7b83edbf1d1d.md +title: Code Review Feedback description: Preserved review artifacts and rationale. audience: [contributors] domain: [quality] tags: [review] status: archive --- -# Code Review Feedback +<!-- SPDX-License-Identifier: LicenseRef-MIND-UCAL-1.0 --> + +# Code Review Feedback + +## Table of Contents + - Overview + - Commits Reviewed + - Notes + - Finishing TouchesAs per coding guidelines
112-115: Remove{response}placeholders and marketing/tips/internal-state blocksStatic artifacts shouldn’t contain unresolved placeholders or third‑party promo/tips and massive internal state blobs. Remove these sections to keep the doc concise and repo‑appropriate.
Suggested deletions:
- Literal “{response}” lines
- “tips_start … tips_end”
- “internal state start … end” (large embedded block)
Also applies to: 179-181, 264-265, 375-377, 678-681
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/src/adapters/git/libgit2_repository_port.c(2 hunks)core/src/cache/query.c(4 hunks)core/src/domain/cache/edge_map.c(3 hunks)core/src/hooks/augment.c(4 hunks)core/tests/unit/test_cache_branch_limits.c(2 hunks)core/tests/unit/test_cache_query.c(4 hunks)core/tests/unit/test_edge_cbor_oid.c(3 hunks)core/tests/unit/test_edge_oid_fallback.c(2 hunks)core/tests/unit/test_journal_mixed_cbor.c(2 hunks)core/tests/unit/test_journal_safety.c(2 hunks)docs/activity/2025-09.md(1 hunks)docs/code-reviews/PR173/17ddad1aa3747f40ecacc0bd202e7b83edbf1d1d.md(1 hunks)docs/code-reviews/PR173/7445bcf00c71b6f768e9eb6b1fd99d2b1c60783f.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/code-reviews/PR173/7445bcf00c71b6f768e9eb6b1fd99d2b1c60783f.md
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/hooks/augment.c
- docs/activity/2025-09.md
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{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_edge_cbor_oid.ccore/tests/unit/test_cache_query.ccore/tests/unit/test_journal_mixed_cbor.ccore/tests/unit/test_journal_safety.ccore/src/adapters/git/libgit2_repository_port.ccore/src/cache/query.ccore/src/domain/cache/edge_map.ccore/tests/unit/test_cache_branch_limits.ccore/tests/unit/test_edge_oid_fallback.c
{core,apps,tests}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
{core,apps,tests}/**/*.{c,h}: Use gm_snprintf, gm_strcpy_safe, and gm_memcpy_span for all string/memory operations; check return codes and treat truncation as an error; zero output buffers on error paths before copying/formatting
C23 only with warnings-as-errors; no VLAs; no shadowing; all functions have explicit prototypes
Equality and identity are OID-first: when OIDs are present, compare via git_oid_cmp (typedef gm_oid_t) and do not fall back to SHA if OIDs exist
Prefer specific headers in includes; follow include order/regrouping enforced by .clang-format
Build refs via gm_build_ref and reject inputs starting with "refs/" to avoid double-prefixing; follow libgit2 ref-name safety via normalization
Files:
core/tests/unit/test_edge_cbor_oid.ccore/tests/unit/test_cache_query.ccore/tests/unit/test_journal_mixed_cbor.ccore/tests/unit/test_journal_safety.ccore/src/adapters/git/libgit2_repository_port.ccore/src/cache/query.ccore/src/domain/cache/edge_map.ccore/tests/unit/test_cache_branch_limits.ccore/tests/unit/test_edge_oid_fallback.c
{core,apps}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Naming: functions/variables lower_snake_case with gm_ prefix; macros UPPER_SNAKE; typedef’d types end with t; header guards begin with GITMIND
Files:
core/tests/unit/test_edge_cbor_oid.ccore/tests/unit/test_cache_query.ccore/tests/unit/test_journal_mixed_cbor.ccore/tests/unit/test_journal_safety.ccore/src/adapters/git/libgit2_repository_port.ccore/src/cache/query.ccore/src/domain/cache/edge_map.ccore/tests/unit/test_cache_branch_limits.ccore/tests/unit/test_edge_oid_fallback.c
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Formatting must follow .clang-format (LLVM style, 4 spaces, 80 cols, pointer alignment right); run clang-format via pre-commit
Files:
core/tests/unit/test_edge_cbor_oid.ccore/tests/unit/test_cache_query.ccore/tests/unit/test_journal_mixed_cbor.ccore/tests/unit/test_journal_safety.ccore/src/adapters/git/libgit2_repository_port.ccore/src/cache/query.ccore/src/domain/cache/edge_map.ccore/tests/unit/test_cache_branch_limits.ccore/tests/unit/test_edge_oid_fallback.c
core/tests/unit/test_*.c
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests live in core/tests/unit/ and must be deterministic and isolated; name files test_.c
Files:
core/tests/unit/test_edge_cbor_oid.ccore/tests/unit/test_cache_query.ccore/tests/unit/test_journal_mixed_cbor.ccore/tests/unit/test_journal_safety.ccore/tests/unit/test_cache_branch_limits.ccore/tests/unit/test_edge_oid_fallback.c
core/src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
Do not allocate GM_PATH_MAX*2 buffers; prefer GM_PATH_MAX and allocate dynamically if larger is required
Files:
core/src/adapters/git/libgit2_repository_port.ccore/src/cache/query.ccore/src/domain/cache/edge_map.c
core/src/domain/cache/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
Cache lives under refs/gitmind/cache/ and is rebuildable; do not merge cache refs
Files:
core/src/domain/cache/edge_map.c
core/src/domain/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep domain logic pure (no direct IO/libgit2/global state) and one responsibility per translation unit
Files:
core/src/domain/cache/edge_map.c
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Docs: include front matter first, a single H1 matching the title, a '## Table of Contents' section, and SPDX license comments after front matter; API docs must include api_version
Conform to markdownlint rules configured by .markdownlint.jsonc (e.g., underscore emphasis/strong, blockquote spacing, code fence and heading punctuation)
Files:
docs/code-reviews/PR173/17ddad1aa3747f40ecacc0bd202e7b83edbf1d1d.md
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Applies to {core,apps,tests}/**/*.{c,h} : Equality and identity are OID-first: when OIDs are present, compare via git_oid_cmp (typedef gm_oid_t) and do not fall back to SHA if OIDs exist
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-29T06:07:18.746Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Applies to {core,apps,tests}/**/*.{c,h} : Equality and identity are OID-first: when OIDs are present, compare via git_oid_cmp (typedef gm_oid_t) and do not fall back to SHA if OIDs exist
Applied to files:
core/tests/unit/test_edge_cbor_oid.ccore/tests/unit/test_cache_query.ccore/tests/unit/test_journal_mixed_cbor.ccore/tests/unit/test_journal_safety.ccore/src/adapters/git/libgit2_repository_port.ccore/tests/unit/test_cache_branch_limits.ccore/tests/unit/test_edge_oid_fallback.c
📚 Learning: 2025-09-29T06:07:18.746Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Applies to {core,apps,tests}/**/*.{c,h} : Use gm_snprintf, gm_strcpy_safe, and gm_memcpy_span for all string/memory operations; check return codes and treat truncation as an error; zero output buffers on error paths before copying/formatting
Applied to files:
core/tests/unit/test_cache_query.c
📚 Learning: 2025-09-29T06:07:18.746Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Applies to core/src/**/*.c : Do not allocate GM_PATH_MAX*2 buffers; prefer GM_PATH_MAX and allocate dynamically if larger is required
Applied to files:
core/tests/unit/test_cache_query.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 include/gitmind.h : Maintain umbrella API at include/gitmind.h
Applied to files:
core/tests/unit/test_cache_query.ccore/tests/unit/test_journal_mixed_cbor.ccore/src/adapters/git/libgit2_repository_port.ccore/tests/unit/test_cache_branch_limits.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 include/gitmind/**/*.h : Public, namespaced headers live under include/gitmind/
Applied to files:
core/tests/unit/test_cache_query.c
🧬 Code graph analysis (7)
core/tests/unit/test_edge_cbor_oid.c (1)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
core/tests/unit/test_cache_query.c (2)
core/src/types/ulid.c (1)
gm_ulid_generate(118-126)core/src/cache/query.c (2)
gm_cache_query_fanout(391-395)gm_cache_result_free(405-411)
core/tests/unit/test_journal_mixed_cbor.c (2)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)core/src/edge/edge.c (1)
gm_edge_encode_cbor(260-361)
core/tests/unit/test_journal_safety.c (1)
core/src/edge/edge.c (1)
gm_edge_equal(112-141)
core/src/cache/query.c (1)
core/include/gitmind/security/memory.h (1)
gm_memset_safe(47-59)
core/tests/unit/test_cache_branch_limits.c (1)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
core/tests/unit/test_edge_oid_fallback.c (2)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)core/src/edge/edge.c (1)
gm_edge_equal(112-141)
🔇 Additional comments (12)
core/src/adapters/git/libgit2_repository_port.c (1)
40-40: LGTM: include added for gm OID utilities is correct.Header placement looks consistent with local-project includes and supports the new gm_oid_* calls.
core/tests/unit/test_edge_oid_fallback.c (1)
24-27: LGTM: OID-first equality/fallback behavior is correctly exercised
- Good use of gm_strcpy_safe with return checks and gm_oid_from_raw with GM_OK asserts. The test validates SHA fallback only when OIDs are zero, and switches to OID equality when non-zero, matching project semantics. Based on learnings
Also applies to: 35-38
core/tests/unit/test_cache_branch_limits.c (1)
78-80: Good migration to gm helpers in tests
- gm_oid_from_raw and gm_strcpy_safe with asserts are correct and follow guidelines.
Also applies to: 83-84
core/src/domain/cache/edge_map.c (3)
85-89: Good input validation for bucket_countRejecting bucket_count == 0 prevents modulo-by-zero and aligns with defensive API design.
131-136: Correctly switched to gm_oid_equal for identity checksThis matches the OID-first equality rule and detaches from libgit2 helpers. Based on learnings
11-11: Repo-wide migration check
- Verified removal of all
git_oid_{fromraw,cmp,is_zero}usages across core/apps/tests.- Tests under core/tests/unit still call
strcpy—replace each withgm_strcpy_safeand handle return codes per string/memory guidelines.⛔ Skipped due to learnings
Learnt from: CR PR: neuroglyph/git-mind#0 File: AGENTS.md:0-0 Timestamp: 2025-09-29T06:07:18.746Z Learning: Applies to {core,apps,tests}/**/*.{c,h} : Use gm_snprintf, gm_strcpy_safe, and gm_memcpy_span for all string/memory operations; check return codes and treat truncation as an error; zero output buffers on error paths before copying/formattingLearnt from: CR PR: neuroglyph/git-mind#0 File: AGENTS.md:0-0 Timestamp: 2025-09-29T06:07:18.746Z Learning: Applies to {core,apps,tests}/**/*.{c,h} : Equality and identity are OID-first: when OIDs are present, compare via git_oid_cmp (typedef gm_oid_t) and do not fall back to SHA if OIDs existLearnt from: CR PR: neuroglyph/git-mind#0 File: AGENTS.md:0-0 Timestamp: 2025-09-29T06:07:18.746Z Learning: Applies to {core,apps,tests}/**/*.{c,h} : Build refs via gm_build_ref and reject inputs starting with "refs/" to avoid double-prefixing; follow libgit2 ref-name safety via normalizationLearnt from: CR PR: neuroglyph/git-mind#0 File: AGENTS.md:0-0 Timestamp: 2025-09-13T03:13:54.145Z Learning: Applies to include/gitmind.h : Maintain umbrella API at include/gitmind.hcore/tests/unit/test_cache_query.c (3)
39-41: Good: gm_snprintf with bounds checkThe format/size check is correct and consistent across tests.
93-99: Good: OID construction via gm_oid_from_raw with GM_OK assertsEdges and query OIDs are populated correctly.
Also applies to: 110-116
17-19: Replace raw strcpy with gm_strcpy_safe and assert resultsTwo raw strcpy calls remain; switch to gm_strcpy_safe and check return. Also include the correct header used elsewhere in tests.
#include "gitmind/edge.h" #include "gitmind/types.h" #include "gitmind/util/oid.h" #include "gitmind/security/string.h" +#include "gitmind/util/memory.h" @@ - edges[0].rel_type = GM_REL_IMPLEMENTS; edges[0].confidence = 0x3C00; strcpy(edges[0].src_path, "A"); strcpy(edges[0].tgt_path, "B"); - (void)gm_ulid_generate(edges[0].ulid); + edges[0].rel_type = GM_REL_IMPLEMENTS; + edges[0].confidence = 0x3C00; + assert(gm_strcpy_safe(edges[0].src_path, sizeof edges[0].src_path, "A") == GM_OK); + assert(gm_strcpy_safe(edges[0].tgt_path, sizeof edges[0].tgt_path, "B") == GM_OK); + gm_result_ulid_t ulr0 = gm_ulid_generate(edges[0].ulid); + assert(ulr0.ok); @@ - edges[1].rel_type = GM_REL_IMPLEMENTS; edges[1].confidence = 0x3C00; strcpy(edges[1].src_path, "A"); strcpy(edges[1].tgt_path, "C"); - (void)gm_ulid_generate(edges[1].ulid); + edges[1].rel_type = GM_REL_IMPLEMENTS; + edges[1].confidence = 0x3C00; + assert(gm_strcpy_safe(edges[1].src_path, sizeof edges[1].src_path, "A") == GM_OK); + assert(gm_strcpy_safe(edges[1].tgt_path, sizeof edges[1].tgt_path, "C") == GM_OK); + gm_result_ulid_t ulr1 = gm_ulid_generate(edges[1].ulid); + assert(ulr1.ok);As per coding guidelines.
Also applies to: 95-100
core/tests/unit/test_journal_mixed_cbor.c (1)
56-63: Good migration to safe string ops and gm_oid APIUsing gm_strcpy_safe and gm_oid_from_raw with explicit GM_OK assertions hardens the tests and aligns with OID‑first semantics. LGTM.
Also applies to: 67-79
core/tests/unit/test_edge_cbor_oid.c (1)
32-37: LGTM: OID creation + equality via gm helpersgm_oid_from_raw and gm_oid_equal usage is correct and matches OID‑first comparisons.
Also applies to: 50-51
core/tests/unit/test_journal_safety.c (1)
61-79: LGTM: OID‑first equality semantics coveredNice coverage: equal with same OIDs despite differing legacy SHAs, and negative cases when OIDs diverge.
Based on learnings
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/include/gitmind/util/oid.h (1)
40-48: New OID helpers are well-scoped and umbrella-safe.Signatures and NULL-handling are clear; align with OID-first equality policy. Consider clarifying in gm_oid_from_hex docs whether mixed-case hex is accepted (implementation via gm_hex_value likely allows both).
Also applies to: 49-56, 57-66, 68-76
core/tests/unit/test_cache_query.c (1)
17-19: Drop redundant header to avoid dual declarations.gm_strcpy_safe appears to be provided by gitmind/util/memory.h; including gitmind/security/string.h as well risks duplicate prototypes. Remove it unless it’s the canonical shim.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/include/gitmind/util/oid.h(2 hunks)core/src/util/oid.c(2 hunks)core/tests/unit/test_cache_query.c(4 hunks)docs/code-reviews/PR173/e0243058aac14ff3a8c83af2f6d4dcfe97dd2804.md(1 hunks)include/gitmind.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/code-reviews/PR173/e0243058aac14ff3a8c83af2f6d4dcfe97dd2804.md
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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/src/util/oid.cinclude/gitmind.hcore/tests/unit/test_cache_query.ccore/include/gitmind/util/oid.h
{core,apps,tests}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
{core,apps,tests}/**/*.{c,h}: Use gm_snprintf, gm_strcpy_safe, and gm_memcpy_span for all string/memory operations; check return codes and treat truncation as an error; zero output buffers on error paths before copying/formatting
C23 only with warnings-as-errors; no VLAs; no shadowing; all functions have explicit prototypes
Equality and identity are OID-first: when OIDs are present, compare via git_oid_cmp (typedef gm_oid_t) and do not fall back to SHA if OIDs exist
Prefer specific headers in includes; follow include order/regrouping enforced by .clang-format
Build refs via gm_build_ref and reject inputs starting with "refs/" to avoid double-prefixing; follow libgit2 ref-name safety via normalization
Files:
core/src/util/oid.ccore/tests/unit/test_cache_query.ccore/include/gitmind/util/oid.h
{core,apps}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Naming: functions/variables lower_snake_case with gm_ prefix; macros UPPER_SNAKE; typedef’d types end with t; header guards begin with GITMIND
Files:
core/src/util/oid.ccore/tests/unit/test_cache_query.ccore/include/gitmind/util/oid.h
core/src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
Do not allocate GM_PATH_MAX*2 buffers; prefer GM_PATH_MAX and allocate dynamically if larger is required
Files:
core/src/util/oid.c
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Formatting must follow .clang-format (LLVM style, 4 spaces, 80 cols, pointer alignment right); run clang-format via pre-commit
Files:
core/src/util/oid.cinclude/gitmind.hcore/tests/unit/test_cache_query.ccore/include/gitmind/util/oid.h
{include,core/include/gitmind}/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
{include,core/include/gitmind}/**/*.h: Public headers must be umbrella-safe, compile standalone, use stable header guards, and include C++ linkage guards (extern "C")
Avoid public ABI breaks: when changing public structs, only append new fields at the end; mark old fields deprecated rather than removing/reordering
Files:
include/gitmind.hcore/include/gitmind/util/oid.h
core/tests/unit/test_*.c
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests live in core/tests/unit/ and must be deterministic and isolated; name files test_.c
Files:
core/tests/unit/test_cache_query.c
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Applies to {core,apps,tests}/**/*.{c,h} : Equality and identity are OID-first: when OIDs are present, compare via git_oid_cmp (typedef gm_oid_t) and do not fall back to SHA if OIDs exist
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-29T06:07:18.746Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Applies to {core,apps,tests}/**/*.{c,h} : Equality and identity are OID-first: when OIDs are present, compare via git_oid_cmp (typedef gm_oid_t) and do not fall back to SHA if OIDs exist
Applied to files:
core/src/util/oid.ccore/tests/unit/test_cache_query.ccore/include/gitmind/util/oid.h
📚 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 include/gitmind.h : Maintain umbrella API at include/gitmind.h
Applied to files:
include/gitmind.hcore/tests/unit/test_cache_query.ccore/include/gitmind/util/oid.h
📚 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 include/gitmind/**/*.h : Public, namespaced headers live under include/gitmind/
Applied to files:
include/gitmind.hcore/tests/unit/test_cache_query.ccore/include/gitmind/util/oid.h
📚 Learning: 2025-09-29T06:07:18.746Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Applies to {include,core/include/gitmind}/**/*.h : Public headers must be umbrella-safe, compile standalone, use stable header guards, and include C++ linkage guards (extern "C")
Applied to files:
include/gitmind.hcore/include/gitmind/util/oid.h
📚 Learning: 2025-09-29T06:07:18.746Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Applies to {include,core/include/gitmind}/**/*.h : Avoid public ABI breaks: when changing public structs, only append new fields at the end; mark old fields deprecated rather than removing/reordering
Applied to files:
include/gitmind.hcore/include/gitmind/util/oid.h
📚 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 {include,core/include}/**/*.h : Header guards use GITMIND_* naming
Applied to files:
include/gitmind.hcore/include/gitmind/util/oid.h
📚 Learning: 2025-09-29T06:07:18.746Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Applies to {core,apps,tests}/**/*.{c,h} : Use gm_snprintf, gm_strcpy_safe, and gm_memcpy_span for all string/memory operations; check return codes and treat truncation as an error; zero output buffers on error paths before copying/formatting
Applied to files:
core/tests/unit/test_cache_query.c
📚 Learning: 2025-09-29T06:07:18.746Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:07:18.746Z
Learning: Applies to core/src/domain/journal/**/*.c : Journal is append-only under refs/gitmind/edges/<branch>; never mutate history entries
Applied to files:
core/tests/unit/test_cache_query.c
🧬 Code graph analysis (3)
core/src/util/oid.c (2)
core/include/gitmind/security/memory.h (1)
gm_memset_safe(47-59)core/include/gitmind/util/memory.h (1)
gm_memcpy_span(32-47)
core/tests/unit/test_cache_query.c (3)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)core/src/types/ulid.c (1)
gm_ulid_generate(118-126)core/src/cache/query.c (2)
gm_cache_query_fanout(391-395)gm_cache_result_free(405-411)
core/include/gitmind/util/oid.h (1)
core/src/util/oid.c (6)
bool(75-80)bool(82-92)int(36-63)int(65-73)int(94-112)int(114-144)
🔇 Additional comments (11)
include/gitmind.h (1)
31-31: Umbrella exposure of OID utilities added — LGTM.Public OID helpers are now re-exported via the umbrella header as required.
core/include/gitmind/util/oid.h (1)
7-7: Include <stdbool.h> — LGTM.Ensures bool in public signatures without relying on transitive includes.
core/tests/unit/test_cache_query.c (3)
39-41: Correct bounds-checked gm_snprintf usage — LGTM.Asserting no truncation matches guidelines.
91-110: Good migration to gm_oid_from_raw and gm_strcpy_safe.Zero-init, explicit sizes, and result checks look solid.
120-121: Query OIDs built via gm_oid_from_raw — LGTM.Consistent with gm_cache_query_* contracts.
Also applies to: 126-126
core/src/util/oid.c (6)
7-7: LGTM! Header includes properly added for new functionality.The new includes (
stdbool.h,string.h,gitmind/util/memory.h) and hex offset constant are correctly added to support the new OID utility functions.Also applies to: 10-10, 16-16, 21-22
23-34: LGTM! Hex parsing implementation is correct and defensive.The
gm_hex_valuehelper correctly handles both uppercase and lowercase hex digits with proper bounds checking, returning -1 for invalid characters.
75-80: Past review concern already addressed - raw memcmp usage is acceptable here.While the past review comment suggested replacing
memcmpwithgm_oid_equal, this would create a recursive call since we're implementinggm_oid_equalitself. The rawmemcmpusage is appropriate and correct for the core equality implementation.
82-92: LGTM! Zero-check implementation follows secure coding practices.The function correctly handles null input and uses a loop instead of
memcmpagainst a zero buffer, which is both clear and avoids potential timing attacks.
94-112: LGTM! Secure raw OID initialization with proper validation.The implementation correctly:
- Validates input parameters including exact length requirement
- Zero-initializes output on both entry and error paths using
gm_memset_safe- Uses
gm_memcpy_spanfor bounds-checked copying as per coding guidelines- Returns appropriate error codes for different failure modes
114-144: LGTM! Hex-to-OID conversion follows secure patterns.The implementation is well-structured with:
- Proper null checking and defensive programming
- Safe error handling with output zeroing on all error paths
- Correct hex string length validation (
GM_OID_HEX_CHARS)- Manual nibble assembly with appropriate bit shifting and masking
- Consistent error reporting
The manual hex parsing approach provides good control over validation and error handling compared to library functions.
1 participant
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests