Add hex ports, cache coordinators, and journal adapter robustness - #177
Conversation
… context; wire in build\n\n- Add gm_logger_port and gm_metrics_port headers (umbrella-safe, no-op wrappers)\n- Add stdio logger + null metrics adapters under core/src/adapters/**\n- Append logger/metrics to gm_context_t at tail (no ABI break)\n- Update meson build + header checks\n\nCI: builds/tests remain green in container
…dinators; wire into build\n\n- cache_build_port.h and cache_query_port.h (vtbls)\n- default coordinators in core/src/ports/cache/** delegating to existing APIs\n- meson updated to compile new coordinators\n\nGreen CI in container
…; use from cache query\n\n- New internal header cache/internal/staleness.h (pure, inline)\n- gm_cache_is_stale uses gm_cache_staleness_time for age check
…cache flows\n\n- Tolerate missing parent ref during journal commit (empty history)\n- Update provided ref after journal commit\n- Create ref parent directories for custom namespaces; respect force flag\n- Signature fallback for bare repos\n\nAll tests pass in container
…d record status\n\n- Add minimal PRDs, planning, specs, and wishlist placeholders with frontmatter\n- Add/refresh migration-progress.md (tracker)\n- CI docs link checks pass
WalkthroughIntroduces telemetry (logs/metrics/diagnostics) infrastructure and DI seams; adds logger/metrics/diagnostic ports and adapters; implements cache and journal driving/query ports; adds journal codec/plan/read decoding; instruments cache rebuild and journal read/write with telemetry; factors cache OID prefix and staleness helpers; updates CLI to wire logging/diagnostics; expands tests and build integration; broad docs overhaul. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI
participant BuildPort as Cache Build Port
participant Service as Cache Rebuild Service
participant Repo as Git Repo Port
participant Log as Logger Port
participant Met as Metrics Port
participant Diag as Diagnostics Port
User->>CLI: gitmind cache-rebuild (--json/--verbose)
CLI->>BuildPort: request_build(branch, force_full)
BuildPort->>Service: gm_cache_rebuild_execute(ctx, branch)
Service->>Log: log(event=rebuild_start, tags)
Service->>Diag: emit(component=cache, event=rebuild_start, kvs)
Service->>Repo: build_tree_from_directory(...)
alt success
Service->>Repo: commit_create + reference_update
Service->>Met: timing_ms(name=cache.rebuild.duration_ms)
Service->>Met: gauge_set/counter_add (edges_total, tree_size)
Service->>Log: log(event=rebuild_ok, tags)
else failure
Service->>Diag: emit(..., event=rebuild_*_failed, code)
Service->>Log: log(event=rebuild_failed, code)
end
Service-->>BuildPort: result
BuildPort-->>CLI: result
sequenceDiagram
autonumber
participant Client as Client
participant JPort as Journal Command Port
participant Writer as Journal Writer
participant Codec as Codec/Planner
participant Repo as Git Repo Port
participant Reader as Journal Reader
participant Log as Logger
participant Met as Metrics
participant Diag as Diagnostics
Client->>JPort: append(edges)
JPort->>Writer: gm_journal_append(...)
Writer->>Log: log(journal_append_start, tags)
Writer->>Codec: encode message + build commit plan
Writer->>Repo: commit_create + reference_update
alt success
Writer->>Met: timing(counter edges_total)
Writer->>Log: log(journal_append_ok)
else NFF retry
Writer->>Diag: emit(journal_nff_retry)
Writer->>Repo: reference_update (retry)
end
Client->>Reader: walk_journal(...)
Reader->>Log: log(journal_read_start, tags)
Reader->>Repo: walk_commits / read messages
Reader->>Codec: decode edge(s)
alt success
Reader->>Met: timing(counter edges_total)
Reader->>Log: log(journal_read_ok)
else failure
Reader->>Diag: emit(journal_read_failed, code)
Reader->>Log: log(journal_read_failed, code)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 11
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
core/include/gitmind/cache/internal/staleness.h(1 hunks)core/include/gitmind/context.h(2 hunks)core/include/gitmind/ports/cache_build_port.h(1 hunks)core/include/gitmind/ports/cache_query_port.h(1 hunks)core/include/gitmind/ports/logger_port.h(1 hunks)core/include/gitmind/ports/metrics_port.h(1 hunks)core/src/adapters/git/libgit2_repository_port.c(3 hunks)core/src/adapters/logging/stdio_logger_adapter.c(1 hunks)core/src/adapters/logging/stdio_logger_adapter.h(1 hunks)core/src/adapters/metrics/null_metrics_adapter.c(1 hunks)core/src/adapters/metrics/null_metrics_adapter.h(1 hunks)core/src/cache/query.c(2 hunks)core/src/journal/writer.c(2 hunks)core/src/ports/cache/cache_build_port.c(1 hunks)core/src/ports/cache/cache_query_port.c(1 hunks)docs/PRDs/PRD-co-thought-mcp-service.md(1 hunks)docs/PRDs/PRD-git-mind-semantics-time-travel-prototype.md(1 hunks)docs/code-reviews/PR177/47a1bf537feb065ef3cbac02a8bf78e75944111c.md(1 hunks)docs/planning/Milestones.md(1 hunks)docs/planning/Product_Roadmap.md(1 hunks)docs/planning/Release_Plans.md(1 hunks)docs/planning/Sprint_Plans.md(1 hunks)docs/specs/Technical_Specifications.md(1 hunks)docs/wish-list-features/README.md(1 hunks)meson.build(2 hunks)migration-progress.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{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/adapters/metrics/null_metrics_adapter.ccore/include/gitmind/cache/internal/staleness.hcore/src/adapters/metrics/null_metrics_adapter.hcore/src/ports/cache/cache_build_port.ccore/include/gitmind/ports/cache_query_port.hcore/src/adapters/logging/stdio_logger_adapter.hcore/src/adapters/git/libgit2_repository_port.ccore/src/journal/writer.ccore/include/gitmind/ports/logger_port.hcore/include/gitmind/ports/metrics_port.hcore/include/gitmind/ports/cache_build_port.hcore/src/ports/cache/cache_query_port.ccore/src/cache/query.ccore/src/adapters/logging/stdio_logger_adapter.ccore/include/gitmind/context.h
{core,src,apps,tests}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
{core,src,apps,tests}/**/*.{c,h}: Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
Do not use buffers sized GM_PATH_MAX*2; prefer GM_PATH_MAX and allocate if larger is required
Equality is OID-first; if both OIDs exist they must match; only fall back to SHA when OIDs are absent
Build refs via gm_build_ref and reject inputs that start with "refs/"
Language: C23 with warnings-as-errors; no VLAs, no shadowing; declare explicit prototypes
Formatting via .clang-format (LLVM style, 4 spaces, 80 cols, pointer alignment right); pre-commit runs clang-format
Naming: functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE; types end with t; header guards start with GITMIND
Includes: prefer specific headers; order/regroup per .clang-format; include-what-you-use
No new clang-tidy warnings; keep touched files tidy-clean
Surface failures via gm_result_t (and variants); ports must not return raw integers
Maintain one responsibility per translation unit where practical; split multi-concern files when touched
Prefer OID types and helpers (gm_oid_t, git_oid_cmp via wrappers, gm_oid_to_hex); avoid direct libgit2 helpers in domain/application layers
Zero output buffers before formatting/copying on error paths
Files:
core/src/adapters/metrics/null_metrics_adapter.ccore/include/gitmind/cache/internal/staleness.hcore/src/adapters/metrics/null_metrics_adapter.hcore/src/ports/cache/cache_build_port.ccore/include/gitmind/ports/cache_query_port.hcore/src/adapters/logging/stdio_logger_adapter.hcore/src/adapters/git/libgit2_repository_port.ccore/src/journal/writer.ccore/include/gitmind/ports/logger_port.hcore/include/gitmind/ports/metrics_port.hcore/include/gitmind/ports/cache_build_port.hcore/src/ports/cache/cache_query_port.ccore/src/cache/query.ccore/src/adapters/logging/stdio_logger_adapter.ccore/include/gitmind/context.h
core/src/adapters/**/*_adapter.c
📄 CodeRabbit inference engine (AGENTS.md)
Runtime adapters live under core/src/adapters//_adapter.c and expose factories returning the port vtable plus state with teardown hooks
Files:
core/src/adapters/metrics/null_metrics_adapter.ccore/src/adapters/logging/stdio_logger_adapter.c
core/include/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Public headers under core/include must be umbrella-safe: compile standalone, include only what they use, stable header guards, and provide extern "C" when included from C++
Files:
core/include/gitmind/cache/internal/staleness.hcore/include/gitmind/ports/cache_query_port.hcore/include/gitmind/ports/logger_port.hcore/include/gitmind/ports/metrics_port.hcore/include/gitmind/ports/cache_build_port.hcore/include/gitmind/context.h
core/include/gitmind/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Public structs in headers must avoid ABI breaks; append new fields at the struct tail and mark deprecated fields instead of removing
Files:
core/include/gitmind/cache/internal/staleness.hcore/include/gitmind/ports/cache_query_port.hcore/include/gitmind/ports/logger_port.hcore/include/gitmind/ports/metrics_port.hcore/include/gitmind/ports/cache_build_port.hcore/include/gitmind/context.h
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Docs must have front matter first, a single H1, a '## Table of Contents', and the title must match the H1
Add api_version field to API docs front matter
Place License/SPDX comments immediately after front matter in docs
README and other Markdown must comply with markdownlint rules: underscore emphasis/strong, blockquote spacing, code fence formatting, no inline HTML wrappers, heading punctuation
Files:
docs/wish-list-features/README.mddocs/code-reviews/PR177/47a1bf537feb065ef3cbac02a8bf78e75944111c.mddocs/planning/Product_Roadmap.mddocs/PRDs/PRD-co-thought-mcp-service.mddocs/planning/Release_Plans.mddocs/planning/Sprint_Plans.mddocs/planning/Milestones.mddocs/specs/Technical_Specifications.mddocs/PRDs/PRD-git-mind-semantics-time-travel-prototype.md
core/src/adapters/**/*_adapter.h
📄 CodeRabbit inference engine (AGENTS.md)
Adapter private headers colocate under core/src/adapters//_adapter.h
Files:
core/src/adapters/metrics/null_metrics_adapter.hcore/src/adapters/logging/stdio_logger_adapter.h
core/src/ports/**
📄 CodeRabbit inference engine (AGENTS.md)
Default implementations for simple inbound coordinators that remain in C may live under core/src/ports/**
Files:
core/src/ports/cache/cache_build_port.ccore/src/ports/cache/cache_query_port.c
core/src/**/{hooks,cache,journal}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid direct libgit2 usage; route repository/ref operations through gm_git_repository_port/gm_git_ref_port adapters
Files:
core/src/ports/cache/cache_build_port.ccore/src/journal/writer.ccore/src/ports/cache/cache_query_port.ccore/src/cache/query.c
core/include/gitmind/ports/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Define all inbound/outbound ports as headers only under core/include/gitmind/ports/** with POD structs and function-pointer vtables (no non-trivial inline behavior)
Files:
core/include/gitmind/ports/cache_query_port.hcore/include/gitmind/ports/logger_port.hcore/include/gitmind/ports/metrics_port.hcore/include/gitmind/ports/cache_build_port.h
meson.build
📄 CodeRabbit inference engine (AGENTS.md)
Target C23 via Meson c2x and keep warnings-as-errors; register new unit test targets in meson.build
Files:
meson.build
🧠 Learnings (6)
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to core/src/**/{hooks,cache,journal}/**/*.{c,h} : Avoid direct libgit2 usage; route repository/ref operations through gm_git_repository_port/gm_git_ref_port adapters
Applied to files:
core/src/adapters/git/libgit2_repository_port.cmeson.build
📚 Learning: 2025-10-01T04:13:25.749Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.749Z
Learning: Applies to core/include/gitmind/ports/**/*.h : Define all inbound/outbound ports as headers only under core/include/gitmind/ports/** with POD structs and function-pointer vtables (no non-trivial inline behavior)
Applied to files:
core/include/gitmind/ports/logger_port.hcore/include/gitmind/context.hmeson.build
📚 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/include/gitmind/context.hmeson.build
📚 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:
meson.build
📚 Learning: 2025-10-01T04:13:25.749Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.749Z
Learning: Applies to docs/activity/**/*.md : Document migration progress and lessons under docs/activity/<date>_hexagonal.md
Applied to files:
migration-progress.md
📚 Learning: 2025-10-01T04:13:25.749Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.749Z
Learning: Applies to docs/architecture/hexagonal/** : Track adapter status and migrations in docs/architecture/hexagonal/**; keep diagrams and status tables updated
Applied to files:
migration-progress.md
🧬 Code graph analysis (10)
core/src/adapters/metrics/null_metrics_adapter.c (1)
core/src/adapters/logging/stdio_logger_adapter.c (1)
gm_result_void_t(51-65)
core/src/adapters/metrics/null_metrics_adapter.h (1)
core/src/adapters/metrics/null_metrics_adapter.c (2)
gm_result_void_t(34-45)gm_null_metrics_port_dispose(47-52)
core/src/ports/cache/cache_build_port.c (2)
core/src/cache/builder.c (1)
gm_cache_rebuild(9-19)core/src/ports/cache/cache_query_port.c (1)
gm_result_void_t(72-87)
core/include/gitmind/ports/cache_query_port.h (1)
core/src/ports/cache/cache_query_port.c (2)
gm_result_void_t(72-87)gm_qry_cache_port_dispose(89-96)
core/src/adapters/logging/stdio_logger_adapter.h (1)
core/src/adapters/logging/stdio_logger_adapter.c (2)
gm_result_void_t(51-65)gm_stdio_logger_port_dispose(67-72)
core/src/journal/writer.c (1)
core/src/error/error.c (1)
gm_error_free(161-178)
core/include/gitmind/ports/cache_build_port.h (1)
core/src/ports/cache/cache_build_port.c (2)
gm_result_void_t(42-57)gm_cmd_cache_build_port_dispose(59-64)
core/src/ports/cache/cache_query_port.c (2)
core/src/cache/query.c (3)
gm_cache_query_fanout(392-396)gm_cache_query_fanin(399-403)gm_cache_stats(415-448)core/src/ports/cache/cache_build_port.c (1)
gm_result_void_t(42-57)
core/src/cache/query.c (1)
core/include/gitmind/cache/internal/staleness.h (1)
gm_cache_staleness_time(15-20)
core/src/adapters/logging/stdio_logger_adapter.c (1)
core/src/adapters/metrics/null_metrics_adapter.c (1)
gm_result_void_t(34-45)
🔇 Additional comments (23)
core/include/gitmind/ports/metrics_port.h (3)
1-10: LGTM!Header guard, includes, and license header are correct and follow the project conventions.
20-34: Well-designed port interface.The vtable-based design with non-owning vtbl pointer and opaque adapter state follows the hexagonal architecture pattern correctly. Function signatures are consistent and the
tagsparameter enables flexible metadata attachment.
36-62: Excellent null-safety pattern for optional metrics.The inline wrappers correctly default to no-op success when the port or specific function is unset, which is the right behavior for optional observability features.
core/src/cache/query.c (2)
23-23: LGTM!The new internal staleness header is appropriately included and positioned correctly among other internal headers.
175-177: Good refactor with improved robustness.The centralized staleness helper adds a defensive check for
now_time <= journal_tip_time, which prevents potential unsigned arithmetic issues with clock skew or test scenarios. This is a robustness improvement over the manual check.migration-progress.md (1)
6-115: Excellent migration tracking structure.The comprehensive checklist with clear modules, completion criteria, and verification steps provides a solid foundation for tracking the hexagonal architecture migration. The structure aligns well with the project's documented approach.
core/include/gitmind/cache/internal/staleness.h (2)
1-13: LGTM!Header structure, guard naming, and includes are correct and follow project conventions.
14-20: Clean and correct staleness logic.The function correctly handles edge cases (clock skew via the
now_time <= journal_tip_timecheck) and provides clear staleness semantics. Good encapsulation for reuse across cache operations.core/src/adapters/metrics/null_metrics_adapter.h (1)
1-21: LGTM!Adapter header follows project conventions: correct header guard pattern, appropriate use of
GM_NODISCARDon the init function, and proper location undercore/src/adapters/metrics/.core/include/gitmind/context.h (2)
15-16: LGTM!New port header includes are correctly positioned and follow the existing include pattern.
47-54: Well-structured context extension.The new logger and metrics ports are correctly appended at the struct tail, preserving ABI compatibility. The comment accurately describes their optional nature and the no-op behavior of wrappers when uninitialized.
core/src/adapters/logging/stdio_logger_adapter.h (1)
1-23: LGTM!Adapter header is well-structured with correct header guard, appropriate includes (stdio.h for FILE*), and proper use of
GM_NODISCARDon the init function. The signature design allowing FILE* and min_level configuration is sound.core/src/adapters/metrics/null_metrics_adapter.c (2)
9-11: LGTM: Minimal state for null adapter.The dummy state struct with
_unusedfield maintains consistency with the dispose pattern and is safe, even though the adapter requires no actual state.
34-45: LGTM: Proper initialization with validation and error handling.The function correctly validates inputs, handles allocation failure, and returns appropriate error codes following the port pattern.
core/src/ports/cache/cache_query_port.c (2)
16-31: LGTM: Proper fanout query wrapper with validation.The implementation correctly validates all required inputs, retrieves state, delegates to the underlying API, and wraps the result appropriately.
50-64: LGTM: Stats implementation correctly handles optional outputs.The function properly validates required inputs (self, state, branch) while allowing
edge_countandcache_size_bytesto be NULL, which aligns with the underlyinggm_cache_statsAPI contract.core/src/adapters/logging/stdio_logger_adapter.c (1)
51-65: LGTM: Proper initialization and non-owning stream handling.The function validates both port and stream, handles allocation failure correctly, and properly initializes the state. The dispose function correctly does not close the FILE* stream since it's non-owning.
core/include/gitmind/ports/cache_build_port.h (2)
29-32: LGTM: Proper port structure with non-owning vtbl.The port struct correctly uses a const vtbl pointer and opaque state, following the standard port pattern and preventing accidental modification of the vtbl.
34-43: LGTM: Clean vtbl interface with appropriate return types.The vtbl provides two operations with
gm_result_void_treturns, properly surfacing failures per the coding guidelines that "ports must not return raw integers."core/include/gitmind/ports/cache_query_port.h (2)
27-30: LGTM: Consistent port structure.The port type follows the same pattern as other ports in the codebase, with a non-owning vtbl and opaque state.
32-50: LGTM: Well-designed query interface.The vtbl provides separate operations for fanout (by source OID), fanin (by target OID), and statistics retrieval, with appropriate parameter types and consistent use of
gm_result_void_tfor error handling.core/src/ports/cache/cache_build_port.c (2)
15-27: LGTM: Proper wrapper over cache rebuild API.The function validates all required inputs, delegates to
gm_cache_rebuild, and correctly wraps the integer result code intogm_result_void_tas required by the coding guidelines.
29-35: LGTM: Documented no-op invalidate is acceptable.The best-effort no-op with clear documentation is appropriate for this initial port implementation. Future enhancements can add proper invalidation logic when needed.
flyingrobots
changed the title
…to current files" This reverts commit cbf325f.
…at/hex-ports-ci-green
…tle; code: use time ops in stdio logger
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/include/gitmind/ports/logger_port.h (1)
7-7: Remove unused include.The
stddef.hinclude does not appear to be used in this header. The header only requiresgm_result_void_tfromgitmind/result.h.Apply this diff to remove the unused include:
-#include <stddef.h> - #include "gitmind/result.h"core/include/gitmind/ports/cache_query_port.h (1)
61-65: Consider adding parameter documentation for the stats function.The
statsfunction could benefit from brief parameter descriptions (similar toquery_fanout), clarifying whatedge_countandcache_size_bytesrepresent and the expected behavior when the branch doesn't exist.Example enhancement:
- /** Retrieve cache statistics for a branch. */ + /** + * Retrieve cache statistics for a branch. + * @param self Port instance. + * @param branch Branch name. + * @param edge_count Output: total edges in cache for this branch. + * @param cache_size_bytes Output: approximate memory usage in bytes. + */ gm_result_void_t (*stats)(gm_qry_cache_port_t *self, const char *branch, uint64_t *edge_count, uint64_t *cache_size_bytes);
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/include/gitmind/ports/cache_build_port.h(1 hunks)core/include/gitmind/ports/cache_query_port.h(1 hunks)core/include/gitmind/ports/logger_port.h(1 hunks)core/include/gitmind/ports/metrics_port.h(1 hunks)docs/code-reviews/PR177/3b7ce522e107011f8f92dc629facfa5b6a3595fb.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/include/gitmind/ports/metrics_port.h
- core/include/gitmind/ports/cache_build_port.h
🧰 Additional context used
📓 Path-based instructions (6)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Docs must have front matter first, a single H1, a '## Table of Contents', and the title must match the H1
Add api_version field to API docs front matter
Place License/SPDX comments immediately after front matter in docs
README and other Markdown must comply with markdownlint rules: underscore emphasis/strong, blockquote spacing, code fence formatting, no inline HTML wrappers, heading punctuation
Files:
docs/code-reviews/PR177/3b7ce522e107011f8f92dc629facfa5b6a3595fb.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/include/gitmind/ports/logger_port.hcore/include/gitmind/ports/cache_query_port.h
{core,src,apps,tests}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
{core,src,apps,tests}/**/*.{c,h}: Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
Do not use buffers sized GM_PATH_MAX*2; prefer GM_PATH_MAX and allocate if larger is required
Equality is OID-first; if both OIDs exist they must match; only fall back to SHA when OIDs are absent
Build refs via gm_build_ref and reject inputs that start with "refs/"
Language: C23 with warnings-as-errors; no VLAs, no shadowing; declare explicit prototypes
Formatting via .clang-format (LLVM style, 4 spaces, 80 cols, pointer alignment right); pre-commit runs clang-format
Naming: functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE; types end with t; header guards start with GITMIND
Includes: prefer specific headers; order/regroup per .clang-format; include-what-you-use
No new clang-tidy warnings; keep touched files tidy-clean
Surface failures via gm_result_t (and variants); ports must not return raw integers
Maintain one responsibility per translation unit where practical; split multi-concern files when touched
Prefer OID types and helpers (gm_oid_t, git_oid_cmp via wrappers, gm_oid_to_hex); avoid direct libgit2 helpers in domain/application layers
Zero output buffers before formatting/copying on error paths
Files:
core/include/gitmind/ports/logger_port.hcore/include/gitmind/ports/cache_query_port.h
core/include/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Public headers under core/include must be umbrella-safe: compile standalone, include only what they use, stable header guards, and provide extern "C" when included from C++
Files:
core/include/gitmind/ports/logger_port.hcore/include/gitmind/ports/cache_query_port.h
core/include/gitmind/ports/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Define all inbound/outbound ports as headers only under core/include/gitmind/ports/** with POD structs and function-pointer vtables (no non-trivial inline behavior)
Files:
core/include/gitmind/ports/logger_port.hcore/include/gitmind/ports/cache_query_port.h
core/include/gitmind/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Public structs in headers must avoid ABI breaks; append new fields at the struct tail and mark deprecated fields instead of removing
Files:
core/include/gitmind/ports/logger_port.hcore/include/gitmind/ports/cache_query_port.h
🧠 Learnings (1)
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to core/include/gitmind/ports/**/*.h : Define all inbound/outbound ports as headers only under core/include/gitmind/ports/** with POD structs and function-pointer vtables (no non-trivial inline behavior)
Applied to files:
core/include/gitmind/ports/logger_port.h
🧬 Code graph analysis (1)
core/include/gitmind/ports/cache_query_port.h (1)
core/src/ports/cache/cache_query_port.c (2)
gm_result_void_t(72-87)gm_qry_cache_port_dispose(89-96)
🔇 Additional comments (2)
core/include/gitmind/ports/logger_port.h (1)
33-76: LGTM!The logger port design is well-structured and follows all guidelines:
- POD structs with function-pointer vtables (as per coding guidelines)
- Returns
gm_result_void_tinstead of raw integers- Sparse enum values (10/20/30/40) allow future extensibility
- Inline wrapper provides safe no-op behavior when port is unset
- Comprehensive documentation covers thread-safety and lifetime expectations
- Umbrella-safe with proper C++ guards and minimal includes
The trivial inline wrapper correctly implements the safe no-op pattern mentioned in the design goals.
Based on learnings
core/include/gitmind/ports/cache_query_port.h (1)
1-79: LGTM! Well-structured inbound port following hexagonal architecture guidelines.The port header is umbrella-safe, uses POD structs with function-pointer vtables, returns proper
gm_result_void_ttypes, and documents memory ownership clearly. The lifecycle functions (init/dispose) match the implementation incache_query_port.c, and all naming conventions are followed.
…emove unused include in logger_port; expand stats() docs
…s to satisfy docs gate
…ont matter+SPDX+TOC)
…ocs fixes (arch links, placeholders); resolve review artifact
… AGENTS.md debrief JSONL and Neo4j instructions
…ind into feat/hex-ports-ci-green
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
docs/code-reviews/PR177/0d1b34b0ee501bc7b7b2d0a9fb82e06a37642c77.md (1)
1-90: LGTM! Port header follows guidelines.The header is umbrella-safe with proper guards, follows the required POD struct + function-pointer vtbl pattern, and declares (but doesn't inline) lifecycle functions as specified. Documentation is comprehensive.
Note: There's a minor pattern inconsistency with
logger_port.h— this port passes the entiregm_qry_cache_port_t *selfto vtbl functions whilelogger_port.hpasses onlyvoid *self. While both work, consistent patterns across ports would improve API predictability.core/include/gitmind/ports/logger_port.h (1)
40-59: Consider port pattern consistency.This port uses
void *selfin both the struct (line 42) and vtbl signature (line 55), whereascache_query_port.husesvoid *statein the struct and passes the entiregm_qry_cache_port_t *to vtbl functions.For API predictability, consider standardizing the pattern across all ports. The current approach (passing just the state pointer) is more efficient but less flexible for multi-port scenarios.
core/include/gitmind/ports/cache_query_port.h (1)
7-14: Consider removing unused standard library includes.The header includes
stdbool.handstddef.h, but neitherboolnorsize_t/NULLare used in the declarations. Onlystdint.h(foruint64_t) is required. Removing unused includes aligns with the include-what-you-use guideline.Apply this diff to remove unused includes:
-#include <stdbool.h> -#include <stddef.h> #include <stdint.h>
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.gm_cache_query_tmp/HEAD(1 hunks).gm_cache_query_tmp/config(1 hunks).gm_cache_query_tmp/description(1 hunks).gm_cache_query_tmp/hooks/README.sample(1 hunks).gm_cache_query_tmp/info/exclude(1 hunks).gm_cache_query_tmp/objects/21/943f67f4f74c4e95e464fed49773b2cdd90201(1 hunks).gm_cache_query_tmp/objects/38/7b18948ca7f5a90328f210a8bdb9a31921511d(1 hunks).gm_cache_query_tmp/objects/3f/91d2035120c147e5a56e1b425d914fb0987bc8(1 hunks).gm_cache_query_tmp/objects/97/11a7f1328e87390eacceb41e29f2bf9be83e0f(1 hunks).gm_cache_query_tmp/objects/b1/784fa26fc13a9305a66378693c180d89edff40(1 hunks).gm_cache_query_tmp/objects/bd/ad5e8ce0946d7d4fd942975062575b27b34764(1 hunks).gm_cache_query_tmp/objects/e2/db68efc1d70cb7d07963ef63f17757c2231b9b(1 hunks).gm_cache_query_tmp/objects/ec/8d09625e4264e13ceb2eb50b7b28e97ca58cfe(1 hunks).gm_cache_query_tmp/objects/fc/7c7afad58fbf322f23a2848a4462887a98bc52(1 hunks).gm_cache_query_tmp/refs/gitmind/cache/testq(1 hunks).gm_cache_query_tmp/refs/gitmind/edges/testq(1 hunks).gm_cache_query_tmp/refs/heads/testq(1 hunks)core/include/gitmind/ports/cache_query_port.h(1 hunks)core/include/gitmind/ports/logger_port.h(1 hunks)docs/README.md(1 hunks)docs/architecture/attribution-integration-guide.md(0 hunks)docs/architecture/attribution-system.md(0 hunks)docs/architecture/attribution-use-cases.md(0 hunks)docs/architecture/augments.md(1 hunks)docs/architecture/cache-architecture.md(1 hunks)docs/architecture/journal-architecture.md(1 hunks)docs/architecture/ref-name-validation.md(1 hunks)docs/architecture/system-overview.md(1 hunks)docs/code-reviews/PR177/0d1b34b0ee501bc7b7b2d0a9fb82e06a37642c77.md(1 hunks)docs/code-reviews/PR177/3b7ce522e107011f8f92dc629facfa5b6a3595fb.md(1 hunks)
💤 Files with no reviewable changes (3)
- docs/architecture/attribution-integration-guide.md
- docs/architecture/attribution-use-cases.md
- docs/architecture/attribution-system.md
✅ Files skipped from review due to trivial changes (18)
- .gm_cache_query_tmp/objects/21/943f67f4f74c4e95e464fed49773b2cdd90201
- .gm_cache_query_tmp/HEAD
- .gm_cache_query_tmp/objects/ec/8d09625e4264e13ceb2eb50b7b28e97ca58cfe
- .gm_cache_query_tmp/refs/gitmind/edges/testq
- .gm_cache_query_tmp/config
- .gm_cache_query_tmp/objects/97/11a7f1328e87390eacceb41e29f2bf9be83e0f
- .gm_cache_query_tmp/objects/fc/7c7afad58fbf322f23a2848a4462887a98bc52
- .gm_cache_query_tmp/objects/bd/ad5e8ce0946d7d4fd942975062575b27b34764
- .gm_cache_query_tmp/objects/3f/91d2035120c147e5a56e1b425d914fb0987bc8
- .gm_cache_query_tmp/description
- .gm_cache_query_tmp/info/exclude
- .gm_cache_query_tmp/hooks/README.sample
- .gm_cache_query_tmp/refs/gitmind/cache/testq
- docs/architecture/cache-architecture.md
- .gm_cache_query_tmp/objects/38/7b18948ca7f5a90328f210a8bdb9a31921511d
- .gm_cache_query_tmp/refs/heads/testq
- .gm_cache_query_tmp/objects/b1/784fa26fc13a9305a66378693c180d89edff40
- docs/architecture/augments.md
🧰 Additional context used
📓 Path-based instructions (6)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Docs must have front matter first, a single H1, a '## Table of Contents', and the title must match the H1
Add api_version field to API docs front matter
Place License/SPDX comments immediately after front matter in docs
README and other Markdown must comply with markdownlint rules: underscore emphasis/strong, blockquote spacing, code fence formatting, no inline HTML wrappers, heading punctuation
Files:
docs/code-reviews/PR177/3b7ce522e107011f8f92dc629facfa5b6a3595fb.mddocs/README.mddocs/architecture/ref-name-validation.mddocs/code-reviews/PR177/0d1b34b0ee501bc7b7b2d0a9fb82e06a37642c77.mddocs/architecture/system-overview.mddocs/architecture/journal-architecture.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/include/gitmind/ports/logger_port.hcore/include/gitmind/ports/cache_query_port.h
{core,src,apps,tests}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
{core,src,apps,tests}/**/*.{c,h}: Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
Do not use buffers sized GM_PATH_MAX*2; prefer GM_PATH_MAX and allocate if larger is required
Equality is OID-first; if both OIDs exist they must match; only fall back to SHA when OIDs are absent
Build refs via gm_build_ref and reject inputs that start with "refs/"
Language: C23 with warnings-as-errors; no VLAs, no shadowing; declare explicit prototypes
Formatting via .clang-format (LLVM style, 4 spaces, 80 cols, pointer alignment right); pre-commit runs clang-format
Naming: functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE; types end with t; header guards start with GITMIND
Includes: prefer specific headers; order/regroup per .clang-format; include-what-you-use
No new clang-tidy warnings; keep touched files tidy-clean
Surface failures via gm_result_t (and variants); ports must not return raw integers
Maintain one responsibility per translation unit where practical; split multi-concern files when touched
Prefer OID types and helpers (gm_oid_t, git_oid_cmp via wrappers, gm_oid_to_hex); avoid direct libgit2 helpers in domain/application layers
Zero output buffers before formatting/copying on error paths
Files:
core/include/gitmind/ports/logger_port.hcore/include/gitmind/ports/cache_query_port.h
core/include/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Public headers under core/include must be umbrella-safe: compile standalone, include only what they use, stable header guards, and provide extern "C" when included from C++
Files:
core/include/gitmind/ports/logger_port.hcore/include/gitmind/ports/cache_query_port.h
core/include/gitmind/ports/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Define all inbound/outbound ports as headers only under core/include/gitmind/ports/** with POD structs and function-pointer vtables (no non-trivial inline behavior)
Files:
core/include/gitmind/ports/logger_port.hcore/include/gitmind/ports/cache_query_port.h
core/include/gitmind/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Public structs in headers must avoid ABI breaks; append new fields at the struct tail and mark deprecated fields instead of removing
Files:
core/include/gitmind/ports/logger_port.hcore/include/gitmind/ports/cache_query_port.h
🧠 Learnings (2)
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to core/include/gitmind/ports/**/*.h : Define all inbound/outbound ports as headers only under core/include/gitmind/ports/** with POD structs and function-pointer vtables (no non-trivial inline behavior)
Applied to files:
core/include/gitmind/ports/logger_port.h
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to docs/architecture/Ref_Name_Validation.md : Document and adhere to ref-name validation policy (reject leading refs/; normalize via libgit2) as specified
Applied to files:
docs/architecture/ref-name-validation.md
🧬 Code graph analysis (1)
core/include/gitmind/ports/cache_query_port.h (2)
core/src/ports/cache/cache_build_port.c (1)
gm_result_void_t(42-57)core/src/ports/cache/cache_query_port.c (2)
gm_result_void_t(72-87)gm_qry_cache_port_dispose(89-96)
🔇 Additional comments (4)
docs/code-reviews/PR177/3b7ce522e107011f8f92dc629facfa5b6a3595fb.md (1)
1-33: LGTM! Documentation compliance achieved.All previously flagged issues have been addressed: the front matter title now matches the H1 heading, the SPDX/license block is correctly positioned after the front matter, and the Table of Contents section is present.
core/include/gitmind/ports/logger_port.h (2)
66-74: Verify inline wrapper aligns with port guidelines.The inline wrapper
gm_logger_log()provides safe no-op behavior when the port is unset, which aligns with the documented design goals. However, the coding guidelines specify "no non-trivial inline behavior" for ports.While this wrapper is simple (null checks + delegation), ensure this pattern is acceptable. If approved, consider documenting this exception explicitly or updating guidelines to clarify what constitutes "non-trivial."
Based on learnings.
1-80: LGTM! Port header is umbrella-safe and well-documented.The header follows the required POD struct + vtbl pattern with proper extern "C" guards, minimal includes, and stable header guards. Documentation clearly explains design goals, thread-safety expectations, and lifecycle ownership.
core/include/gitmind/ports/cache_query_port.h (1)
1-90: LGTM! Port design follows hexagonal architecture guidelines.The cache query port header adheres to the port design guidelines:
- POD struct with vtbl and opaque state
- Function pointers return
gm_result_void_t(no raw integers)- Umbrella-safe: standalone compilation, stable header guard, C++ extern guard
- Clear memory ownership documentation for dynamically allocated results
- Lifecycle functions marked with
GM_NODISCARDfor proper error handlingThe thin coordinator design cleanly separates the port interface from implementation details.
| --- | ||
| title: Project: No Tux Given 🐧⚡ | ||
| description: Edge-journal commits with branch-aware graphs; the final architecture for storing semantics in Git. | ||
| audience: [developers] | ||
| domain: [architecture] | ||
| tags: [journal, cbor, refs] | ||
| status: stable | ||
| last_updated: 2025-09-15 | ||
| --- |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add the SPDX notice after the front matter
The documentation standard requires an SPDX/license comment immediately following the YAML header.
---
title: Project: No Tux Given 🐧⚡
description: Edge-journal commits with branch-aware graphs; the final architecture for storing semantics in Git.
audience: [developers]
domain: [architecture]
tags: [journal, cbor, refs]
status: stable
last_updated: 2025-09-15
---
+<!-- SPDX-License-Identifier: Apache-2.0 -->📝 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: Project: No Tux Given 🐧⚡ | |
| description: Edge-journal commits with branch-aware graphs; the final architecture for storing semantics in Git. | |
| audience: [developers] | |
| domain: [architecture] | |
| tags: [journal, cbor, refs] | |
| status: stable | |
| last_updated: 2025-09-15 | |
| --- | |
| --- | |
| title: Project: No Tux Given 🐧⚡ | |
| description: Edge-journal commits with branch-aware graphs; the final architecture for storing semantics in Git. | |
| audience: [developers] | |
| domain: [architecture] | |
| tags: [journal, cbor, refs] | |
| status: stable | |
| last_updated: 2025-09-15 | |
| --- | |
| <!-- SPDX-License-Identifier: Apache-2.0 --> |
🤖 Prompt for AI Agents
In docs/architecture/journal-architecture.md around lines 1 to 9, the YAML front
matter is present but missing the required SPDX/license comment immediately
after it; add a single-line SPDX identifier (e.g., "SPDX-License-Identifier:
Apache-2.0" or the project's chosen SPDX tag) on the line immediately following
the closing --- of the front matter so the file begins with the YAML block and
the SPDX notice directly after it.
| Table of Contents | ||
|
|
||
| - [Executive Summary](#executive-summary) | ||
| - [Core Design Principles](#core-design-principles) | ||
| - [The Two-Layer Architecture](#the-two-layer-architecture) | ||
| - [Implementation Details](#implementation-details) | ||
| - [Solving the Hard Problems](#solving-the-hard-problems) | ||
| - [Migration Plan (Fresh Start)](#migration-plan-fresh-start) | ||
| - [Code Structure](#code-structure) | ||
| - [Why This Wins](#why-this-wins) | ||
| - [Potential Gotchas & Solutions](#potential-gotchas--solutions) | ||
| - [The Payoff](#the-payoff) | ||
| - [Next Steps](#next-steps) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the mandated H2 heading for the Table of Contents
Please change the “Table of Contents” label to a ## Table of Contents heading to satisfy the docs guideline.
-Table of Contents
+## Table of Contents📝 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.
| Table of Contents | |
| - [Executive Summary](#executive-summary) | |
| - [Core Design Principles](#core-design-principles) | |
| - [The Two-Layer Architecture](#the-two-layer-architecture) | |
| - [Implementation Details](#implementation-details) | |
| - [Solving the Hard Problems](#solving-the-hard-problems) | |
| - [Migration Plan (Fresh Start)](#migration-plan-fresh-start) | |
| - [Code Structure](#code-structure) | |
| - [Why This Wins](#why-this-wins) | |
| - [Potential Gotchas & Solutions](#potential-gotchas--solutions) | |
| - [The Payoff](#the-payoff) | |
| - [Next Steps](#next-steps) | |
| ## Table of Contents | |
| - [Executive Summary](#executive-summary) | |
| - [Core Design Principles](#core-design-principles) | |
| - [The Two-Layer Architecture](#the-two-layer-architecture) | |
| - [Implementation Details](#implementation-details) | |
| - [Solving the Hard Problems](#solving-the-hard-problems) | |
| - [Migration Plan (Fresh Start)](#migration-plan-fresh-start) | |
| - [Code Structure](#code-structure) | |
| - [Why This Wins](#why-this-wins) | |
| - [Potential Gotchas & Solutions](#potential-gotchas--solutions) | |
| - [The Payoff](#the-payoff) | |
| - [Next Steps](#next-steps) |
🤖 Prompt for AI Agents
In docs/architecture/journal-architecture.md around lines 15 to 28 the plain
text "Table of Contents" needs to be converted into a level-2 Markdown heading;
replace the current label with "## Table of Contents" (i.e., prepend "## " and
ensure it's on its own line) so the document follows the mandated H2 heading
guideline for the table of contents.
| --- | ||
| title: Ref Name Validation | ||
| description: Building refs safely with Git-style branch shorthands. | ||
| audience: [contributors, developers] | ||
| domain: [architecture] | ||
| tags: [git, refs] | ||
| status: stable | ||
| last_updated: 2025-09-30 | ||
| --- |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add the required SPDX comment after front matter
Docs must include the license/SPDX comment immediately after the YAML front matter. Please add it here to satisfy the doc guidelines.
---
title: Ref Name Validation
description: Building refs safely with Git-style branch shorthands.
audience: [contributors, developers]
domain: [architecture]
tags: [git, refs]
status: stable
last_updated: 2025-09-30
---
+<!-- SPDX-License-Identifier: Apache-2.0 -->📝 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: Ref Name Validation | |
| description: Building refs safely with Git-style branch shorthands. | |
| audience: [contributors, developers] | |
| domain: [architecture] | |
| tags: [git, refs] | |
| status: stable | |
| last_updated: 2025-09-30 | |
| --- | |
| --- | |
| title: Ref Name Validation | |
| description: Building refs safely with Git-style branch shorthands. | |
| audience: [contributors, developers] | |
| domain: [architecture] | |
| tags: [git, refs] | |
| status: stable | |
| last_updated: 2025-09-30 | |
| --- | |
| <!-- SPDX-License-Identifier: Apache-2.0 --> |
🤖 Prompt for AI Agents
In docs/architecture/ref-name-validation.md around lines 1 to 9, the YAML front
matter is present but the required SPDX license comment is missing immediately
after the front matter; add the SPDX identifier comment line (e.g., "//
SPDX-License-Identifier: <LICENSE-ID>" or the project’s canonical SPDX comment
format) directly below the closing "---" of the front matter so the file
contains the license/SPDX comment immediately after the YAML block per doc
guidelines.
| --- | ||
| title: System Architecture | ||
| description: High-level view of git-mind components and interactions. | ||
| audience: [developers] | ||
| domain: [architecture] | ||
| tags: [system, overview] | ||
| status: draft | ||
| last_updated: 2025-09-15 | ||
| --- |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Insert SPDX notice after the front matter
Per the docs guidelines, place the SPDX/license comment immediately after the YAML header.
---
title: System Architecture
description: High-level view of git-mind components and interactions.
audience: [developers]
domain: [architecture]
tags: [system, overview]
status: draft
last_updated: 2025-09-15
---
+<!-- SPDX-License-Identifier: Apache-2.0 -->📝 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: System Architecture | |
| description: High-level view of git-mind components and interactions. | |
| audience: [developers] | |
| domain: [architecture] | |
| tags: [system, overview] | |
| status: draft | |
| last_updated: 2025-09-15 | |
| --- | |
| --- | |
| title: System Architecture | |
| description: High-level view of git-mind components and interactions. | |
| audience: [developers] | |
| domain: [architecture] | |
| tags: [system, overview] | |
| status: draft | |
| last_updated: 2025-09-15 | |
| --- | |
| <!-- SPDX-License-Identifier: Apache-2.0 --> |
🤖 Prompt for AI Agents
In docs/architecture/system-overview.md around lines 1–9 the YAML front matter
is present but missing the required SPDX/license notice; add the SPDX license
comment immediately after the closing --- of the front matter by inserting the
project's SPDX identifier as a comment (e.g. an HTML or Markdown comment
containing "SPDX-License-Identifier: <identifier>") directly after the header
end so the notice is the first thing after the YAML block.
| Table of Contents | ||
|
|
||
| - [Executive Summary](#executive-summary) | ||
| - [High-Level Design](#high-level-design) | ||
| - [Components and Technology Stack](#components-and-technology-stack) | ||
| - [Data Flows and Storage](#data-flows-and-storage) | ||
| - [Scaling Strategy and Performance Targets](#scaling-strategy-and-performance-targets) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Promote “Table of Contents” to an H2 heading
Docs must include a ## Table of Contents section. Please update the heading accordingly.
-Table of Contents
+## Table of Contents📝 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.
| Table of Contents | |
| - [Executive Summary](#executive-summary) | |
| - [High-Level Design](#high-level-design) | |
| - [Components and Technology Stack](#components-and-technology-stack) | |
| - [Data Flows and Storage](#data-flows-and-storage) | |
| - [Scaling Strategy and Performance Targets](#scaling-strategy-and-performance-targets) | |
| ## Table of Contents | |
| - [Executive Summary](#executive-summary) | |
| - [High-Level Design](#high-level-design) | |
| - [Components and Technology Stack](#components-and-technology-stack) | |
| - [Data Flows and Storage](#data-flows-and-storage) | |
| - [Scaling Strategy and Performance Targets](#scaling-strategy-and-performance-targets) |
🤖 Prompt for AI Agents
In docs/architecture/system-overview.md around lines 13 to 20, the "Table of
Contents" text is plain text but should be an H2 heading; change the line to use
Markdown H2 syntax ("## Table of Contents") so the docs include the required
section header and render correctly in the table of contents.
| --- | ||
| title: 0d1b34b0ee501bc7b7b2d0a9fb82e06a37642c77.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.
Doc header must match guidelines.
The front matter title must be "Code Review Feedback" (matching the H1), the SPDX/license comment must appear immediately after the front matter, and a "## Table of Contents" section must follow the H1.
Apply this diff:
---
-title: 0d1b34b0ee501bc7b7b2d0a9fb82e06a37642c77.md
+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 -->
+<!-- © 2025 J. Kirby Ross / Neuroglyph Collective -->
# Code Review Feedback
+
+## Table of ContentsAs per coding guidelines.
📝 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: 0d1b34b0ee501bc7b7b2d0a9fb82e06a37642c77.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 --> | |
| <!-- © 2025 J. Kirby Ross / Neuroglyph Collective --> | |
| # Code Review Feedback | |
| ## Table of Contents |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
AGENTS.md(1 hunks)docs/code-reviews/PR177/9f49dd2ad65837735ac218c3d159b36f8b840819.md(1 hunks)docs/operations/Telemetry_Config.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/operations/Telemetry_Config.md
🧰 Additional context used
📓 Path-based instructions (1)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Docs must have front matter first, a single H1, a '## Table of Contents', and the title must match the H1
Add api_version field to API docs front matter
Place License/SPDX comments immediately after front matter in docs
README and other Markdown must comply with markdownlint rules: underscore emphasis/strong, blockquote spacing, code fence formatting, no inline HTML wrappers, heading punctuation
Files:
docs/code-reviews/PR177/9f49dd2ad65837735ac218c3d159b36f8b840819.md
| --- | ||
| title: 9f49dd2ad65837735ac218c3d159b36f8b840819.md | ||
| description: Preserved review artifacts and rationale. | ||
| audience: [contributors] | ||
| domain: [quality] | ||
| tags: [review] | ||
| status: archive | ||
| --- | ||
|
|
||
| # Code Review Feedback | ||
|
|
||
| | Date | Agent | SHA | Branch | PR | | ||
| |------|-------|-----|--------|----| | ||
| | 2025-10-08 | CodeRabbit (and reviewers) | `9f49dd2ad65837735ac218c3d159b36f8b840819` | [feat/hex-ports-ci-green](https://github.com/neuroglyph/git-mind/tree/feat/hex-ports-ci-green "neuroglyph/git-mind:feat/hex-ports-ci-green") | [PR#177](https://github.com/neuroglyph/git-mind/pull/177) | | ||
|
|
||
| ## CODE REVIEW FEEDBACK | ||
|
|
||
| ### core/src/adapters/logging/stdio_logger_adapter.c:41 — coderabbitai[bot] |
There was a problem hiding this comment.
Align front matter with doc standards.
Please update the front matter title to match the H1, insert the required SPDX/license comments immediately after the front matter, and add the mandatory ## Table of Contents section. These steps are required for every docs/**/*.md file. As per coding guidelines
---
-title: 9f49dd2ad65837735ac218c3d159b36f8b840819.md
+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 -->
+<!-- © 2025 J. Kirby Ross / Neuroglyph Collective -->
# Code Review Feedback
+
+## Table of Contents
+
+- [Code Review Feedback](#code-review-feedback)
+- [CODE REVIEW FEEDBACK](#code-review-feedback-1)📝 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: 9f49dd2ad65837735ac218c3d159b36f8b840819.md | |
| description: Preserved review artifacts and rationale. | |
| audience: [contributors] | |
| domain: [quality] | |
| tags: [review] | |
| status: archive | |
| --- | |
| # Code Review Feedback | |
| | Date | Agent | SHA | Branch | PR | | |
| |------|-------|-----|--------|----| | |
| | 2025-10-08 | CodeRabbit (and reviewers) | `9f49dd2ad65837735ac218c3d159b36f8b840819` | [feat/hex-ports-ci-green](https://github.com/neuroglyph/git-mind/tree/feat/hex-ports-ci-green "neuroglyph/git-mind:feat/hex-ports-ci-green") | [PR#177](https://github.com/neuroglyph/git-mind/pull/177) | | |
| ## CODE REVIEW FEEDBACK | |
| ### core/src/adapters/logging/stdio_logger_adapter.c:41 — coderabbitai[bot] | |
| --- | |
| 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 --> | |
| <!-- © 2025 J. Kirby Ross / Neuroglyph Collective --> | |
| # Code Review Feedback | |
| ## Table of Contents | |
| - [Code Review Feedback](#code-review-feedback) | |
| - [CODE REVIEW FEEDBACK](#code-review-feedback-1) | |
| | Date | Agent | SHA | Branch | PR | | |
| |------|-------|-----|--------|----| | |
| | 2025-10-08 | CodeRabbit (and reviewers) | `9f49dd2ad65837735ac218c3d159b36f8b840819` | [feat/hex-ports-ci-green](https://github.com/neuroglyph/git-mind/tree/feat/hex-ports-ci-green "neuroglyph/git-mind:feat/hex-ports-ci-green") | [PR#177](https://github.com/neuroglyph/git-mind/pull/177) | | |
| ## CODE REVIEW FEEDBACK | |
| ### core/src/adapters/logging/stdio_logger_adapter.c:41 — coderabbitai[bot] |
🤖 Prompt for AI Agents
In docs/code-reviews/PR177/9f49dd2ad65837735ac218c3d159b36f8b840819.md lines
1-18, the front matter title doesn't match the H1 and the required SPDX/license
comment and Table of Contents section are missing; update the front matter title
to exactly match the H1, add the required SPDX/license comment block immediately
after the front matter, and insert a mandatory "## Table of Contents" heading
(with any needed anchors/placeholder entries) below the SPDX/license block so
the file conforms to docs/**/*.md standards.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/README.md (2)
10-12: Add the required SPDX comment after the front matter.Docs must place the SPDX/license comment immediately after the YAML front matter, but this file doesn’t. Add the project’s SPDX identifier comment right after the front matter block.
13-20: Promote “Table of Contents” to an H2 heading.Documentation guidelines require a
## Table of Contentsheading; this section is plain text. Update it to use the proper H2 heading.
♻️ Duplicate comments (1)
migration-progress.md (1)
1-5: Missing front matter (duplicate concern).This issue has already been flagged in a previous review. The file is missing the required YAML front matter block at the beginning.
🧹 Nitpick comments (5)
AGENTS.md (1)
551-556: Approve with note on unconventional format.The JSON activity logs are unusual for markdown documentation but appear intentional for structured, machine-parseable tracking. Consider documenting this format choice if it's part of a broader automation strategy.
core/tests/unit/test_cache_oid_prefix.c (1)
12-32: Consider documenting test cases.The test validates OID prefix extraction at various bit lengths (4, 8, 12, 20), but the expected values and rationale aren't documented. Adding brief comments explaining the hex-to-prefix mapping would improve test maintainability.
Example:
+ /* 4 bits = 1 hex char → "0" */ assert(gm_cache_oid_prefix(&oid, 4, out, sizeof(out)) == GM_OK); assert(strcmp(out, "0") == 0);core/tests/fakes/metrics/fake_metrics_port.h (1)
16-23: Consider defining capacity constants.The fake state struct uses magic numbers for array sizes (32 entries, 64-char names, 256-char tags). Defining named constants would improve readability and maintainability.
Example:
+#define GM_FAKE_METRICS_MAX_ENTRIES 32 +#define GM_FAKE_METRICS_NAME_SIZE 64 +#define GM_FAKE_METRICS_TAGS_SIZE 256 + typedef struct { - struct { char name[64]; uint64_t value; char tags[256]; } counters[32]; + struct { char name[GM_FAKE_METRICS_NAME_SIZE]; uint64_t value; char tags[GM_FAKE_METRICS_TAGS_SIZE]; } counters[GM_FAKE_METRICS_MAX_ENTRIES];core/tests/unit/test_telemetry_cfg.c (1)
14-24: Note on memory management in test helper.The
set_envhelper allocates heap strings (line 20) that are never freed, causing a memory leak. This is acceptable for short-lived test processes that exit immediately after running tests, asputenvrequires the string to persist for the process lifetime.Consider adding a comment documenting this intentional leak:
+ /* Note: allocated string intentionally not freed; putenv requires it to persist */ char *heap = (char *)malloc((size_t)n + 1);core/tests/fakes/logging/fake_logger_port.h (1)
16-21: Consider defining capacity constants.Similar to the metrics fake port, the logger state struct uses magic numbers for array sizes (32 entries, 32-char components, 256-char messages). Defining named constants would improve consistency across test fakes and maintainability.
Example:
+#define GM_FAKE_LOGGER_MAX_ENTRIES 32 +#define GM_FAKE_LOGGER_COMPONENT_SIZE 32 +#define GM_FAKE_LOGGER_MESSAGE_SIZE 256 + typedef struct { - gm_log_level_t level[32]; - char component[32][32]; - char message[32][256]; + gm_log_level_t level[GM_FAKE_LOGGER_MAX_ENTRIES]; + char component[GM_FAKE_LOGGER_MAX_ENTRIES][GM_FAKE_LOGGER_COMPONENT_SIZE]; + char message[GM_FAKE_LOGGER_MAX_ENTRIES][GM_FAKE_LOGGER_MESSAGE_SIZE]; size_t count; } gm_fake_logger_state_t;
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
AGENTS.md(4 hunks)core/include/gitmind/cache/internal/oid_prefix.h(1 hunks)core/include/gitmind/telemetry/internal/config.h(1 hunks)core/src/app/cache/cache_rebuild_service.c(8 hunks)core/src/domain/cache/oid_prefix.c(1 hunks)core/src/telemetry/config.c(1 hunks)core/tests/fakes/logging/fake_logger_port.c(1 hunks)core/tests/fakes/logging/fake_logger_port.h(1 hunks)core/tests/fakes/metrics/fake_metrics_port.c(1 hunks)core/tests/fakes/metrics/fake_metrics_port.h(1 hunks)core/tests/unit/test_cache_oid_prefix.c(1 hunks)core/tests/unit/test_cache_telemetry_emit.c(1 hunks)core/tests/unit/test_telemetry_cfg.c(1 hunks)docs/README.md(1 hunks)docs/cli/README.md(0 hunks)docs/cli/combined_markdown.md(0 hunks)docs/cli/gitmind-cache-rebuild.md(0 hunks)docs/cli/gitmind-install-hooks.md(0 hunks)docs/cli/gitmind-link.md(0 hunks)docs/cli/gitmind-list.md(0 hunks)docs/cli/gitmind.md(0 hunks)docs/code-reviews/PR177/62e7ab80a904e1bc42f06c250efb102af17ea962.md(1 hunks)meson.build(4 hunks)migration-progress.md(1 hunks)
💤 Files with no reviewable changes (7)
- docs/cli/gitmind-link.md
- docs/cli/gitmind-cache-rebuild.md
- docs/cli/combined_markdown.md
- docs/cli/gitmind.md
- docs/cli/gitmind-install-hooks.md
- docs/cli/README.md
- docs/cli/gitmind-list.md
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{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/fakes/metrics/fake_metrics_port.hcore/tests/unit/test_cache_oid_prefix.ccore/tests/unit/test_telemetry_cfg.ccore/tests/unit/test_cache_telemetry_emit.ccore/include/gitmind/cache/internal/oid_prefix.hcore/tests/fakes/logging/fake_logger_port.hcore/src/telemetry/config.ccore/include/gitmind/telemetry/internal/config.hcore/src/app/cache/cache_rebuild_service.ccore/tests/fakes/logging/fake_logger_port.ccore/src/domain/cache/oid_prefix.ccore/tests/fakes/metrics/fake_metrics_port.c
{core,src,apps,tests}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
{core,src,apps,tests}/**/*.{c,h}: Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
Do not use buffers sized GM_PATH_MAX*2; prefer GM_PATH_MAX and allocate if larger is required
Equality is OID-first; if both OIDs exist they must match; only fall back to SHA when OIDs are absent
Build refs via gm_build_ref and reject inputs that start with "refs/"
Language: C23 with warnings-as-errors; no VLAs, no shadowing; declare explicit prototypes
Formatting via .clang-format (LLVM style, 4 spaces, 80 cols, pointer alignment right); pre-commit runs clang-format
Naming: functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE; types end with t; header guards start with GITMIND
Includes: prefer specific headers; order/regroup per .clang-format; include-what-you-use
No new clang-tidy warnings; keep touched files tidy-clean
Surface failures via gm_result_t (and variants); ports must not return raw integers
Maintain one responsibility per translation unit where practical; split multi-concern files when touched
Prefer OID types and helpers (gm_oid_t, git_oid_cmp via wrappers, gm_oid_to_hex); avoid direct libgit2 helpers in domain/application layers
Zero output buffers before formatting/copying on error paths
Files:
core/tests/fakes/metrics/fake_metrics_port.hcore/tests/unit/test_cache_oid_prefix.ccore/tests/unit/test_telemetry_cfg.ccore/tests/unit/test_cache_telemetry_emit.ccore/include/gitmind/cache/internal/oid_prefix.hcore/tests/fakes/logging/fake_logger_port.hcore/src/telemetry/config.ccore/include/gitmind/telemetry/internal/config.hcore/src/app/cache/cache_rebuild_service.ccore/tests/fakes/logging/fake_logger_port.ccore/src/domain/cache/oid_prefix.ccore/tests/fakes/metrics/fake_metrics_port.c
core/tests/fakes/**
📄 CodeRabbit inference engine (AGENTS.md)
Provide deterministic fakes/mocks/stubs for every outbound port under core/tests/fakes/** with harnesses verifying contract invariants
Files:
core/tests/fakes/metrics/fake_metrics_port.hcore/tests/fakes/logging/fake_logger_port.hcore/tests/fakes/logging/fake_logger_port.ccore/tests/fakes/metrics/fake_metrics_port.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_oid_prefix.ccore/tests/unit/test_telemetry_cfg.ccore/tests/unit/test_cache_telemetry_emit.c
core/tests/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests should use fakes only; integration tests may use real adapters but only inside Docker
Files:
core/tests/unit/test_cache_oid_prefix.ccore/tests/unit/test_telemetry_cfg.ccore/tests/unit/test_cache_telemetry_emit.ccore/tests/fakes/logging/fake_logger_port.ccore/tests/fakes/metrics/fake_metrics_port.c
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Docs must have front matter first, a single H1, a '## Table of Contents', and the title must match the H1
Add api_version field to API docs front matter
Place License/SPDX comments immediately after front matter in docs
README and other Markdown must comply with markdownlint rules: underscore emphasis/strong, blockquote spacing, code fence formatting, no inline HTML wrappers, heading punctuation
Files:
docs/README.mddocs/code-reviews/PR177/62e7ab80a904e1bc42f06c250efb102af17ea962.md
core/include/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Public headers under core/include must be umbrella-safe: compile standalone, include only what they use, stable header guards, and provide extern "C" when included from C++
Files:
core/include/gitmind/cache/internal/oid_prefix.hcore/include/gitmind/telemetry/internal/config.h
core/include/gitmind/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Public structs in headers must avoid ABI breaks; append new fields at the struct tail and mark deprecated fields instead of removing
Files:
core/include/gitmind/cache/internal/oid_prefix.hcore/include/gitmind/telemetry/internal/config.h
meson.build
📄 CodeRabbit inference engine (AGENTS.md)
Target C23 via Meson c2x and keep warnings-as-errors; register new unit test targets in meson.build
Files:
meson.build
core/src/**/{hooks,cache,journal}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid direct libgit2 usage; route repository/ref operations through gm_git_repository_port/gm_git_ref_port adapters
Files:
core/src/app/cache/cache_rebuild_service.ccore/src/domain/cache/oid_prefix.c
core/src/domain/**
📄 CodeRabbit inference engine (AGENTS.md)
Domain core code under core/src/domain/** must be pure/deterministic (no direct IO/libgit2/global state)
Files:
core/src/domain/cache/oid_prefix.c
core/src/domain/{edge,journal,cache,attribution,hooks}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Wrap all side effects behind outbound ports; no direct IO or libgit2 in domain modules
Files:
core/src/domain/cache/oid_prefix.c
🧠 Learnings (16)
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to docs/activity/**/*.md : Document migration progress and lessons under docs/activity/<date>_hexagonal.md
Applied to files:
migration-progress.md
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to docs/architecture/hexagonal/** : Track adapter status and migrations in docs/architecture/hexagonal/**; keep diagrams and status tables updated
Applied to files:
migration-progress.md
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Prefer OID types and helpers (gm_oid_t, git_oid_cmp via wrappers, gm_oid_to_hex); avoid direct libgit2 helpers in domain/application layers
Applied to files:
core/include/gitmind/cache/internal/oid_prefix.hcore/src/app/cache/cache_rebuild_service.c
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to core/include/gitmind/ports/**/*.h : Define all inbound/outbound ports as headers only under core/include/gitmind/ports/** with POD structs and function-pointer vtables (no non-trivial inline behavior)
Applied to files:
meson.build
📚 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:
meson.buildAGENTS.md
📚 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:
meson.buildAGENTS.md
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Naming: functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE; types end with _t; header guards start with GITMIND_
Applied to files:
AGENTS.md
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{h} : Header guards must be stable and named GITMIND_*; add extern "C" guards for C++ consumers in public headers
Applied to files:
AGENTS.md
📚 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,include}/**/*.{c,h} : Language: C23 with warnings-as-errors; no VLAs; no variable shadowing; use explicit prototypes
Applied to files:
AGENTS.md
📚 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:
AGENTS.md
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Language: C23 with warnings-as-errors; no VLAs, no shadowing; declare explicit prototypes
Applied to files:
AGENTS.md
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to include/**/*.h : Umbrella API headers (include/gitmind.h and include/gitmind/**) must be umbrella-safe and carry extern "C" guards for C++
Applied to files:
AGENTS.md
📚 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,include}/**/*.{c,h} : Formatting: follow .clang-format (LLVM-based, 4 spaces, 80 cols, pointer alignment right); pre-commit enforces clang-format
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: Applies to **/*.{c,cc,cpp,cxx,h,hpp,hxx,m,mm} : NEVER use `NOLINT` to suppress clang-tidy warnings; ALWAYS fix the underlying issue
Applied to files:
AGENTS.md
📚 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: Conventional commits (type(scope): description) and reference issues; PRs must describe changes, link issues, include test plan, and update docs when applicable
Applied to files:
AGENTS.md
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Maintain one responsibility per translation unit where practical; split multi-concern files when touched
Applied to files:
AGENTS.md
🧬 Code graph analysis (8)
core/tests/fakes/metrics/fake_metrics_port.h (1)
core/tests/fakes/metrics/fake_metrics_port.c (2)
gm_fake_metrics_port_init(60-73)gm_fake_metrics_port_dispose(75-80)
core/tests/unit/test_cache_telemetry_emit.c (6)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)core/include/gitmind/security/memory.h (1)
gm_memset_safe(47-59)core/tests/fakes/logging/fake_logger_port.c (2)
gm_fake_logger_port_init(31-46)gm_fake_logger_port_dispose(48-53)core/tests/fakes/metrics/fake_metrics_port.c (2)
gm_fake_metrics_port_init(60-73)gm_fake_metrics_port_dispose(75-80)core/src/app/cache/cache_rebuild_service.c (1)
gm_cache_rebuild_execute(476-623)core/tests/fakes/fs/fake_fs_temp_port.c (1)
gm_fake_fs_temp_port_dispose(252-259)
core/include/gitmind/cache/internal/oid_prefix.h (1)
core/src/domain/cache/oid_prefix.c (1)
int(13-44)
core/tests/fakes/logging/fake_logger_port.h (1)
core/tests/fakes/logging/fake_logger_port.c (2)
gm_fake_logger_port_init(31-46)gm_fake_logger_port_dispose(48-53)
core/src/telemetry/config.c (1)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
core/src/app/cache/cache_rebuild_service.c (3)
core/include/gitmind/ports/logger_port.h (1)
gm_logger_log(66-74)core/include/gitmind/ports/metrics_port.h (3)
gm_metrics_timing_ms(72-79)gm_metrics_counter_add(52-59)gm_metrics_gauge_set(62-69)core/src/error/error.c (1)
gm_error_free(161-178)
core/tests/fakes/logging/fake_logger_port.c (1)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
core/tests/fakes/metrics/fake_metrics_port.c (1)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
🔇 Additional comments (8)
AGENTS.md (3)
129-129: LGTM!The explicit ban on
NOLINTusage is consistent with the coding guidelines and ensures that all warnings are addressed rather than suppressed.
142-149: LGTM!The added git workflow guardrails properly emphasize intentional staging and forbid circumventing hooks or tests, which helps maintain code quality and repository integrity.
397-400: LGTM!The integration of legacy security and memory safety concerns into the hexagonal architecture framework ensures that these critical aspects are addressed during module migrations.
core/tests/fakes/logging/fake_logger_port.c (2)
13-25: LGTM with note on truncation handling.The implementation correctly uses
gm_strcpy_safefor string operations and intentionally ignores truncation (via(void)casts). This is acceptable for test fakes where silent truncation is preferable to test failures.
31-53: LGTM!The init and dispose functions properly handle NULL checks, allocation failures, and cleanup. Error codes are appropriate and memory management is correct.
core/tests/unit/test_telemetry_cfg.c (1)
26-98: LGTM!The test functions comprehensively validate telemetry configuration loading, extras handling, and repository tagging. Assertions are appropriate and test coverage is thorough.
core/include/gitmind/cache/internal/oid_prefix.h (1)
1-31: LGTM!The header is well-structured with proper guards, clear documentation, and correct use of
GM_NODISCARD. The API is concise and follows project conventions.meson.build (1)
124-125: LGTM! Build system changes are complete and consistent.All new source files (domain helpers, telemetry config, cache ports, adapters), test executables with proper fakes, test registrations, and public header exposures are correctly added and align with the PR objectives.
Also applies to: 131-132, 140-141, 374-393, 429-431, 448-451
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
core/tests/unit/test_repo_build_tree.c (1)
66-72: Clean up created test directories.The test creates
.gm_build_tree_repoand.gm_build_tree_srcdirectories but never removes them. Add cleanup to prevent accumulating test artifacts across runs.Consider adding cleanup after line 71:
git_repository_free(repo); git_libgit2_shutdown(); + // Clean up test directories + system("rm -rf .gm_build_tree_repo .gm_build_tree_src"); printf("OK\n"); return 0;Note: Alternatively, use a more robust cleanup approach with proper error handling, or rely on the test harness to clean up ephemeral directories.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/tests/unit/test_repo_build_tree.c(1 hunks)docs/code-reviews/PR177/191912ae67648beab20b6c597cc130e73db5a1e5.md(1 hunks)meson.build(5 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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_repo_build_tree.c
{core,src,apps,tests}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
{core,src,apps,tests}/**/*.{c,h}: Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
Do not use buffers sized GM_PATH_MAX*2; prefer GM_PATH_MAX and allocate if larger is required
Equality is OID-first; if both OIDs exist they must match; only fall back to SHA when OIDs are absent
Build refs via gm_build_ref and reject inputs that start with "refs/"
Language: C23 with warnings-as-errors; no VLAs, no shadowing; declare explicit prototypes
Formatting via .clang-format (LLVM style, 4 spaces, 80 cols, pointer alignment right); pre-commit runs clang-format
Naming: functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE; types end with t; header guards start with GITMIND
Includes: prefer specific headers; order/regroup per .clang-format; include-what-you-use
No new clang-tidy warnings; keep touched files tidy-clean
Surface failures via gm_result_t (and variants); ports must not return raw integers
Maintain one responsibility per translation unit where practical; split multi-concern files when touched
Prefer OID types and helpers (gm_oid_t, git_oid_cmp via wrappers, gm_oid_to_hex); avoid direct libgit2 helpers in domain/application layers
Zero output buffers before formatting/copying on error paths
Files:
core/tests/unit/test_repo_build_tree.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_repo_build_tree.c
core/tests/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests should use fakes only; integration tests may use real adapters but only inside Docker
Files:
core/tests/unit/test_repo_build_tree.c
meson.build
📄 CodeRabbit inference engine (AGENTS.md)
Target C23 via Meson c2x and keep warnings-as-errors; register new unit test targets in meson.build
Files:
meson.build
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Docs must have front matter first, a single H1, a '## Table of Contents', and the title must match the H1
Add api_version field to API docs front matter
Place License/SPDX comments immediately after front matter in docs
README and other Markdown must comply with markdownlint rules: underscore emphasis/strong, blockquote spacing, code fence formatting, no inline HTML wrappers, heading punctuation
Files:
docs/code-reviews/PR177/191912ae67648beab20b6c597cc130e73db5a1e5.md
🧠 Learnings (3)
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to core/include/gitmind/ports/**/*.h : Define all inbound/outbound ports as headers only under core/include/gitmind/ports/** with POD structs and function-pointer vtables (no non-trivial inline behavior)
Applied to files:
meson.build
📚 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:
meson.build
📚 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:
meson.build
🔇 Additional comments (2)
meson.build (2)
357-361: Verify test categorization aligns with test type.
test_repo_build_treeuses real libgit2 adapters (see the test source review), making it an integration test rather than a unit test. If the test is moved to the integration test directory as suggested in the file review, update the executable path here and consider whether it needs different test registration flags or environment setup (e.g., Docker-only execution).The build configuration looks correct for the current location. Coordinate changes with test relocation.
Also applies to: 431-431
455-458: LGTM: New port headers properly exposed.The new public port headers (
logger_port.h,metrics_port.h,cache_build_port.h,cache_query_port.h) are correctly placed undercore/include/gitmind/ports/and registered for compilation checks. This aligns with the learnings about defining ports as headers undercore/include/gitmind/ports/**.Based on learnings.
…SH_ALGO=sha256|fnv); update docs with custom logger adapter guidance; tests exercise sha256 path; CI green
…at/hex-ports-ci-green
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/src/telemetry/config.c (1)
255-255: Checkgm_strcpy_safereturn value and handle truncation.The
(void)cast ignores truncation when copyingrepo_canon_path, which could be longer than the 65-byterepo_valbuffer. Per coding guidelines, truncation must be treated as an error.Apply this fix:
if (cfg->repo_tag == GM_REPO_TAG_PLAIN) { if (repo_canon_path != NULL && repo_canon_path[0] != '\0') { - (void)gm_strcpy_safe(repo_val, sizeof(repo_val), repo_canon_path); + if (gm_strcpy_safe(repo_val, sizeof(repo_val), repo_canon_path) != 0) { + return gm_err_void(GM_ERROR(GM_ERR_BUFFER_TOO_SMALL, "repo path too long")); + } }As per coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/include/gitmind/telemetry/internal/config.h(1 hunks)core/src/telemetry/config.c(1 hunks)core/tests/unit/test_telemetry_cfg.c(1 hunks)docs/operations/Telemetry_Config.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/include/gitmind/telemetry/internal/config.h
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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_telemetry_cfg.ccore/src/telemetry/config.c
{core,src,apps,tests}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
{core,src,apps,tests}/**/*.{c,h}: Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
Do not use buffers sized GM_PATH_MAX*2; prefer GM_PATH_MAX and allocate if larger is required
Equality is OID-first; if both OIDs exist they must match; only fall back to SHA when OIDs are absent
Build refs via gm_build_ref and reject inputs that start with "refs/"
Language: C23 with warnings-as-errors; no VLAs, no shadowing; declare explicit prototypes
Formatting via .clang-format (LLVM style, 4 spaces, 80 cols, pointer alignment right); pre-commit runs clang-format
Naming: functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE; types end with t; header guards start with GITMIND
Includes: prefer specific headers; order/regroup per .clang-format; include-what-you-use
No new clang-tidy warnings; keep touched files tidy-clean
Surface failures via gm_result_t (and variants); ports must not return raw integers
Maintain one responsibility per translation unit where practical; split multi-concern files when touched
Prefer OID types and helpers (gm_oid_t, git_oid_cmp via wrappers, gm_oid_to_hex); avoid direct libgit2 helpers in domain/application layers
Zero output buffers before formatting/copying on error paths
Files:
core/tests/unit/test_telemetry_cfg.ccore/src/telemetry/config.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_telemetry_cfg.c
core/tests/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests should use fakes only; integration tests may use real adapters but only inside Docker
Files:
core/tests/unit/test_telemetry_cfg.c
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Docs must have front matter first, a single H1, a '## Table of Contents', and the title must match the H1
Add api_version field to API docs front matter
Place License/SPDX comments immediately after front matter in docs
README and other Markdown must comply with markdownlint rules: underscore emphasis/strong, blockquote spacing, code fence formatting, no inline HTML wrappers, heading punctuation
Files:
docs/operations/Telemetry_Config.md
🧠 Learnings (2)
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
Applied to files:
core/src/telemetry/config.c
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Do not use buffers sized GM_PATH_MAX*2; prefer GM_PATH_MAX and allocate if larger is required
Applied to files:
core/src/telemetry/config.c
🧬 Code graph analysis (2)
core/tests/unit/test_telemetry_cfg.c (1)
core/tests/unit/test_cache_telemetry_emit.c (1)
set_env(110-119)
core/src/telemetry/config.c (4)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)core/src/crypto/backend.c (2)
gm_crypto_context_create(81-99)gm_crypto_backend_libsodium(75-77)core/src/crypto/sha256.c (1)
gm_sha256_with_context(19-37)core/src/error/error.c (1)
gm_error_free(161-178)
🔇 Additional comments (11)
core/src/telemetry/config.c (5)
23-61: LGTM! Parsing helpers are defensive and sensible.The parsing helpers handle null/empty inputs gracefully with appropriate defaults, use case-insensitive string comparison, and provide sensible fallback values for unrecognized inputs.
63-93: LGTM! Character and key/value validation are appropriately defensive.The validation helpers use conservative character sets for keys and values, properly check lengths against struct field sizes, and reject null/empty inputs where appropriate.
136-179: LGTM! Config loading is properly implemented.The function validates inputs, initializes sensible defaults, reads environment variables through the port abstraction, and returns appropriate results.
181-225: LGTM! Tag building helpers are correctly implemented.The helpers properly:
- Check buffer overflow in
append_kvusinggm_snprintfreturn value- Implement standard FNV-1a hashing in
fnv1a64_hex12- Handle crypto backend errors gracefully in
sha256_hex12, freeing error structures before falling back to FNV
227-301: Tag building logic is sound beyond the truncation issue.The function properly validates inputs, respects configuration flags, handles multiple tag sources (branch, mode, repo, extras), enforces the total tag limit, and returns appropriate overflow errors.
docs/operations/Telemetry_Config.md (1)
16-141: LGTM! Telemetry configuration documentation is comprehensive and well-structured.The document provides clear guidance on:
- Safe defaults and customization boundaries
- Environment variable knobs with descriptions
- Guardrails for preventing high-cardinality issues
- Implementation plan and examples
- Extension points for custom adapters
The content aligns well with the code changes in
core/src/telemetry/config.candcore/include/gitmind/telemetry/internal/config.h.core/tests/unit/test_telemetry_cfg.c (5)
14-24: LGTM! Portable environment helper is correctly implemented.The
set_envfunction allocates heap strings forputenvas required by POSIX. The memory leak is acceptable in test code, as the strings must persist for the process lifetime.
26-49: LGTM! Default configuration test validates expected behavior.The test properly:
- Clears all environment variables to ensure defaults
- Loads configuration and asserts default values
- Builds tags with branch and mode
- Verifies the expected tag string format
51-73: LGTM! Extras parsing test covers validation and capacity limits.The test verifies:
- Valid extras are parsed and added (team, role)
- Invalid entries are dropped (space in key)
- Capacity limits are enforced (max 3 extras)
- The
extras_droppedflag is set appropriately- All expected tags appear in the output
75-92: LGTM! Repo hash test validates tag generation from repo ID.The test confirms:
- Repo tag mode is configured from environment
- Hash algorithm selection works (SHA256)
- Tags are built from repo ID when path is absent
- The repo tag appears in the output along with branch and mode
94-99: LGTM! Test suite is properly structured.The main function executes all tests in sequence and returns success.
…n) + default renderer; refactor cache service to use it; add unit tests; docs/telemetry updated; CI green
…at/hex-ports-ci-green
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/src/app/cache/cache_rebuild_service.c (1)
494-520: Honor gm_telemetry_ return values to avoid leaked errors.*Both
gm_telemetry_cfg_loadandgm_telemetry_build_tagsreturngm_result_void_t, yet their results are cast to void. If either surfaces an error, thegm_error_tleaks and we proceed with possibly uninitialized config/tag buffers. Please capture the results, free any error payloads, and decide whether to continue (with defaults) or abort.As per coding guidelines
Apply this diff:
- gm_telemetry_cfg_t tcfg = {0}; - (void)gm_telemetry_cfg_load(&tcfg, gm_env_port_system()); + gm_telemetry_cfg_t tcfg = {0}; + gm_result_void_t cfg_rc = + gm_telemetry_cfg_load(&tcfg, gm_env_port_system()); + if (!cfg_rc.ok) { + if (cfg_rc.u.err != NULL) { + gm_error_free(cfg_rc.u.err); + } + /* Defaults in tcfg stay zeroed; continue without telemetry tweaks. */ + } @@ - (void)gm_telemetry_build_tags(&tcfg, branch, mode, repo_canon, &repo_id, - tags, sizeof(tags)); + gm_result_void_t tags_rc = gm_telemetry_build_tags( + &tcfg, branch, mode, repo_canon, &repo_id, tags, sizeof(tags)); + if (!tags_rc.ok) { + if (tags_rc.u.err != NULL) { + gm_error_free(tags_rc.u.err); + } + tags[0] = '\0'; + }
🧹 Nitpick comments (3)
core/src/app/cache/cache_rebuild_service.c (2)
523-535: Check log formatter return to handle buffer overflow.The log formatter can fail with
GM_ERR_BUFFER_TOO_SMALLif the message buffer is insufficient. While telemetry is best-effort, checking the return allows you to truncate gracefully or skip the log rather than emitting potentially malformed output.As per coding guidelines
Apply this diff:
gm_log_formatter_fn fmt = ctx->log_formatter ? ctx->log_formatter : gm_log_format_render_default; - (void)fmt(kvs, sizeof(kvs) / sizeof(kvs[0]), - (tcfg.log_format == GM_LOG_FMT_JSON), msg, sizeof(msg)); - (void)gm_logger_log(&ctx->logger_port, GM_LOG_INFO, "cache", msg); + gm_result_void_t fmt_rc = fmt(kvs, sizeof(kvs) / sizeof(kvs[0]), + (tcfg.log_format == GM_LOG_FMT_JSON), msg, sizeof(msg)); + if (fmt_rc.ok) { + (void)gm_logger_log(&ctx->logger_port, GM_LOG_INFO, "cache", msg); + } else if (fmt_rc.u.err != NULL) { + gm_error_free(fmt_rc.u.err); + }
602-622: Consider checking log formatter returns for consistency.Similar to the rebuild_start logging, these success and failure log paths ignore the formatter return value. While best-effort telemetry is acceptable, checking the return ensures consistent error handling across all telemetry call sites.
Apply similar error handling as suggested for lines 523-535 to both the rebuild_ok and rebuild_failed logging blocks.
Also applies to: 626-641
core/tests/unit/test_log_formatter.c (1)
10-46: Consider expanding test coverage for error paths and edge cases.The current tests validate happy paths for text and JSON rendering, which is a good start. However, consider adding tests for:
- Error conditions: buffer too small, null output buffer, null kvs with non-zero count
- Edge cases: empty kvs array (kv_count=0), null or empty keys/values, special characters requiring escaping (quotes, backslashes, control characters)
- Boundary conditions: exactly-sized buffers, off-by-one scenarios
These additional tests would improve robustness and catch potential regressions.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
core/include/gitmind/context.h(2 hunks)core/include/gitmind/telemetry/internal/log_format.h(1 hunks)core/src/app/cache/cache_rebuild_service.c(8 hunks)core/src/telemetry/log_format.c(1 hunks)core/tests/unit/test_log_formatter.c(1 hunks)docs/code-reviews/PR173/17ddad1aa3747f40ecacc0bd202e7b83edbf1d1d.md(0 hunks)docs/code-reviews/PR173/7445bcf00c71b6f768e9eb6b1fd99d2b1c60783f.md(0 hunks)docs/code-reviews/PR173/e0243058aac14ff3a8c83af2f6d4dcfe97dd2804.md(0 hunks)docs/code-reviews/PR174/bc1fc319303bf1a00b43b03a40992ba788996b1a.md(0 hunks)docs/code-reviews/PR176/d4a10748b38ad2be13f639288bd14eaee0cf89dc.md(0 hunks)docs/code-reviews/PR176/ecc3e4ac371ce646fd9a31573000ab4599d8f7ab.md(0 hunks)docs/operations/Telemetry_Config.md(1 hunks)meson.build(6 hunks)migration-progress.md(1 hunks)
💤 Files with no reviewable changes (6)
- docs/code-reviews/PR173/7445bcf00c71b6f768e9eb6b1fd99d2b1c60783f.md
- docs/code-reviews/PR173/17ddad1aa3747f40ecacc0bd202e7b83edbf1d1d.md
- docs/code-reviews/PR176/d4a10748b38ad2be13f639288bd14eaee0cf89dc.md
- docs/code-reviews/PR174/bc1fc319303bf1a00b43b03a40992ba788996b1a.md
- docs/code-reviews/PR176/ecc3e4ac371ce646fd9a31573000ab4599d8f7ab.md
- docs/code-reviews/PR173/e0243058aac14ff3a8c83af2f6d4dcfe97dd2804.md
✅ Files skipped from review due to trivial changes (1)
- docs/operations/Telemetry_Config.md
🚧 Files skipped from review as they are similar to previous changes (3)
- core/include/gitmind/context.h
- meson.build
- migration-progress.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/include/gitmind/telemetry/internal/log_format.hcore/tests/unit/test_log_formatter.ccore/src/app/cache/cache_rebuild_service.ccore/src/telemetry/log_format.c
{core,src,apps,tests}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
{core,src,apps,tests}/**/*.{c,h}: Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
Do not use buffers sized GM_PATH_MAX*2; prefer GM_PATH_MAX and allocate if larger is required
Equality is OID-first; if both OIDs exist they must match; only fall back to SHA when OIDs are absent
Build refs via gm_build_ref and reject inputs that start with "refs/"
Language: C23 with warnings-as-errors; no VLAs, no shadowing; declare explicit prototypes
Formatting via .clang-format (LLVM style, 4 spaces, 80 cols, pointer alignment right); pre-commit runs clang-format
Naming: functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE; types end with t; header guards start with GITMIND
Includes: prefer specific headers; order/regroup per .clang-format; include-what-you-use
No new clang-tidy warnings; keep touched files tidy-clean
Surface failures via gm_result_t (and variants); ports must not return raw integers
Maintain one responsibility per translation unit where practical; split multi-concern files when touched
Prefer OID types and helpers (gm_oid_t, git_oid_cmp via wrappers, gm_oid_to_hex); avoid direct libgit2 helpers in domain/application layers
Zero output buffers before formatting/copying on error paths
Files:
core/include/gitmind/telemetry/internal/log_format.hcore/tests/unit/test_log_formatter.ccore/src/app/cache/cache_rebuild_service.ccore/src/telemetry/log_format.c
core/include/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Public headers under core/include must be umbrella-safe: compile standalone, include only what they use, stable header guards, and provide extern "C" when included from C++
Files:
core/include/gitmind/telemetry/internal/log_format.h
core/include/gitmind/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Public structs in headers must avoid ABI breaks; append new fields at the struct tail and mark deprecated fields instead of removing
Files:
core/include/gitmind/telemetry/internal/log_format.h
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_log_formatter.c
core/tests/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests should use fakes only; integration tests may use real adapters but only inside Docker
Files:
core/tests/unit/test_log_formatter.c
core/src/**/{hooks,cache,journal}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid direct libgit2 usage; route repository/ref operations through gm_git_repository_port/gm_git_ref_port adapters
Files:
core/src/app/cache/cache_rebuild_service.c
🧠 Learnings (1)
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Prefer OID types and helpers (gm_oid_t, git_oid_cmp via wrappers, gm_oid_to_hex); avoid direct libgit2 helpers in domain/application layers
Applied to files:
core/src/app/cache/cache_rebuild_service.c
🧬 Code graph analysis (3)
core/include/gitmind/telemetry/internal/log_format.h (1)
core/src/telemetry/log_format.c (1)
gm_log_format_render_default(71-107)
core/tests/unit/test_log_formatter.c (1)
core/src/telemetry/log_format.c (1)
gm_log_format_render_default(71-107)
core/src/app/cache/cache_rebuild_service.c (4)
core/include/gitmind/ports/logger_port.h (1)
gm_logger_log(66-74)core/src/telemetry/log_format.c (1)
gm_log_format_render_default(71-107)core/include/gitmind/ports/metrics_port.h (3)
gm_metrics_timing_ms(72-79)gm_metrics_counter_add(52-59)gm_metrics_gauge_set(62-69)core/src/error/error.c (1)
gm_error_free(161-178)
🔇 Additional comments (4)
core/src/app/cache/cache_rebuild_service.c (2)
161-165: LGTM: Proper error handling for OID prefix extraction.The refactored call to
gm_cache_oid_prefixcorrectly checks the return value and propagates errors. This aligns with the extracted helper approach.
334-335: LGTM: Safe signature extension for commit OID capture.The optional
out_commit_oidparameter enables telemetry integration without breaking existing callers. The NULL check before assignment is correct.Also applies to: 372-379
core/src/telemetry/log_format.c (1)
13-69: LGTM: Helper functions implement thorough bounds checking.The helper functions (
append_text,append_json_escaped,append_json_kv) correctly usegm_snprintfand perform defensive bounds checking before each write operation. The JSON escaping properly handles quotes, backslashes, and control characters via \uXXXX encoding. The control flow is clear and the error handling is consistent.core/include/gitmind/telemetry/internal/log_format.h (1)
1-43: log_format.h is umbrella-safe. It includes<stdbool.h>,<stddef.h>, andresult.h(which definesgm_result_void_t), provides proper include guards andextern "C", and compiles standalone.
… test (build-only) and header compile; keep tests green by not running journal_port in suite; CI green
…at/hex-ports-ci-green
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/include/gitmind/ports/journal_command_port.h(1 hunks)core/src/ports/journal/journal_command_port.c(1 hunks)core/tests/unit/test_journal_port.c(1 hunks)meson.build(6 hunks)
🧰 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/ports/journal/journal_command_port.ccore/tests/unit/test_journal_port.ccore/include/gitmind/ports/journal_command_port.h
{core,src,apps,tests}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
{core,src,apps,tests}/**/*.{c,h}: Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
Do not use buffers sized GM_PATH_MAX*2; prefer GM_PATH_MAX and allocate if larger is required
Equality is OID-first; if both OIDs exist they must match; only fall back to SHA when OIDs are absent
Build refs via gm_build_ref and reject inputs that start with "refs/"
Language: C23 with warnings-as-errors; no VLAs, no shadowing; declare explicit prototypes
Formatting via .clang-format (LLVM style, 4 spaces, 80 cols, pointer alignment right); pre-commit runs clang-format
Naming: functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE; types end with t; header guards start with GITMIND
Includes: prefer specific headers; order/regroup per .clang-format; include-what-you-use
No new clang-tidy warnings; keep touched files tidy-clean
Surface failures via gm_result_t (and variants); ports must not return raw integers
Maintain one responsibility per translation unit where practical; split multi-concern files when touched
Prefer OID types and helpers (gm_oid_t, git_oid_cmp via wrappers, gm_oid_to_hex); avoid direct libgit2 helpers in domain/application layers
Zero output buffers before formatting/copying on error paths
Files:
core/src/ports/journal/journal_command_port.ccore/tests/unit/test_journal_port.ccore/include/gitmind/ports/journal_command_port.h
core/src/ports/**
📄 CodeRabbit inference engine (AGENTS.md)
Default implementations for simple inbound coordinators that remain in C may live under core/src/ports/**
Files:
core/src/ports/journal/journal_command_port.c
core/src/**/{hooks,cache,journal}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid direct libgit2 usage; route repository/ref operations through gm_git_repository_port/gm_git_ref_port adapters
Files:
core/src/ports/journal/journal_command_port.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_journal_port.c
core/tests/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests should use fakes only; integration tests may use real adapters but only inside Docker
Files:
core/tests/unit/test_journal_port.c
core/include/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Public headers under core/include must be umbrella-safe: compile standalone, include only what they use, stable header guards, and provide extern "C" when included from C++
Files:
core/include/gitmind/ports/journal_command_port.h
core/include/gitmind/ports/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Define all inbound/outbound ports as headers only under core/include/gitmind/ports/** with POD structs and function-pointer vtables (no non-trivial inline behavior)
Files:
core/include/gitmind/ports/journal_command_port.h
core/include/gitmind/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Public structs in headers must avoid ABI breaks; append new fields at the struct tail and mark deprecated fields instead of removing
Files:
core/include/gitmind/ports/journal_command_port.h
meson.build
📄 CodeRabbit inference engine (AGENTS.md)
Target C23 via Meson c2x and keep warnings-as-errors; register new unit test targets in meson.build
Files:
meson.build
🧠 Learnings (3)
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to core/include/gitmind/ports/**/*.h : Define all inbound/outbound ports as headers only under core/include/gitmind/ports/** with POD structs and function-pointer vtables (no non-trivial inline behavior)
Applied to files:
meson.build
📚 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:
meson.build
📚 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:
meson.build
🧬 Code graph analysis (3)
core/src/ports/journal/journal_command_port.c (1)
core/src/journal/writer.c (2)
gm_journal_append(352-356)gm_journal_append_attributed(359-365)
core/tests/unit/test_journal_port.c (2)
core/tests/fakes/git/fake_git_repository_port.c (3)
gm_fake_git_repository_port_init(195-229)gm_fake_git_repository_port_last_update_ref(482-485)gm_fake_git_repository_port_dispose(231-237)core/src/ports/journal/journal_command_port.c (2)
gm_cmd_journal_port_init(50-65)gm_cmd_journal_port_dispose(67-72)
core/include/gitmind/ports/journal_command_port.h (1)
core/src/ports/journal/journal_command_port.c (2)
gm_cmd_journal_port_init(50-65)gm_cmd_journal_port_dispose(67-72)
🔇 Additional comments (9)
core/tests/unit/test_journal_port.c (1)
1-68: LGTM! Test structure and logic are sound.The test correctly exercises the journal command port's append operation by:
- Wiring a fake Git repository port to capture ref updates
- Creating a minimal valid edge with OIDs
- Invoking the port's append method
- Verifying the expected ref update at
refs/gitmind/edges/main- Properly cleaning up the test environment variable
Note: The heap allocation at line 24 is intentional—
putenvtakes ownership of the string, so it should not be freed.core/src/ports/journal/journal_command_port.c (3)
16-28: LGTM! Proper validation and delegation.The
append_implfunction correctly validates inputs, delegates to the existinggm_journal_appendAPI, and translates the return code into agm_result_void_twith appropriate error context.
30-43: LGTM! Consistent with append implementation.The
append_attr_implfunction mirrors the structure and validation ofappend_impl, correctly delegating togm_journal_append_attributed.
50-72: LGTM! Lifecycle functions are properly implemented.The initialization and disposal functions follow best practices:
- Thorough input validation
- Proper error handling for allocation failures
- Clean state initialization via
calloc- Defensive NULL checks in dispose
- Proper pointer nullification during cleanup
core/include/gitmind/ports/journal_command_port.h (2)
1-38: LGTM! Header follows port design pattern correctly.The header is well-structured and umbrella-safe:
- Proper SPDX license and header guard with
GITMIND_prefix- Complete dependency includes for standalone compilation
- POD struct with non-owning vtable pointer and opaque state
- Vtable with function pointers only (no inline behavior)
extern "C"guard for C++ compatibilityAs per coding guidelines for
core/include/gitmind/ports/**/*.h.
40-43: LGTM! Lifecycle functions properly declared.The
initfunction correctly usesGM_NODISCARDto ensure callers check the result, and both function signatures align with their implementations incore/src/ports/journal/journal_command_port.c.meson.build (3)
124-126: LGTM! New source files properly integrated.The new source files for telemetry, OID prefix support, port implementations, and adapters are correctly added to the build. This aligns with the PR's hexagonal architecture migration goals.
Also applies to: 132-134, 142-143
341-345: LGTM! Test executables properly defined.All new test executables are correctly configured with:
- Appropriate source files and fake ports
- Required dependencies (libsodium, libgit2, threads)
- Correct include directories
Also applies to: 364-368, 387-394, 396-400, 402-406, 408-413
471-475: LGTM! New port headers added to compilation checks.The new port headers are correctly added to the
public_headerslist, ensuring they compile standalone and are umbrella-safe. This follows the established pattern for public API headers.
…ter; remove env branch override; add journal port test using fake head; keep CI close to green
…at/hex-ports-ci-green
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/tests/unit/test_cache_telemetry_emit.c (2)
110-118: Replace snprintf/memcpy with gm_snprintf/gm_strcpy_safe and handle truncation.The helper bypasses the mandated safe wrappers, so truncation or format errors would go unnoticed. Please switch to
gm_snprintf, copy viagm_strcpy_safe, and assert on their return values before callingputenv.static void set_env(const char *k, const char *v) { char buf[256]; if (v == NULL) v = ""; - int n = snprintf(buf, sizeof(buf), "%s=%s", k, v); - assert(n > 0 && (size_t)n < sizeof(buf)); + int n = gm_snprintf(buf, sizeof(buf), "%s=%s", k, v); + assert(n >= 0 && (size_t)n < sizeof(buf)); char *heap = (char *)malloc((size_t)n + 1); assert(heap != NULL); - memcpy(heap, buf, (size_t)n + 1); + if (gm_strcpy_safe(heap, (size_t)n + 1, buf) != 0) { + heap[0] = '\0'; + free(heap); + assert(!"set_env: gm_strcpy_safe truncated"); + } assert(putenv(heap) == 0); }
133-138: Check gm_strcpy_safe result before usingsr.gitdir.We drop the return code, so a truncation would silently leave
sr.gitdirinvalid. Capture the result and assert success (or handle the failure) before proceeding.- gm_strcpy_safe(sr.gitdir, sizeof(sr.gitdir), "/fake/state"); + int copy_rc = gm_strcpy_safe(sr.gitdir, sizeof(sr.gitdir), "/fake/state"); + assert(copy_rc == 0);
🧹 Nitpick comments (3)
core/tests/unit/test_journal_port.c (1)
43-51: LGTM!The verification and cleanup are properly implemented:
- Correctly retrieves and verifies the ref update from the fake port.
- Proper resource disposal prevents leaks.
- The substring check using
strstris acceptable for testing.For slightly more precise verification, you could use exact string comparison:
- assert(strstr(last_ref, "refs/gitmind/edges/main") != NULL); + assert(strcmp(last_ref, "refs/gitmind/edges/main") == 0);However, the current approach is more flexible if the exact format evolves.
core/src/journal/reader.c (2)
36-36: Remove duplicate include of constants_internal.hThis header is already included at Line 10. Drop the duplicate to keep includes tidy and avoid IWYU noise.
-#include "gitmind/constants_internal.h" /* MILLIS_PER_SECOND */
415-419: Use monotonic wall-clock for duration; guard clock() failuresclock() measures CPU time, not wall time, and may return (clock_t)-1. Prefer a monotonic timer; at minimum, guard failures to avoid bogus large durations.
- clock_t st = clock(); + clock_t st = clock(); int rc_walk = walk_journal_generic(&rctx, ref_name); - - uint64_t dur_ms = (uint64_t)((clock() - st) / CLOCKS_PER_MS); + uint64_t dur_ms = 0; + clock_t en = clock(); + if (st != (clock_t)-1 && en != (clock_t)-1 && en >= st) { + dur_ms = (uint64_t)((en - st) / CLOCKS_PER_MS); + }Optional (preferred): switch to a monotonic timer (e.g., clock_gettime(CLOCK_MONOTONIC) or timespec_get) to avoid CPU-time semantics; I can provide a small helper if desired.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/journal/reader.c(3 hunks)core/src/journal/writer.c(5 hunks)core/tests/unit/test_cache_telemetry_emit.c(1 hunks)core/tests/unit/test_journal_port.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/journal/writer.c
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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_journal_port.ccore/src/journal/reader.ccore/tests/unit/test_cache_telemetry_emit.c
{core,src,apps,tests}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
{core,src,apps,tests}/**/*.{c,h}: Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
Do not use buffers sized GM_PATH_MAX*2; prefer GM_PATH_MAX and allocate if larger is required
Equality is OID-first; if both OIDs exist they must match; only fall back to SHA when OIDs are absent
Build refs via gm_build_ref and reject inputs that start with "refs/"
Language: C23 with warnings-as-errors; no VLAs, no shadowing; declare explicit prototypes
Formatting via .clang-format (LLVM style, 4 spaces, 80 cols, pointer alignment right); pre-commit runs clang-format
Naming: functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE; types end with t; header guards start with GITMIND
Includes: prefer specific headers; order/regroup per .clang-format; include-what-you-use
No new clang-tidy warnings; keep touched files tidy-clean
Surface failures via gm_result_t (and variants); ports must not return raw integers
Maintain one responsibility per translation unit where practical; split multi-concern files when touched
Prefer OID types and helpers (gm_oid_t, git_oid_cmp via wrappers, gm_oid_to_hex); avoid direct libgit2 helpers in domain/application layers
Zero output buffers before formatting/copying on error paths
Files:
core/tests/unit/test_journal_port.ccore/src/journal/reader.ccore/tests/unit/test_cache_telemetry_emit.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_journal_port.ccore/tests/unit/test_cache_telemetry_emit.c
core/tests/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests should use fakes only; integration tests may use real adapters but only inside Docker
Files:
core/tests/unit/test_journal_port.ccore/tests/unit/test_cache_telemetry_emit.c
core/src/**/{hooks,cache,journal}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid direct libgit2 usage; route repository/ref operations through gm_git_repository_port/gm_git_ref_port adapters
Files:
core/src/journal/reader.c
🧠 Learnings (1)
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
Applied to files:
core/tests/unit/test_cache_telemetry_emit.c
🧬 Code graph analysis (3)
core/tests/unit/test_journal_port.c (2)
core/tests/fakes/git/fake_git_repository_port.c (4)
gm_fake_git_repository_port_init(195-229)gm_fake_git_repository_port_set_head_branch(247-261)gm_fake_git_repository_port_last_update_ref(482-485)gm_fake_git_repository_port_dispose(231-237)core/src/ports/journal/journal_command_port.c (2)
gm_cmd_journal_port_init(50-65)gm_cmd_journal_port_dispose(67-72)
core/src/journal/reader.c (3)
core/src/telemetry/log_format.c (1)
gm_log_format_render_default(71-107)core/include/gitmind/ports/logger_port.h (1)
gm_logger_log(66-74)core/include/gitmind/ports/metrics_port.h (1)
gm_metrics_timing_ms(72-79)
core/tests/unit/test_cache_telemetry_emit.c (7)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)core/include/gitmind/security/memory.h (1)
gm_memset_safe(47-59)core/tests/unit/test_telemetry_cfg.c (1)
set_env(14-24)core/tests/fakes/logging/fake_logger_port.c (2)
gm_fake_logger_port_init(31-46)gm_fake_logger_port_dispose(48-53)core/tests/fakes/metrics/fake_metrics_port.c (2)
gm_fake_metrics_port_init(60-73)gm_fake_metrics_port_dispose(75-80)core/src/app/cache/cache_rebuild_service.c (1)
gm_cache_rebuild_execute(476-643)core/tests/fakes/fs/fake_fs_temp_port.c (1)
gm_fake_fs_temp_port_dispose(252-259)
🔇 Additional comments (2)
core/tests/unit/test_journal_port.c (2)
1-13: LGTM!The includes are appropriate and properly organized. SPDX header and copyright are correctly placed.
17-41: LGTM!The test setup and execution are well-structured:
- Properly uses the fake repository port as required for unit tests.
- Context and port initialization follow the expected patterns.
- Minimal edge initialization is sufficient for testing the append path.
- Assertions provide clear failure points.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/journal/reader.c (1)
10-11: Remove duplicate includegitmind/constants_internal.h is included twice.
-#include "gitmind/constants_internal.h" @@ -#include "gitmind/constants_internal.h" /* MILLIS_PER_SECOND */ +#include "gitmind/constants_internal.h" /* MILLIS_PER_SECOND */Also applies to: 39-41
♻️ Duplicate comments (6)
core/src/app/cache/cache_rebuild_service.c (2)
513-538: Honor gm_telemetry_ return values to avoid leaked errors*The returns from
gm_telemetry_cfg_loadandgm_telemetry_build_tagsare still ignored. When either fails, the embeddedgm_error_tleaks and we continue with undefined telemetry state. Capture each result, free any error payload, and decide whether to proceed with defaults or abort.- gm_telemetry_cfg_t tcfg = {0}; - (void)gm_telemetry_cfg_load(&tcfg, gm_env_port_system()); + gm_telemetry_cfg_t tcfg = {0}; + gm_result_void_t cfg_rc = gm_telemetry_cfg_load(&tcfg, gm_env_port_system()); + if (!cfg_rc.ok) { + if (cfg_rc.u.err != NULL) { + gm_error_free(cfg_rc.u.err); + } + gm_memset_safe(&tcfg, sizeof(tcfg), 0, sizeof(tcfg)); + } @@ - (void)gm_telemetry_build_tags(&tcfg, branch, mode, repo_canon, &repo_id, - tags, sizeof(tags)); + gm_result_void_t tags_rc = + gm_telemetry_build_tags(&tcfg, branch, mode, repo_canon, &repo_id, + tags, sizeof(tags)); + if (!tags_rc.ok) { + if (tags_rc.u.err != NULL) { + gm_error_free(tags_rc.u.err); + } + tags[0] = '\0'; + }As per coding guidelines
633-672: Check gm_snprintf results per project guidelines
gm_snprintfcalls still discard their return values. The guidelines require detecting truncation/failure and treating it as an error (and freeing any cascading errors). Please capture each return, verify it is within bounds, and handle failure before logging or emitting diagnostics.- (void)gm_snprintf(edge_count_buf, sizeof(edge_count_buf), "%u", - (unsigned)meta.edge_count); + int edge_fmt = gm_snprintf(edge_count_buf, sizeof(edge_count_buf), "%u", + (unsigned)meta.edge_count); + if (edge_fmt < 0 || (size_t)edge_fmt >= sizeof(edge_count_buf)) { + return GM_ERR_BUFFER_TOO_SMALL; + }Replicate this pattern for
dur_bufand the failurecode_buf.As per coding guidelines
core/src/journal/reader.c (1)
298-353: Handle gm_result_void_t; free repo_canon; init/validate buffers (same as earlier review)Return values from gm_* calls are ignored and repo_canon is leaked; msg/dur_buf need init and gm_snprintf rc checks. Please apply the previously suggested fixes (capture/handle gm_result_void_t, free repo_canon after gm_telemetry_build_tags, zero/validate buffers) to this block.
See earlier review for concrete diff.
core/tests/unit/test_cache_telemetry_emit.c (2)
110-119: Use gm_snprintf/gm_strcpy_safe in set_env (already flagged)Replace snprintf/memcpy with gm_snprintf + gm_strcpy_safe and check rc; bail on truncation. Also include gitmind/security/string.h if not present.
As per coding guidelines
133-138: Check gm_strcpy_safe result when setting sr.gitdir (already flagged)Capture and assert rc to avoid silent truncation.
As per coding guidelines
meson.build (1)
440-445: Missing test registration fortest_journal_port.The
test_journal_portexecutable is defined but not registered in the test registration section (lines 485-501). This means the test won't run as part of the Meson test suite.Add the missing test registration:
test('cache_oid_prefix', test_cache_oid_prefix) +test('journal_port', test_journal_port)
🧹 Nitpick comments (17)
core/tests/unit/test_cache_branch_limits.c (1)
53-53: Consider migrating to gm_snprintf with return checking.The existing code uses
snprintfwithout checking the return value. The coding guidelines requiregm_snprintfwith return checking for all string operations.Apply this diff to align with coding guidelines (similar pattern used in test_cache_query.c:40-41):
- 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);core/tests/fakes/git/fake_git_repository_port.c (1)
787-803: Check return value from gm_strcpy_safe on Line 797-799.The call to
gm_strcpy_safeon Lines 797-799 explicitly discards the return value with(void), which means truncation errors are silently ignored. While this might be acceptable for test code, it could hide issues if commit messages unexpectedly exceed the buffer. Consider whether truncation should be detected and flagged, or document why silent truncation is acceptable here.For better test diagnostics, consider checking the return value:
commit->has_message = (fake->last_commit_message[0] != '\0'); if (commit->has_message) { - (void)gm_strcpy_safe(commit->message, - sizeof(commit->message), - fake->last_commit_message); + int rc = gm_strcpy_safe(commit->message, + sizeof(commit->message), + fake->last_commit_message); + if (rc != 0) { + /* Truncated; reset has_message to avoid misleading data */ + commit->has_message = false; + commit->message[0] = '\0'; + } }core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (2)
16-34: Consider flushing stderr after diagnostic emission.The
emit_implfunction writes diagnostic output to stderr without flushing. If stderr is line-buffered or block-buffered in some environments, diagnostic messages may not appear immediately. Consider addingfflush(stderr)after Line 32 to ensure timely visibility of diagnostics.Apply this diff to flush stderr:
} fputc('\n', stderr); + fflush(stderr); return gm_ok_void(); }
12-14: Unused field 'enabled' in state struct.The
enabledfield ingm_stderr_diag_state_tis marked as unused but allocated and never referenced. If this is a placeholder for future functionality, consider adding a comment explaining the intent or removing it until needed.If the field is truly unused, consider removing it:
typedef struct { - int enabled; /* future toggle; unused for now */ + /* Future: add toggle field when needed */ } gm_stderr_diag_state_t;core/src/domain/journal/append_planner.c (1)
20-23: Document lifetime requirements for out_plan pointers.Lines 20-23 assign pointers (
tree_oid,message,parents) directly intoout_planwithout copying. This means the caller must ensure these inputs remain valid for the lifetime ofout_plan. Consider adding a brief doc comment in the header (gitmind/journal/internal/append_plan.h) clarifying that the plan does not own the pointed-to data.Example header comment:
/** * Build a journal commit plan. * * @param empty_tree_oid Tree OID (must remain valid for plan's lifetime) * @param parent_oid_opt Optional parent OID (must remain valid if non-NULL) * @param message Commit message (must remain valid for plan's lifetime) * @param out_plan Output plan (does not own pointed-to data) * @return gm_result_void_t */core/tests/unit/test_journal_nff_retry.c (1)
84-92: Simplify or clarify test comment.Lines 84-92 contain a lengthy comment explaining the head resolution bypass. While the intent is clear, the comment is somewhat verbose and fragmented. Consider condensing it or moving the explanation to a function-level docstring for better readability.
Example condensed comment:
- /* The writer resolves head branch through repo port; our stub lacks it, - so we call the internal append via current-branch path by bypassing head. - To keep the test minimal, set branch name inline by initializing the - ref in the writer (we can't). Instead, rely on the fact that when head - lookup fails, writer returns error; so we simulate head by injecting a - pre-known branch via overriding resolve function is not possible. We - avoid head resolution by calling gm_journal_create_commit with an - explicit ref and spec assembly paths exercised by flush/updates. - */ + /* Writer uses head_branch from stub repo; on NFF, retry logic kicks in */core/tests/fakes/diagnostics/fake_diagnostics_port.h (1)
15-20: Document or enforce limits on diagnostic event count.The
gm_fake_diag_state_tstruct uses fixed-size arrays (meta[64],kvs[64][8], etc.). If a test emits more than 64 diagnostic events, silent overflow occurs. Consider documenting this limit clearly or adding runtime checks in the implementation to prevent silent data loss during tests.Add a comment documenting the limit:
typedef struct { + /* Capacity: up to 64 diagnostic events, 8 KVs per event */ struct { char component[32]; char event[64]; } meta[64]; struct { char key[32]; char value[64]; } kvs[64][8]; size_t kv_counts[64]; size_t count; } gm_fake_diag_state_t;core/tests/unit/test_journal_port_append_flow.c (1)
52-67: Consider helper function for edge construction.Lines 52-67 manually construct two edges with similar patterns. Consider extracting a helper function to reduce duplication and improve maintainability, especially if more tests need edge construction.
Example helper:
static void make_test_edge(gm_edge_t *edge, const uint8_t *src_raw, const uint8_t *tgt_raw, gm_rel_type_t rel, const char *src_path, const char *tgt_path) { assert(gm_oid_from_raw(&edge->src_oid, src_raw, GM_OID_RAWSZ) == GM_OK); assert(gm_oid_from_raw(&edge->tgt_oid, tgt_raw, GM_OID_RAWSZ) == GM_OK); edge->rel_type = rel; edge->confidence = 0x3C00; assert(gm_strcpy_safe(edge->src_path, sizeof edge->src_path, src_path) == 0); assert(gm_strcpy_safe(edge->tgt_path, sizeof edge->tgt_path, tgt_path) == 0); assert(gm_ulid_generate(edge->ulid).ok); }Then use:
make_test_edge(&edges[0], A, B, GM_REL_IMPLEMENTS, "A", "B"); make_test_edge(&edges[1], A, C, GM_REL_DEPENDS_ON, "A", "C");core/src/domain/journal/codec.c (2)
29-31: Redundant check before null-terminating encoded string.Line 29 checks
if (required > 0U)before null-terminating atrequired - 1U, butrequiredis guaranteed to be > 0 becausesodium_base64_ENCODED_LENalways includes space for the null terminator. The check is redundant and can be removed for clarity.sodium_bin2base64(encoded, required, cbor_data, cbor_len, variant); - if (required > 0U) { - encoded[required - 1U] = '\0'; - } + encoded[required - 1U] = '\0'; *message_out = encoded;
42-44: Document caller responsibility for decoded buffer size.Line 42 validates that
decodedanddecoded_lengthare non-NULL, but does not verify that*decoded_lengthis large enough to hold the decoded output.sodium_base642binwill fail if the buffer is too small, but the error message "decode requires buffers" is generic. Consider documenting in the header that the caller must pass a sufficiently sized buffer and that*decoded_lengthmust be pre-initialized to the buffer capacity.Add a header comment in
core/include/gitmind/journal/internal/codec.h:/** * Decode a base64 commit message into binary CBOR. * * @param raw_message Base64-encoded string * @param decoded Output buffer (caller must allocate sufficient space) * @param decoded_length [in/out] Buffer capacity on input; decoded size on output * @return gm_result_void_t (GM_ERR_INVALID_FORMAT if base64 is invalid) */core/include/gitmind/journal/internal/codec.h (2)
16-20: Add documentation for memory ownership and lifetime.Lines 16-20 declare
gm_journal_encode_messagebut do not document that*message_outis heap-allocated and must be freed by the caller. Consider adding a brief doc comment clarifying memory ownership and lifetime expectations.-/* Encode a CBOR payload into a base64 message string for commit bodies. */ +/** + * Encode a CBOR payload into a base64 message string for commit bodies. + * + * @param cbor_data Input CBOR bytes + * @param cbor_len Size of CBOR payload + * @param message_out [out] Heap-allocated base64 string (caller must free) + * @param message_len_out [out, optional] Length of allocated string (including null terminator) + * @return gm_result_void_t + */ GM_NODISCARD gm_result_void_t gm_journal_encode_message(const uint8_t *cbor_data,
22-25: Add documentation for buffer requirements.Lines 22-25 declare
gm_journal_decode_messagewithout documenting thatdecodedmust be a pre-allocated buffer of sufficient size and thatdecoded_lengthis an in/out parameter. Add a doc comment clarifying these requirements.-/* Decode a commit message (base64) into binary CBOR bytes. */ +/** + * Decode a base64 commit message into binary CBOR bytes. + * + * @param raw_message Base64-encoded commit message + * @param decoded [out] Pre-allocated buffer for decoded CBOR + * @param decoded_length [in/out] Buffer capacity on input; decoded size on output + * @return gm_result_void_t (GM_ERR_INVALID_FORMAT if base64 is malformed) + */ GM_NODISCARD gm_result_void_t gm_journal_decode_message(const char *raw_message,core/include/gitmind/journal/internal/read_decoder.h (1)
19-25: Doc nit: return wordingChange "Returns GM_OK" to "returns gm_ok_void (gm_result_void_t.ok==true)" to match API semantics.
core/src/journal/reader.c (3)
325-333: Prefer monotonic wall-clock for durationclock() is CPU time and may misrepresent I/O-bound durations. Use a monotonic wall clock (e.g., gm_time_monotonic_ms if available) for metrics.
69-72: Check gm_snprintf in debug loggerValidate gm_snprintf return and null-terminate/zero on failure to honor guidelines.
As per coding guidelines
200-216: Emit diagnostics ok, but also surface code safelyConsider initializing cbuf to empty and checking gm_snprintf return before use. Optional.
core/tests/unit/test_journal_e2e_libgit2.c (1)
1-15: Classify as integration testThis uses real libgit2 and filesystem. Consider moving to an integration tests directory or gating under Docker-only to align with test policy.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
.gitignore(1 hunks)AGENTS.md(5 hunks)README.md(2 hunks)apps/cli/README.md(1 hunks)apps/cli/Scripting_Patterns.md(1 hunks)apps/cli/link.md(1 hunks)apps/cli/list.md(1 hunks)apps/cli/main.c(4 hunks)apps/cli/main.md(2 hunks)core/include/gitmind/context.h(2 hunks)core/include/gitmind/journal/internal/append_plan.h(1 hunks)core/include/gitmind/journal/internal/codec.h(1 hunks)core/include/gitmind/journal/internal/read_decoder.h(1 hunks)core/include/gitmind/ports/diagnostic_port.h(1 hunks)core/src/adapters/diagnostics/stderr_diagnostics_adapter.c(1 hunks)core/src/adapters/diagnostics/stderr_diagnostics_adapter.h(1 hunks)core/src/app/cache/cache_rebuild_service.c(10 hunks)core/src/domain/journal/append_planner.c(1 hunks)core/src/domain/journal/codec.c(1 hunks)core/src/domain/journal/read_decoder.c(1 hunks)core/src/journal/reader.c(9 hunks)core/src/journal/writer.c(11 hunks)core/tests/fakes/diagnostics/fake_diagnostics_port.c(1 hunks)core/tests/fakes/diagnostics/fake_diagnostics_port.h(1 hunks)core/tests/fakes/git/fake_git_repository_port.c(1 hunks)core/tests/unit/test_cache_branch_limits.c(1 hunks)core/tests/unit/test_cache_query.c(1 hunks)core/tests/unit/test_cache_telemetry_emit.c(1 hunks)core/tests/unit/test_cli_json_env.c(1 hunks)core/tests/unit/test_diagnostics_port.c(1 hunks)core/tests/unit/test_journal_e2e_libgit2.c(1 hunks)core/tests/unit/test_journal_nff_retry.c(1 hunks)core/tests/unit/test_journal_port_append_flow.c(1 hunks)docs/architecture/hexagonal/Journal.md(1 hunks)docs/operations/Diagnostics_Events.md(1 hunks)docs/operations/Observability.md(1 hunks)docs/operations/Telemetry_Config.md(1 hunks)meson.build(6 hunks)
✅ Files skipped from review due to trivial changes (8)
- docs/operations/Observability.md
- .gitignore
- apps/cli/link.md
- apps/cli/Scripting_Patterns.md
- apps/cli/README.md
- apps/cli/list.md
- docs/architecture/hexagonal/Journal.md
- docs/operations/Diagnostics_Events.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/operations/Telemetry_Config.md
- core/include/gitmind/context.h
- AGENTS.md
🧰 Additional context used
📓 Path-based instructions (14)
**/*.{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/adapters/diagnostics/stderr_diagnostics_adapter.hcore/tests/unit/test_diagnostics_port.ccore/tests/unit/test_journal_e2e_libgit2.ccore/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/src/domain/journal/append_planner.ccore/include/gitmind/journal/internal/codec.hcore/src/domain/journal/read_decoder.ccore/tests/unit/test_cli_json_env.ccore/tests/unit/test_journal_port_append_flow.ccore/include/gitmind/journal/internal/read_decoder.hcore/src/domain/journal/codec.ccore/tests/unit/test_cache_query.ccore/tests/unit/test_cache_telemetry_emit.ccore/tests/fakes/diagnostics/fake_diagnostics_port.hcore/src/journal/writer.capps/cli/main.ccore/include/gitmind/journal/internal/append_plan.hcore/tests/fakes/diagnostics/fake_diagnostics_port.ccore/src/journal/reader.ccore/tests/fakes/git/fake_git_repository_port.ccore/include/gitmind/ports/diagnostic_port.hcore/tests/unit/test_journal_nff_retry.ccore/tests/unit/test_cache_branch_limits.ccore/src/app/cache/cache_rebuild_service.c
{core,src,apps,tests}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
{core,src,apps,tests}/**/*.{c,h}: Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
Do not use buffers sized GM_PATH_MAX*2; prefer GM_PATH_MAX and allocate if larger is required
Equality is OID-first; if both OIDs exist they must match; only fall back to SHA when OIDs are absent
Build refs via gm_build_ref and reject inputs that start with "refs/"
Language: C23 with warnings-as-errors; no VLAs, no shadowing; declare explicit prototypes
Formatting via .clang-format (LLVM style, 4 spaces, 80 cols, pointer alignment right); pre-commit runs clang-format
Naming: functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE; types end with t; header guards start with GITMIND
Includes: prefer specific headers; order/regroup per .clang-format; include-what-you-use
No new clang-tidy warnings; keep touched files tidy-clean
Surface failures via gm_result_t (and variants); ports must not return raw integers
Maintain one responsibility per translation unit where practical; split multi-concern files when touched
Prefer OID types and helpers (gm_oid_t, git_oid_cmp via wrappers, gm_oid_to_hex); avoid direct libgit2 helpers in domain/application layers
Zero output buffers before formatting/copying on error paths
Files:
core/src/adapters/diagnostics/stderr_diagnostics_adapter.hcore/tests/unit/test_diagnostics_port.ccore/tests/unit/test_journal_e2e_libgit2.ccore/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/src/domain/journal/append_planner.ccore/include/gitmind/journal/internal/codec.hcore/src/domain/journal/read_decoder.ccore/tests/unit/test_cli_json_env.ccore/tests/unit/test_journal_port_append_flow.ccore/include/gitmind/journal/internal/read_decoder.hcore/src/domain/journal/codec.ccore/tests/unit/test_cache_query.ccore/tests/unit/test_cache_telemetry_emit.ccore/tests/fakes/diagnostics/fake_diagnostics_port.hcore/src/journal/writer.capps/cli/main.ccore/include/gitmind/journal/internal/append_plan.hcore/tests/fakes/diagnostics/fake_diagnostics_port.ccore/src/journal/reader.ccore/tests/fakes/git/fake_git_repository_port.ccore/include/gitmind/ports/diagnostic_port.hcore/tests/unit/test_journal_nff_retry.ccore/tests/unit/test_cache_branch_limits.ccore/src/app/cache/cache_rebuild_service.c
core/src/adapters/**/*_adapter.h
📄 CodeRabbit inference engine (AGENTS.md)
Adapter private headers colocate under core/src/adapters//_adapter.h
Files:
core/src/adapters/diagnostics/stderr_diagnostics_adapter.h
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_diagnostics_port.ccore/tests/unit/test_journal_e2e_libgit2.ccore/tests/unit/test_cli_json_env.ccore/tests/unit/test_journal_port_append_flow.ccore/tests/unit/test_cache_query.ccore/tests/unit/test_cache_telemetry_emit.ccore/tests/unit/test_journal_nff_retry.ccore/tests/unit/test_cache_branch_limits.c
core/tests/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests should use fakes only; integration tests may use real adapters but only inside Docker
Files:
core/tests/unit/test_diagnostics_port.ccore/tests/unit/test_journal_e2e_libgit2.ccore/tests/unit/test_cli_json_env.ccore/tests/unit/test_journal_port_append_flow.ccore/tests/unit/test_cache_query.ccore/tests/unit/test_cache_telemetry_emit.ccore/tests/fakes/diagnostics/fake_diagnostics_port.ccore/tests/fakes/git/fake_git_repository_port.ccore/tests/unit/test_journal_nff_retry.ccore/tests/unit/test_cache_branch_limits.c
core/src/adapters/**/*_adapter.c
📄 CodeRabbit inference engine (AGENTS.md)
Runtime adapters live under core/src/adapters//_adapter.c and expose factories returning the port vtable plus state with teardown hooks
Files:
core/src/adapters/diagnostics/stderr_diagnostics_adapter.c
core/src/domain/**
📄 CodeRabbit inference engine (AGENTS.md)
Domain core code under core/src/domain/** must be pure/deterministic (no direct IO/libgit2/global state)
Files:
core/src/domain/journal/append_planner.ccore/src/domain/journal/read_decoder.ccore/src/domain/journal/codec.c
core/src/domain/{edge,journal,cache,attribution,hooks}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Wrap all side effects behind outbound ports; no direct IO or libgit2 in domain modules
Files:
core/src/domain/journal/append_planner.ccore/src/domain/journal/read_decoder.ccore/src/domain/journal/codec.c
core/src/**/{hooks,cache,journal}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid direct libgit2 usage; route repository/ref operations through gm_git_repository_port/gm_git_ref_port adapters
Files:
core/src/domain/journal/append_planner.ccore/src/domain/journal/read_decoder.ccore/src/domain/journal/codec.ccore/src/journal/writer.ccore/src/journal/reader.ccore/src/app/cache/cache_rebuild_service.c
core/include/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Public headers under core/include must be umbrella-safe: compile standalone, include only what they use, stable header guards, and provide extern "C" when included from C++
Files:
core/include/gitmind/journal/internal/codec.hcore/include/gitmind/journal/internal/read_decoder.hcore/include/gitmind/journal/internal/append_plan.hcore/include/gitmind/ports/diagnostic_port.h
core/include/gitmind/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Public structs in headers must avoid ABI breaks; append new fields at the struct tail and mark deprecated fields instead of removing
Files:
core/include/gitmind/journal/internal/codec.hcore/include/gitmind/journal/internal/read_decoder.hcore/include/gitmind/journal/internal/append_plan.hcore/include/gitmind/ports/diagnostic_port.h
core/tests/fakes/**
📄 CodeRabbit inference engine (AGENTS.md)
Provide deterministic fakes/mocks/stubs for every outbound port under core/tests/fakes/** with harnesses verifying contract invariants
Files:
core/tests/fakes/diagnostics/fake_diagnostics_port.hcore/tests/fakes/diagnostics/fake_diagnostics_port.ccore/tests/fakes/git/fake_git_repository_port.c
core/include/gitmind/ports/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Define all inbound/outbound ports as headers only under core/include/gitmind/ports/** with POD structs and function-pointer vtables (no non-trivial inline behavior)
Files:
core/include/gitmind/ports/diagnostic_port.h
meson.build
📄 CodeRabbit inference engine (AGENTS.md)
Target C23 via Meson c2x and keep warnings-as-errors; register new unit test targets in meson.build
Files:
meson.build
🧠 Learnings (7)
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
Applied to files:
core/tests/unit/test_cache_telemetry_emit.ccore/src/app/cache/cache_rebuild_service.c
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Naming: functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE; types end with _t; header guards start with GITMIND_
Applied to files:
core/src/journal/writer.c
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to core/src/**/{hooks,cache,journal}/**/*.{c,h} : Avoid direct libgit2 usage; route repository/ref operations through gm_git_repository_port/gm_git_ref_port adapters
Applied to files:
apps/cli/main.c
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to core/include/gitmind/ports/**/*.h : Define all inbound/outbound ports as headers only under core/include/gitmind/ports/** with POD structs and function-pointer vtables (no non-trivial inline behavior)
Applied to files:
meson.build
📚 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:
meson.build
📚 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:
meson.build
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Prefer OID types and helpers (gm_oid_t, git_oid_cmp via wrappers, gm_oid_to_hex); avoid direct libgit2 helpers in domain/application layers
Applied to files:
core/src/app/cache/cache_rebuild_service.c
🧬 Code graph analysis (14)
core/src/adapters/diagnostics/stderr_diagnostics_adapter.h (1)
core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (1)
gm_stderr_diagnostics_port_init(40-52)
core/tests/unit/test_diagnostics_port.c (3)
core/tests/fakes/diagnostics/fake_diagnostics_port.c (2)
gm_fake_diag_port_init(38-52)gm_fake_diag_port_dispose(54-59)core/src/app/cache/cache_rebuild_service.c (1)
gm_cache_rebuild_execute(494-676)core/tests/fakes/fs/fake_fs_temp_port.c (1)
gm_fake_fs_temp_port_dispose(252-259)
core/tests/unit/test_journal_e2e_libgit2.c (7)
core/src/adapters/fs/posix_temp_adapter.c (1)
gm_posix_fs_temp_port_create(628-653)core/src/adapters/git/libgit2_repository_port.c (1)
gm_libgit2_repository_port_create(1111-1139)core/tests/fakes/logging/fake_logger_port.c (1)
gm_fake_logger_port_init(31-46)core/tests/fakes/metrics/fake_metrics_port.c (1)
gm_fake_metrics_port_init(60-73)core/src/ports/journal/journal_command_port.c (2)
gm_cmd_journal_port_init(50-65)gm_cmd_journal_port_dispose(67-72)core/src/edge/edge.c (1)
gm_edge_create(56-107)core/src/journal/reader.c (1)
gm_journal_read(357-361)
core/src/domain/journal/read_decoder.c (4)
core/include/gitmind/security/memory.h (1)
gm_memset_safe(47-59)core/include/gitmind/util/memory.h (2)
gm_memcpy_span(32-47)gm_strcpy_safe(113-138)core/src/edge/attributed.c (1)
gm_edge_attributed_decode_cbor_ex(290-294)core/src/edge/edge.c (1)
gm_edge_decode_cbor_ex(515-517)
core/tests/unit/test_journal_port_append_flow.c (9)
core/tests/fakes/git/fake_git_repository_port.c (5)
gm_fake_git_repository_port_init(195-229)gm_fake_git_repository_port_set_head_branch(247-261)gm_fake_git_repository_port_last_update_ref(482-485)gm_fake_git_repository_port_last_commit_message(472-475)gm_fake_git_repository_port_dispose(231-237)core/tests/fakes/logging/fake_logger_port.c (2)
gm_fake_logger_port_init(31-46)gm_fake_logger_port_dispose(48-53)core/tests/fakes/metrics/fake_metrics_port.c (2)
gm_fake_metrics_port_init(60-73)gm_fake_metrics_port_dispose(75-80)core/tests/fakes/diagnostics/fake_diagnostics_port.c (2)
gm_fake_diag_port_init(38-52)gm_fake_diag_port_dispose(54-59)core/src/ports/journal/journal_command_port.c (2)
gm_cmd_journal_port_init(50-65)gm_cmd_journal_port_dispose(67-72)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/journal/reader.c (1)
gm_journal_read(357-361)core/tests/fakes/fs/fake_fs_temp_port.c (1)
gm_fake_fs_temp_port_dispose(252-259)
core/tests/unit/test_cache_telemetry_emit.c (6)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)core/include/gitmind/security/memory.h (1)
gm_memset_safe(47-59)core/tests/fakes/logging/fake_logger_port.c (2)
gm_fake_logger_port_init(31-46)gm_fake_logger_port_dispose(48-53)core/tests/fakes/metrics/fake_metrics_port.c (2)
gm_fake_metrics_port_init(60-73)gm_fake_metrics_port_dispose(75-80)core/src/app/cache/cache_rebuild_service.c (1)
gm_cache_rebuild_execute(494-676)core/tests/fakes/fs/fake_fs_temp_port.c (1)
gm_fake_fs_temp_port_dispose(252-259)
core/tests/fakes/diagnostics/fake_diagnostics_port.h (1)
core/tests/fakes/diagnostics/fake_diagnostics_port.c (2)
gm_fake_diag_port_init(38-52)gm_fake_diag_port_dispose(54-59)
core/src/journal/writer.c (5)
core/src/error/error.c (1)
gm_error_free(161-178)core/include/gitmind/ports/diagnostic_port.h (1)
gm_diag_emit(37-46)core/src/telemetry/log_format.c (1)
gm_log_format_render_default(71-107)core/include/gitmind/ports/logger_port.h (1)
gm_logger_log(66-74)core/include/gitmind/ports/metrics_port.h (2)
gm_metrics_timing_ms(72-79)gm_metrics_counter_add(52-59)
apps/cli/main.c (1)
core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (1)
gm_stderr_diagnostics_port_init(40-52)
core/tests/fakes/diagnostics/fake_diagnostics_port.c (1)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
core/src/journal/reader.c (5)
core/src/error/error.c (1)
gm_error_free(161-178)core/include/gitmind/ports/diagnostic_port.h (1)
gm_diag_emit(37-46)core/src/telemetry/log_format.c (1)
gm_log_format_render_default(71-107)core/include/gitmind/ports/logger_port.h (1)
gm_logger_log(66-74)core/include/gitmind/ports/metrics_port.h (2)
gm_metrics_timing_ms(72-79)gm_metrics_counter_add(52-59)
core/tests/fakes/git/fake_git_repository_port.c (2)
core/include/gitmind/security/memory.h (1)
gm_memset_safe(47-59)core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
core/tests/unit/test_journal_nff_retry.c (4)
core/tests/fakes/diagnostics/fake_diagnostics_port.c (2)
gm_fake_diag_port_init(38-52)gm_fake_diag_port_dispose(54-59)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/journal/writer.c (1)
gm_journal_append(436-440)
core/src/app/cache/cache_rebuild_service.c (6)
core/include/gitmind/ports/diagnostic_port.h (1)
gm_diag_emit(37-46)core/include/gitmind/ports/logger_port.h (1)
gm_logger_log(66-74)core/src/telemetry/log_format.c (1)
gm_log_format_render_default(71-107)core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)core/include/gitmind/ports/metrics_port.h (3)
gm_metrics_timing_ms(72-79)gm_metrics_counter_add(52-59)gm_metrics_gauge_set(62-69)core/src/error/error.c (1)
gm_error_free(161-178)
🔇 Additional comments (19)
core/tests/unit/test_cache_query.c (1)
72-72: LGTM: Repository initialization change aligns with CI parity goals.Switching from bare to non-bare repository initialization is safe for this test, as the subsequent operations (branch creation, commits, cache queries) do not depend on bare repository semantics.
core/tests/unit/test_cache_branch_limits.c (1)
96-96: LGTM: Repository initialization change aligns with CI parity goals.Switching from bare to non-bare repository initialization is safe for this test, as the subsequent operations (branch creation, commits, cache rebuilds) do not depend on bare repository semantics.
core/tests/unit/test_cli_json_env.c (2)
8-10: Conditional putenv declaration is acceptable for portability.The conditional declaration of
putenvfor non-Windows platforms is reasonable for ensuring portability, though some compilers may issue warnings about implicit declarations.
20-22: Verify test isolation and side effects.The test modifies the process environment, which could affect other tests if they run in the same process. While the environment change is scoped to this process and implicitly cleaned up on exit, consider whether this test should:
- Be run in isolation (separate process)
- Clean up the environment variable explicitly
- Document the side effect
Based on the test framework's execution model, verify that this test runs in isolation or that subsequent tests are not affected by the modified environment.
core/tests/fakes/git/fake_git_repository_port.c (1)
801-801: Retain defaultparent_count = 0; tests intest_hook_augment.cexplicitly callgm_fake_git_repository_port_set_commit_parentsto configure parents.Likely an incorrect or invalid review comment.
core/include/gitmind/journal/internal/read_decoder.h (1)
4-18: Header looks umbrella-safe; API surface LGTMIncludes are minimal, guard and extern "C" present; types resolved.
Also applies to: 26-32
core/tests/unit/test_diagnostics_port.c (3)
40-56: LGTM!The test setup correctly initializes fakes and stubs without direct I/O or global state. The use of fakes aligns with unit testing guidelines.
57-75: LGTM!The test execution and assertions correctly verify that diagnostic events are emitted during cache rebuild failures. The event name checks cover the expected failure scenarios.
77-81: LGTM!Cleanup is properly handled, and the success output follows the expected test pattern.
core/src/domain/journal/read_decoder.c (2)
47-60: LGTM!The function correctly uses
gm_memcpy_spanandgm_strcpy_safefor memory and string operations, following coding guidelines.
62-107: LGTM!The decoder function correctly implements fallback logic between attributed and basic formats, with proper input validation and error handling using the gm_result_void_t pattern.
core/src/journal/writer.c (6)
46-81: LGTM!The addition of
app_ctxtojournal_ctx_tenables diagnostics integration without breaking existing functionality.
97-105: LGTM!The refactoring to use
gm_journal_encode_messagesimplifies the code while maintaining proper error handling and cleanup.
131-183: LGTM!The changes correctly handle the first-commit case by treating
GM_ERR_NOT_FOUNDas empty history, and the use ofgm_journal_build_commit_planprovides better structure.
200-255: LGTM!The diagnostic emissions provide valuable observability for commit creation and ref update failures, with proper null checks on
app_ctx.
311-401: LGTM!The telemetry integration correctly loads configuration, builds tags, captures timing, and emits metrics and logs. The implementation follows telemetry patterns and provides good observability.
474-496: LGTM!The addition of the ref update step ensures the provided ref points to the newly created commit, with proper error propagation.
meson.build (2)
124-148: LGTM!The new source files align with the PR objectives to add domain modules, telemetry, and adapters for the hexagonal architecture.
519-524: LGTM!The new port headers are correctly added to the public header compilation checks, ensuring they can be included standalone.
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 (3)
docs/risk/Risk_Register.md (1)
17-20: Fix mismatch: “Top 10 Risks” vs 12 entries; align TOC text/anchor with sectionThere are 12 risks (R1–R12) but the header says “Top 10 Risks”. Also TOC label says “Top Risks” while linking to “#top-10-risks”. Recommend making the section title stable (“Top Risks”) and updating the TOC anchor accordingly.
Apply this diff:
- - [Top Risks](#top-10-risks) + - [Top Risks](#top-risks) -## Top 10 Risks +## Top RisksAlternatively, if you intend exactly 10 items, trim to 10 and keep the existing anchors/text in sync. As per coding guidelines.
Also applies to: 25-26
docs/README.md (1)
27-27: Fix stale link in docs/README.md
Replace the removed “Journal Architecture Pivot” link with “Journal Architecture” pointing to the existing architecture/journal-architecture.md:-- Start here if you’re new: [Journal Architecture Pivot](architecture/journal-architecture-pivot.md) +- Start here if you’re new: [Journal Architecture](architecture/journal-architecture.md)core/tests/integration/test_cache_query.c (1)
139-159: Fix dispose order and double-dispose of fs_temp_portfs_temp_port is disposed before gm_fs_temp_port_remove_tree, risking use-after-free; also disposed twice. Remove the tree first, then dispose once.
Apply:
- git_repository *saved_repo = repo; - if (ctx.fs_temp_port_dispose != NULL) { - ctx.fs_temp_port_dispose(&ctx.fs_temp_port); - } - if (ctx.git_repo_port_dispose != NULL) { - ctx.git_repo_port_dispose(&ctx.git_repo_port); - } - assert(repo == saved_repo); + git_repository *saved_repo = repo; + if (ctx.git_repo_port_dispose != NULL) { + ctx.git_repo_port_dispose(&ctx.git_repo_port); + } git_repository_free(repo); repo = NULL; - gm_result_void_t rm_rc = - gm_fs_temp_port_remove_tree(&ctx.fs_temp_port, repo_path); + gm_result_void_t rm_rc = + gm_fs_temp_port_remove_tree(&ctx.fs_temp_port, repo_path); if (!rm_rc.ok) { if (rm_rc.u.err != NULL) { gm_error_free(rm_rc.u.err); } } - if (ctx.fs_temp_port_dispose != NULL) { - ctx.fs_temp_port_dispose(&ctx.fs_temp_port); - } + if (ctx.fs_temp_port_dispose != NULL) { + ctx.fs_temp_port_dispose(&ctx.fs_temp_port); + }
♻️ Duplicate comments (1)
meson.build (1)
440-446: Resolved: test_journal_port now registeredPreviously flagged missing registration is fixed; test_journal_port is defined and registered.
Also applies to: 493-494
🧹 Nitpick comments (8)
docs/README.md (3)
21-21: TOC nesting is inaccurate; make “Docs Conventions” a top‑level entry“Docs Conventions” is not a child of “Getting Started”. Promote it to top‑level.
- - [Docs Conventions](#docs-conventions) +- [Docs Conventions](#docs-conventions)
52-53: Fix top‑level bullet indentation for “CLI (deferred)”Align the top‑level bullet with the others; keep the explanatory sub‑bullet indented.
- - CLI (deferred) +- CLI (deferred) - CLI docs are being restructured as part of the hexagonal migration and will return in a future update.
84-92: Fence the YAML example to satisfy markdownlint and prevent accidental parsingThe front‑matter example should be in a fenced code block (yaml) to avoid thematic breaks from raw
---and to pass MD linting.- --- - title: Page Title - description: One-line summary - audience: [developers] - domain: [architecture] - tags: [journal, cache] - status: stable - last_updated: 2025-09-15 - --- +```yaml +--- +title: Page Title +description: One-line summary +audience: [developers] +domain: [architecture] +tags: [journal, cache] +status: stable +last_updated: 2025-09-15 +--- +```core/tests/integration/test_journal_mixed_cbor.c (1)
10-15: Remove duplicate include of gitmind/error.hgitmind/error.h is included twice (Line 10 and Line 13). Drop the second include to keep IWYU clean.
core/tests/unit/test_journal_e2e_libgit2.c (1)
186-194: Dispose fake logger/metrics ports to avoid leaksRelease fake port allocations during cleanup.
gm_cmd_journal_port_dispose(&jport); if (ctx.git_repo_port_dispose) ctx.git_repo_port_dispose(&ctx.git_repo_port); git_repository_free(repo); + gm_fake_logger_port_dispose(&ctx.logger_port); + gm_fake_metrics_port_dispose(&ctx.metrics_port); gm_result_void_t rm_repo = gm_fs_temp_port_remove_tree(&ctx.fs_temp_port, repo_dir); assert(rm_repo.ok); if (ctx.fs_temp_port_dispose) ctx.fs_temp_port_dispose(&ctx.fs_temp_port); git_libgit2_shutdown();core/tests/unit/test_cache_telemetry_emit.c (1)
34-43: Zero the output buffer on failure from sr_repository_pathTreat truncation as error and clear outputs. Ensure 'out' is nulled on error per guidelines.
static gm_result_void_t sr_repository_path(void *self, gm_git_repository_path_kind_t kind, char *out, size_t out_size) { (void)kind; stub_repo_t *sr = (stub_repo_t *)self; - if (gm_strcpy_safe(out, out_size, sr->gitdir) != 0) { - return gm_err_void(GM_ERROR(GM_ERR_BUFFER_TOO_SMALL, "stub repo path too long")); - } + int rc = gm_strcpy_safe(out, out_size, sr->gitdir); + if (rc != GM_OK) { + if (out != NULL && out_size > 0) out[0] = '\0'; + return gm_err_void( + GM_ERROR(GM_ERR_BUFFER_TOO_SMALL, "stub repo path too long")); + } return gm_ok_void(); }As per coding guidelines
core/tests/support/temp_repo_helpers.h (1)
7-17: Include <stdbool.h> and fix _getcwd size type on Windows to avoid -Wconversion
- Use include-what-you-use: bool/true require <stdbool.h>.
- On Windows, _getcwd expects int; passing size_t triggers -Wconversion (werror).
Apply:
#include <errno.h> #include <stddef.h> #include <string.h> +#include <stdbool.h> +#include <limits.h> @@ - if (gm_test_getcwd(cwd, sizeof(cwd)) == NULL) { + #ifdef _WIN32 + const int cwd_cap = (sizeof(cwd) > INT_MAX) ? INT_MAX : (int)sizeof(cwd); + if (gm_test_getcwd(cwd, cwd_cap) == NULL) { + #else + if (gm_test_getcwd(cwd, sizeof(cwd)) == NULL) { + #endif return gm_err_void(GM_ERROR(GM_ERR_IO_FAILED, "getcwd failed: %s", strerror(errno))); }As per coding guidelines
Also applies to: 60-65
core/tests/integration/test_cache_branch_limits.c (1)
140-156: Duplicate disposal of fs_temp_port.The
fs_temp_portis disposed twice: once at lines 140–142 (before tree removal) and again at lines 154–156 (after tree removal). The second call is safe (checks for NULL dispose function) but redundant. Consider removing the first disposal and keeping only the final cleanup, or restructure to dispose once at the end.Apply this diff to remove the first disposal:
- if (ctx.fs_temp_port_dispose != NULL) { - ctx.fs_temp_port_dispose(&ctx.fs_temp_port); - } if (ctx.git_repo_port_dispose != NULL) { ctx.git_repo_port_dispose(&ctx.git_repo_port); }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.github/workflows/auto-seed-review.yml(0 hunks)core/include/gitmind/ports/cache_query_port.h(1 hunks)core/src/app/cache/cache_rebuild_service.c(10 hunks)core/src/domain/journal/read_decoder.c(1 hunks)core/src/journal/reader.c(9 hunks)core/src/journal/writer.c(11 hunks)core/src/ports/cache/cache_query_port.c(1 hunks)core/src/telemetry/config.c(1 hunks)core/src/telemetry/log_format.c(1 hunks)core/tests/fakes/diagnostics/fake_diagnostics_port.c(1 hunks)core/tests/fakes/metrics/fake_metrics_port.c(1 hunks)core/tests/integration/test_cache_branch_limits.c(4 hunks)core/tests/integration/test_cache_meta.c(3 hunks)core/tests/integration/test_cache_query.c(3 hunks)core/tests/integration/test_cache_tree_size.c(3 hunks)core/tests/integration/test_journal_mixed_cbor.c(3 hunks)core/tests/integration/test_repo_build_tree.c(1 hunks)core/tests/support/temp_repo_helpers.h(1 hunks)core/tests/unit/test_cache_telemetry_emit.c(1 hunks)core/tests/unit/test_cli_json_env.c(1 hunks)core/tests/unit/test_journal_e2e_libgit2.c(1 hunks)core/tests/unit/test_journal_nff_retry.c(1 hunks)docs/README.md(2 hunks)docs/operations/Telemetry_Config.md(1 hunks)docs/requirements/Requirements.md(1 hunks)docs/risk/Risk_Register.md(1 hunks)meson.build(8 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/auto-seed-review.yml
✅ Files skipped from review due to trivial changes (1)
- docs/requirements/Requirements.md
🚧 Files skipped from review as they are similar to previous changes (8)
- core/tests/unit/test_cli_json_env.c
- core/tests/fakes/metrics/fake_metrics_port.c
- core/src/telemetry/log_format.c
- core/include/gitmind/ports/cache_query_port.h
- docs/operations/Telemetry_Config.md
- core/tests/unit/test_journal_nff_retry.c
- core/src/domain/journal/read_decoder.c
- core/src/telemetry/config.c
🧰 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/integration/test_cache_meta.ccore/tests/integration/test_cache_tree_size.ccore/tests/integration/test_journal_mixed_cbor.ccore/src/ports/cache/cache_query_port.ccore/tests/unit/test_cache_telemetry_emit.ccore/tests/integration/test_repo_build_tree.ccore/tests/support/temp_repo_helpers.hcore/tests/integration/test_cache_query.ccore/tests/unit/test_journal_e2e_libgit2.ccore/tests/integration/test_cache_branch_limits.ccore/tests/fakes/diagnostics/fake_diagnostics_port.ccore/src/journal/writer.ccore/src/journal/reader.ccore/src/app/cache/cache_rebuild_service.c
{core,src,apps,tests}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
{core,src,apps,tests}/**/*.{c,h}: Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
Do not use buffers sized GM_PATH_MAX*2; prefer GM_PATH_MAX and allocate if larger is required
Equality is OID-first; if both OIDs exist they must match; only fall back to SHA when OIDs are absent
Build refs via gm_build_ref and reject inputs that start with "refs/"
Language: C23 with warnings-as-errors; no VLAs, no shadowing; declare explicit prototypes
Formatting via .clang-format (LLVM style, 4 spaces, 80 cols, pointer alignment right); pre-commit runs clang-format
Naming: functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE; types end with t; header guards start with GITMIND
Includes: prefer specific headers; order/regroup per .clang-format; include-what-you-use
No new clang-tidy warnings; keep touched files tidy-clean
Surface failures via gm_result_t (and variants); ports must not return raw integers
Maintain one responsibility per translation unit where practical; split multi-concern files when touched
Prefer OID types and helpers (gm_oid_t, git_oid_cmp via wrappers, gm_oid_to_hex); avoid direct libgit2 helpers in domain/application layers
Zero output buffers before formatting/copying on error paths
Files:
core/tests/integration/test_cache_meta.ccore/tests/integration/test_cache_tree_size.ccore/tests/integration/test_journal_mixed_cbor.ccore/src/ports/cache/cache_query_port.ccore/tests/unit/test_cache_telemetry_emit.ccore/tests/integration/test_repo_build_tree.ccore/tests/support/temp_repo_helpers.hcore/tests/integration/test_cache_query.ccore/tests/unit/test_journal_e2e_libgit2.ccore/tests/integration/test_cache_branch_limits.ccore/tests/fakes/diagnostics/fake_diagnostics_port.ccore/src/journal/writer.ccore/src/journal/reader.ccore/src/app/cache/cache_rebuild_service.c
core/tests/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests should use fakes only; integration tests may use real adapters but only inside Docker
Files:
core/tests/integration/test_cache_meta.ccore/tests/integration/test_cache_tree_size.ccore/tests/integration/test_journal_mixed_cbor.ccore/tests/unit/test_cache_telemetry_emit.ccore/tests/integration/test_repo_build_tree.ccore/tests/integration/test_cache_query.ccore/tests/unit/test_journal_e2e_libgit2.ccore/tests/integration/test_cache_branch_limits.ccore/tests/fakes/diagnostics/fake_diagnostics_port.c
core/src/ports/**
📄 CodeRabbit inference engine (AGENTS.md)
Default implementations for simple inbound coordinators that remain in C may live under core/src/ports/**
Files:
core/src/ports/cache/cache_query_port.c
core/src/**/{hooks,cache,journal}/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid direct libgit2 usage; route repository/ref operations through gm_git_repository_port/gm_git_ref_port adapters
Files:
core/src/ports/cache/cache_query_port.ccore/src/journal/writer.ccore/src/journal/reader.ccore/src/app/cache/cache_rebuild_service.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_telemetry_emit.ccore/tests/unit/test_journal_e2e_libgit2.c
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Docs must have front matter first, a single H1, a '## Table of Contents', and the title must match the H1
Add api_version field to API docs front matter
Place License/SPDX comments immediately after front matter in docs
README and other Markdown must comply with markdownlint rules: underscore emphasis/strong, blockquote spacing, code fence formatting, no inline HTML wrappers, heading punctuation
Files:
docs/README.mddocs/risk/Risk_Register.md
core/tests/fakes/**
📄 CodeRabbit inference engine (AGENTS.md)
Provide deterministic fakes/mocks/stubs for every outbound port under core/tests/fakes/** with harnesses verifying contract invariants
Files:
core/tests/fakes/diagnostics/fake_diagnostics_port.c
meson.build
📄 CodeRabbit inference engine (AGENTS.md)
Target C23 via Meson c2x and keep warnings-as-errors; register new unit test targets in meson.build
Files:
meson.build
🧠 Learnings (8)
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
Applied to files:
core/tests/unit/test_cache_telemetry_emit.ccore/tests/unit/test_journal_e2e_libgit2.ccore/src/app/cache/cache_rebuild_service.c
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Do not use buffers sized GM_PATH_MAX*2; prefer GM_PATH_MAX and allocate if larger is required
Applied to files:
core/tests/unit/test_journal_e2e_libgit2.c
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to core/src/**/{hooks,cache,journal}/**/*.{c,h} : Avoid direct libgit2 usage; route repository/ref operations through gm_git_repository_port/gm_git_ref_port adapters
Applied to files:
core/tests/integration/test_cache_branch_limits.c
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Naming: functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE; types end with _t; header guards start with GITMIND_
Applied to files:
core/src/journal/writer.c
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Prefer OID types and helpers (gm_oid_t, git_oid_cmp via wrappers, gm_oid_to_hex); avoid direct libgit2 helpers in domain/application layers
Applied to files:
core/src/app/cache/cache_rebuild_service.c
📚 Learning: 2025-10-01T04:13:25.799Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T04:13:25.799Z
Learning: Applies to core/include/gitmind/ports/**/*.h : Define all inbound/outbound ports as headers only under core/include/gitmind/ports/** with POD structs and function-pointer vtables (no non-trivial inline behavior)
Applied to files:
meson.build
📚 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:
meson.build
📚 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:
meson.build
🧬 Code graph analysis (14)
core/tests/integration/test_cache_meta.c (1)
core/src/adapters/fs/posix_temp_adapter.c (1)
gm_posix_fs_temp_port_create(628-653)
core/tests/integration/test_cache_tree_size.c (1)
core/src/adapters/fs/posix_temp_adapter.c (1)
gm_posix_fs_temp_port_create(628-653)
core/tests/integration/test_journal_mixed_cbor.c (1)
core/src/adapters/fs/posix_temp_adapter.c (1)
gm_posix_fs_temp_port_create(628-653)
core/src/ports/cache/cache_query_port.c (2)
core/src/cache/query.c (3)
gm_cache_query_fanout(392-396)gm_cache_query_fanin(399-403)gm_cache_stats(415-448)core/src/ports/cache/cache_build_port.c (1)
gm_result_void_t(42-57)
core/tests/unit/test_cache_telemetry_emit.c (6)
core/include/gitmind/util/memory.h (2)
gm_strcpy_safe(113-138)gm_memcpy_span(32-47)core/include/gitmind/security/memory.h (1)
gm_memset_safe(47-59)core/tests/fakes/logging/fake_logger_port.c (2)
gm_fake_logger_port_init(31-46)gm_fake_logger_port_dispose(48-53)core/tests/fakes/metrics/fake_metrics_port.c (2)
gm_fake_metrics_port_init(94-107)gm_fake_metrics_port_dispose(109-114)core/src/app/cache/cache_rebuild_service.c (1)
gm_cache_rebuild_execute(494-769)core/tests/fakes/fs/fake_fs_temp_port.c (1)
gm_fake_fs_temp_port_dispose(252-259)
core/tests/integration/test_repo_build_tree.c (2)
core/src/adapters/fs/posix_temp_adapter.c (1)
gm_posix_fs_temp_port_create(628-653)core/src/adapters/git/libgit2_repository_port.c (1)
gm_libgit2_repository_port_create(1111-1139)
core/tests/support/temp_repo_helpers.h (1)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
core/tests/integration/test_cache_query.c (2)
core/src/adapters/fs/posix_temp_adapter.c (1)
gm_posix_fs_temp_port_create(628-653)core/src/error/error.c (1)
gm_error_free(161-178)
core/tests/unit/test_journal_e2e_libgit2.c (8)
core/tests/integration/test_repo_build_tree.c (1)
write_file(18-25)core/src/adapters/fs/posix_temp_adapter.c (1)
gm_posix_fs_temp_port_create(628-653)core/src/adapters/git/libgit2_repository_port.c (1)
gm_libgit2_repository_port_create(1111-1139)core/tests/fakes/logging/fake_logger_port.c (1)
gm_fake_logger_port_init(31-46)core/tests/fakes/metrics/fake_metrics_port.c (1)
gm_fake_metrics_port_init(94-107)core/src/ports/journal/journal_command_port.c (2)
gm_cmd_journal_port_init(50-65)gm_cmd_journal_port_dispose(67-72)core/src/edge/edge.c (1)
gm_edge_create(56-107)core/src/journal/reader.c (1)
gm_journal_read(458-462)
core/tests/integration/test_cache_branch_limits.c (2)
core/src/adapters/fs/posix_temp_adapter.c (1)
gm_posix_fs_temp_port_create(628-653)core/src/error/error.c (1)
gm_error_free(161-178)
core/tests/fakes/diagnostics/fake_diagnostics_port.c (1)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
core/src/journal/writer.c (5)
core/src/error/error.c (1)
gm_error_free(161-178)core/include/gitmind/ports/diagnostic_port.h (1)
gm_diag_emit(37-46)core/src/telemetry/log_format.c (1)
gm_log_format_render_default(71-121)core/include/gitmind/ports/logger_port.h (1)
gm_logger_log(66-74)core/include/gitmind/ports/metrics_port.h (2)
gm_metrics_timing_ms(72-79)gm_metrics_counter_add(52-59)
core/src/journal/reader.c (6)
core/src/error/error.c (1)
gm_error_free(161-178)core/include/gitmind/ports/diagnostic_port.h (1)
gm_diag_emit(37-46)core/include/gitmind/ports/logger_port.h (1)
gm_logger_log(66-74)core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)core/src/telemetry/log_format.c (1)
gm_log_format_render_default(71-121)core/include/gitmind/ports/metrics_port.h (2)
gm_metrics_timing_ms(72-79)gm_metrics_counter_add(52-59)
core/src/app/cache/cache_rebuild_service.c (6)
core/include/gitmind/ports/diagnostic_port.h (1)
gm_diag_emit(37-46)core/src/error/error.c (1)
gm_error_free(161-178)core/include/gitmind/ports/logger_port.h (1)
gm_logger_log(66-74)core/src/telemetry/log_format.c (1)
gm_log_format_render_default(71-121)core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)core/include/gitmind/ports/metrics_port.h (3)
gm_metrics_timing_ms(72-79)gm_metrics_counter_add(52-59)gm_metrics_gauge_set(62-69)
🔇 Additional comments (16)
docs/risk/Risk_Register.md (2)
10-11: SPDX and copyright placement LGTMLicense/SPDX comments are correctly placed immediately after front matter. No further changes needed.
As per coding guidelines.
15-15: TOC heading conforms“## Table of Contents” heading is present and correctly leveled.
As per coding guidelines.docs/README.md (2)
10-11: SPDX/license placement looks correctLicense and copyright comments are immediately after front matter, per docs guidelines.
15-15: TOC present and correctly headedThe “## Table of Contents” section is present and positioned after the H1.
core/tests/fakes/diagnostics/fake_diagnostics_port.c (1)
12-70: Emit path handles truncation and invalid args correctlyGood: validates inputs, checks gm_strcpy_safe results, zeros partial outputs, and returns structured errors. Capacity capping is clear and safe.
core/tests/unit/test_journal_e2e_libgit2.c (1)
173-177: Confirm NULL branch semantics for gm_journal_readPassing branch=NULL relies on reader defaulting to a sensible branch. Please confirm this is supported; otherwise pass an explicit branch (e.g., "main") to avoid undefined behavior.
core/tests/integration/test_repo_build_tree.c (1)
89-97: Ignore cast warning gm_oid_t is a direct alias of git_oid, so casting &tree_oid to git_oid* is safe.Likely an incorrect or invalid review comment.
core/tests/integration/test_cache_tree_size.c (1)
32-38: Temp FS/repo setup and teardown LGTMIsolated temp repo creation, use, and cleanup are correct. Good assertions and disposer usage.
Also applies to: 39-44, 46-47, 127-131
core/tests/integration/test_cache_meta.c (1)
51-64: Temp repo orchestration and cleanup LGTMUses gm_posix_fs_temp_port and gm_test_make_temp_repo_dir correctly; cleanup order is sound.
Also applies to: 66-67, 87-91
core/src/ports/cache/cache_query_port.c (1)
1-102: LGTM!The port implementation follows best practices:
- Defensive zeroing of output parameters before validation (lines 54–59)
- Consistent error handling with descriptive messages
- Proper state allocation/disposal lifecycle
- Clean separation of concerns via vtable dispatch
core/src/journal/writer.c (2)
39-42: LGTM! _Static_assert prevents CLOCKS_PER_MS division by zero.The
_Static_assertat lines 39–40 ensuresCLOCKS_PER_SEC >= 1000, preventing the division-by-zero issue previously flagged. This is a robust compile-time check.
323-334: No free needed after gm_fs_temp_port_canonicalize_ex
canonicalize_impl returns the internal state buffer (state->scratch), not malloc’ed memory; freeing is unnecessary.Likely an incorrect or invalid review comment.
core/src/app/cache/cache_rebuild_service.c (3)
513-601: LGTM! Telemetry and logging properly instrumented.The telemetry setup and logging paths correctly:
- Capture and free errors from
gm_telemetry_cfg_load(lines 515–537)- Capture and free errors from
gm_telemetry_build_tags(lines 556–566)- Check formatter results and fall back on errors (lines 583–594)
- Initialize buffers and check for empty messages before logging
This addresses the past review concerns about honoring gm_telemetry_* return values.
686-697: LGTM! gm_snprintf returns properly checked.The code now checks
gm_snprintfreturns and treats truncation/errors as failures, returningGM_ERR_BUFFER_TOO_SMALL. This satisfies the coding guideline requirement.As per coding guidelines
Also applies to: 733-739
544-555: Don’t freerepo_canon; it’s an internal buffer
gm_fs_temp_port_canonicalize_exreturnsstate->scratch(a fixed‐size array ingm_posix_fs_state_t), not a malloc’d string—no free required.Likely an incorrect or invalid review comment.
core/src/journal/reader.c (1)
384-390: Check gm_snprintf return in fallback path.The fallback
gm_snprintfat line 384 is not checked per coding guidelines. Treat truncation as an error by verifying the return value.As per coding guidelines
Apply this diff:
if (!fmt_rc.ok) { if (fmt_rc.u.err != NULL) gm_error_free(fmt_rc.u.err); int alt = gm_snprintf(msg, sizeof(msg), "event=journal_read_start branch=%s mode=%s", resolved_branch, mode); if (alt < 0 || (size_t)alt >= sizeof(msg)) { msg[0] = '\0'; } }⛔ Skipped due to learnings
Learnt from: CR PR: neuroglyph/git-mind#0 File: AGENTS.md:0-0 Timestamp: 2025-10-01T04:13:25.799Z Learning: Applies to {core,src,apps,tests}/**/*.{c,h} : Use gm_snprintf/gm_strcpy_safe/gm_memcpy_span for all string/memory ops; check returns and treat truncation as error; zero outputs on failure paths
| - [Journal Architecture](architecture/journal-architecture.md) | ||
| - [Cache Architecture](architecture/cache-architecture.md) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
New architecture links look good; verify they resolve
Links align with PR intent. Please confirm the files exist to keep link hygiene green.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in docs/architecture/journal-architecture.md docs/architecture/cache-architecture.md; do
if [[ -f "$f" ]]; then
echo "OK: $f"
# Check each has a single H1
rg -n '^# ' "$f" | wc -l | awk -v f="$f" '{print "H1 count in " f ": " $0}'
else
echo "MISSING: $f"
fi
doneLength of output: 201
Enforce a single H1 per architecture doc: docs/architecture/journal-architecture.md contains 8 H1 headings; convert all but the main title to H2+ to maintain one H1.
🤖 Prompt for AI Agents
In docs/README.md around lines 49 to 50: the linked architecture doc
docs/architecture/journal-architecture.md currently contains 8 H1 headings; open
that file and retain only the main document title as a single H1, converting all
other top-level headings from "# " to "## " (or deeper levels like "### " where
appropriate) so they become H2+; ensure the main title remains the first H1,
update any TOC or cross-references if needed, and run the markdown
linter/preview to verify only one H1 remains.
1 participant
Summary
This PR advances the hexagonal migration by introducing explicit ports and thin coordinators, improving journal robustness, and ensuring CI parity:
gm_logger_port,gm_metrics_port(umbrella-safe, no-op friendly).gm_context_twith logger/metrics (appended at tail; no ABI break).cache_build_port+cache_query_portdelegating to existing APIs.migration-progress.mdtracker.Change Areas
Risk
Mitigations: All tests pass in Docker (32/32).
make ci-localis green (docs + build + tests + clang-tidy). New functionality defaults to safe no-op adapters.Code Review Guidance
core/include/gitmind/ports/{logger_port,metrics_port}.hcore/include/gitmind/ports/{cache_build_port,cache_query_port}.hcore/src/ports/cache/{cache_build_port.c,cache_query_port.c}core/src/adapters/logging/stdio_logger_adapter.{c,h}core/src/adapters/metrics/null_metrics_adapter.{c,h}core/src/journal/writer.c(ref update, missing-parent tolerance)core/src/adapters/git/libgit2_repository_port.c(ref dir creation, signature fallback)core/include/gitmind/cache/internal/staleness.hcore/src/cache/query.cusageValidation: run
make ci-local(Dockerized). All tests pass; docs/link checks are green. The migration tracker (migration-progress.md) lists the completed steps.Summary by CodeRabbit
New Features
Bug Fixes
Documentation