Repository navigation
Refactor temp repos and isolate CI workspace - #178
flyingrobots wants to merge 26 commits into
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Telemetry & Log Formatter core/include/gitmind/telemetry/log_format.h, core/include/gitmind/telemetry/internal/log_format.h, core/src/telemetry/log_format.c, core/src/telemetry/config.c, core/include/gitmind/context.h, docs/operations/Telemetry_Config.md |
Add public gm_log_kv_t/gm_log_formatter_fn and gm_log_format_render_default; replace internal header with forward include; deterministic JSON ordering, robust escaping, repo-hash formatters, gm_telemetry_tag_context_t and context-based gm_telemetry_build_tags. |
Diagnostics port & adapters core/include/gitmind/ports/diagnostic_port.h, core/include/gitmind/adapters/diagnostics/stderr_diagnostics_adapter.h, core/src/adapters/diagnostics/stderr_diagnostics_adapter.c, core/tests/fakes/diagnostics/*, docs/operations/Diagnostics_Events.md |
Add vtable dispose hook and gm_diag_reset helper; stderr adapter gains state, escaped output and explicit dispose path; fake diag port updated with dispose; docs show teardown usage. |
CLI wiring & docs apps/cli/main.c, apps/cli/main.md, apps/cli/README.md, README.md |
Switch to public adapter headers; guarded init storing dispose callbacks for diagnostics/logger; robust disposal fallback; expand CLI Quickstart, porcelain/JSON examples and diagnostics notes. |
Journal codec / decoder / timing core/include/gitmind/journal/internal/codec.h, core/include/gitmind/journal/internal/read_decoder.h, core/include/gitmind/journal/internal/append_plan.h, core/src/domain/journal/codec.c, core/src/domain/journal/read_decoder.c, core/src/domain/journal/append_planner.c, core/src/journal/reader.c, core/src/journal/writer.c |
Switch base64 to URLSAFE_NO_PADDING; encode reports payload length excluding NUL; decode API adds decoded_capacity and capacity checks; stricter capacity-aware conversions; rename empty_tree_oid→tree_oid; reader/writer use monotonic_ms_now and integrate edge-count and canonicalization view handling. |
Cache & OID prefix core/include/gitmind/cache/internal/oid_prefix.h, core/src/domain/cache/oid_prefix.c, core/src/app/cache/cache_rebuild_service.c, core/tests/unit/test_cache_oid_prefix.c |
Document limits and rationale; migrate to size_t arithmetic, clamp conservatively, early-return zero-length, propagate real error codes; add extensive unit tests for rounding, small buffers, and bounds. |
Git / libgit2 adapter & refs core/src/adapters/git/libgit2_repository_port.c |
Large refactor: iterative directory/tree builder, safer commit signature fallback, normalized ref name handling, ensure_reference_directories and create/update reference flows, tree-size iteration utilities, and lifecycle/bridge functions—internal rework exposed via stable vtbl bridges. |
Logger adapter & includes core/src/adapters/logging/stdio_logger_adapter.c, core/include/gitmind/adapters/logging/stdio_logger_adapter.h |
Update include paths and add <stdio.h>; cross-platform gmtime_s/gmtime_r usage; header adds gitmind/result.h include. |
Ports & fs_temp semantics core/src/ports/cache/cache_query_port.c, core/include/gitmind/ports/fs_temp_port.h, docs/architecture/fs_temp_service_plan.md |
Zero-initialize cache query outputs; document that canonicalize_ex returns a _view into adapter-owned scratch storage and must be copied/promoted by callers prior to reuse. |
POSIX fs_temp adapter (major refactor) core/src/adapters/fs/posix_temp_adapter.c |
Massive rewrite: POSIX-centric canonicalization, path_builder, safer normalization, create/remove tree rework, deterministic/random temp dir creation, errno->result helpers, renamed vtbl to GmPosixFsTempVtbl and refactored internals for clang-tidy cleanliness. |
Test support & helpers core/tests/support/temp_repo_helpers.c, core/tests/support/temp_repo_helpers.h, core/tests/unit/support/temp_repo_helpers.h, meson.build |
Add reusable temp-repo helpers and provider model, expose make/cleanup APIs; add gitmind_test_support static lib and link tests; add new integration test target. |
Tests: updates & new core/tests/integration/*, core/tests/unit/* |
New integration regression for cache canonicalize; unit tests updated for log_format JSON escaping/order, telemetry context usage, diagnostics disposal, env handling, and expanded cache oid_prefix coverage. |
CI & debug tooling tools/ci/ci_local.sh, tools/debug/debug-container.sh, docs/operations/Debug_Container.md, AGENTS.md |
ci_local: staged workspace snapshot, rsync/tar fallback, artifact collection and clang-tidy diffing, propagate container exit; add debug-container script and docs for staged debugging workflow; update AGENTS.md with CI/telemetry notes. |
Hooks & workflows .githooks/pre-push, .github/workflows/apply-feedback.yml |
Removed worksheet enforcement from pre-push; deleted Apply PR Feedback workflow file. |
Crypto context dispose core/include/gitmind/crypto/backend.h, core/src/crypto/backend.c |
Add gm_crypto_context_dispose public API to clear crypto backend references. |
Misc includes, lint & config core/src/adapters/metrics/null_metrics_adapter.h, .markdownlint-cli2.jsonc, .markdownlintignore, docs/activity/* |
Add missing result.h includes where needed; add markdownlint config/ignore; expand activity docs summarizing CI/tooling/formatter changes. |
Sequence Diagram(s)
sequenceDiagram
autonumber
actor User
participant CLI as apps/cli/main
participant Diag as gm_diagnostics_port
participant Stderr as stderr_adapter
participant Logger as logger_port
User->>CLI: run git-mind command
CLI->>Diag: init diag port
Diag->>Stderr: allocate state, set emit_enabled
alt diag init ok
Stderr-->>CLI: vtbl + dispose callback
else diag init failed
Stderr-->>CLI: error
end
CLI->>Logger: init logger port (store dispose)
CLI->>User: execute subcommand
CLI->>Diag: on shutdown -> gm_diag_reset(&diag_port)
Diag->>Stderr: call dispose if present, clear port
CLI->>Logger: call logger_port_dispose() or fallback
CLI-->>User: exit
sequenceDiagram
autonumber
participant Writer as journal/writer
participant Timer as monotonic_ms_now
participant Codec as journal/codec
participant Reader as journal/reader
Writer->>Timer: monotonic_ms_now()
Timer-->>Writer: start_ms
Writer->>Codec: gm_journal_encode_message(message) [URLSAFE_NO_PADDING]
Codec-->>Writer: encoded payload (payload_len excludes NUL)
Writer->>Writer: write entry
Reader->>Timer: monotonic_ms_now()
Reader->>Codec: gm_journal_decode_message(raw, buf, decoded_capacity, &decoded_len)
alt decode ok
Codec-->>Reader: decoded_len
Reader->>Reader: process edges, increment edge_count (if provided)
else error
Codec-->>Reader: error
end
Estimated code review effort
🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related issues
- feat: git-mind v2 — rewrite on @git-stunts/git-warp #180 — V2_PLAN.md addition aligns directly with the V2 rewrite proposal described in the issue.
Possibly related PRs
- chore/more fixes #171 — Strong code-level overlap on libgit2 repository adapter refactors, refs handling, and test wiring.
- Add hex ports, cache coordinators, and journal adapter robustness #177 — Overlap on telemetry/log_format exposure, diagnostics dispose wiring, fs-temp semantics, and CI/test helpers.
- chore(core/migration): remove legacy src/ duplicates; convert include/gitmind.h to pure umbrella; flatten CLI path to apps/cli/ #165 — Related through CBOR/edge and OID-first serialization changes and header migrations.
Poem
I nibble logs in tidy rows and sort each kv pair,
I hush the stderr chatter, then tidy up with care.
I count monotonic heartbeats, copy views before I roam,
I seed a temp repo, run twice, and bring each test back home.
A rabbit’s patch: small hops, safe paths, and one tidy comb. 🐇
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.67% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | |
| Description check | ❓ Inconclusive | The PR description covers the three main changes with a testing instruction, but lacks the template structure including Risk and Code Review Guidance sections. | Fill in the Change Areas checkboxes and Risk level from the template (e.g., mark Core, CI/Tooling; note Public API change for test_support_lib); add Code Review Guidance summarizing key risks like path buffer lifecycle and workspace isolation. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly describes the main changes: refactoring temporary repository helpers and isolating the CI workspace to avoid workspace pollution. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
chore/temp-port-ci-copy
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 help to get the list of available commands and usage tips.
…ore/temp-port-ci-copy
…ore/temp-port-ci-copy
…ore/temp-port-ci-copy
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/journal/writer.c (2)
290-296: Propagate real error codes from flush_journal_batchCurrent code returns GM_ERR_UNKNOWN, masking the underlying cause. Return the actual status.
- if (should_flush_buffer(offset)) { - if (flush_journal_batch(jctx, buffer, offset) != GM_OK) { - return GM_ERR_UNKNOWN; - } - offset = 0U; - } + if (should_flush_buffer(offset)) { + int rc = flush_journal_batch(jctx, buffer, offset); + if (rc != GM_OK) { + return rc; + } + offset = 0U; + } ... - if (offset > 0U) { - if (flush_journal_batch(jctx, buffer, offset) != GM_OK) { - return GM_ERR_UNKNOWN; - } - } + if (offset > 0U) { + int rc = flush_journal_batch(jctx, buffer, offset); + if (rc != GM_OK) { + return rc; + } + }Also applies to: 298-302
452-472: Validate inputs in gm_journal_create_commitMissing null checks can dereference ctx/ref or data. Add fast-fail validation.
int gm_journal_create_commit(gm_context_t *ctx, const char *ref, const void *data, size_t len) { journal_ctx_t jctx; gm_oid_t commit_oid; + /* Argument validation */ + if (ctx == NULL || ref == NULL || data == NULL) { + return GM_ERR_INVALID_ARGUMENT; + } + /* Initialize context */ jctx.repo_port = &ctx->git_repo_port;
🧹 Nitpick comments (53)
docs/code-reviews/PR178/a39bfee82cb78194ff33fc588d43321c98d9cc8c.md (4)
2-2: Use a human-readable title for discoverability.Prefer a descriptive title including PR number and topic.
-title: a39bfee82cb78194ff33fc588d43321c98d9cc8c.md +title: PR178: Refactor temp repos and isolate CI workspace (a39bfee)
14-14: Linkify the commit SHA in the review log.Improves navigation from the archive.
-| 2025-10-08 | CodeRabbit (and reviewers) | `a39bfee82cb78194ff33fc588d43321c98d9cc8c` | [chore/temp-port-ci-copy](https://github.com/neuroglyph/git-mind/tree/chore/temp-port-ci-copy "neuroglyph/git-mind:chore/temp-port-ci-copy") | [PR#178](https://github.com/neuroglyph/git-mind/pull/178) | +| 2025-10-08 | CodeRabbit (and reviewers) | [`a39bfee`](https://github.com/neuroglyph/git-mind/commit/a39bfee82cb78194ff33fc588d43321c98d9cc8c) | [chore/temp-port-ci-copy](https://github.com/neuroglyph/git-mind/tree/chore/temp-port-ci-copy "neuroglyph/git-mind:chore/temp-port-ci-copy") | [PR#178](https://github.com/neuroglyph/git-mind/pull/178) |
44-50: Strip transient checkbox metadata; keep archive clean.The JSON attributes and branch-specific checkbox are ephemeral. Convert to plain TODOs.
-<summary>🧪 Generate unit tests (beta)</summary> +<summary>🧪 Unit tests TODOs</summary> @@ -- [ ] <!-- {"checkboxId": "f47ac10b-58cc-4372-a567-0e02b2c3d479", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Create PR with unit tests -- [ ] <!-- {"checkboxId": "07f1e7d6-8a8e-4e23-9900-8731c2c87f58", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Post copyable unit tests in a comment -- [ ] <!-- {"checkboxId": "6ba7b810-9dad-11d1-80b4-00c04fd430c8", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Commit unit tests in branch `chore/temp-port-ci-copy` +- Create PR with unit tests +- Post copyable unit tests in a comment +- Commit unit tests in branch `chore/temp-port-ci-copy`
77-77: Optional: add commit context to Meta.Include the commit link next to the comment for quick cross-reference.
-_Meta_: https://github.com/neuroglyph/git-mind/pull/178#issuecomment-3382223172 +_Meta_: https://github.com/neuroglyph/git-mind/pull/178#issuecomment-3382223172 · Commit: https://github.com/neuroglyph/git-mind/commit/a39bfee82cb78194ff33fc588d43321c98d9cc8cdocs/code-reviews/PR178/1a49c6f455c33f1dcf87905f804020b6654be938.md (4)
2-2: Use a human-readable title (avoid file-name/extension).Consider a descriptive title like “PR 178 — CI workspace isolation and temp repo helpers” and drop the “.md”.
14-14: Verify row metadata (SHA/branch/PR) corresponds to the archived commit.Please confirm the SHA matches the content referenced by this doc and, if useful, include the commit subject for context.
22-35: Make the config instruction explicit with a YAML snippet.Add a concrete example so readers can copy/paste:
# .coderabbit.yaml reviews: review_status: falseAlso note explicitly which branch is the default (main) to reduce ambiguity.
44-49: Checkbox attributes and hardcoded branch may age poorly.
- The embedded JSON attributes in the checkboxes may leak into the rendered page. If the renderer doesn’t hide them, switch to plain GitHub-style checkboxes.
- “Commit unit tests in branch chore/temp-port-ci-copy” will become stale post-merge. Prefer a generic instruction or placeholder.
tools/ci/ci_local.sh (4)
12-17: Optional: Allow keeping stage dir for debugging and broaden signal handlingAdd a guard env var to keep the staged workspace on failure; optionally trap INT/TERM too. Keeps artifacts for inspection.
-cleanup() { - if [ -n "${CI_LOCAL_STAGE_DIR:-}" ] && [ -d "$CI_LOCAL_STAGE_DIR" ]; then - rm -rf "$CI_LOCAL_STAGE_DIR" - fi -} -trap cleanup EXIT +cleanup() { + if [ -n "${CI_LOCAL_STAGE_DIR:-}" ] && [ -d "$CI_LOCAL_STAGE_DIR" ]; then + if [ -n "${GITMIND_KEEP_STAGE:-}" ]; then + echo "Keeping staging dir: $CI_LOCAL_STAGE_DIR" + else + rm -rf "$CI_LOCAL_STAGE_DIR" + fi + fi +} +trap cleanup EXIT INT TERM
62-75: Respect .gitignore during rsync copyReduces noise and size of the staged workspace. Tar fallback can stay as-is.
-if command -v rsync >/dev/null 2>&1; then - rsync -a --delete \ +if command -v rsync >/dev/null 2>&1; then + rsync -a --delete --filter=':- .gitignore' \ --exclude '/build-local/' \ --exclude '/ci_logs.zip' \ --exclude '/clang-tidy-report.txt' \ --exclude '/clang-tidy-report-full.txt' \ "$ROOT_DIR"/ "$WORKSPACE_COPY"/
78-78: Status propagation: quote variablesDefensive quoting avoids edge cases even with set -u/-e.
-set +e +set +e @@ -status=$? +status=$? set -e @@ -[ $status -eq 0 ] || exit $status +[ "$status" -eq 0 ] || exit "$status"Also applies to: 105-107, 127-127
108-117: Avoid duplicate compile_commands.json copyYou already copy compile_commands.json in-container to workspace root; then you copy it again from build-local. Keep only one to reduce redundancy.
-for artifact in clang-tidy-report.txt clang-tidy-report-full.txt compile_commands.json; do +for artifact in clang-tidy-report.txt clang-tidy-report-full.txt; do if [ -f "$WORKSPACE_COPY/$artifact" ]; then cp "$WORKSPACE_COPY/$artifact" "$ROOT_DIR/$artifact" fi done -if [ -f "$WORKSPACE_COPY/build-local/compile_commands.json" ]; then - cp "$WORKSPACE_COPY/build-local/compile_commands.json" "$ROOT_DIR/compile_commands.json" -fi +# Prefer the build-local one if present; otherwise rely on the in-container copy to root +if [ -f "$WORKSPACE_COPY/build-local/compile_commands.json" ]; then + cp "$WORKSPACE_COPY/build-local/compile_commands.json" "$ROOT_DIR/compile_commands.json" +ficore/src/adapters/git/libgit2_repository_port.c (1)
800-808: Limit fallback signature to ENOTFOUND only (avoid masking other failures).If git_signature_default fails for reasons other than missing user config, we should surface the error instead of silently falling back. Recommend checking for GIT_ENOTFOUND explicitly; otherwise return an error.
- git_signature *sig = NULL; - if (git_signature_default(&sig, state->repo) < 0) { - /* Fallback to a synthetic signature if repo/user config is missing */ - if (git_signature_now(&sig, "gitmind", "gitmind@example.invalid") < 0) { - return gm_err_void( - GM_ERROR(GM_ERR_UNKNOWN, "unable to create commit signature")); - } - } + git_signature *sig = NULL; + int sig_status = git_signature_default(&sig, state->repo); + if (sig_status < 0) { + if (sig_status == GIT_ENOTFOUND) { + /* Fallback to a synthetic signature if repo/user config is missing */ + if (git_signature_now(&sig, "gitmind", "gitmind@example.invalid") < 0) { + return gm_err_void( + GM_ERROR(GM_ERR_UNKNOWN, "unable to create commit signature")); + } + } else { + return gm_err_void( + GM_ERROR(GM_ERR_UNKNOWN, "default commit signature lookup failed")); + } + }core/tests/unit/test_repo_build_tree.c (1)
88-98: Strengthen assertions: verify tree contents.Currently only checks type. Also assert that root.txt and dir exist with expected kinds.
/* After verifying obj is a tree, also validate entries */ git_tree *tree = NULL; rc = git_tree_lookup(&tree, repo, (const git_oid *)&tree_oid); assert(rc == 0 && tree); const git_tree_entry *e_root = git_tree_entry_byname(tree, "root.txt"); assert(e_root && git_tree_entry_type(e_root) == GIT_OBJECT_BLOB); const git_tree_entry *e_dir = git_tree_entry_byname(tree, "dir"); assert(e_dir && git_tree_entry_type(e_dir) == GIT_OBJECT_TREE); git_tree_free(tree);core/include/gitmind/journal/internal/append_plan.h (2)
16-21: Clarify ownership/lifetime and parent semantics in plan structPointers imply borrowed storage, but ownership and lifetime are undocumented. Also, parents + parent_count (0/1) suggests a single optional parent; plural naming may mislead.
Recommend:
- Document that tree_oid, message, and parents are borrowed and must outlive the plan use; specify NULLability constraints (e.g., parents may be NULL when parent_count == 0; message must be non-NULL).
- Or make OIDs by-value to avoid lifetime hazards and keep message explicitly borrowed.
Example doc-only tweak:
typedef struct gm_journal_commit_plan { - const gm_oid_t *tree_oid; /* required */ - const char *message; /* required, base64-encoded CBOR */ - const gm_oid_t *parents; /* optional pointer to single parent */ - size_t parent_count; /* 0 or 1 for now */ + const gm_oid_t *tree_oid; /* required, borrowed; non-NULL */ + const char *message; /* required, borrowed; base64-encoded CBOR; non-NULL */ + const gm_oid_t *parents; /* optional, borrowed; points to single parent when parent_count == 1, NULL otherwise */ + size_t parent_count; /* 0 or 1 for now */ } gm_journal_commit_plan_t;If you prefer stronger guarantees, switch to by-value OIDs and a bool has_parent to eliminate aliasing risks.
23-28: Parameter naming consistency (empty_tree_oid vs tree_oid)The struct uses tree_oid, while the API parameter is empty_tree_oid. If this function accepts any tree, rename to tree_oid for clarity.
-GM_NODISCARD gm_result_void_t gm_journal_build_commit_plan( - const gm_oid_t *empty_tree_oid, +GM_NODISCARD gm_result_void_t gm_journal_build_commit_plan( + const gm_oid_t *tree_oid, const gm_oid_t *parent_oid_opt, const char *message, gm_journal_commit_plan_t *out_plan);If it truly must be the empty tree, document that invariant explicitly. Also clarify nullability: tree_oid and message must be non-NULL; parent_oid_opt may be NULL; out_plan must be non-NULL.
core/include/gitmind/journal/internal/read_decoder.h (1)
19-33: Specify out-parameter nullability and success invariantsPlease tighten the contract:
- Allow passing NULL for outputs the caller doesn't need (out_basic, out_attr, consumed, got_attr), and document which are required for each decode path.
- On success:
- If got_attr == true, out_attr must be non-NULL and fully populated; out_basic remains untouched.
- If got_attr == false, out_basic must be non-NULL and fully populated; out_attr remains untouched.
- Guarantee consumed <= len and define whether consumed is set on failure.
Suggested doc-only update:
/* Decode a single edge from CBOR bytes. * - If prefer_attributed is true, try attributed first and fall back to legacy. * - If false, try legacy first and fall back to attributed; when falling back, * the attributed edge is converted into a basic edge. - * Returns GM_OK on success and sets consumed to bytes read and got_attr when an - * attributed edge was produced. + * Outputs (NULLable): + * - out_basic: populated when producing a basic (legacy) edge; may be NULL if caller doesn't need it. + * - out_attr: populated when producing an attributed edge; may be NULL if caller doesn't need it. + * - consumed: set to bytes read on success; may be NULL. + * - got_attr: set to true when an attributed edge was produced; may be NULL. + * Invariants: + * - On success, exactly one of (out_basic, out_attr) is written, matching got_attr. + * - consumed <= len. On failure, consumed is left unchanged. + * Returns GM_OK on success. */If NULL outputs are not allowed, mark required ones as non-NULL in comments/macros and define behavior for the unused output.
core/include/gitmind/telemetry/internal/config.h (1)
12-12: Remove unused include.The header includes
gitmind/ports/fs_temp_port.hbut none of the declarations in this file reference the fs_temp port types. This include appears to be unnecessary.Apply this diff to remove the unused include:
#include "gitmind/ports/env_port.h" -#include "gitmind/ports/fs_temp_port.h" #include "gitmind/ports/logger_port.h"core/tests/unit/test_cache_oid_prefix.c (1)
12-32: Expand test coverage for bit rounding and error conditions.The test only validates bit values that are multiples of 4. According to the function's documentation in
oid_prefix.h, bits are "rounded up to the next whole hex character (4 bits per char)". Consider adding tests for:
- Non-multiple-of-4 bit values (e.g., 5, 7, 9 bits) to verify rounding behavior
- Boundary conditions (0 bits, negative bits, bits > 160)
- Buffer size edge cases (buffer too small for requested prefix)
- Error handling paths
Example test additions:
/* Test rounding: 5 bits should round up to 8 bits (2 hex chars) */ assert(gm_cache_oid_prefix(&oid, 5, out, sizeof(out)) == GM_OK); assert(strcmp(out, "01") == 0); /* Test boundary: 0 bits */ assert(gm_cache_oid_prefix(&oid, 0, out, sizeof(out)) == GM_OK); assert(strcmp(out, "") == 0); /* Test error: NULL oid */ assert(gm_cache_oid_prefix(NULL, 4, out, sizeof(out)) != GM_OK); /* Test error: NULL output buffer */ assert(gm_cache_oid_prefix(&oid, 4, NULL, sizeof(out)) != GM_OK);core/tests/unit/test_log_formatter.c (1)
25-40: Strengthen JSON format validation.The JSON test only verifies the presence of braces and specific substrings, which doesn't guarantee valid JSON. Consider:
- Parsing the output with a JSON parser to verify structural validity
- Testing special character escaping (quotes, newlines, backslashes in values)
- Verifying field ordering or using a more robust JSON comparison
- Testing edge cases (empty KV array, NULL keys/values)
Example improvements:
/* After rendering, verify it's parseable JSON (if a JSON parser is available) */ /* Or at minimum, check for proper quote escaping */ const gm_log_kv_t kvs_special[] = { {.key = "event", .value = "test\"with\"quotes"}, {.key = "msg", .value = "line1\nline2"}, }; char out_special[256]; assert(gm_log_format_render_default(kvs_special, 2, true, out_special, sizeof(out_special)).ok); /* Verify escaped quotes and newlines */ assert(strstr(out_special, "\\\"") != NULL); assert(strstr(out_special, "\\n") != NULL);core/include/gitmind/cache/internal/oid_prefix.h (1)
15-24: LGTM! Consider documenting edge case behavior.The header is well-structured. To improve clarity, consider documenting:
- Why
GM_CACHE_MAX_SHARD_PATHis 32 (relationship to SHA-1/SHA-256 hex length, performance considerations, etc.)- Behavior when
bitsexceeds the maximum representable OID bits- Thread-safety guarantees of
gm_cache_oid_prefix- Return value semantics (GM_OK, specific error codes)
Example documentation enhancement:
/* * Maximum characters we ever write for an OID shard prefix. * 32 chars allows up to 128 bits (32 * 4) of OID prefix for sharding, * which is sufficient for SHA-1 (160 bits) and SHA-256 (256 bits). */ #define GM_CACHE_MAX_SHARD_PATH 32 /* * Compute a hexadecimal OID prefix for sharding. The number of bits is rounded * up to the next whole hex character (4 bits per char). The output is * NUL-terminated and clamped to GM_CACHE_MAX_SHARD_PATH - 1 characters. * * Thread-safe: yes (pure function, no global state) * @return GM_OK on success, GM_ERR_INVALID_ARGUMENT if oid/out is NULL */core/tests/unit/test_journal_port.c (1)
17-52: Expand test coverage beyond the happy path.The test validates basic append functionality but lacks coverage for:
- Error conditions (NULL port, NULL edges, invalid edge data)
- Multiple edge append in a single call
- Edge cases (zero edges, extremely large edge counts)
- Append failures and error propagation
- Different branch names (currently hardcoded to "main")
Consider adding test functions for error paths:
static void test_append_null_port(void) { printf("test_journal_port.append_null_port... "); gm_edge_t e = {0}; gm_result_void_t r = gm_cmd_journal_port_append(NULL, &e, 1); assert(!r.ok); assert(r.error.code == GM_ERR_INVALID_ARGUMENT); printf("OK\n"); } static void test_append_zero_edges(void) { printf("test_journal_port.append_zero_edges... "); /* Setup port as in main test... */ gm_result_void_t r = port.vtbl->append(&port, NULL, 0); assert(!r.ok || r.ok); /* Document expected behavior */ printf("OK\n"); }core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (1)
16-34: Consider special character handling in diagnostic output.The
emit_implfunction writes keys and values directly to stderr without escaping special characters (newlines, control characters, etc.). This could result in malformed or misleading diagnostic output if keys/values contain such characters.Consider adding basic escaping for newlines and control characters:
static void emit_escaped(const char *str) { if (!str) { fputs("(null)", stderr); return; } for (const char *p = str; *p; ++p) { if (*p == '\n') { fputs("\\n", stderr); } else if (*p == '\r') { fputs("\\r", stderr); } else if (*p == '\t') { fputs("\\t", stderr); } else { fputc(*p, stderr); } } }Then use
emit_escapedinstead offputsfor component, event, and KV pairs.core/tests/unit/test_journal_mixed_cbor.c (1)
20-22: Deduplicate includes to reduce churnDuplicate headers included:
- gitmind/error.h appears twice (Line 13 and earlier)
- gitmind/types.h appears twice (Line 21 and earlier)
Safe to remove the later duplicates. Keeps compile units cleaner.
apps/cli/main.c (1)
14-15: Avoid including headers from core/src in appsIncluding core/src/... headers couples the CLI to internal paths. Prefer public headers (e.g., move these to core/include and include via gitmind/adapters/...).
core/tests/unit/test_journal_nff_retry.c (2)
84-92: Stale/misleading comment about head resolutionThe stub does implement head_branch (Lines 46-50). Update/remove this block to avoid confusion.
80-82: Prefer GM_OK over literal 0 in assertsUse GM_OK for readability and to avoid coupling to specific numeric values.
core/tests/unit/test_cache_tree_size.c (1)
128-133: Implement guaranteed cleanup in test teardown
File: core/tests/unit/test_cache_tree_size.c:128-133
Assertion failures bypass the current cleanup block, leaving temp trees behind. Move removal and disposal into a teardown hook or register withatexit()(or use your test framework’s fixture teardown) to ensure it always runs.core/src/domain/cache/oid_prefix.c (1)
22-28: Avoid int overflow; use size_t for counts and add early exit when no chars to emitCasting out_size to int can overflow on very large buffers, unexpectedly producing 0 chars. Use size_t for all char-count math and return early when chars == 0. Behavior remains identical otherwise.
- int chars = (bits + (BITS_PER_HEX_CHAR - 1)) / BITS_PER_HEX_CHAR; - if (chars > GM_OID_HEX_CHARS) chars = GM_OID_HEX_CHARS; - if ((size_t)chars >= out_size) { - /* clamp to buffer, still try to fill */ - chars = (int)out_size - 1; - if (chars < 0) chars = 0; - } + size_t chars = (size_t)((bits + (BITS_PER_HEX_CHAR - 1)) / BITS_PER_HEX_CHAR); + if (chars > (size_t)GM_OID_HEX_CHARS) chars = (size_t)GM_OID_HEX_CHARS; + if (chars >= out_size) { + /* clamp to buffer, still try to fill */ + chars = out_size - 1; + } @@ - if (chars > (GM_CACHE_MAX_SHARD_PATH - 1)) { - chars = GM_CACHE_MAX_SHARD_PATH - 1; - } + if (chars > (size_t)(GM_CACHE_MAX_SHARD_PATH - 1)) { + chars = (size_t)(GM_CACHE_MAX_SHARD_PATH - 1); + } + if (chars == 0) { + return GM_OK; + } @@ - for (int i = 0; i < chars; ++i) { + for (size_t i = 0; i < chars; ++i) { out[i] = hex[i]; } out[chars] = '\0';Please confirm unit tests cover very large out_size values to prevent regressions.
Also applies to: 34-37, 39-43
core/src/adapters/logging/stdio_logger_adapter.c (2)
4-4: Ensure stdio types are declaredThis file uses FILE and fprintf. If stdio_logger_adapter.h does not include <stdio.h>, compilation will fail here.
#include "stdio_logger_adapter.h" +#include <stdio.h> #include "gitmind/error.h" #include "gitmind/result.h"If the header already includes <stdio.h>, this remains harmless. Please confirm.
35-41: Make UTC time formatting portable (Windows vs POSIX)gmtime_r isn’t available on MSVC. Add a guarded fallback to gmtime_s (Windows) to keep builds portable.
- struct tm tm; - gmtime_r(&now, &tm); + struct tm tm; +#if defined(_WIN32) + gmtime_s(&tm, &now); +#else + gmtime_r(&now, &tm); +#endifOptional: if logs can be emitted concurrently, consider a simple mutex around fprintf to avoid interleaving lines.
apps/cli/main.md (2)
92-95: Clarify accepted values for GITMIND_SAFETYSpecify all accepted values (e.g., off/0/false vs on/1/true) and default behavior to avoid ambiguity in CI/scripts.
253-261: Add a quick JSON logging usage exampleA one-liner improves discoverability. Example:
GITMIND_LOG_FORMAT=json git-mind list 2> logs.json && jq . < logs.jsonAlso applies to: 262-281
core/tests/fakes/logging/fake_logger_port.c (1)
13-25: Test fake looks solidCaptures entries safely and disposes correctly. Good test double.
Optional: expose the 32-entry capacity as a public constant in the fake header to avoid magic numbers in tests.
Also applies to: 31-46, 48-53
core/tests/unit/test_diagnostics_port.c (2)
20-29: Guard against zero-sized buffers in sr_repo_path; prefer snprintf for safetyIf out_size == 0, writing out[i] = '\0' is UB. Simplify and harden the copy.
Apply:
static gm_result_void_t sr_repo_path(void *self, gm_git_repository_path_kind_t kind, char *out, size_t out_size) { (void)self; (void)kind; - /* Any path; fake fs will report not found for canonicalize in prep, but we - * want tree-build failure path in commit stage; use a valid-ish path. */ + /* Use a valid-ish path; fake FS canonicalization will fail during prep. */ const char *p = "/fake/state/.git"; - size_t i = 0; while (p[i] && i + 1 < out_size) { out[i] = p[i]; ++i; } - out[i] = '\0'; - return gm_ok_void(); + if (out_size == 0) { + return gm_err_void(GM_ERROR(GM_ERR_INVALID_ARGUMENT, "buffer too small")); + } + (void)snprintf(out, out_size, "%s", p); + return gm_ok_void(); }
17-18: Align comments with actual failure pathTop comment says “fail tree build,” but the execution path intentionally fails during prep via fake FS. Update wording for consistency.
Also applies to: 57-59
core/tests/unit/test_telemetry_cfg.c (2)
8-11: Make set_env portable (_putenv on Windows) and keep heap lifetimeOn Windows, putenv is _putenv; current code will likely fail to build. Wrap per-platform.
Apply:
-#ifndef _WIN32 -extern int putenv(char *); -#endif +#ifdef _WIN32 +extern int _putenv(const char *); +#else +extern int putenv(char *); +#endif @@ static void set_env(const char *k, const char *v) { /* Portable: use putenv with a heap string that must persist */ char buf[256]; if (v == NULL) v = ""; /* empty resets to default behavior */ int n = 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); - assert(putenv(heap) == 0); + /* do not free(heap); it must persist after putenv/_putenv */ +#ifdef _WIN32 + assert(_putenv(heap) == 0); +#else + assert(putenv(heap) == 0); +#endif }Also applies to: 14-24
43-48: Loosen exact tag ordering to reduce brittlenessOrder could evolve; assert presence instead of exact equality.
Apply:
- assert(strcmp(tags, "branch=main,mode=full") == 0); + assert(strstr(tags, "branch=main") != NULL); + assert(strstr(tags, "mode=full") != NULL);core/tests/fakes/diagnostics/fake_diagnostics_port.h (1)
15-21: Replace magic numbers with named constants for clarity and reuseIntroduce size macros to document limits and ease future changes.
Apply:
+/* Tunable limits for the fake diagnostics sink */ +#define GM_FAKE_DIAG_MAX_EVENTS 64 +#define GM_FAKE_DIAG_MAX_KVS_PER_EVENT 8 +#define GM_FAKE_DIAG_MAX_COMPONENT 32 +#define GM_FAKE_DIAG_MAX_EVENT 64 +#define GM_FAKE_DIAG_MAX_KEY 32 +#define GM_FAKE_DIAG_MAX_VALUE 64 + typedef struct { - struct { char component[32]; char event[64]; } meta[64]; - struct { char key[32]; char value[64]; } kvs[64][8]; - size_t kv_counts[64]; + struct { char component[GM_FAKE_DIAG_MAX_COMPONENT]; char event[GM_FAKE_DIAG_MAX_EVENT]; } meta[GM_FAKE_DIAG_MAX_EVENTS]; + struct { char key[GM_FAKE_DIAG_MAX_KEY]; char value[GM_FAKE_DIAG_MAX_VALUE]; } kvs[GM_FAKE_DIAG_MAX_EVENTS][GM_FAKE_DIAG_MAX_KVS_PER_EVENT]; + size_t kv_counts[GM_FAKE_DIAG_MAX_EVENTS]; size_t count; } gm_fake_diag_state_t;core/src/ports/cache/cache_query_port.c (1)
16-31: Zero/initialize outputs on failure paths for safety.Consider clearing gm_cache_result_t on entry and/or before returning errors so callers never read garbage on failures.
Example pattern:
- memset(out, 0, sizeof(*out)) before delegating
- Ensure out remains zeroed when returning gm_err_void
As per coding guidelines
Also applies to: 33-48
core/src/domain/journal/codec.c (1)
22-31: Confirm base64 variant choice (ORIGINAL vs URL-safe).You’re using sodium_base64_VARIANT_ORIGINAL. If the transport requires URL-safe strings, switch to sodium_base64_VARIANT_URLSAFE[_NO_PADDING] for both encode and decode.
If URL-safe is desired:
- const int variant = sodium_base64_VARIANT_ORIGINAL; + const int variant = sodium_base64_VARIANT_URLSAFE_NO_PADDING;Apply the same variant in gm_journal_decode_message. As per coding guidelines
Also applies to: 39-54
core/tests/unit/test_journal_e2e_libgit2.c (1)
46-55: Use GM_PATH_MAX for paths and check IO results.Replace fixed-size 1024 buffers with GM_PATH_MAX and verify fwrite returns expected bytes.
- char a_path[1024]; - char b_path[1024]; + char a_path[GM_PATH_MAX]; + char b_path[GM_PATH_MAX];And in write_file:
- size_t n = fwrite(content, 1, strlen(content), f); - (void)n; + size_t n = fwrite(content, 1, strlen(content), f); + assert(n == strlen(content));As per coding guidelines
core/src/domain/journal/read_decoder.c (1)
13-45: Harden conversion and simplify with gm_ helpers*Zero-init the destination and use safe copy helpers to avoid manual loops/strlen assumptions and to keep consistency with convert_attributed_to_basic.
-static void convert_legacy_to_attributed(const gm_edge_t *legacy, - gm_edge_attributed_t *attributed) { - for (size_t i = 0; i < GM_SHA1_SIZE; i++) { - attributed->src_sha[i] = legacy->src_sha[i]; - attributed->tgt_sha[i] = legacy->tgt_sha[i]; - } - attributed->src_oid = legacy->src_oid; - attributed->tgt_oid = legacy->tgt_oid; - attributed->rel_type = legacy->rel_type; - attributed->confidence = legacy->confidence; - attributed->timestamp = legacy->timestamp; - size_t src_len = strlen(legacy->src_path); - if (src_len >= sizeof(attributed->src_path)) src_len = sizeof(attributed->src_path) - 1U; - for (size_t i = 0; i < src_len; i++) { attributed->src_path[i] = legacy->src_path[i]; } - attributed->src_path[src_len] = '\0'; - size_t tgt_len = strlen(legacy->tgt_path); - if (tgt_len >= sizeof(attributed->tgt_path)) tgt_len = sizeof(attributed->tgt_path) - 1U; - for (size_t i = 0; i < tgt_len; i++) { attributed->tgt_path[i] = legacy->tgt_path[i]; } - attributed->tgt_path[tgt_len] = '\0'; - size_t ulid_len = strlen(legacy->ulid); - if (ulid_len >= sizeof(attributed->ulid)) ulid_len = sizeof(attributed->ulid) - 1U; - for (size_t i = 0; i < ulid_len; i++) { attributed->ulid[i] = legacy->ulid[i]; } - attributed->ulid[ulid_len] = '\0'; - attributed->attribution.source_type = GM_SOURCE_HUMAN; - attributed->attribution.author[0] = '\0'; - attributed->attribution.session_id[0] = '\0'; - attributed->attribution.flags = 0; - attributed->lane = GM_LANE_DEFAULT; -} +static void convert_legacy_to_attributed(const gm_edge_t *legacy, + gm_edge_attributed_t *attributed) { + gm_memset_safe(attributed, sizeof(*attributed), 0, sizeof(*attributed)); + (void)gm_memcpy_span(attributed->src_sha, GM_SHA1_SIZE, legacy->src_sha, GM_SHA1_SIZE); + (void)gm_memcpy_span(attributed->tgt_sha, GM_SHA1_SIZE, legacy->tgt_sha, GM_SHA1_SIZE); + attributed->src_oid = legacy->src_oid; + attributed->tgt_oid = legacy->tgt_oid; + attributed->rel_type = legacy->rel_type; + attributed->confidence = legacy->confidence; + attributed->timestamp = legacy->timestamp; + (void)gm_strcpy_safe(attributed->src_path, sizeof attributed->src_path, legacy->src_path); + (void)gm_strcpy_safe(attributed->tgt_path, sizeof attributed->tgt_path, legacy->tgt_path); + (void)gm_strcpy_safe(attributed->ulid, sizeof attributed->ulid, legacy->ulid); + attributed->attribution.source_type = GM_SOURCE_HUMAN; + attributed->attribution.flags = 0; + attributed->lane = GM_LANE_DEFAULT; +}core/src/journal/reader.c (2)
29-41: Minor: duplicate includegitmind/constants_internal.h is included twice (Line 10 and Line 39). Drop the second include.
325-352: Prefer monotonic wall-clock for durationsclock() measures CPU time; journal read is I/O bound. Use a monotonic wall-clock (e.g., clock_gettime(CLOCK_MONOTONIC)/timespec_get or platform abstraction) for accurate durations.
- clock_t st = clock(); - int rc_walk = walk_journal_generic(&rctx, ref_name); - uint64_t dur_ms = (uint64_t)((clock() - st) / CLOCKS_PER_MS); + gm_timepoint_t st = gm_time_now_monotonic(); + int rc_walk = walk_journal_generic(&rctx, ref_name); + uint64_t dur_ms = gm_time_elapsed_ms(st, gm_time_now_monotonic());core/src/app/cache/cache_rebuild_service.c (2)
175-179: Propagate underlying error from gm_cache_oid_prefix for better diagnosticsReturn the exact error from gm_cache_oid_prefix instead of GM_ERR_INVALID_STATE.
- if (gm_cache_oid_prefix(oid, ctx->shard_bits, prefix, sizeof(prefix)) != - GM_OK) { - return GM_ERR_INVALID_STATE; - } + int pcode = gm_cache_oid_prefix(oid, ctx->shard_bits, prefix, sizeof(prefix)); + if (pcode != GM_OK) { + return pcode; + }
346-350: Remove duplicate error check after build_tree_from_tempThe second result_code != GM_OK check is redundant and unreachable.
int result_code = build_tree_from_temp(port, inputs->temp_dir->path, &tree_oid); - if (result_code != GM_OK) { - /* Emit via diagnostics port when available. We don't have ctx here, so skip. */ - return result_code; - } - if (result_code != GM_OK) { - return result_code; - } + if (result_code != GM_OK) { + /* Emit via diagnostics port when available. We don't have ctx here, so skip. */ + return result_code; + }Also applies to: 357-361
core/src/telemetry/config.c (2)
23-61: Use a portable case-insensitive compare wrapperDirect strcasecmp ties you to POSIX; Windows builds will fail. Prefer a project-provided wrapper (e.g., gm_strcasecmp/gm_ascii_strcasecmp) if available, or add a small shim.
Please confirm the available string-compare helper in gitmind/security/string.h and switch to it for parse_bool_default_true, parse_repo_tag_mode, parse_log_level, parse_log_format, and parse_hash_algo_sha256.
251-287: Handle long repo paths in PLAIN mode (avoid silent drop)repo_val[65] truncates or drops tags for long canonical paths. Either size this buffer generously or fall back to HASH when copy fails.
For example:
- Enlarge repo_val (e.g., 256 or GM_PATH_MAX if available), or
- Check gm_strcpy_safe result; if not GM_OK, compute a short hash instead so the repo tag is still emitted.
core/src/journal/writer.c (4)
321-333: Confirm ownership of repo_canon; free if allocatedgm_fs_temp_port_canonicalize_ex likely allocates repo_canon. It’s never freed here, which would leak per append call.
If caller-owns, free it after tag building (e.g., gm_fs_temp_port_free(ctx->fs_temp_port, repo_canon) or equivalent).
39-41: Use a monotonic wall-clock for durationclock() measures CPU time on many platforms; it’s not suitable for elapsed wall-clock. Prefer a monotonic clock (e.g., clock_gettime(CLOCK_MONOTONIC) or a project helper).
Example sketch:
- Capture t0 = monotonic_now_ms();
- Capture t1 = monotonic_now_ms();
- dur_ms = t1 - t0;
Also applies to: 364-370
157-168: Check whether commit plan needs disposalIf gm_journal_build_commit_plan allocates (e.g., parents/message buffers), ensure plan resources are released after commit, beyond freeing message.
If an explicit disposer exists (e.g., gm_journal_commit_plan_dispose(&plan)), call it after commit_result handling.
200-253: Note: potential unreachable commit on ref update raceOn ALREADY_EXISTS retry, a previous commit object remains unreachable. Likely acceptable, but if you want to avoid object buildup, consider a CAS-style reference update (expected OID) to fail fast before creating the commit.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (106)
.github/workflows/auto-seed-review.yml(0 hunks).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/cache/internal/oid_prefix.h(1 hunks)core/include/gitmind/cache/internal/staleness.h(1 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/cache_build_port.h(1 hunks)core/include/gitmind/ports/cache_query_port.h(1 hunks)core/include/gitmind/ports/diagnostic_port.h(1 hunks)core/include/gitmind/ports/journal_command_port.h(1 hunks)core/include/gitmind/ports/logger_port.h(1 hunks)core/include/gitmind/ports/metrics_port.h(1 hunks)core/include/gitmind/telemetry/internal/config.h(1 hunks)core/include/gitmind/telemetry/internal/log_format.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/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/app/cache/cache_rebuild_service.c(10 hunks)core/src/cache/query.c(2 hunks)core/src/domain/cache/oid_prefix.c(1 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/src/ports/cache/cache_build_port.c(1 hunks)core/src/ports/cache/cache_query_port.c(1 hunks)core/src/ports/journal/journal_command_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/diagnostics/fake_diagnostics_port.h(1 hunks)core/tests/fakes/git/fake_git_repository_port.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/support/temp_repo_helpers.h(1 hunks)core/tests/unit/test_cache_branch_limits.c(3 hunks)core/tests/unit/test_cache_meta.c(3 hunks)core/tests/unit/test_cache_oid_prefix.c(1 hunks)core/tests/unit/test_cache_query.c(3 hunks)core/tests/unit/test_cache_telemetry_emit.c(1 hunks)core/tests/unit/test_cache_tree_size.c(3 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_mixed_cbor.c(3 hunks)core/tests/unit/test_journal_nff_retry.c(1 hunks)core/tests/unit/test_journal_port.c(1 hunks)core/tests/unit/test_journal_port_append_flow.c(1 hunks)core/tests/unit/test_log_formatter.c(1 hunks)core/tests/unit/test_repo_build_tree.c(1 hunks)core/tests/unit/test_telemetry_cfg.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/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/hexagonal/Journal.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/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/PR173/17ddad1aa3747f40ecacc0bd202e7b83edbf1d1d.md(0 hunks)docs/code-reviews/PR173/7445bcf00c71b6f768e9eb6b1fd99d2b1c60783f.md(0 hunks)docs/code-reviews/PR173/e0243058aac14ff3a8c83af2f6d4dcfe97dd2804.md(0 hunks)docs/code-reviews/PR178/1a49c6f455c33f1dcf87905f804020b6654be938.md(3 hunks)docs/code-reviews/PR178/9b1f53d013ae6aa4f30edf4dea8dc4ddf7de16bc.md(1 hunks)docs/code-reviews/PR178/a39bfee82cb78194ff33fc588d43321c98d9cc8c.md(3 hunks)docs/code-reviews/PR178/e1215ee158d04aca9718a9e775212be3b70a15f8.md(3 hunks)docs/operations/Diagnostics_Events.md(1 hunks)docs/operations/Observability.md(1 hunks)docs/operations/Telemetry_Config.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(6 hunks)migration-progress.md(1 hunks)tools/ci/ci_local.sh(3 hunks)
💤 Files with no reviewable changes (14)
- docs/architecture/attribution-system.md
- docs/code-reviews/PR173/e0243058aac14ff3a8c83af2f6d4dcfe97dd2804.md
- .github/workflows/auto-seed-review.yml
- docs/cli/gitmind-list.md
- docs/cli/gitmind-cache-rebuild.md
- docs/architecture/attribution-use-cases.md
- docs/cli/README.md
- docs/cli/combined_markdown.md
- docs/code-reviews/PR173/7445bcf00c71b6f768e9eb6b1fd99d2b1c60783f.md
- docs/architecture/attribution-integration-guide.md
- docs/code-reviews/PR173/17ddad1aa3747f40ecacc0bd202e7b83edbf1d1d.md
- docs/cli/gitmind.md
- docs/cli/gitmind-link.md
- docs/cli/gitmind-install-hooks.md
🧰 Additional context used
🧠 Learnings (19)
📚 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/unit/test_cache_query.capps/cli/main.ccore/src/adapters/git/libgit2_repository_port.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/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 .coderabbit.yml : Maintain CodeRabbit config for summary-first reviews and limited per-line comments on docs
Applied to files:
docs/code-reviews/PR178/a39bfee82cb78194ff33fc588d43321c98d9cc8c.mddocs/code-reviews/PR178/e1215ee158d04aca9718a9e775212be3b70a15f8.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/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
📚 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.cAGENTS.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 include/gitmind.h : Maintain umbrella API at include/gitmind.h
Applied to files:
AGENTS.mdcore/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 {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-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:
AGENTS.mdmeson.build
📚 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
📚 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/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/context.hmeson.build
…ore/temp-port-ci-copy
|
Added debug-container workflow tooling and canonicalize regression coverage. Latest run: |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/ci/ci_local.sh (1)
31-41: Require explicit host-build override instead of auto-fallbackTo align with the “Docker-only unless explicitly overridden” policy, fail when Docker is absent unless GITMIND_ALLOW_HOST_BUILD=1 is set by the user, rather than exporting it automatically.
Based on learnings
-if ! command -v docker >/dev/null 2>&1; then - echo "⚠️ Docker not found. Falling back to host build (override guard)." - export GITMIND_ALLOW_HOST_BUILD=1 +if ! command -v docker >/dev/null 2>&1; then + if [ "${GITMIND_ALLOW_HOST_BUILD:-}" != "1" ]; then + echo "❌ Docker not found. Set GITMIND_ALLOW_HOST_BUILD=1 to run a host build at your own risk." + exit 1 + fi + echo "⚠️ Docker not found. Proceeding with host build (explicit override)." CC=${CC:-clang} echo "==> Host build with $CC" rm -rf build-local CC=$CC meson setup build-local ninja -C build-local ninja -C build-local test exit 0 ficore/src/telemetry/config.c (1)
272-291: Dispose the crypto context before returning
gm_crypto_context_createhands back a context that must be released.sha256_hex12exits on every path without callinggm_crypto_context_dispose, so each invocation leaks the backend resources. Please dispose the context whether the hash succeeds or we fall back to FNV.static void sha256_hex12(const uint8_t *data, size_t len, char *out12) { uint8_t digest[GM_SHA256_DIGEST_SIZE]; gm_result_crypto_context_t cr = gm_crypto_context_create(gm_crypto_backend_libsodium()); if (!cr.ok) { /* Fallback to FNV on any error */ fnv1a64_hex12(data, len, out12); return; } - gm_result_void_t hr = gm_sha256_with_context(&cr.u.val, data, len, digest); + gm_result_void_t hr = gm_sha256_with_context(&cr.u.val, data, len, digest); if (!hr.ok) { if (hr.u.err) gm_error_free(hr.u.err); + gm_crypto_context_dispose(&cr.u.val); fnv1a64_hex12(data, len, out12); return; } + gm_crypto_context_dispose(&cr.u.val); static const char HEX[] = "0123456789abcdef"; for (int i = 0; i < 6; ++i) { out12[2 * i] = HEX[(digest[i] >> 4) & 0xF]; out12[2 * i + 1] = HEX[digest[i] & 0xF]; } out12[12] = '\0'; }
🧹 Nitpick comments (12)
tools/ci/ci_local.sh (1)
72-88: Rsync filter: ensure required CI bits can’t be lost to .gitignoreIf any needed paths (e.g., .ci/, quality/) are ignored locally, the rsync will drop them. Consider explicit includes as a guard.
rsync -a --delete \ --include '/core/' \ --include '/core/***' \ + --include '/.ci/' \ + --include '/.ci/***' \ + --include '/quality/' \ + --include '/quality/***' \ --filter=':- .gitignore' \ --exclude '/build-local/' \ --exclude '/ci_logs.zip' \ --exclude '/clang-tidy-report.txt' \ --exclude '/clang-tidy-report-full.txt' \ "$ROOT_DIR"/ "$WORKSPACE_COPY"/core/src/domain/journal/read_decoder.c (1)
45-48: Use sizeof(field) for consistencyPrefer sizeof(basic->src_path)/sizeof(basic->ulid) over GM_PATH_MAX/GM_ULID_SIZE+1 to avoid drift if struct definitions change.
- (void)gm_strcpy_safe(basic->src_path, GM_PATH_MAX, aedge->src_path); - (void)gm_strcpy_safe(basic->tgt_path, GM_PATH_MAX, aedge->tgt_path); - (void)gm_strcpy_safe(basic->ulid, GM_ULID_SIZE + 1, aedge->ulid); + (void)gm_strcpy_safe(basic->src_path, sizeof(basic->src_path), aedge->src_path); + (void)gm_strcpy_safe(basic->tgt_path, sizeof(basic->tgt_path), aedge->tgt_path); + (void)gm_strcpy_safe(basic->ulid, sizeof(basic->ulid), aedge->ulid);tools/debug/debug-container.sh (2)
66-81: Handle absence of rsync (tar fallback like ci_local.sh)Mirror the tar-based fallback to avoid hard dependency on rsync.
-if [[ ${RESYNC} -eq 1 ]]; then +if [[ ${RESYNC} -eq 1 ]]; then echo "==> Syncing workspace into ${STAGE_DIR}" >&2 - rsync -a --delete \ - --filter=':- .gitignore' \ - --exclude '/build/' \ - --exclude '/build-asan/' \ - --exclude '/build-debug/' \ - --exclude '/build-local/' \ - --exclude '/ci_logs.zip' \ - --exclude '/clang-tidy-report.txt' \ - --exclude '/clang-tidy-report-full.txt' \ - "${ROOT_DIR}/" "${STAGE_DIR}/" + if command -v rsync >/dev/null 2>&1; then + rsync -a --delete \ + --filter=':- .gitignore' \ + --exclude '/build/' \ + --exclude '/build-asan/' \ + --exclude '/build-debug/' \ + --exclude '/build-local/' \ + --exclude '/ci_logs.zip' \ + --exclude '/clang-tidy-report.txt' \ + --exclude '/clang-tidy-report-full.txt' \ + "${ROOT_DIR}/" "${STAGE_DIR}/" + else + (cd "${ROOT_DIR}" && tar -cf - . \ + --exclude='./build' \ + --exclude='./build-asan' \ + --exclude='./build-debug' \ + --exclude='./build-local' \ + --exclude='./ci_logs.zip' \ + --exclude='./clang-tidy-report.txt' \ + --exclude='./clang-tidy-report-full.txt') | (cd "${STAGE_DIR}" && tar -xf -) + fi
90-98: Sanitize/quote package list to avoid command injectionINSTALL_PKGS is interpolated into a shell command. Quote or validate tokens (alnum, dash, plus) before composing CONTAINER_ENTRY.
- INSTALL_SNIPPET="apt-get update >/dev/null && apt-get install -y --no-install-recommends ${INSTALL_PKGS} >/dev/null && " + # split and safely quote packages + read -r -a PKG_ARR <<<"${INSTALL_PKGS}" + SAFE_PKGS=() + for p in "${PKG_ARR[@]}"; do + if [[ "${p}" =~ ^[A-Za-z0-9.+-]+$ ]]; then + SAFE_PKGS+=("${p}") + else + echo "Invalid package name: ${p}" >&2; exit 1 + fi + done + INSTALL_SNIPPET="apt-get update >/dev/null && apt-get install -y --no-install-recommends ${SAFE_PKGS[*]} >/dev/null && "core/tests/support/temp_repo_helpers.c (2)
47-79: Windows path separators not handled in mkdir recursionParent derivation uses '/' only. On Windows, paths use backslashes. Consider using a cross-platform path helper or checking both separators.
- char *slash = strrchr(parent, '/'); + char *slash = strrchr(parent, '/'); +#ifdef _WIN32 + char *bslash = strrchr(parent, '\\'); + if (bslash != NULL && (slash == NULL || bslash > slash)) slash = bslash; +#endif
219-231: Cleanup staged temp dir on template copy failureIf gm_test_copy_tree fails, the created temp dir remains. Clean it up to avoid clutter.
- gm_result_void_t copy_result = gm_test_copy_tree(template_path, out_path); - if (!copy_result.ok) { - return copy_result; - } + gm_result_void_t copy_result = gm_test_copy_tree(template_path, out_path); + if (!copy_result.ok) { + (void)gm_fs_temp_port_remove_tree(port, out_path); + return copy_result; + }core/tests/fakes/diagnostics/fake_diagnostics_port.c (1)
72-80: Solid dispose path; add fallback free for degenerate stateLifecycle wiring is correct. As a hardening tweak, free self when vtbl is NULL to avoid leaks if the struct is partially initialized.
void gm_fake_diag_port_dispose(gm_diagnostics_port_t *port) { if (port == NULL) { return; } if (port->vtbl == &VTBL) { dispose_impl(port->self); } else if (port->vtbl != NULL && port->vtbl->dispose != NULL) { port->vtbl->dispose(port->self); + } else if (port->vtbl == NULL && port->self != NULL) { + /* Degenerate case: free allocated state */ + free(port->self); } port->self = NULL; port->vtbl = NULL; }Also applies to: 98-106
core/src/journal/writer.c (2)
45-62: Centralize monotonic_ms_now to avoid duplicationThis implementation mirrors reader.c. Consider moving to a shared utility (e.g., gitmind/util/time.h) to prevent drift across modules and ease platform fixes.
63-66: Remove unused macroCLOCKS_PER_MS is no longer used after switching to monotonic timing.
-#define CLOCKS_PER_MS \ - ((clock_t)((CLOCKS_PER_SEC + (MILLIS_PER_SECOND - 1)) / MILLIS_PER_SECOND))core/src/adapters/git/libgit2_repository_port.c (1)
914-931: Use normalized ref name for creation for consistencyDirectories are created using norm_ref; pass norm_ref to git_reference_create to keep paths and logs consistent.
- int create_status = git_reference_create(&created, state->repo, - spec->ref_name, spec->target_oid, + int create_status = git_reference_create(&created, state->repo, + norm_ref, spec->target_oid, false, spec->log_message);Also applies to: 945-954
core/tests/support/temp_repo_helpers.h (2)
17-20: Verify line length compliance and run clang-format.Line 17 appears to exceed the 80-column limit specified in the coding guidelines. Please run
clang-formaton this file to ensure consistent formatting.You can verify line lengths with:
#!/bin/bash # Check for lines exceeding 80 columns awk 'length > 80 {print NR": "length" chars: "$0}' core/tests/support/temp_repo_helpers.h
31-37: Consider adding brief documentation comments.While the function names are descriptive, brief doc comments would help developers understand the contract, especially regarding:
- Expected
componentformat/constraints- Ownership of the created temporary directory
- When/how to call cleanup
Example format:
+/** + * Creates a temporary repository directory using the configured provider. + * @param port Filesystem temp port to use + * @param component Component name for the temp directory (e.g., "test_foo") + * @param out_path Buffer to receive the created path + * @param out_size Size of out_path buffer + * @return OK on success, error otherwise + */ gm_result_void_t gm_test_make_temp_repo_dir(const gm_fs_temp_port_t *port, const char *component, char *out_path, size_t out_size); +/** + * Removes a temporary repository directory created by gm_test_make_temp_repo_dir. + * @param port Filesystem temp port to use + * @param path Path to remove (must match a prior make_temp_repo_dir output) + * @return OK on success, error otherwise + */ gm_result_void_t gm_test_cleanup_temp_repo_dir(const gm_fs_temp_port_t *port, const char *path);Otherwise, LGTM!
The make/cleanup API pair is well-designed for safe resource management in tests.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
.githooks/pre-push(0 hunks).github/workflows/apply-feedback.yml(0 hunks)apps/cli/README.md(1 hunks)apps/cli/main.c(3 hunks)apps/cli/main.md(2 hunks)core/include/gitmind/adapters/diagnostics/stderr_diagnostics_adapter.h(1 hunks)core/include/gitmind/adapters/logging/stdio_logger_adapter.h(1 hunks)core/include/gitmind/cache/internal/oid_prefix.h(1 hunks)core/include/gitmind/context.h(1 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(2 hunks)core/include/gitmind/ports/fs_temp_port.h(2 hunks)core/include/gitmind/telemetry/internal/config.h(1 hunks)core/include/gitmind/telemetry/internal/log_format.h(1 hunks)core/include/gitmind/telemetry/log_format.h(1 hunks)core/src/adapters/diagnostics/stderr_diagnostics_adapter.c(2 hunks)core/src/adapters/git/libgit2_repository_port.c(2 hunks)core/src/adapters/logging/stdio_logger_adapter.c(2 hunks)core/src/app/cache/cache_rebuild_service.c(2 hunks)core/src/domain/cache/oid_prefix.c(1 hunks)core/src/domain/journal/append_planner.c(1 hunks)core/src/domain/journal/codec.c(2 hunks)core/src/domain/journal/read_decoder.c(2 hunks)core/src/journal/reader.c(5 hunks)core/src/journal/writer.c(4 hunks)core/src/ports/cache/cache_query_port.c(3 hunks)core/src/telemetry/config.c(3 hunks)core/src/telemetry/log_format.c(5 hunks)core/tests/fakes/diagnostics/fake_diagnostics_port.c(2 hunks)core/tests/fakes/diagnostics/fake_diagnostics_port.h(1 hunks)core/tests/integration/test_cache_branch_limits.c(1 hunks)core/tests/integration/test_cache_query.c(1 hunks)core/tests/integration/test_cache_rebuild_canonicalize.c(1 hunks)core/tests/support/temp_repo_helpers.c(1 hunks)core/tests/support/temp_repo_helpers.h(1 hunks)core/tests/unit/support/temp_repo_helpers.h(1 hunks)core/tests/unit/test_cache_oid_prefix.c(1 hunks)core/tests/unit/test_diagnostics_port.c(1 hunks)core/tests/unit/test_journal_nff_retry.c(1 hunks)core/tests/unit/test_log_formatter.c(2 hunks)core/tests/unit/test_telemetry_cfg.c(3 hunks)docs/activity/2025-10-09_pr_feedback.md(1 hunks)docs/architecture/fs_temp_service_plan.md(1 hunks)docs/operations/Debug_Container.md(1 hunks)docs/operations/Diagnostics_Events.md(1 hunks)docs/operations/Telemetry_Config.md(2 hunks)meson.build(7 hunks)tools/ci/ci_local.sh(3 hunks)tools/debug/debug-container.sh(1 hunks)
💤 Files with no reviewable changes (2)
- .githooks/pre-push
- .github/workflows/apply-feedback.yml
✅ Files skipped from review due to trivial changes (4)
- docs/operations/Debug_Container.md
- core/tests/unit/support/temp_repo_helpers.h
- core/include/gitmind/ports/fs_temp_port.h
- core/include/gitmind/telemetry/internal/config.h
🚧 Files skipped from review as they are similar to previous changes (11)
- core/tests/unit/test_diagnostics_port.c
- core/tests/unit/test_log_formatter.c
- core/include/gitmind/cache/internal/oid_prefix.h
- apps/cli/README.md
- core/include/gitmind/journal/internal/append_plan.h
- core/src/ports/cache/cache_query_port.c
- core/include/gitmind/ports/diagnostic_port.h
- core/src/adapters/logging/stdio_logger_adapter.c
- core/src/domain/journal/codec.c
- docs/operations/Diagnostics_Events.md
- core/tests/fakes/diagnostics/fake_diagnostics_port.h
🧰 Additional context used
📓 Path-based instructions (14)
{docs/**/*.md,README.md}
📄 CodeRabbit inference engine (AGENTS.md)
Docs must have front matter, a single H1, a '## Table of Contents', and titles matching the H1; API docs include api_version
Files:
docs/architecture/fs_temp_service_plan.mddocs/operations/Telemetry_Config.mddocs/activity/2025-10-09_pr_feedback.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Markdown must satisfy repository markdownlint configuration (.markdownlint.jsonc); use make md-lint/md-fix
Files:
docs/architecture/fs_temp_service_plan.mdapps/cli/main.mddocs/operations/Telemetry_Config.mddocs/activity/2025-10-09_pr_feedback.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/journal/internal/codec.hcore/include/gitmind/adapters/diagnostics/stderr_diagnostics_adapter.hcore/tests/unit/test_journal_nff_retry.ccore/src/app/cache/cache_rebuild_service.ccore/include/gitmind/telemetry/internal/log_format.hcore/tests/fakes/diagnostics/fake_diagnostics_port.ccore/tests/unit/test_cache_oid_prefix.ccore/include/gitmind/telemetry/log_format.hcore/tests/integration/test_cache_rebuild_canonicalize.capps/cli/main.ccore/src/domain/journal/read_decoder.ccore/src/domain/cache/oid_prefix.ccore/tests/integration/test_cache_query.ccore/src/journal/reader.ccore/tests/support/temp_repo_helpers.ccore/tests/integration/test_cache_branch_limits.ccore/include/gitmind/adapters/logging/stdio_logger_adapter.hcore/src/domain/journal/append_planner.ccore/include/gitmind/context.hcore/src/telemetry/config.ccore/tests/unit/test_telemetry_cfg.ccore/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/include/gitmind/journal/internal/read_decoder.hcore/tests/support/temp_repo_helpers.hcore/src/journal/writer.ccore/src/telemetry/log_format.ccore/src/adapters/git/libgit2_repository_port.c
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Use project-safe string/memory helpers: gm_snprintf, gm_strcpy_safe, gm_memcpy_span; check return codes; treat truncation as error
Zero output buffers on error paths before formatting or copying
Prefer GM_PATH_MAX; never use GM_PATH_MAX*2; allocate if larger is required
Language rules: C23, warnings-as-errors; no VLAs; no shadowing; explicit prototypes
Formatting follows .clang-format (LLVM-based, 4 spaces, 80 cols, right-aligned pointers); run clang-format
NEVER use NOLINT or other mechanisms to suppress clang-tidy warnings; fix the warnings instead
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 and order/regroup per .clang-format
Equality is OID-first: when OIDs exist they must match; SHA fallback only when OIDs are absent
Files:
core/include/gitmind/journal/internal/codec.hcore/include/gitmind/adapters/diagnostics/stderr_diagnostics_adapter.hcore/tests/unit/test_journal_nff_retry.ccore/src/app/cache/cache_rebuild_service.ccore/include/gitmind/telemetry/internal/log_format.hcore/tests/fakes/diagnostics/fake_diagnostics_port.ccore/tests/unit/test_cache_oid_prefix.ccore/include/gitmind/telemetry/log_format.hcore/tests/integration/test_cache_rebuild_canonicalize.capps/cli/main.ccore/src/domain/journal/read_decoder.ccore/src/domain/cache/oid_prefix.ccore/tests/integration/test_cache_query.ccore/src/journal/reader.ccore/tests/support/temp_repo_helpers.ccore/tests/integration/test_cache_branch_limits.ccore/include/gitmind/adapters/logging/stdio_logger_adapter.hcore/src/domain/journal/append_planner.ccore/include/gitmind/context.hcore/src/telemetry/config.ccore/tests/unit/test_telemetry_cfg.ccore/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/include/gitmind/journal/internal/read_decoder.hcore/tests/support/temp_repo_helpers.hcore/src/journal/writer.ccore/src/telemetry/log_format.ccore/src/adapters/git/libgit2_repository_port.c
{include/**/*.h,core/include/**/*.h}
📄 CodeRabbit inference engine (AGENTS.md)
{include/**/*.h,core/include/**/*.h}: Public headers must be umbrella-safe: compile standalone, correct include order, stable header guards, and extern "C" guards for C++
Public headers must compile standalone (ninja -C build header-compile)
Files:
core/include/gitmind/journal/internal/codec.hcore/include/gitmind/adapters/diagnostics/stderr_diagnostics_adapter.hcore/include/gitmind/telemetry/internal/log_format.hcore/include/gitmind/telemetry/log_format.hcore/include/gitmind/adapters/logging/stdio_logger_adapter.hcore/include/gitmind/context.hcore/include/gitmind/journal/internal/read_decoder.h
{include/**/*.h,core/include/gitmind/**/*.h}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid public ABI breaks: append new public struct fields at the tail and mark deprecated fields instead of reordering/removing
Files:
core/include/gitmind/journal/internal/codec.hcore/include/gitmind/adapters/diagnostics/stderr_diagnostics_adapter.hcore/include/gitmind/telemetry/internal/log_format.hcore/include/gitmind/telemetry/log_format.hcore/include/gitmind/adapters/logging/stdio_logger_adapter.hcore/include/gitmind/context.hcore/include/gitmind/journal/internal/read_decoder.h
core/include/gitmind/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Header guards lint must pass (lint_header_guards); ensure consistent GITMIND_* guards in public headers
Files:
core/include/gitmind/journal/internal/codec.hcore/include/gitmind/adapters/diagnostics/stderr_diagnostics_adapter.hcore/include/gitmind/telemetry/internal/log_format.hcore/include/gitmind/telemetry/log_format.hcore/include/gitmind/adapters/logging/stdio_logger_adapter.hcore/include/gitmind/context.hcore/include/gitmind/journal/internal/read_decoder.h
core/tests/unit/test_*.c
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests live in core/tests/unit as test_.c, deterministic and isolated using fakes
Files:
core/tests/unit/test_journal_nff_retry.ccore/tests/unit/test_cache_oid_prefix.ccore/tests/unit/test_telemetry_cfg.c
core/src/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
core/src/**/*.{c,h}: Refs must be constructed via gm_build_ref and inputs starting with 'refs/' must be rejected per ref-name safety rules
Journal is append-only under refs/gitmind/edges/; cache under refs/gitmind/cache/ (no merges into cache)
Prefer OID-first APIs using git_oid (gm_oid_t) and compare via git_oid_cmp or gm-level helpers
Wrap all IO behind outbound ports; inject via gm_runtime_context_t; do not call external systems directly (e.g., libgit2) from domain/services
Maintain one responsibility per translation unit; split touched multi-concern files so the edited item lives alone (One-Thing rule)
Files:
core/src/app/cache/cache_rebuild_service.ccore/src/domain/journal/read_decoder.ccore/src/domain/cache/oid_prefix.ccore/src/journal/reader.ccore/src/domain/journal/append_planner.ccore/src/telemetry/config.ccore/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/src/journal/writer.ccore/src/telemetry/log_format.ccore/src/adapters/git/libgit2_repository_port.c
core/tests/fakes/**
📄 CodeRabbit inference engine (AGENTS.md)
Deterministic fakes/mocks/stubs for every port live under core/tests/fakes and are kept in sync with port capabilities
Files:
core/tests/fakes/diagnostics/fake_diagnostics_port.c
core/src/domain/**
📄 CodeRabbit inference engine (AGENTS.md)
Pure domain logic resides under core/src/domain/ respecting one-responsibility-per-translation-unit
Files:
core/src/domain/journal/read_decoder.ccore/src/domain/cache/oid_prefix.ccore/src/domain/journal/append_planner.c
meson.build
📄 CodeRabbit inference engine (AGENTS.md)
Project uses Meson with c_std set to c2x (targeting C23) and options centralized in meson_options.txt
Files:
meson.build
core/src/telemetry/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Use the injected log formatter DI seam for structured logs; adapters emit via gm_logger_port
Files:
core/src/telemetry/config.ccore/src/telemetry/log_format.c
core/src/adapters/**
📄 CodeRabbit inference engine (AGENTS.md)
Runtime adapters live under core/src/adapters//_adapter.c with matching private headers; provide factory/teardown
Files:
core/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/src/adapters/git/libgit2_repository_port.c
🧠 Learnings (8)
📚 Learning: 2025-10-09T02:29:56.534Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-09T02:29:56.534Z
Learning: Applies to {core/src/adapters/logging/**/*.c,core/include/gitmind/ports/logger_port.h} : Emit logs via gm_logger_port; format via injected formatter seam
Applied to files:
apps/cli/main.cdocs/operations/Telemetry_Config.mdcore/include/gitmind/adapters/logging/stdio_logger_adapter.hcore/include/gitmind/context.h
📚 Learning: 2025-10-09T02:29:56.534Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-09T02:29:56.534Z
Learning: Applies to core/src/telemetry/*.{c,h} : Use the injected log formatter DI seam for structured logs; adapters emit via gm_logger_port
Applied to files:
apps/cli/main.cdocs/operations/Telemetry_Config.mdcore/include/gitmind/context.h
📚 Learning: 2025-10-09T02:29:56.534Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-09T02:29:56.534Z
Learning: Applies to {core/src/adapters/libgit2/**/*.c,core/include/gitmind/ports/git_repository_port.h} : Access Git data via gm_git_repository_port (adapter-backed); avoid raw libgit2 usage outside adapters
Applied to files:
apps/cli/main.ccore/src/adapters/git/libgit2_repository_port.c
📚 Learning: 2025-09-13T03:13:54.145Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-09-13T03:13:54.145Z
Learning: Applies to include/gitmind.h : Maintain umbrella API at include/gitmind.h
Applied to files:
core/include/gitmind/context.h
📚 Learning: 2025-10-09T02:29:56.534Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-09T02:29:56.534Z
Learning: Applies to {include/**/*.h,core/include/gitmind/**/*.h} : Avoid public ABI breaks: append new public struct fields at the tail and mark deprecated fields instead of reordering/removing
Applied to files:
core/include/gitmind/context.h
📚 Learning: 2025-10-09T02:29:56.534Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-09T02:29:56.534Z
Learning: Applies to {core/src/adapters/metrics/**/*.c,core/include/gitmind/ports/metrics_port.h} : Emit metrics via gm_metrics_port; default adapter may be null sink
Applied to files:
core/include/gitmind/context.h
📚 Learning: 2025-10-09T02:29:56.534Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-09T02:29:56.534Z
Learning: Build and tests must run inside the Docker CI image (make ci-local/tools/ci/ci_local.sh); host builds only with explicit override and acceptance of risk
Applied to files:
tools/ci/ci_local.sh
📚 Learning: 2025-10-09T02:29:56.534Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-09T02:29:56.534Z
Learning: Applies to core/src/**/*.{c,h} : Refs must be constructed via gm_build_ref and inputs starting with 'refs/' must be rejected per ref-name safety rules
Applied to files:
core/src/adapters/git/libgit2_repository_port.c
🧬 Code graph analysis (16)
core/include/gitmind/journal/internal/codec.h (1)
core/src/domain/journal/codec.c (2)
gm_result_void_t(14-38)gm_result_void_t(40-57)
core/include/gitmind/adapters/diagnostics/stderr_diagnostics_adapter.h (1)
core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (2)
gm_stderr_diagnostics_port_init(78-91)gm_stderr_diagnostics_port_dispose(93-104)
core/tests/unit/test_journal_nff_retry.c (1)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
core/tests/fakes/diagnostics/fake_diagnostics_port.c (1)
core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (2)
dispose_impl(69-71)emit_impl(46-67)
core/include/gitmind/telemetry/log_format.h (1)
core/src/telemetry/log_format.c (1)
gm_log_format_render_default(94-181)
core/tests/integration/test_cache_rebuild_canonicalize.c (7)
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/adapters/fs/posix_temp_adapter.c (1)
gm_posix_fs_temp_port_create(628-653)core/tests/support/temp_repo_helpers.c (2)
gm_test_make_temp_repo_dir(239-245)gm_test_cleanup_temp_repo_dir(247-254)core/src/adapters/git/libgit2_repository_port.c (1)
gm_libgit2_repository_port_create(1131-1159)core/src/journal/writer.c (1)
gm_journal_append(466-470)core/src/cache/builder.c (1)
gm_cache_rebuild(9-19)
apps/cli/main.c (4)
core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (2)
gm_stderr_diagnostics_port_init(78-91)gm_stderr_diagnostics_port_dispose(93-104)core/src/error/error.c (1)
gm_error_free(161-178)core/src/adapters/logging/stdio_logger_adapter.c (1)
gm_stdio_logger_port_dispose(74-79)core/include/gitmind/ports/diagnostic_port.h (1)
gm_diag_reset(50-59)
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/integration/test_cache_query.c (1)
core/tests/support/temp_repo_helpers.c (1)
gm_test_cleanup_temp_repo_dir(247-254)
core/src/journal/reader.c (2)
core/src/journal/writer.c (1)
monotonic_ms_now(45-62)core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
core/tests/support/temp_repo_helpers.c (2)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)core/include/gitmind/ports/fs_temp_port.h (6)
gm_result_void_t(76-84)gm_result_void_t(86-97)gm_result_void_t(99-108)gm_result_void_t(110-124)gm_result_void_t(126-143)gm_result_void_t(145-152)
core/tests/integration/test_cache_branch_limits.c (1)
core/tests/support/temp_repo_helpers.c (1)
gm_test_cleanup_temp_repo_dir(247-254)
core/src/telemetry/config.c (1)
core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (1)
core/tests/fakes/diagnostics/fake_diagnostics_port.c (2)
emit_impl(12-70)dispose_impl(72-75)
core/tests/support/temp_repo_helpers.h (1)
core/tests/support/temp_repo_helpers.c (4)
gm_test_default_temp_repo_provider(188-232)gm_test_set_temp_repo_dir_provider(234-237)gm_test_make_temp_repo_dir(239-245)gm_test_cleanup_temp_repo_dir(247-254)
core/src/journal/writer.c (2)
core/src/journal/reader.c (1)
monotonic_ms_now(476-493)core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Compat Canaries (non-blocking) (clang-19)
- GitHub Check: Compat Canaries (non-blocking) (gcc-13)
- GitHub Check: Compat Canaries (non-blocking) (gcc-12)
- GitHub Check: Compat Canaries (non-blocking) (clang-18)
- GitHub Check: lint-and-sanitize
🔇 Additional comments (39)
core/src/domain/journal/append_planner.c (1)
11-25: LGTM! Parameter rename improves clarity.The refactoring from
empty_tree_oidtotree_oidis accurate and improves code clarity, as the function accepts any tree OID, not just empty trees. All references to the parameter have been correctly updated, and the function behavior remains unchanged.core/src/app/cache/cache_rebuild_service.c (3)
176-180: LGTM! Better error propagation.Storing the result in
prefix_rcand returning the actual error code improves diagnostics compared to masking failures with a generic error.
347-395: LGTM! Output parameter correctly implemented.The addition of
out_commit_oidallows the caller to capture the commit OID. The implementation follows best practices:
- Only written on the success path (Line 391-393)
- Properly null-checked before writing
- All error paths return early without touching the output parameter
- Call site at Line 649-651 correctly initializes the variable before passing it
540-540: LGTM! Improved semantic clarity.Renaming to
repo_canon_viewbetter conveys that this is a borrowed reference to canonicalized path data, aligning with the fact thatgm_fs_temp_port_canonicalize_exlikely returns a pointer to internal storage. Usage is consistent across the telemetry scaffolding.Also applies to: 548-549, 554-554
tools/ci/ci_local.sh (1)
55-62: Nice: cross‑platform mktemp fallback addedBSD/macOS compatibility covered with the -t fallback. LGTM.
core/include/gitmind/telemetry/internal/log_format.h (1)
7-7: Good: internal header now forwards to the public APIKeeps a stable include path while consolidating declarations in the public header. LGTM.
core/src/domain/journal/read_decoder.c (1)
68-95: Clarify output requirements in prefer_ modes*When prefer_attributed=true, out_attr is mandatory (and similarly out_basic when prefer_attributed=false). Ensure header docs reflect this contract so callers don’t expect cross-mode fallback to populate the other output.
Would you like me to update the header docstring in gitmind/journal/internal/read_decoder.h accordingly?
Also applies to: 97-123
core/tests/support/temp_repo_helpers.c (1)
239-254: API alignment: verify header declares provider setter/storageEnsure temp_repo_helpers.h declares gm_test_set_temp_repo_dir_provider and extern gm_test_temp_repo_provider_storage consistently with intended public surface; otherwise, remove the setter or mark as test-internal.
I can scan headers/usages to confirm and propose a header diff if desired.
core/include/gitmind/adapters/diagnostics/stderr_diagnostics_adapter.h (2)
8-8: LGTM: Include added for completeness.The
result.hinclude is appropriate sincegm_stderr_diagnostics_port_initreturnsgm_result_void_t. This ensures the header is self-contained and can compile standalone.
16-16: LGTM: Dispose function completes lifecycle API.The new
gm_stderr_diagnostics_port_disposefunction provides proper cleanup for the diagnostics port, complementing the existing initializer. The signature is consistent with other disposal functions in the codebase.core/tests/integration/test_cache_branch_limits.c (1)
145-145: LGTM: Centralized cleanup improves consistency.The replacement of the direct
gm_fs_temp_port_remove_treecall withgm_test_cleanup_temp_repo_diraligns with the PR's goal of centralizing temporary repository handling. The helper adds input validation and provides a consistent cleanup interface across integration tests.core/tests/unit/test_cache_oid_prefix.c (5)
30-36: LGTM: Rounding behavior tests added.The new tests verify that non-multiples of 4 bits are correctly rounded up to the next hex character boundary (5→2 chars, 7→2 chars, 9→3 chars). This validates the implementation's bit-to-hex conversion logic.
38-40: LGTM: Zero-bit edge case covered.Testing that zero bits yields an empty string ensures the implementation handles the degenerate case correctly without attempting to generate prefix characters.
42-46: LGTM: Error handling paths validated.The tests correctly verify:
- NULL oid: returns
GM_OKwith empty string (graceful degradation per lines 19-21 in oid_prefix.c)- NULL dest: returns error code (cannot write output)
- Zero-sized dest: returns error code (no space for output)
48-50: LGTM: Small buffer edge case tested.Verifying that a 2-byte buffer is properly NUL-terminated and limited to 1 hex character ensures the implementation respects buffer boundaries and prevents overflows.
52-54: LGTM: Boundary clamping verified.Testing that excessive bit counts (1024) are clamped to
GM_CACHE_MAX_SHARD_PATH - 1ensures the implementation enforces conservative maximum prefix lengths for directory names.apps/cli/main.md (2)
94-94: LGTM: Safety override options clearly documented.The enumeration of falsy (
off,false,0,no) and truthy (on,true,1, default unset) values forGITMIND_SAFETYprovides clear guidance for users who need to disable guardrails in CI/E2E environments.
259-259: LGTM: Practical JSON logging example added.The example demonstrates a complete workflow: setting the format, redirecting logs, and processing with
jq. This helps users understand how to leverage structured JSON logs in practice.core/tests/unit/test_telemetry_cfg.c (3)
8-12: LGTM: Windows portability added.The conditional compilation correctly handles the difference between Windows (
_putenv) and POSIX (putenv) environment variable APIs. The#ifdef _WIN32guard is the standard approach for Windows detection.
25-30: LGTM: Conditional putenv call is correct.The runtime call to
_putenvon Windows andputenvon other platforms matches the declarations and ensures cross-platform compatibility. The heap-allocated string is intentionally leaked per putenv semantics (the string must persist).
54-55: LGTM: Substring checks provide flexibility.Changing from exact string comparison to substring checks (
strstr) allows the tests to pass when additional tags are present, making the tests more robust to implementation changes in tag ordering or extra tag injection.core/include/gitmind/journal/internal/codec.h (1)
24-32: LGTM: Unsafe decode API fixed with capacity parameter.The addition of the
decoded_capacityparameter addresses the critical memory-safety issue flagged in the past review. The API now:
- Explicitly requires the caller to provide buffer capacity
- Documents ownership clearly ("caller owns the decoded buffer")
- Returns the actual decoded length via
decoded_lengthThe implementation (codec.c lines 39-56) validates
decoded_capacity != 0and passes it tosodium_base642bin, preventing buffer overflows.Note: This resolves the past review comment about the unsafe decode API.
core/src/domain/cache/oid_prefix.c (4)
22-24: LGTM: Type safety improved with size_t.Using
size_tfor the character count calculation and clamping eliminates potential signed/unsigned comparison issues and aligns with the buffer size type (out_sizeissize_t). The explicit cast tosize_ton Line 22 and clamping on Line 24 ensure safe arithmetic.
25-27: LGTM: Simplified boundary check.The direct
size_tcomparison againstout_sizeis clearer and safer than the previous cast-based approach. Clamping toout_size - 1correctly reserves space for the NUL terminator.
34-39: LGTM: Conservative maximum enforced with early exit.The explicit cast of
GM_CACHE_MAX_SHARD_PATH - 1tosize_tensures type consistency, and the early return whenchars == 0avoids unnecessary loop iteration. This enforces the documented directory name length limit.
41-43: LGTM: Loop iterator type aligned with size_t.Changing the loop variable from
inttosize_tmatches the type ofcharsand eliminates signed/unsigned comparison warnings, improving type safety and consistency.docs/operations/Telemetry_Config.md (3)
57-57: LGTM: Extras cap documented clearly.The documentation now explicitly states the cap of 3 slots for extra metrics tags and notes that it auto-derives from
gm_telemetry_cfg_t.extras. This sets clear expectations for users and ties the limit to the implementation.
61-62: LGTM: Deterministic JSON ordering documented.Documenting that the JSON formatter emits keys in lexical order helps users understand that the output is stable and predictable, which is important for downstream tooling and testing.
Based on learnings: Structured logs should use the injected log formatter DI seam.
87-92: LGTM: Formatter seam extension point documented.The new section clearly explains:
- Where the formatter hook lives (
gitmind/telemetry/log_format.h)- How to override it (
ctx.log_formatter = my_formatter)- What formatters receive (structured key/value list, JSON toggle)
- Expected return behavior
This provides a clear extension point for custom formatting needs.
Based on learnings: The formatter DI seam is now properly documented for adapters.
core/tests/integration/test_cache_query.c (1)
147-151: Switch to gm_test_cleanup_temp_repo_dir looks goodCentralizes temp-repo cleanup with input validation; error is freed on failure. Cleanup order remains correct.
apps/cli/main.c (3)
252-269: Diagnostics and logger wiring is robust
- Guarded init with error-freeing.
- Stores dispose hooks on success.
- Correct log-level enum usage.
284-295: Cleanup correctly disposes logger and diagnostics with safe fallbacksBalanced teardown using stored disposers; gm_diag_reset used when not set. Looks good.
14-16: Redundant include:gitmind/context.h(included at the top) already pulls intelemetry/log_format.h, so no extra include is required.Likely an incorrect or invalid review comment.
core/src/telemetry/log_format.c (1)
72-121: Deterministic JSON ordering and cleanup paths look correctInsertion sort over a small kv set is fine; ordered array is freed on all paths. Bounds checks leave space for the final NUL.
core/src/journal/writer.c (1)
342-363: Repo canonicalization lifetime and monotonic timing: LGTM
- Copying canon_view into a local buffer before deriving repo_id avoids dangling pointers.
- Monotonic timing for duration is correct and cross‑platform.
Also applies to: 398-406
core/src/adapters/git/libgit2_repository_port.c (1)
800-814: Commit signature fallback behavior is correctHandles ENOTFOUND by synthesizing a signature; surfaces other errors. Good guardrails.
core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (1)
69-76: Escape handling and disposal wiring look good
- emit_escaped safely handles control chars.
- VTBL.dispose and public dispose free state and reset pointers.
Also applies to: 93-104
core/tests/support/temp_repo_helpers.h (2)
22-22: LGTM!Exposing the provider storage as an extern symbol is appropriate for test flexibility, and the implementation properly handles initialization.
24-29: LGTM!The function signatures are well-designed and follow project conventions. The provider pattern with set/default functions offers good flexibility for test scenarios.
|
Addressed feedback: diagnostics emit guard, journal converter safety with error propagation, telemetry context disposal, mktemp portability, doc front matter/TOC, and docker-only guard in ci-local. Latest run: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/telemetry/config.c (1)
273-278: Free the error returned fromgm_crypto_context_create.If
gm_crypto_context_createfails,cr.u.erris never released, leaking agm_error_t. Please free it before falling back to FNV.- if (!cr.ok) { + if (!cr.ok) { + if (cr.u.err != NULL) { + gm_error_free(cr.u.err); + } fnv1a64_hex12(data, len, out12); return; }
♻️ Duplicate comments (1)
core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (1)
49-51: Inconsistent NULL handling compared to fake diagnostics port.The stderr adapter silently returns
gm_ok_void()whenkvs == NULLandkv_count > 0, while the fake diagnostics port (core/tests/fakes/diagnostics/fake_diagnostics_port.c:17-19) returns an error. Silent success can hide bugs in caller code.Apply this diff to match the fake port's behavior:
if (kv_count > 0U && kvs == NULL) { - return gm_ok_void(); + return gm_err_void(GM_ERROR(GM_ERR_INVALID_ARGUMENT, + "diagnostics kv list missing")); }
🧹 Nitpick comments (2)
core/src/crypto/backend.c (1)
105-112: LGTM! Consider lenient NULL handling.The disposal logic is correct and safe. The function properly validates input and clears the backend reference.
Optional: Many disposal APIs treat NULL as a no-op rather than an error, which simplifies cleanup code (callers don't need to check before disposing). Consider whether this stricter approach aligns with the project's disposal conventions, especially given similar disposal functions mentioned in the AI summary (e.g.,
gm_diag_reset,gm_stderr_diagnostics_port_dispose).If lenient NULL handling is preferred:
gm_result_void_t gm_crypto_context_dispose(gm_crypto_context_t *ctx) { - if (ctx == NULL) { - return gm_err_void(GM_ERROR(GM_ERR_INVALID_ARGUMENT, - "crypto context cannot be null")); + if (ctx != NULL) { + ctx->backend = NULL; } - ctx->backend = NULL; return gm_ok_void(); }core/include/gitmind/crypto/backend.h (1)
70-71: Add documentation for the new public API.The function declaration is correct, but it lacks documentation. Public API functions should be documented to describe their purpose, parameters, return values, and behavior (e.g., idempotency, NULL handling).
Add a documentation comment above the declaration:
+/** + * @brief Dispose of a crypto context by clearing its backend reference + * + * Clears the backend pointer in the provided context. The context structure + * itself is not freed; the caller retains ownership. + * + * @param ctx Pointer to the crypto context to dispose (must not be NULL) + * @return GM_OK on success, or GM_ERR_INVALID_ARGUMENT if ctx is NULL + */ gm_result_void_t gm_crypto_context_dispose(gm_crypto_context_t *ctx);
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/include/gitmind/crypto/backend.h(1 hunks)core/src/adapters/diagnostics/stderr_diagnostics_adapter.c(2 hunks)core/src/crypto/backend.c(1 hunks)core/src/domain/journal/read_decoder.c(2 hunks)core/src/telemetry/config.c(4 hunks)docs/activity/2025-10-09_pr_feedback.md(1 hunks)tools/ci/ci_local.sh(4 hunks)tools/debug/debug-container.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/debug/debug-container.sh
🧰 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/include/gitmind/crypto/backend.hcore/src/domain/journal/read_decoder.ccore/src/crypto/backend.ccore/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/src/telemetry/config.c
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Use project-safe string/memory helpers: gm_snprintf, gm_strcpy_safe, gm_memcpy_span; check return codes; treat truncation as error
Zero output buffers on error paths before formatting or copying
Prefer GM_PATH_MAX; never use GM_PATH_MAX*2; allocate if larger is required
Language rules: C23, warnings-as-errors; no VLAs; no shadowing; explicit prototypes
Formatting follows .clang-format (LLVM-based, 4 spaces, 80 cols, right-aligned pointers); run clang-format
NEVER use NOLINT or other mechanisms to suppress clang-tidy warnings; fix the warnings instead
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 and order/regroup per .clang-format
Equality is OID-first: when OIDs exist they must match; SHA fallback only when OIDs are absent
Files:
core/include/gitmind/crypto/backend.hcore/src/domain/journal/read_decoder.ccore/src/crypto/backend.ccore/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/src/telemetry/config.c
{include/**/*.h,core/include/**/*.h}
📄 CodeRabbit inference engine (AGENTS.md)
{include/**/*.h,core/include/**/*.h}: Public headers must be umbrella-safe: compile standalone, correct include order, stable header guards, and extern "C" guards for C++
Public headers must compile standalone (ninja -C build header-compile)
Files:
core/include/gitmind/crypto/backend.h
{include/**/*.h,core/include/gitmind/**/*.h}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid public ABI breaks: append new public struct fields at the tail and mark deprecated fields instead of reordering/removing
Files:
core/include/gitmind/crypto/backend.h
core/include/gitmind/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Header guards lint must pass (lint_header_guards); ensure consistent GITMIND_* guards in public headers
Files:
core/include/gitmind/crypto/backend.h
core/src/domain/**
📄 CodeRabbit inference engine (AGENTS.md)
Pure domain logic resides under core/src/domain/ respecting one-responsibility-per-translation-unit
Files:
core/src/domain/journal/read_decoder.c
core/src/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
core/src/**/*.{c,h}: Refs must be constructed via gm_build_ref and inputs starting with 'refs/' must be rejected per ref-name safety rules
Journal is append-only under refs/gitmind/edges/; cache under refs/gitmind/cache/ (no merges into cache)
Prefer OID-first APIs using git_oid (gm_oid_t) and compare via git_oid_cmp or gm-level helpers
Wrap all IO behind outbound ports; inject via gm_runtime_context_t; do not call external systems directly (e.g., libgit2) from domain/services
Maintain one responsibility per translation unit; split touched multi-concern files so the edited item lives alone (One-Thing rule)
Files:
core/src/domain/journal/read_decoder.ccore/src/crypto/backend.ccore/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/src/telemetry/config.c
{docs/**/*.md,README.md}
📄 CodeRabbit inference engine (AGENTS.md)
Docs must have front matter, a single H1, a '## Table of Contents', and titles matching the H1; API docs include api_version
Files:
docs/activity/2025-10-09_pr_feedback.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Markdown must satisfy repository markdownlint configuration (.markdownlint.jsonc); use make md-lint/md-fix
Files:
docs/activity/2025-10-09_pr_feedback.md
core/src/adapters/**
📄 CodeRabbit inference engine (AGENTS.md)
Runtime adapters live under core/src/adapters//_adapter.c with matching private headers; provide factory/teardown
Files:
core/src/adapters/diagnostics/stderr_diagnostics_adapter.c
core/src/telemetry/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Use the injected log formatter DI seam for structured logs; adapters emit via gm_logger_port
Files:
core/src/telemetry/config.c
🧠 Learnings (1)
📚 Learning: 2025-10-09T02:29:56.534Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-09T02:29:56.534Z
Learning: Build and tests must run inside the Docker CI image (make ci-local/tools/ci/ci_local.sh); host builds only with explicit override and acceptance of risk
Applied to files:
tools/ci/ci_local.sh
🧬 Code graph analysis (4)
core/include/gitmind/crypto/backend.h (1)
core/src/crypto/backend.c (1)
gm_crypto_context_dispose(105-112)
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/src/adapters/diagnostics/stderr_diagnostics_adapter.c (1)
core/tests/fakes/diagnostics/fake_diagnostics_port.c (2)
emit_impl(12-70)dispose_impl(72-75)
core/src/telemetry/config.c (3)
core/src/crypto/backend.c (1)
gm_crypto_context_dispose(105-112)core/src/error/error.c (1)
gm_error_free(161-178)core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: coderabbit
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (12)
core/src/domain/journal/read_decoder.c (3)
18-53: Past review feedback successfully addressed.The converter now returns
gm_result_void_t, validates inputs, and checks everygm_memcpy_spanandgm_strcpy_safereturn code. Truncation and buffer overflows are treated as errors with field-specific error messages. This implementation follows the recommended option to check returns and propagate failures.Based on past review comments
55-88: Converter correctly implements safe-copy error checking.This converter mirrors the approach in
convert_legacy_to_attributed: validates inputs, checks all copy operations, and returns field-specific errors on overflow or truncation.Based on past review comments
90-165: Decoder properly propagates converter errors.The decode function correctly handles the new converter signatures by:
- Validating output pointers are non-NULL before use (lines 112-115, 127-130, 142-145, 156-159)
- Checking converter results and returning early on failure (lines 131-132, 160-161)
- Properly setting metadata (
consumed,got_attr) in all success pathsThis ensures conversion errors are propagated to the caller rather than silently ignored.
Based on past review comments
tools/ci/ci_local.sh (4)
12-21: LGTM! Clean staging cleanup with optional retention.The cleanup function and trap properly handle temporary staging directory removal with an opt-in retention flag for debugging.
58-68: Portable mktemp implementation addresses previous feedback.The two-stage fallback (GNU syntax first, then BSD
-tflag) ensures cross-platform compatibility. This resolves the portability concern raised in the previous review.
94-122: LGTM! Proper status capture from container execution.The script correctly disables
set -ebefore the docker run, captures the container's exit status, then re-enablesset -ebefore artifact collection. This ensures CI failures are properly propagated.
124-143: LGTM! Comprehensive artifact collection with diff reporting.The artifact collection properly copies reports back from staging and generates a useful diff when previous clang-tidy reports exist.
core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (4)
18-44: LGTM! Robust escaping for diagnostic output.The
emit_escapedhelper properly handles NULL values and escapes control characters, preventing garbled or misleading diagnostic output.
52-55: LGTM! Proper emit guard respects disabled state.The emit guard correctly checks for NULL state and the
emit_enabledflag, allowing diagnostics to be disabled at runtime.
96-107: Clarify the fallback disposal logic.Lines 100-103 check if the vtbl matches the stderr adapter's VTBL, and if not, fall back to calling
port->vtbl->dispose. This suggestsgm_stderr_diagnostics_port_disposemight be called with a port that wasn't initialized by this adapter.Is this fallback intentional (e.g., polymorphic cleanup) or defensive programming? If callers should only pass stderr-initialized ports, the fallback is misleading and should be removed or documented.
14-16: State field refactor addresses previous feedback.The field is now
bool emit_enabled(previouslyint enabled) and is actively used inemit_implto control diagnostic output. This resolves the "unused field" concern from the previous review.docs/activity/2025-10-09_pr_feedback.md (1)
1-30: LGTM! Document structure complies with guidelines.The document now includes required front matter, a matching H1 title, and a Table of Contents, addressing all concerns from the previous review. The content provides a clear summary of PR feedback remediation.
As per coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/telemetry/config.c (1)
278-299: Free the crypto error before falling back.If
gm_crypto_context_create()fails,cr.u.erris left allocated—nothing frees it before returning via the FNV fallback. Please release the error (and similarly guard any other early returns) so we do not leakgm_error_tinstances on repeated failures.- gm_result_crypto_context_t cr = gm_crypto_context_create(gm_crypto_backend_libsodium()); - if (!cr.ok) { - /* Fallback to FNV on any error */ - fnv1a64_hex12(data, len, out12); - return; - } + gm_result_crypto_context_t cr = + gm_crypto_context_create(gm_crypto_backend_libsodium()); + if (!cr.ok) { + if (cr.u.err != NULL) { + gm_error_free(cr.u.err); + } + /* Fallback to FNV on any error */ + fnv1a64_hex12(data, len, out12); + return; + }core/src/adapters/fs/posix_temp_adapter.c (1)
279-308: Handlegm_snprintftruncation paths.
gm_snprintfreturns the would-be length on truncation, so the< 0guard misses overflow cases. We end up accepting truncated paths (violating the “treat truncation as error” rule) and operating on the wrong directories. Please capture the return value and reject any result whose length is>= sizeof(buffer)here and in the othergm_snprintfcall sites in this file (e.g., Lines 308, 446, 465).- if (gm_snprintf(state->base_state, sizeof(state->base_state), - "%s/.gitmind", home) < 0) { + int wrote = gm_snprintf(state->base_state, sizeof(state->base_state), + "%s/.gitmind", home); + if (wrote < 0 || (size_t)wrote >= sizeof(state->base_state)) { return gm_err_void(GM_ERROR(GM_ERR_PATH_TOO_LONG, "failed to compose state base")); }Please apply the same pattern to the other
gm_snprintfsites touched in this file. As per coding guidelines.
🧹 Nitpick comments (1)
core/src/journal/reader.c (1)
149-156: Consider simplifying error handling for clarity.The error handling is correct but checks
dr.oktwice (lines 152 and 155). You could simplify:- gm_result_void_t dr = - gm_journal_decode_message(raw_message, decoded, sizeof(decoded), &message_len); - int decode_status = dr.ok ? GM_OK : (dr.u.err ? dr.u.err->code : GM_ERR_UNKNOWN); - if (!dr.ok) { - message_len = 0; - } - if (!dr.ok && dr.u.err) gm_error_free(dr.u.err); - if (decode_status != GM_OK) { - return decode_status; + gm_result_void_t dr = + gm_journal_decode_message(raw_message, decoded, sizeof(decoded), &message_len); + if (!dr.ok) { + message_len = 0; + int decode_status = dr.u.err ? dr.u.err->code : GM_ERR_UNKNOWN; + if (dr.u.err) gm_error_free(dr.u.err); + return decode_status; }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
AGENTS.md(2 hunks)core/include/gitmind/telemetry/internal/config.h(4 hunks)core/src/adapters/config/env_adapter.c(2 hunks)core/src/adapters/diagnostics/stderr_diagnostics_adapter.c(1 hunks)core/src/adapters/fs/posix_temp_adapter.c(18 hunks)core/src/app/cache/cache_rebuild_service.c(2 hunks)core/src/journal/reader.c(6 hunks)core/src/journal/writer.c(4 hunks)core/src/telemetry/config.c(7 hunks)core/src/telemetry/log_format.c(3 hunks)core/tests/unit/test_telemetry_cfg.c(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- AGENTS.md
- core/include/gitmind/telemetry/internal/config.h
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{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/journal/writer.ccore/src/journal/reader.ccore/tests/unit/test_telemetry_cfg.ccore/src/app/cache/cache_rebuild_service.ccore/src/telemetry/log_format.ccore/src/adapters/config/env_adapter.ccore/src/telemetry/config.ccore/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/src/adapters/fs/posix_temp_adapter.c
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Use project-safe string/memory helpers: gm_snprintf, gm_strcpy_safe, gm_memcpy_span; check return codes; treat truncation as error
Zero output buffers on error paths before formatting or copying
Prefer GM_PATH_MAX; never use GM_PATH_MAX*2; allocate if larger is required
Language rules: C23, warnings-as-errors; no VLAs; no shadowing; explicit prototypes
Formatting follows .clang-format (LLVM-based, 4 spaces, 80 cols, right-aligned pointers); run clang-format
NEVER use NOLINT or other mechanisms to suppress clang-tidy warnings; fix the warnings instead
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 and order/regroup per .clang-format
Equality is OID-first: when OIDs exist they must match; SHA fallback only when OIDs are absent
Files:
core/src/journal/writer.ccore/src/journal/reader.ccore/tests/unit/test_telemetry_cfg.ccore/src/app/cache/cache_rebuild_service.ccore/src/telemetry/log_format.ccore/src/adapters/config/env_adapter.ccore/src/telemetry/config.ccore/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/src/adapters/fs/posix_temp_adapter.c
core/src/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
core/src/**/*.{c,h}: Refs must be constructed via gm_build_ref and inputs starting with 'refs/' must be rejected per ref-name safety rules
Journal is append-only under refs/gitmind/edges/; cache under refs/gitmind/cache/ (no merges into cache)
Prefer OID-first APIs using git_oid (gm_oid_t) and compare via git_oid_cmp or gm-level helpers
Wrap all IO behind outbound ports; inject via gm_runtime_context_t; do not call external systems directly (e.g., libgit2) from domain/services
Maintain one responsibility per translation unit; split touched multi-concern files so the edited item lives alone (One-Thing rule)
Files:
core/src/journal/writer.ccore/src/journal/reader.ccore/src/app/cache/cache_rebuild_service.ccore/src/telemetry/log_format.ccore/src/adapters/config/env_adapter.ccore/src/telemetry/config.ccore/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/src/adapters/fs/posix_temp_adapter.c
core/tests/unit/test_*.c
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests live in core/tests/unit as test_.c, deterministic and isolated using fakes
Files:
core/tests/unit/test_telemetry_cfg.c
core/src/telemetry/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Use the injected log formatter DI seam for structured logs; adapters emit via gm_logger_port
Files:
core/src/telemetry/log_format.ccore/src/telemetry/config.c
core/src/adapters/**
📄 CodeRabbit inference engine (AGENTS.md)
Runtime adapters live under core/src/adapters//_adapter.c with matching private headers; provide factory/teardown
Files:
core/src/adapters/config/env_adapter.ccore/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/src/adapters/fs/posix_temp_adapter.c
{core/src/adapters/config/**/*.c,core/include/gitmind/ports/env_port.h}
📄 CodeRabbit inference engine (AGENTS.md)
Read environment/config via gm_env_port; avoid direct getenv
Files:
core/src/adapters/config/env_adapter.c
{core/src/adapters/fs/**/*.c,core/include/gitmind/ports/filesystem_port.h}
📄 CodeRabbit inference engine (AGENTS.md)
Use gm_fs_port for filesystem operations; no direct POSIX calls in domain/services
Files:
core/src/adapters/fs/posix_temp_adapter.c
🧠 Learnings (2)
📚 Learning: 2025-10-09T02:29:56.534Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-09T02:29:56.534Z
Learning: Applies to {core/src/adapters/config/**/*.c,core/include/gitmind/ports/env_port.h} : Read environment/config via gm_env_port; avoid direct getenv
Applied to files:
core/src/adapters/config/env_adapter.c
📚 Learning: 2025-10-09T02:29:56.534Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-09T02:29:56.534Z
Learning: Applies to {core/src/adapters/fs/**/*.c,core/include/gitmind/ports/filesystem_port.h} : Use gm_fs_port for filesystem operations; no direct POSIX calls in domain/services
Applied to files:
core/src/adapters/fs/posix_temp_adapter.c
🧬 Code graph analysis (5)
core/src/journal/writer.c (2)
core/src/journal/reader.c (1)
monotonic_ms_now(481-498)core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
core/src/journal/reader.c (2)
core/src/journal/writer.c (1)
monotonic_ms_now(45-62)core/include/gitmind/util/memory.h (1)
gm_strcpy_safe(113-138)
core/src/telemetry/config.c (4)
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/src/crypto/backend.c (1)
gm_crypto_context_dispose(105-112)core/src/error/error.c (1)
gm_error_free(161-178)
core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (1)
core/tests/fakes/diagnostics/fake_diagnostics_port.c (2)
emit_impl(12-70)dispose_impl(72-75)
core/src/adapters/fs/posix_temp_adapter.c (2)
core/include/gitmind/util/memory.h (1)
gm_memcpy_span(32-47)core/src/error/error.c (1)
gm_error_free(161-178)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Compat Canaries (non-blocking) (gcc-12)
- GitHub Check: Compat Canaries (non-blocking) (clang-18)
- GitHub Check: Compat Canaries (non-blocking) (gcc-13)
- GitHub Check: Compat Canaries (non-blocking) (clang-19)
- GitHub Check: lint-and-sanitize
- GitHub Check: build-test
🔇 Additional comments (10)
core/src/adapters/config/env_adapter.c (2)
8-38: LGTM: Proper error handling and safe string operations.The enhanced error handling with
GM_ERRORmacros and the use ofgm_strcpy_safefor buffer management align with the project's coding guidelines. The NULL check forgetenvreturn value with proper empty-string fallback is defensive and correct.
40-49: LGTM: Naming improvements for clarity.Renaming the vtbl to
GmEnvPortSystemVtbland the port instance tosystem_portmakes the code more explicit and consistent with other adapter implementations in the PR.core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (5)
17-17: Past issue resolved: struct field now actively used.The struct field has been changed from
int enabledtobool emit_enabledand is now actively used in the emit logic (line 62). This addresses the previous feedback about the unused field.
20-53: LGTM: Robust escaping with proper safety checks.The
emit_escapedhelper correctly handles NULL input, escapes common control characters, and usesgm_snprintffor hex escaping with appropriate buffer sizing (5 bytes for\xNN\0). The cast tounsigned charon line 26 is correct foriscntrl. The error check on line 44 silently drops malformed escapes, which is acceptable for diagnostic output.
55-83: Past issue resolved: NULL kvs guard added.Lines 58-60 now guard against dereferencing NULL
kvswhenkv_count > 0, addressing the critical issue flagged in the previous review. The implementation returns early withgm_ok_void()rather than an error, which is a reasonable choice for diagnostic output.
85-109: Past issue resolved: dispose implementation added.The
dispose_implfunction (lines 85-87) now properly frees the allocated state, and is wired into the vtbl (line 91). Theinitfunction allocates state viacalloc(lines 99-104) and initializesemit_enabledtotrue(line 105). This addresses the memory leak concern from the previous review.
111-122: Defensive dispose implementation handles multiple vtbl types.The public dispose function uses identity comparison (
port->vtbl == &GmStderrDiagnosticsPortVtbl) to detect the stderr vtbl, then falls back to a generic vtbl dispose call if available. This defensive pattern safely handles both the specific adapter and any other vtbl implementations, then clears port fields to prevent use-after-free.core/src/journal/reader.c (3)
28-42: LGTM! Header inclusions and forward declaration are correct.The Windows-specific header is needed for performance counter APIs, the log_format.h enables the telemetry formatter seam, and the forward declaration properly handles the function being used before its definition.
336-342: LGTM! Canonicalization view pattern improves safety.The change from copying to using a view reference is good design. The error handling correctly treats truncation as an error per coding guidelines: if
gm_strcpy_safereturns-1,repo_canonremainsNULLand a warning is logged (lines 352-354).
366-413: LGTM! Telemetry and timing improvements are well-designed.The structured
gm_telemetry_tag_context_t(lines 366-373) provides better organization, and using monotonic time (lines 410, 413) is more reliable than wall-clock time for duration measurements. The edge counting viaedge_count_ptrcorrectly increments for both attributed and non-attributed edges (lines 112, 116).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
core/src/journal/reader.c (1)
482-499: Address the duplication flagged in previous review.The
monotonic_ms_nowimplementation is correct (proper Windows QueryPerformanceCounter, POSIX CLOCK_MONOTONIC, and fallback), but the duplication acrossreader.candwriter.cremains unresolved. Please refer to the existing review comment that suggests extracting this function to a timing utility module to satisfy the One-Thing rule.
🧹 Nitpick comments (3)
core/src/telemetry/log_format.c (2)
151-162: Add input validation to prevent undefined behavior.
sort_kvsdoes not validate thatorderedis non-null or thatcountis reasonable. If a caller passesNULLor an uninitialized pointer, the loop will dereference invalid memory.Apply this diff to add safety checks:
static void sort_kvs(const gm_log_kv_t **ordered, size_t count) { + if (ordered == NULL || count == 0U) { + return; + } for (size_t entry_index = 1; entry_index < count; ++entry_index) {
176-181: Consider checked allocation for robustness.While
kv_countis bounded byMAX_TAGS_TOTALin practice, the multiplicationsizeof(*sorted) * kv_countcould theoretically overflow with a very large count. Consider using a safer allocation pattern that checks for overflow.Example approach:
+ if (kv_count > SIZE_MAX / sizeof(*sorted)) { + return gm_err_void(GM_ERROR(GM_ERR_OUT_OF_MEMORY, + "allocation size overflow")); + } const gm_log_kv_t **sorted = (const gm_log_kv_t **)malloc(sizeof(*sorted) * kv_count);core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (1)
116-127: Simplify disposal to use the vtable consistently.The special-case check for
GmStderrDiagnosticsPortVtblat line 120 is overly defensive. SinceGmStderrDiagnosticsPortVtbl.disposealready points todispose_impl, you can simplify by using the vtable consistently:void gm_stderr_diagnostics_port_dispose(gm_diagnostics_port_t *port) { if (port == NULL) { return; } - if (port->vtbl == &GmStderrDiagnosticsPortVtbl && port->self != NULL) { - dispose_impl(port->self); - } else if (port->vtbl != NULL && port->vtbl->dispose != NULL) { + if (port->vtbl != NULL && port->vtbl->dispose != NULL) { port->vtbl->dispose(port->self); } port->self = NULL; port->vtbl = NULL; }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/src/adapters/diagnostics/stderr_diagnostics_adapter.c(1 hunks)core/src/adapters/fs/posix_temp_adapter.c(18 hunks)core/src/journal/reader.c(7 hunks)core/src/telemetry/config.c(7 hunks)core/src/telemetry/log_format.c(3 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/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/src/telemetry/config.ccore/src/journal/reader.ccore/src/telemetry/log_format.ccore/src/adapters/fs/posix_temp_adapter.c
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Use project-safe string/memory helpers: gm_snprintf, gm_strcpy_safe, gm_memcpy_span; check return codes; treat truncation as error
Zero output buffers on error paths before formatting or copying
Prefer GM_PATH_MAX; never use GM_PATH_MAX*2; allocate if larger is required
Language rules: C23, warnings-as-errors; no VLAs; no shadowing; explicit prototypes
Formatting follows .clang-format (LLVM-based, 4 spaces, 80 cols, right-aligned pointers); run clang-format
NEVER use NOLINT or other mechanisms to suppress clang-tidy warnings; fix the warnings instead
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 and order/regroup per .clang-format
Equality is OID-first: when OIDs exist they must match; SHA fallback only when OIDs are absent
Files:
core/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/src/telemetry/config.ccore/src/journal/reader.ccore/src/telemetry/log_format.ccore/src/adapters/fs/posix_temp_adapter.c
core/src/adapters/**
📄 CodeRabbit inference engine (AGENTS.md)
Runtime adapters live under core/src/adapters//_adapter.c with matching private headers; provide factory/teardown
Files:
core/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/src/adapters/fs/posix_temp_adapter.c
core/src/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
core/src/**/*.{c,h}: Refs must be constructed via gm_build_ref and inputs starting with 'refs/' must be rejected per ref-name safety rules
Journal is append-only under refs/gitmind/edges/; cache under refs/gitmind/cache/ (no merges into cache)
Prefer OID-first APIs using git_oid (gm_oid_t) and compare via git_oid_cmp or gm-level helpers
Wrap all IO behind outbound ports; inject via gm_runtime_context_t; do not call external systems directly (e.g., libgit2) from domain/services
Maintain one responsibility per translation unit; split touched multi-concern files so the edited item lives alone (One-Thing rule)
Files:
core/src/adapters/diagnostics/stderr_diagnostics_adapter.ccore/src/telemetry/config.ccore/src/journal/reader.ccore/src/telemetry/log_format.ccore/src/adapters/fs/posix_temp_adapter.c
core/src/telemetry/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Use the injected log formatter DI seam for structured logs; adapters emit via gm_logger_port
Files:
core/src/telemetry/config.ccore/src/telemetry/log_format.c
{core/src/adapters/fs/**/*.c,core/include/gitmind/ports/filesystem_port.h}
📄 CodeRabbit inference engine (AGENTS.md)
Use gm_fs_port for filesystem operations; no direct POSIX calls in domain/services
Files:
core/src/adapters/fs/posix_temp_adapter.c
🧠 Learnings (1)
📚 Learning: 2025-10-09T02:29:56.534Z
Learnt from: CR
PR: neuroglyph/git-mind#0
File: AGENTS.md:0-0
Timestamp: 2025-10-09T02:29:56.534Z
Learning: Applies to {core/src/adapters/fs/**/*.c,core/include/gitmind/ports/filesystem_port.h} : Use gm_fs_port for filesystem operations; no direct POSIX calls in domain/services
Applied to files:
core/src/adapters/fs/posix_temp_adapter.c
🧬 Code graph analysis (4)
core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (1)
core/tests/fakes/diagnostics/fake_diagnostics_port.c (2)
emit_impl(12-70)dispose_impl(72-75)
core/src/telemetry/config.c (5)
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/src/crypto/backend.c (2)
gm_crypto_context_create(81-99)gm_crypto_context_dispose(105-112)core/src/error/error.c (1)
gm_error_free(161-178)core/src/crypto/sha256.c (1)
gm_sha256_with_context(19-37)
core/src/journal/reader.c (3)
core/src/error/error.c (1)
gm_error_free(161-178)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/src/adapters/fs/posix_temp_adapter.c (2)
core/include/gitmind/util/memory.h (2)
gm_strcpy_safe(113-138)gm_memcpy_span(32-47)core/src/error/error.c (1)
gm_error_free(161-178)
🪛 GitHub Check: CodeQL
core/src/adapters/fs/posix_temp_adapter.c
[failure] 521-521: Time-of-check time-of-use filesystem race condition
The filename being operated upon was previously checked, but the underlying file may have been changed since then.
The filename being operated upon was previously checked, but the underlying file may have been changed since then.
[failure] 606-606: Time-of-check time-of-use filesystem race condition
The filename being operated upon was previously checked, but the underlying file may have been changed since then.
The filename being operated upon was previously checked, but the underlying file may have been changed since then.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Compat Canaries (non-blocking) (clang-18)
- GitHub Check: Compat Canaries (non-blocking) (gcc-12)
- GitHub Check: Compat Canaries (non-blocking) (clang-19)
- GitHub Check: Compat Canaries (non-blocking) (gcc-13)
- GitHub Check: lint-and-sanitize
- GitHub Check: build-test
🔇 Additional comments (21)
core/src/telemetry/log_format.c (2)
58-73: LGTM! Capacity check correctly accounts for null terminator.The fix properly addresses the previous review by checking for
JsonHexEscapeLength + 1Ubytes before callinggm_snprintf, ensuring space for both the six-character escape sequence and the terminating null. The additional validation ofhex_writtenagainst buffer bounds provides defense in depth.
223-267: LGTM! Proper resource cleanup on all error paths.The function correctly releases the sorted entries array in all scenarios: success path (line 265), append overflow (line 250), and final capacity check (line 258). This prevents memory leaks regardless of the execution path.
core/src/telemetry/config.c (5)
27-35: LGTM! Cross-platform case-insensitive comparison properly implemented.The wrapper correctly uses
_stricmpon Windows andstrcasecmpon POSIX platforms, ensuring portable ASCII case-insensitive string comparison across the codebase.
152-175: LGTM! Magic number eliminated per previous feedback.The capacity check now correctly derives the array size from the struct definition (
sizeof(cfg->extras) / sizeof(cfg->extras[0])), preventing divergence if the array size changes. The use ofgm_memset_safefor error-path cleanup adds defense in depth.Based on past review comments.
276-308: LGTM! Crypto context properly disposed on all paths.The function correctly disposes the crypto context whether
gm_sha256_with_contextsucceeds (lines 298-301) or fails (lines 291-294), and frees any disposal errors. The fallback to FNV on any crypto operation failure ensures robustness.
415-462: LGTM! Context-based tag builder correctly implements deterministic tag generation.The new signature with
gm_telemetry_tag_context_tcleanly separates concerns, and the builder pattern manages buffer bounds and tag limits effectively. The use ofGM_PATH_MAXfor the repo value buffer andGM_TRYfor error propagation aligns with coding guidelines.As per coding guidelines.
248-261: LGTM! Improved bounds checking prevents overflow.The explicit
availablecalculation and the check thatwrote < availableensure the buffer is never overrun. The early return for empty keys/values avoids unnecessary formatting.core/src/journal/reader.c (3)
28-30: LGTM!The Windows header inclusion is properly guarded for platform-specific timing support, the new telemetry log format include is appropriate, and the forward declaration follows best practices.
Also applies to: 38-38, 42-42
149-158: LGTM!The updated CBOR decoding API usage demonstrates proper error handling: zeroing
message_lenon failure, extracting the error code with a safe fallback, and freeing the error structure. This aligns with the coding guidelines for error path handling.
336-361: LGTM!The canonicalization logic correctly updates to the new API with proper view handling:
- Variable rename to
canon_viewbetter reflects that it's a view rather than a copygm_strcpy_safereturn value is properly checked and truncation is treated as an error per coding guidelinesgm_memset_safeis correctly used to zerorepo_idon error paths- Error handling is thorough with proper freeing throughout
core/src/adapters/diagnostics/stderr_diagnostics_adapter.c (7)
4-14: LGTM! Includes support the new functionality.The new includes (
ctype.h,stdbool.h,diagnostic_port.h,security/string.h) properly support the character escaping logic, boolean state field, and disposal infrastructure.
16-18: LGTM! State field is now meaningful and properly typed.The
emit_enabledboolean addresses the previous feedback about the unused field and serves a clear purpose in gating diagnostic emission.
20-58: LGTM! Escaping logic follows safe string practices.The
emit_escapedfunction properly:
- Handles NULL input
- Uses
unsigned charcast to avoid sign-extension issues withiscntrl- Validates
gm_snprintfreturn value before using the buffer- Correctly sizes the hex escape buffer (5 bytes for
\xNN\0)
66-88: LGTM! Emission logic properly uses escaped output.The emit_enabled check, NULL handling for component/event, and use of
emit_escapedfor all string output are correct.
90-92: LGTM! Disposal implementation is correct.Simple and correct: frees the
calloc'd state. This addresses the previous feedback about missing cleanup.
94-97: LGTM! Vtable correctly wires disposal.The vtable properly includes the
disposecallback, enabling proper cleanup via the port abstraction.
99-114: LGTM! Initialization properly allocates and configures state.The function correctly:
- Validates input
- Allocates state with
calloc- Initializes
emit_enabledtotrue- Wires the vtable and state into the port
- Returns an error on allocation failure
core/src/adapters/fs/posix_temp_adapter.c (4)
33-37: LGTM: Cross-platform strtok_r compatibility.The macro correctly handles Windows (
strtok_s) vs. POSIX (strtok_r) differences and is properly undefined at the end of the file (lines 817-819).
92-176: LGTM: Safer incremental directory creation.The refactored implementation correctly:
- Normalizes the path before processing
- Uses
strtok_rfor clean segment iteration- Employs project-safe string helpers (
gm_strcpy_safe,gm_memcpy_span) per coding guidelines- Handles the stat/mkdir race condition by checking for
EEXIST- Validates buffer capacity at each step
As per coding guidelines.
667-711: LGTM: Well-designed canonicalization strategy.The three helper functions correctly implement distinct canonicalization modes:
canonicalize_existing_path: Usesrealpath()for full physical resolution (follows symlinks, verifies existence)canonicalize_create_ok_path: Clever approach that canonicalizes the parent directory and appends the basename, allowing the final component to not exist yetcanonicalize_logical_path: Pure logical normalization without filesystem interactionThis design properly separates concerns and handles the common use case where you need a canonical path for a file that doesn't exist yet but whose parent directory does.
98-114: LGTM: Exemplary string safety practices.Throughout the file, you consistently use project-safe string helpers (
gm_strcpy_safe,gm_memcpy_span,gm_snprintf) with proper return value checking and buffer overflow protection. The code properly:
- Checks return values from all string operations
- Validates buffer capacity before operations
- Returns descriptive errors on overflow (e.g.,
GM_ERR_PATH_TOO_LONG)- Zeros buffers on error paths where appropriate
This is a model implementation of the string safety requirements in the coding guidelines.
As per coding guidelines.
Also applies to: 151-157, 181-195, 299-301, 531-535, 569-576
| if (kv_count > 0U && kvs == NULL) { | ||
| return gm_ok_void(); | ||
| } |
There was a problem hiding this comment.
Return an error for invalid arguments (inconsistent with test fake).
The code silently succeeds when kvs == NULL but kv_count > 0, which is inconsistent with the fake diagnostics port that returns GM_ERR_INVALID_ARGUMENT. This breaks test/production parity and could hide caller bugs.
Apply this diff to align with the fake port behavior:
if (kv_count > 0U && kvs == NULL) {
- return gm_ok_void();
+ return gm_err_void(GM_ERROR(GM_ERR_INVALID_ARGUMENT,
+ "diagnostics kv list missing"));
}🤖 Prompt for AI Agents
In core/src/adapters/diagnostics/stderr_diagnostics_adapter.c around lines
63–65, the function currently returns success when kv_count > 0 and kvs == NULL;
change this to return an invalid-argument error to match the fake diagnostics
port. Detect the invalid argument case (kv_count > 0 && kvs == NULL) and return
gm_err(GM_ERR_INVALID_ARGUMENT) (or the project's equivalent error constructor)
instead of gm_ok_void(), ensuring callers/tests see the same error behavior as
the fake implementation.
| #ifndef GM_HAVE_REALPATH_DECL | ||
| #define GM_HAVE_REALPATH_DECL 1 | ||
| extern char *realpath(const char *path, char *resolved_path); | ||
| #endif |
There was a problem hiding this comment.
Unconventional feature detection: realpath declaration guard defaults to 1.
Feature detection macros should typically be defined by the build system (e.g., CMake, autoconf) only when a feature is confirmed absent. Defaulting GM_HAVE_REALPATH_DECL to 1 assumes the declaration is always missing, which is backwards for most modern POSIX systems where realpath is declared in <stdlib.h>.
Consider this approach instead:
-#ifndef GM_HAVE_REALPATH_DECL
-#define GM_HAVE_REALPATH_DECL 1
+#ifdef GM_HAVE_REALPATH_DECL
extern char *realpath(const char *path, char *resolved_path);
#endifThen, configure your build system to define GM_HAVE_REALPATH_DECL only on platforms where the declaration is genuinely missing.
📝 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.
| #ifndef GM_HAVE_REALPATH_DECL | |
| #define GM_HAVE_REALPATH_DECL 1 | |
| extern char *realpath(const char *path, char *resolved_path); | |
| #endif | |
| #ifdef GM_HAVE_REALPATH_DECL | |
| extern char *realpath(const char *path, char *resolved_path); | |
| #endif |
🤖 Prompt for AI Agents
In core/src/adapters/fs/posix_temp_adapter.c around lines 28 to 31, the code
unconditionally defines GM_HAVE_REALPATH_DECL to 1 and declares realpath, which
assumes the declaration is missing; remove the default definition and only emit
the extern declaration when the build system sets GM_HAVE_REALPATH_DECL (e.g.,
wrap the declaration in #if GM_HAVE_REALPATH_DECL / #endif or check
defined(GM_HAVE_REALPATH_DECL) and ensure the project build configuration
(CMake/autoconf) defines GM_HAVE_REALPATH_DECL only on platforms where realpath
is actually missing so the header declaration is not redundantly injected on
systems with a native declaration.
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Line 600: The JSON entry contains a British English spelling "stabilises" in
the future_work text for the "Diagnostics escape helper" topic; change that word
to the American English "stabilize" (update the string value under the
"future_work" key that currently reads "Add focused tests around control escapes
once tidy backlog stabilises.") so the document consistently uses American
English.
In `@core/src/adapters/git/libgit2_repository_port.c`:
- Around line 530-534: The code reads errno after calling git_treebuilder_free,
risking clobbering the opendir failure code; capture errno immediately after
opendir fails and before calling git_treebuilder_free. Specifically, in the
branch where opendir(dir_path) returns NULL, store the failure code (e.g., int
opendir_err = errno) right away, then call git_treebuilder_free(root_builder)
and finally return gm_errno_to_result("opendir", dir_path, opendir_err); ensure
references to opendir, git_treebuilder_free, errno, gm_errno_to_result,
dir_path, and root_builder are used as described.
- Around line 158-164: The error-path calls closedir(dir) and
git_treebuilder_free(builder) even when dir or builder may be NULL; change the
early-return branch in the function that checks (frames, count, capacity, path,
builder, dir) so that you only call closedir(dir) if dir != NULL and only call
git_treebuilder_free(builder) if builder != NULL before returning
gm_err_void(GM_ERROR(GM_ERR_INVALID_ARGUMENT, "invalid directory frame")); keep
the same return value and error creation (gm_err_void/GM_ERROR) but guard the
cleanup calls to avoid undefined behavior.
- Line 1416: Rename the file-scoped constant GM_LIBGIT2_REPOSITORY_PORT_VTBL to
follow project naming (e.g., gm_libgit2_repository_port_vtbl), remove the
trailing "// NOLINT(readability-identifier-naming)" comment, and update its
single reference where it is used (the initializer/assignment site that
currently refers to GM_LIBGIT2_REPOSITORY_PORT_VTBL) to the new name; ensure the
declaration remains "static const" and the symbol uses lower_snake_case with the
gm_ prefix to satisfy clang-tidy and project naming rules.
In `@core/src/telemetry/config.c`:
- Around line 26-40: The file uses static const size_t variables as array
dimensions (e.g., TelemetryMaxTags, TelemetryRepoIdHexPartLen,
TelemetryRepoIdHexStrLen, TelemetryRepoIdBufferLen, TelemetryExtraBufferLen,
TelemetryHashHexDigits, TelemetryTotalFnvNibbles, TelemetryNibbleWidth,
TelemetrySha256PrefixBytes, TelemetryHexDigitMask) which produces VLAs; change
those constants used for array sizes to compile-time constants (use enum { ... }
or `#define` for integer-sized constants) so they are true constant expressions,
and retain TelemetryHexDigits as the char array; keep the 64-bit FNV constants
(TelemetryFnvOffsetBasis, TelemetryFnvPrime) as static const uint64_t or convert
them to `#define` if needed because enum won’t hold 64-bit values. Ensure all call
sites and array declarations (where TelemetryRepoIdBufferLen,
TelemetryExtraBufferLen, TelemetryTotalFnvNibbles, etc. are used) are updated to
reference the new enum/#define names.
In `@docs/issues/SLT.HOTFIX.001_meta_issue.md`:
- Around line 1-14: Add required YAML front matter and a Table of Contents:
create a YAML front matter block at the top with title: "[SLT.HOTFIX.001]
Enforce trust threshold in pre-receive hook (Meta-Issue)" (matching the existing
H1), include any standard meta fields used in docs (e.g., title, description,
tags) per repo guidelines, ensure the file contains only that single H1 line,
and insert a "## Table of Contents" section after the H1 listing the child issue
links so the file complies with the docs rules.
In `@docs/issues/SLT.HOTFIX.001.1_comment1.md`:
- Around line 1-9: Add required YAML front matter, a single H1 heading, and a
"## Table of Contents" to the top of docs/issues/SLT.HOTFIX.001.1_comment1.md;
ensure the front matter includes at least title and description, the H1 matches
the title in the front matter, and the file contains a "## Table of Contents"
section under the H1 (listing major sections like Implementation Details),
replacing the initial bold text block with these required elements so the
document conforms to the {docs/**/*.md,README.md} guidelines.
- Line 26: Replace the developer-specific absolute path with a
repository-relative reference to the pre-receive hook (e.g., use
contrib/hooks/pre-receive.shiplog or a $REPO_ROOT placeholder instead of
/Users/james/...), updating the string in the docs so it points to the hook file
by repo-relative path and not a user home path; locate the hardcoded path string
that mentions pre-receive.shiplog in the diff and change it accordingly.
In `@docs/talk-shop/combined_markdown.md`:
- Line 229: Update the typo in the "Concurrency model" note by replacing the
word "racey" with the correct spelling "racy" in the combined_markdown.md
content where the sentence mentions concurrent applies and multi-user locking
risks; keep the rest of the sentence unchanged.
- Around line 1-12: The combined Markdown file contains multiple front-matter
blocks (---) and multiple H1 headings like "# Graph LvL-Up Tank 🛡️🧠",
violating the docs rules; either split this monolithic file into separate files
(one per original document) each with its own front matter, a single H1 matching
the file title, and a "## Table of Contents", or convert the file into a single
consolidated document (e.g., "Talk Shop Archive") with only one H1 at the top, a
proper front-matter block at the top, a "## Table of Contents", and demote all
other H1s to H2s/H3s; ensure each new file or the consolidated file's
front-matter title matches the top-level H1 and that no mid-file front-matter
blocks remain.
In `@Gemini` Explores git-mind.md:
- Around line 1-447: The file "Gemini Explores git-mind.md" is an accidental AI
transcript that breaks CI (markdownlint errors MD041/MD050/MD004/MD007) and
leaks local paths; remove it from the PR by deleting the file from the branch
(or git rm and amend/rebase the offending commit), push the updated branch to
update the PR, and re-run CI; also scan the commit for any other transcript
files or leaked local paths and remove them before pushing so the markdownlint
gate no longer fails.
In `@Untitled.md`:
- Line 97: Fix the typo in the sentence containing "counsil" by replacing
"counsil" with the correct spelling "council" in the line that reads "consensus,
after discussing it with a counsil of AI haha, was the extracting that core
'append-only git-native ledger'..." so the phrase becomes "consensus, after
discussing it with a council of AI haha, was the extracting that core
'append-only git-native ledger'...".
- Around line 1-8: The Untitled.md file at repo root is a draft causing
markdownlint failures (MD041, MD004, MD033); either delete this scratch file or
move it to docs/talk-shop/ (e.g., libgitledger_design_discussion.md) and then
fix formatting by adding proper front matter and a single H1, generate a Table
of Contents, replace any asterisk-style unordered lists with dashes, remove any
inline HTML, and ensure the first line is a top-level heading to satisfy MD041;
confirm lint passes after changes.
🧹 Nitpick comments (8)
core/include/gitmind/util/errno_compat.h (2)
7-7: Unnecessary<stdint.h>include.The function signature only uses
const char *,int, andgm_result_void_t(fromgitmind/result.h).<stdint.h>is not needed here.-#include <stdint.h> - `#include` "gitmind/result.h"
15-16: MissingGM_NODISCARDon result-returning function.Other new public headers in this PR (e.g.,
attributed_cbor.h) annotategm_result_void_t-returning functions withGM_NODISCARD. This function should follow the same pattern for consistency and to prevent callers from silently discarding errors.-gm_result_void_t gm_errno_to_result(const char *operation, const char *path, - int err); +GM_NODISCARD gm_result_void_t gm_errno_to_result(const char *operation, + const char *path, int err);core/src/util/errno_compat.c (1)
43-44:strerror()is not guaranteed thread-safe on all POSIX platforms.If this function could be called concurrently (e.g., from multiple threads hitting I/O errors),
strerror()may return a pointer to a shared buffer. Considerstrerror_r(the GNU/XSI variant) for thread-safe errno message formatting in a future hardening pass.This is low-risk since it's an error-path utility, but worth tracking.
core/src/domain/edge/attributed_cbor.c (1)
181-296: Narrowing casts fromuint64_twithout range validation.
decode_rel_typeanddecode_confidencecastuint64_t→uint16_t,decode_flagscasts touint32_t, anddecode_source_type/decode_lanecast to enum types — all without bounds checks. With malformed or adversarial CBOR input, these truncate silently.This is low-risk since the encoder only produces in-range values, but worth noting for hardening.
core/src/adapters/fs/posix_temp_adapter.c (3)
654-658:frames[GM_PATH_MAX]allocates ~64 KB on the stack — consider a smaller bound.
GM_PATH_MAX(typically 4096) is the max path length, not the max directory depth. Each nesting level requires at least 2 path characters (x/), so the theoretical max depth isGM_PATH_MAX / 2. In practice, depths beyond a few hundred are vanishingly rare.A constant like 256 would still be generous while reducing the stack footprint of
remove_tree_context_tfrom ~69 KB to ~8 KB — a meaningful difference if this ever runs on a thread with a small stack.♻️ Suggested change
+enum { RemoveTreeMaxDepth = 256 }; + typedef struct { - dir_stack_frame_t frames[GM_PATH_MAX]; + dir_stack_frame_t frames[RemoveTreeMaxDepth]; size_t depth; char current[GM_PATH_MAX]; } remove_tree_context_t;And update the capacity check in
remove_tree_descendaccordingly (line 728 already readssizeof(ctx->frames) / sizeof(ctx->frames[0]), so it will adapt automatically).
509-520:out_dir->pathaliasesstate->scratch— any subsequent port call invalidates it.
copy_path_to_statesetsout_dir->path = state->scratch, butscratchis shared acrossjoin_under_base,canonicalize_*, and other vtbl operations. A second call through the port will silently overwrite the previously returned temp-dir path.If this is intentional (like
strtok-style semantics), consider documenting the lifetime constraint in the port header or ingm_tempdir_t. If callers are expected to copy the path immediately, an assertion or comment here would prevent subtle misuse.
180-190: Nit: file-static constants use PascalCase rather than the project'slower_snake_caseconvention.Per coding guidelines, variables should be
lower_snake_casewithgm_prefix. These file-static constants (FnvOffsetBasis,TempRandomMultiplier, etc.) and enum values (RepoIdHexLength, etc.) use PascalCase instead. Since they're file-private, thegm_prefix is less important, butlower_snake_casewould be more consistent with the rest of the codebase.As per coding guidelines: "functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE."
core/src/adapters/git/libgit2_repository_port.c (1)
44-46: UseGM_PATH_MAXinstead of a locally-definedGIT_PATH_MAX.The coding guidelines require preferring
GM_PATH_MAX. Sincegitmind/types.his already included and definesGM_PATH_MAXas 4096, the local definition at lines 44–46 is redundant. Replace all occurrences ofGIT_PATH_MAXwithGM_PATH_MAXthroughout the file (lines 59, 60, 256, 293, 922, 929).
| - CI failure snapshot: Actions run for PR #178 died in the clang-tidy stage after ~662k diagnostics; key blockers were missing includes for the new port seams, missing dispose hooks on diagnostics adapters/fakes, telemetry constants still magic numbers, and recursion/complexity warnings in `core/src/adapters/fs/posix_temp_adapter.c` plus the telemetry log formatter. Keep this summary handy to avoid re-reading the huge log. | ||
| {"date":"2025-10-10","time":"16:42","summary":"Refactored edge attributed CBOR handling into a dedicated domain module while continuing the clang-tidy cleanup push.","topics":[{"topic":"Edge CBOR refactor","what":"Moved encode/decode logic into core/src/domain/edge/attributed_cbor.c with helper writers/readers.","why":"core/src/edge/attributed.c was the largest tidy hotspot and bundled multiple responsibilities.","context":"Hex migration plan requires each touched surface to become SRP-compliant and test-double friendly before moving on.","issue":"Legacy encode/decode block carried high cognitive complexity, magic numbers, and direct IO coupling.","resolution":"Created new domain serializer/header, rewired public API to call it, and updated Meson sources.","future_work":"Add focused unit coverage for the new serializer once existing tests are stabilized.","time_percent":60},{"topic":"Clang-tidy gate posture","what":"Clarified that we can’t bypass the tidy gate and must continue driving warnings to zero as modules hex-ify.","why":"CI still reports hundreds of diagnostics, raising questions about committing draft work.","context":"Guardrails documented in AGENTS.md require clang-tidy to stay green for hex’d surfaces.","issue":"Large backlog from untouched legacy modules triggered pressure to skip checks.","resolution":"Documented remaining offenders and committed to continue the warning-reduction sequence.","future_work":"Tackle telemetry config and libgit2 repository adapter warnings next.","time_percent":40}],"key_decisions":["Extracted edge attributed CBOR encode/decode into a domain-layer serializer.","Maintained the zero-tolerance clang-tidy gate despite outstanding legacy diagnostics."],"action_items":[{"task":"Reduce telemetry config and libgit2 adapter clang-tidy diagnostics to zero.","owner":"agent"}]} | ||
| {"date":"2025-10-10","time":"20:13","summary":"Eliminated clang-tidy noise in gm_telemetry_build_tags and reran docker tidy to confirm telemetry stays clean.","topics":[{"topic":"Telemetry Config Cleanup","what":"Refactored gm_telemetry_build_tags helpers and constants to remove 455 clang-tidy diagnostics.","why":"CI failure highlighted telemetry warnings blocking PR #178.","context":"Follows prior extraction of edge attributed CBOR serializer.","issue":"Missing includes, magic numbers, and nested GM_TRY macros triggered naming, braces, and complexity violations.","resolution":"Added explicit constants, extracted helper functions, replaced GM_TRY with manual checks, and re-ran docker clang-tidy to verify zero telemetry warnings.","future_work":"Begin libgit2 repository adapter cleanup to reduce remaining tidy backlog.","time_percent":90}],"key_decisions":["Replaced telemetry magic numbers with shared Telemetry* constants","Standardized gm_telemetry_build_tags error handling without GM_TRY macros"],"action_items":[{"task":"Draft refactor plan for libgit2_repository_port.c to drop recursion and satisfy include-cleaner","owner":"agent"}]} | ||
| {"date":"2025-10-09","time":"22:57","summary":"Began major POSIX temp adapter cleanup and rewired diagnostics escapes; clang-tidy still reports include warnings to tackle.","topics":[{"topic":"Diagnostics escape helper","what":"Split stderr emit logic into helpers to lower complexity and tighten gm_snprintf guards.","why":"clang-tidy flagged cognitive complexity 13 on emit_escaped after the CI failure.","context":"PR #178 tidy gate requires diagnostics adapters to stay SRP compliant and warning-free.","issue":"Single switch statement mixed control escapes with gm_snprintf capacity checks, triggering complexity warnings.","resolution":"Introduced emit_simple_escape/control helpers and routed emit_escaped through emit_character.","future_work":"Add focused tests around control escapes once tidy backlog stabilises.","time_percent":35},{"topic":"POSIX temp adapter refactor","what":"Reworked ensure_dir_exists, state_base_dir, make_temp_dir_impl, and remove_tree_impl around reusable helpers.","why":"core/src/adapters/fs/posix_temp_adapter.c generated dozens of clang-tidy errors (complexity, bool conversions, gm_snprintf truncation).","context":"Hex migration demands adapters expose clean seams and remain clang-tidy zero.","issue":"Legacy implementation mixed path normalization, mkdir logic, and recursive deletion with deep nesting and errno macro usage.","resolution":"Added path_builder/appender utilities, iterative remove_tree_context walk, random temp dir factory, and replaced GM_TRY macros with explicit checks.","future_work":"Finish abstracting errno conditions to silence include-cleaner and rerun tidy to confirm reductions.","time_percent":65}],"key_decisions":["Leaned into helper-based path building for POSIX adapter rather than piecemeal GM_TRY swaps."],"action_items":[{"task":"Replace direct errno macro checks in posix temp adapter with error-code helpers and rerun docker clang-tidy.","owner":"agent"},{"task":"Pull in required libgit2 headers (errors/buffer/revwalk) and resolve GIT_ENOTFOUND usage before retrying tidy.","owner":"agent"}]} |
There was a problem hiding this comment.
Minor spelling inconsistency: "stabilises" vs "stabilize".
The rest of the document uses American English ("stabilize"), but this entry uses British English ("stabilises"). Keep consistent per the static analysis hint.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~600-~600: Do not mix variants of the same word (‘stabilise’ and ‘stabilize’) within a single text.
Context: ...round control escapes once tidy backlog stabilises.","time_percent":35},{"topic":"POSIX te...
(EN_WORD_COHERENCY)
🤖 Prompt for AI Agents
In `@AGENTS.md` at line 600, The JSON entry contains a British English spelling
"stabilises" in the future_work text for the "Diagnostics escape helper" topic;
change that word to the American English "stabilize" (update the string value
under the "future_work" key that currently reads "Add focused tests around
control escapes once tidy backlog stabilises.") so the document consistently
uses American English.
| if (frames == NULL || count == NULL || capacity == NULL || path == NULL || | ||
| builder == NULL || dir == NULL) { | ||
| closedir(dir); | ||
| git_treebuilder_free(builder); | ||
| return gm_err_void( | ||
| GM_ERROR(GM_ERR_INVALID_ARGUMENT, "invalid directory frame")); | ||
| } |
There was a problem hiding this comment.
closedir(NULL) is undefined behavior.
When dir is NULL (included in the OR condition on line 158), the error path at line 160 calls closedir(dir), which is UB per POSIX. Guard the cleanup calls with null checks.
Proposed fix
if (frames == NULL || count == NULL || capacity == NULL || path == NULL ||
builder == NULL || dir == NULL) {
- closedir(dir);
- git_treebuilder_free(builder);
+ if (dir != NULL) {
+ closedir(dir);
+ }
+ if (builder != NULL) {
+ git_treebuilder_free(builder);
+ }
return gm_err_void(
GM_ERROR(GM_ERR_INVALID_ARGUMENT, "invalid directory frame"));
}🤖 Prompt for AI Agents
In `@core/src/adapters/git/libgit2_repository_port.c` around lines 158 - 164, The
error-path calls closedir(dir) and git_treebuilder_free(builder) even when dir
or builder may be NULL; change the early-return branch in the function that
checks (frames, count, capacity, path, builder, dir) so that you only call
closedir(dir) if dir != NULL and only call git_treebuilder_free(builder) if
builder != NULL before returning gm_err_void(GM_ERROR(GM_ERR_INVALID_ARGUMENT,
"invalid directory frame")); keep the same return value and error creation
(gm_err_void/GM_ERROR) but guard the cleanup calls to avoid undefined behavior.
| DIR *root_dir = opendir(dir_path); | ||
| if (root_dir == NULL) { | ||
| git_treebuilder_free(root_builder); | ||
| return result; | ||
| int opendir_err = errno; | ||
| return gm_errno_to_result("opendir", dir_path, opendir_err); |
There was a problem hiding this comment.
errno may be clobbered before capture.
errno is read on line 533 after git_treebuilder_free(root_builder) on line 532. If git_treebuilder_free internally makes any syscall that fails (or writes to errno), the original opendir failure code is lost. Capture errno immediately after the failing call.
Proposed fix
DIR *root_dir = opendir(dir_path);
if (root_dir == NULL) {
+ int opendir_err = errno;
git_treebuilder_free(root_builder);
- int opendir_err = errno;
return gm_errno_to_result("opendir", dir_path, opendir_err);
}🤖 Prompt for AI Agents
In `@core/src/adapters/git/libgit2_repository_port.c` around lines 530 - 534, The
code reads errno after calling git_treebuilder_free, risking clobbering the
opendir failure code; capture errno immediately after opendir fails and before
calling git_treebuilder_free. Specifically, in the branch where
opendir(dir_path) returns NULL, store the failure code (e.g., int opendir_err =
errno) right away, then call git_treebuilder_free(root_builder) and finally
return gm_errno_to_result("opendir", dir_path, opendir_err); ensure references
to opendir, git_treebuilder_free, errno, gm_errno_to_result, dir_path, and
root_builder are used as described.
| } | ||
|
|
||
| static const gm_git_repository_port_vtbl_t GM_LIBGIT2_REPOSITORY_PORT_VTBL = { | ||
| static const gm_git_repository_port_vtbl_t GM_LIBGIT2_REPOSITORY_PORT_VTBL = { // NOLINT(readability-identifier-naming) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the NOLINT suppression — fix the naming warning instead.
The coding guidelines explicitly state: "NEVER use NOLINT or other mechanisms to suppress clang-tidy warnings; fix the warnings instead." Line 1416 uses // NOLINT(readability-identifier-naming). Rename the constant to comply with the project's naming convention (e.g., gm_libgit2_repository_port_vtbl for a file-scoped static const, or keep UPPER_SNAKE if the project convention treats vtbl constants as macros).
Proposed fix (lower_snake_case with gm_ prefix, matching naming rules for vars)
-static const gm_git_repository_port_vtbl_t GM_LIBGIT2_REPOSITORY_PORT_VTBL = { // NOLINT(readability-identifier-naming)
+static const gm_git_repository_port_vtbl_t gm_libgit2_repository_port_vtbl = {Update the single reference at line 1464 accordingly:
- out_port->vtbl = &GM_LIBGIT2_REPOSITORY_PORT_VTBL;
+ out_port->vtbl = &gm_libgit2_repository_port_vtbl;As per coding guidelines: NEVER use NOLINT to suppress clang-tidy warnings; ALWAYS fix the underlying issue and Naming: functions/vars lower_snake_case with gm_ prefix; macros UPPER_SNAKE.
🤖 Prompt for AI Agents
In `@core/src/adapters/git/libgit2_repository_port.c` at line 1416, Rename the
file-scoped constant GM_LIBGIT2_REPOSITORY_PORT_VTBL to follow project naming
(e.g., gm_libgit2_repository_port_vtbl), remove the trailing "//
NOLINT(readability-identifier-naming)" comment, and update its single reference
where it is used (the initializer/assignment site that currently refers to
GM_LIBGIT2_REPOSITORY_PORT_VTBL) to the new name; ensure the declaration remains
"static const" and the symbol uses lower_snake_case with the gm_ prefix to
satisfy clang-tidy and project naming rules.
| static const size_t TelemetryMaxTags = 5U; | ||
| static const size_t TelemetryRepoIdHexPartLen = 16U; | ||
| static const size_t TelemetryRepoIdHexStrLen = | ||
| TelemetryRepoIdHexPartLen * 2U; | ||
| static const size_t TelemetryRepoIdBufferLen = | ||
| TelemetryRepoIdHexStrLen + 1U; | ||
| static const size_t TelemetryExtraBufferLen = 256U; | ||
| static const size_t TelemetryHashHexDigits = 12U; | ||
| static const size_t TelemetryTotalFnvNibbles = 16U; | ||
| static const unsigned TelemetryHexDigitMask = 0x0FU; | ||
| static const size_t TelemetryNibbleWidth = 4U; | ||
| static const size_t TelemetrySha256PrefixBytes = 6U; | ||
| static const uint64_t TelemetryFnvOffsetBasis = 0xcbf29ce484222325ULL; | ||
| static const uint64_t TelemetryFnvPrime = 0x100000001b3ULL; | ||
| static const char TelemetryHexDigits[] = "0123456789abcdef"; |
There was a problem hiding this comment.
static const size_t creates VLAs — build is broken.
In C (including C23), static const variables are not constant expressions and cannot size arrays. The pipeline confirms VLA errors at Lines 175, 269, and 313 where these are used as array dimensions. The project bans VLAs with -Wvla + -Werror.
Switch to enum or #define:
Proposed fix (partial — apply pattern to all constants used as array sizes)
-static const size_t TelemetryRepoIdHexPartLen = 16U;
-static const size_t TelemetryRepoIdHexStrLen =
- TelemetryRepoIdHexPartLen * 2U;
-static const size_t TelemetryRepoIdBufferLen =
- TelemetryRepoIdHexStrLen + 1U;
-static const size_t TelemetryExtraBufferLen = 256U;
-static const size_t TelemetryHashHexDigits = 12U;
-static const size_t TelemetryTotalFnvNibbles = 16U;
-static const size_t TelemetrySha256PrefixBytes = 6U;
+enum {
+ TelemetryRepoIdHexPartLen = 16,
+ TelemetryRepoIdHexStrLen = TelemetryRepoIdHexPartLen * 2,
+ TelemetryRepoIdBufferLen = TelemetryRepoIdHexStrLen + 1,
+ TelemetryExtraBufferLen = 256,
+ TelemetryHashHexDigits = 12,
+ TelemetryTotalFnvNibbles = 16,
+ TelemetrySha256PrefixBytes = 6,
+};Note: TelemetryNibbleWidth, TelemetryHexDigitMask, TelemetryFnvOffsetBasis, TelemetryFnvPrime, and TelemetryMaxTags should also be converted if used as array sizes, or kept as static const if only used in expressions. The uint64_t constants (TelemetryFnvOffsetBasis, TelemetryFnvPrime) don't fit in enum (which is int-width) — keep those as static const or use #define.
As per coding guidelines, "Language rules: C23, warnings-as-errors; no VLAs."
🤖 Prompt for AI Agents
In `@core/src/telemetry/config.c` around lines 26 - 40, The file uses static const
size_t variables as array dimensions (e.g., TelemetryMaxTags,
TelemetryRepoIdHexPartLen, TelemetryRepoIdHexStrLen, TelemetryRepoIdBufferLen,
TelemetryExtraBufferLen, TelemetryHashHexDigits, TelemetryTotalFnvNibbles,
TelemetryNibbleWidth, TelemetrySha256PrefixBytes, TelemetryHexDigitMask) which
produces VLAs; change those constants used for array sizes to compile-time
constants (use enum { ... } or `#define` for integer-sized constants) so they are
true constant expressions, and retain TelemetryHexDigits as the char array; keep
the 64-bit FNV constants (TelemetryFnvOffsetBasis, TelemetryFnvPrime) as static
const uint64_t or convert them to `#define` if needed because enum won’t hold
64-bit values. Ensure all call sites and array declarations (where
TelemetryRepoIdBufferLen, TelemetryExtraBufferLen, TelemetryTotalFnvNibbles,
etc. are used) are updated to reference the new enum/#define names.
| # Graph-Lvl-Up-Tank.md | ||
| --- | ||
| title: Graph LvL-Up Tank 🛡️🧠 | ||
| description: Crash-course to power-level new contributors into graph-native thinking for git-mind. | ||
| audience: [contributors] | ||
| domain: [learning] | ||
| tags: [graphs, onboarding, best-practices] | ||
| status: draft | ||
| last_updated: 2025-09-16 | ||
| --- | ||
|
|
||
| # Graph LvL-Up Tank 🛡️🧠 |
There was a problem hiding this comment.
Combined file violates docs structure guidelines: multiple H1s, misplaced front matter, missing ToC.
This file concatenates ~8 separate documents into one, resulting in multiple H1 headings and front matter blocks embedded mid-file (front matter is only valid at the very start of a file). The coding guidelines require a single H1, proper front matter at the top, and a ## Table of Contents.
Consider either:
- Splitting into individual files under
docs/talk-shop/(each with its own front matter, single H1, and ToC), or - Restructuring as a single document with one H1 (e.g., "Talk Shop Archive"), a ToC, and demoting the current H1s to H2s.
As per coding guidelines: {docs/**/*.md,README.md}: Docs must have front matter, a single H1, a '## Table of Contents', and titles matching the H1.
🤖 Prompt for AI Agents
In `@docs/talk-shop/combined_markdown.md` around lines 1 - 12, The combined
Markdown file contains multiple front-matter blocks (---) and multiple H1
headings like "# Graph LvL-Up Tank 🛡️🧠", violating the docs rules; either
split this monolithic file into separate files (one per original document) each
with its own front matter, a single H1 matching the file title, and a "## Table
of Contents", or convert the file into a single consolidated document (e.g.,
"Talk Shop Archive") with only one H1 at the top, a proper front-matter block at
the top, a "## Table of Contents", and demote all other H1s to H2s/H3s; ensure
each new file or the consolidated file's front-matter title matches the
top-level H1 and that no mid-file front-matter blocks remain.
| • Ref explosion – Every transaction spawns a ref subtree. Without a cleanup strategy, you’ll drown in pending/ and applied/ clutter. Right now you punt on retention. | ||
| • Rebase/replay cost – Replay edge CBOR payloads on schema drift is non-trivial. You call it “re-run append logic,” but that may need its own test suite (duplicate ULIDs vs. schema version bumps are very different beasts). | ||
| • Cache invisibility – You say “must be invisible unless explicitly targeted,” but don’t outline how consumers distinguish. A flag in gm_journal_read_txn is fine, but you’ll need guardrails so rogue readers don’t accidentally pull pending refs. | ||
| • Concurrency model – No multi-user locking is fine for v1, but you should acknowledge the risk of racey concurrent applies. Without at least a warning system, two contributors could both “fast-forward” in their own heads and then clobber. |
There was a problem hiding this comment.
Typo: "racey" → "racy".
-but you should acknowledge the risk of racey concurrent applies.
+but you should acknowledge the risk of racy concurrent applies.📝 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.
| • Concurrency model – No multi-user locking is fine for v1, but you should acknowledge the risk of racey concurrent applies. Without at least a warning system, two contributors could both “fast-forward” in their own heads and then clobber. | |
| • Concurrency model – No multi-user locking is fine for v1, but you should acknowledge the risk of racy concurrent applies. Without at least a warning system, two contributors could both "fast-forward" in their own heads and then clobber. |
🧰 Tools
🪛 LanguageTool
[grammar] ~229-~229: Ensure spelling is correct
Context: ... but you should acknowledge the risk of racey concurrent applies. Without at least a ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In `@docs/talk-shop/combined_markdown.md` at line 229, Update the typo in the
"Concurrency model" note by replacing the word "racey" with the correct spelling
"racy" in the combined_markdown.md content where the sentence mentions
concurrent applies and multi-user locking risks; keep the rest of the sentence
unchanged.
| > Please read through @AGENTS.md and @README.md and, I know it's a lot, but I need you to read all of the docs/ and all of the source code lol I really need you to | ||
| be familiar as fuck with this project. Read up, and let me know when you understand how everything works. I've got some work for you! | ||
|
|
||
| ✦ This is a fascinating project. I've reviewed the AGENTS.md and README.md files. I understand the core vision of git-mind as a serverless knowledge graph built | ||
| on top of Git, and I'm aware of the high-priority mandate to migrate the codebase to a hexagonal architecture. The instructions in AGENTS.md are clear, | ||
| especially regarding the build process (make ci-local), the "One-Thing Rule," and the iterative plan for refactoring modules and driving clang-tidy warnings to | ||
| zero. | ||
|
|
||
| To proceed, I will now read the rest of the documentation and the source code to gain the comprehensive understanding you've requested. This will be a | ||
| multi-step process. I'll start by exploring the file structure of the docs, include, and core directories to create a reading plan. | ||
|
|
||
| I will begin by listing the contents of these directories. | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadFolder docs │ | ||
| │ │ | ||
| │ Listed 36 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder include │ | ||
| │ │ | ||
| │ Listed 2 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core │ | ||
| │ │ | ||
| │ Listed 6 item(s). │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ Okay, I have the initial directory structures. Based on this, I'll proceed with a more detailed exploration. My next step is to list the contents of the most | ||
| critical subdirectories to map out the architecture and source code organization. I'll start with docs/architecture, the public API in include/gitmind, the | ||
| core source in core/src, and the tests in core/tests. | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadFolder docs/architecture │ | ||
| │ │ | ||
| │ Listed 20 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder include/gitmind │ | ||
| │ │ | ||
| │ Listed 9 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src │ | ||
| │ │ | ||
| │ Listed 19 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/tests │ | ||
| │ │ | ||
| │ Listed 5 item(s). │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ Excellent. The directory listings confirm that the project structure is closely following the hexagonal architecture plan from AGENTS.md. I see the distinct | ||
| domain, ports, adapters, and app layers in core/src. | ||
|
|
||
| My next step is to create a comprehensive map of all the files I need to read. I will now list the contents of the key architectural directories I've | ||
| identified. This will give me a complete file list to guide my reading, starting from the highest level of abstraction (the documentation and ports) and | ||
| moving down to the concrete implementations. | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadFolder docs/architecture/hexagonal │ | ||
| │ │ | ||
| │ Listed 1 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder include/gitmind │ | ||
| │ │ | ||
| │ Listed 9 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/domain │ | ||
| │ │ | ||
| │ Listed 4 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/ports │ | ||
| │ │ | ||
| │ Listed 3 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/adapters │ | ||
| │ │ | ||
| │ Listed 6 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/app │ | ||
| │ │ | ||
| │ Listed 1 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/tests/unit │ | ||
| │ │ | ||
| │ Listed 39 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/tests/fakes │ | ||
| │ │ | ||
| │ Listed 5 item(s). │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ Okay, I have a comprehensive file map. The structure strongly reflects the hexagonal architecture principles outlined in AGENTS.md. | ||
|
|
||
| I will now begin the reading process in a structured way to build a layered understanding of the project. I'll start with the high-level documentation to | ||
| grasp the core concepts and architectural vision. | ||
|
|
||
| I am now reading the key architecture and philosophy documents. | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadManyFiles Will attempt to read and concatenate files using patterns: │ | ||
| │ /User… │ | ||
| │ │ | ||
| │ ### ReadManyFiles Result (Target Dir: `/Users/james/git/git-mind`) │ | ||
| │ │ | ||
| │ Successfully read and concatenated content from **5 file(s)**. │ | ||
| │ │ | ||
| │ **Processed Files:** │ | ||
| │ - `docs/TECHNICAL.md` │ | ||
| │ - `docs/architecture/System_Architecture.md` │ | ||
| │ - `docs/architecture/hexagonal/Journal.md` │ | ||
| │ - `docs/philosophy.md` │ | ||
| │ - `docs/roadmap.md` │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ I've absorbed the high-level documentation. My understanding is that git-mind is a Git-native knowledge graph that stores its data (a "Journal" of CBOR-encoded | ||
| edges) in custom refs, and uses a separate, rebuildable ref for a performance cache ("Roaring Bitmaps"). The entire project is undergoing a migration to a | ||
| strict hexagonal architecture, decoupling the core domain logic from infrastructure like libgit2 and the filesystem through a system of "ports" and "adapters." | ||
| The development process emphasizes safety via Docker-only builds and code quality via clang-tidy. | ||
|
|
||
| The next logical step is to understand the specific contracts of the API and the architectural seams. I will now inspect the public API headers and the | ||
| crucial port definitions. I'll begin by listing the contents of include/gitmind/ and the ports directories to identify all the relevant header files to | ||
| read. | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadFolder include/gitmind │ | ||
| │ │ | ||
| │ Listed 9 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/include/gitmind/ports │ | ||
| │ │ | ||
| │ Listed 9 item(s). │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ Okay, I have the file lists for the public API and the port definitions. These headers represent the formal contracts of the application and its architectural | ||
| seams, so they are my next reading priority. | ||
|
|
||
| I will now read the main gitmind.h umbrella header along with the key public API headers and all of the port definition headers. This will clarify the core | ||
| data structures and the boundaries for every external interaction. | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadManyFiles Will attempt to read and concatenate files using patterns: │ | ||
| │ /User… │ | ||
| │ │ | ||
| │ ### ReadManyFiles Result (Target Dir: `/Users/james/git/git-mind`) │ | ||
| │ │ | ||
| │ Successfully read and concatenated content from **14 file(s)**. │ | ||
| │ │ | ||
| │ **Processed Files (first 10 shown):** │ | ||
| │ - `core/include/gitmind/ports/cache_build_port.h` │ | ||
| │ - `core/include/gitmind/ports/cache_query_port.h` │ | ||
| │ - `core/include/gitmind/ports/diagnostic_port.h` │ | ||
| │ - `core/include/gitmind/ports/env_port.h` │ | ||
| │ - `core/include/gitmind/ports/fs_temp_port.h` │ | ||
| │ - `core/include/gitmind/ports/git_repository_port.h` │ | ||
| │ - `core/include/gitmind/ports/journal_command_port.h` │ | ||
| │ - `core/include/gitmind/ports/logger_port.h` │ | ||
| │ - `core/include/gitmind/ports/metrics_port.h` │ | ||
| │ - `include/gitmind.h` │ | ||
| │ - …and 4 more. │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ I have now reviewed the public API headers and the port definitions. This clarifies the practical application of the hexagonal architecture. | ||
|
|
||
| My understanding is now much more concrete: | ||
|
|
||
| * Ports as Contracts: The *_port.h files define the system's formal boundaries. They are C vtables (structs of function pointers) that create a powerful | ||
| abstraction layer. | ||
| * Inbound Ports (e.g., journal_command_port, cache_query_port) define what the application can do. They are the entry points for commands and queries. | ||
| * Outbound Ports (e.g., git_repository_port, fs_temp_port, logger_port) define what the application needs. They abstract away all external systems, | ||
| including libgit2, the filesystem, and observability tooling. | ||
| * Dependency Injection: A gm_context_t struct is used to hold all the port implementations, allowing for clean dependency injection throughout the | ||
| application. This makes it possible to swap out real implementations (like a libgit2-backed git port) with fakes for testing. | ||
| * Core Data Structures: The public headers define key structs like gm_edge_t (a graph link), gm_attribution_t (who made the link), and gm_result_t (a | ||
| standardized error-handling mechanism). | ||
|
|
||
| I now have a solid grasp of the application's formal interfaces. The next logical step is to understand the core business logic that operates within these | ||
| boundaries. | ||
|
|
||
| I will now proceed to read the source code of the Domain Layer. I'll start by listing the files within the core/src/domain subdirectories to get a precise | ||
| list, and then I will read them. | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadFolder core/src/domain/attribution │ | ||
| │ │ | ||
| │ Listed 2 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/domain/cache │ | ||
| │ │ | ||
| │ Listed 2 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/domain/edge │ | ||
| │ │ | ||
| │ Listed 1 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/domain/journal │ | ||
| │ │ | ||
| │ Listed 3 item(s). │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ I have the file list for the domain layer. The small, focused nature of these modules is a strong indicator that they contain the pure business logic as | ||
| prescribed by the hexagonal architecture. | ||
|
|
||
| Before I read the implementation files, I need to see their corresponding internal header files to understand their contracts within the core application. | ||
| I'll list the contents of core/include/gitmind/ to find them. | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadFolder core/include/gitmind │ | ||
| │ │ | ||
| │ Listed 28 item(s). (1 git-ignored) │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ I have the complete file list for the internal headers. Now I can create a definitive reading list for the domain layer. | ||
|
|
||
| I will read the C implementation files from core/src/domain/ and their corresponding header files from core/include/gitmind/. I will also include the | ||
| fundamental type, result, and error headers, as they are essential for understanding the domain code. | ||
|
|
||
| This is the heart of the application's logic. I am proceeding to read these files now. | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadManyFiles Will attempt to read and concatenate files using patterns: │ | ||
| │ /User… │ | ||
| │ │ | ||
| │ ### ReadManyFiles Result (Target Dir: `/Users/james/git/git-mind`) │ | ||
| │ │ | ||
| │ Successfully read and concatenated content from **16 file(s)**. │ | ||
| │ │ | ||
| │ **Processed Files (first 10 shown):** │ | ||
| │ - `core/include/gitmind/cache.h` │ | ||
| │ - `core/include/gitmind/context.h` │ | ||
| │ - `core/include/gitmind/edge.h` │ | ||
| │ - `core/include/gitmind/edge_attributed.h` │ | ||
| │ - `core/include/gitmind/error.h` │ | ||
| │ - `core/include/gitmind/journal.h` │ | ||
| │ - `core/include/gitmind/result.h` │ | ||
| │ - `core/include/gitmind/types.h` │ | ||
| │ - `core/src/domain/attribution/defaults.c` │ | ||
| │ - `core/src/domain/attribution/env_loader.c` │ | ||
| │ - …and 6 more. │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ I have finished reading the domain layer source code and headers. My understanding of the project's architecture is now significantly more detailed. | ||
|
|
||
| Here are my key takeaways: | ||
|
|
||
| * Purity and Isolation: The domain layer is, as designed, completely pure. It has no knowledge of libgit2, filesystems, or any other external dependency. Its | ||
| functions operate on data structures, perform business logic (like planning a commit or serializing an edge to CBOR), and return a gm_result_t. This is a | ||
| textbook implementation of a hexagonal core. | ||
| * Robust Error Handling: The gm_result_t type and GM_TRY macro are used consistently, creating a clean, monadic error-handling flow that prevents errors from | ||
| being silently ignored. | ||
| * Clear Responsibilities: | ||
| * The Journal domain is responsible for creating "commit plans" (append_planner.c) and handling the encoding/decoding of the journal's data format | ||
| (codec.c, read_decoder.c), including migrating legacy formats. It decides what should be in a commit, but doesn't perform the commit itself. | ||
| * The Edge domain is responsible for the serialization of the core gm_edge_attributed_t data structure into a CBOR byte stream. | ||
| * The Cache domain contains pure helper logic for the cache, like an in-memory hash map (edge_map.c) and OID prefix calculations (oid_prefix.c). | ||
| * Dependency Injection via `gm_context_t`: The gm_context_t struct is the lynchpin of the architecture. It holds all the ports (the vtables for external | ||
| services), allowing the application layer to inject the necessary dependencies (like the git_repository_port) into the functions that orchestrate the domain | ||
| logic. | ||
|
|
||
| I now have a firm grasp of the "what" (the ports) and the "why" (the pure domain logic). My next step is to understand how the domain logic is orchestrated. | ||
| I will now examine the Application Layer. I'll start by listing the files in core/src/app and the other modules at the top level of core/src. | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadFolder core/src/app │ | ||
| │ │ | ||
| │ Listed 1 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/journal │ | ||
| │ │ | ||
| │ Listed 2 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/cache │ | ||
| │ │ | ||
| │ Listed 3 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/ports │ | ||
| │ │ | ||
| │ Listed 3 item(s). │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ I have the file listings for the Application Layer. These modules appear to be the orchestrators that connect the pure domain logic to the outside world via | ||
| the ports. | ||
|
|
||
| My hypothesis is: | ||
| * journal/writer.c and journal/reader.c handle the application logic for writing and reading the journal, using the git_repository_port for I/O and the pure | ||
| domain/journal functions for planning and decoding. | ||
| * cache/builder.c and cache/query.c manage the cache lifecycle, orchestrating the reading of the journal and the construction/querying of the bitmap indexes. | ||
| * ports/ contains the concrete implementations of the inbound driving ports, acting as the primary entry points for use cases like journal_append. | ||
|
|
||
| I will now read the source and header files for this layer to confirm my understanding. I'll start by listing the relevant directories to get a complete | ||
| file list. | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadFolder core/include/gitmind/journal │ | ||
| │ │ | ||
| │ Listed 1 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/include/gitmind/cache │ | ||
| │ │ | ||
| │ Listed 3 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/ports │ | ||
| │ │ | ||
| │ Listed 3 item(s). │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ The directory listings were not as deep as I needed. The headers for the application-layer services like writer.c and builder.c must be in the internal | ||
| subdirectories. I also need to see the implementation files for the inbound ports. | ||
|
|
||
| I will now perform a more targeted listing of the following directories to get the complete file list for the Application Layer: | ||
| * core/include/gitmind/journal/internal/ | ||
| * core/include/gitmind/cache/internal/ | ||
| * core/src/ports/journal/ | ||
| * core/src/ports/cache/ | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadFolder …core/include/gitmind/journal/internal │ | ||
| │ │ | ||
| │ Listed 3 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/include/gitmind/cache/internal │ | ||
| │ │ | ||
| │ Listed 4 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/ports/journal │ | ||
| │ │ | ||
| │ Listed 1 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/ports/cache │ | ||
| │ │ | ||
| │ Listed 2 item(s). │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ I have the complete file list for the Application Layer. My initial assessment was close: the files in core/src/journal and core/src/cache are the core | ||
| service implementations, and the files in core/src/ports are the concrete implementations of the inbound ports that orchestrate these services. | ||
|
|
||
| I will now read the source files for the services and the inbound port implementations. This will show me how a use case, like appending an edge, flows | ||
| through the application—from the port, through the orchestration service, to the domain logic, and out to the infrastructure via the outbound ports. | ||
|
|
||
| I am now reading the Application Layer files. | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadManyFiles Will attempt to read and concatenate files using patterns: │ | ||
| │ /User… │ | ||
| │ │ | ||
| │ ### ReadManyFiles Result (Target Dir: `/Users/james/git/git-mind`) │ | ||
| │ │ | ||
| │ Successfully read and concatenated content from **9 file(s)**. │ | ||
| │ │ | ||
| │ **Processed Files:** │ | ||
| │ - `core/include/gitmind/cache/internal/rebuild_service.h` │ | ||
| │ - `core/src/app/cache/cache_rebuild_service.c` │ | ||
| │ - `core/src/cache/builder.c` │ | ||
| │ - `core/src/cache/query.c` │ | ||
| │ - `core/src/journal/reader.c` │ | ||
| │ - `core/src/journal/writer.c` │ | ||
| │ - `core/src/ports/cache/cache_build_port.c` │ | ||
| │ - `core/src/ports/cache/cache_query_port.c` │ | ||
| │ - `core/src/ports/journal/journal_command_port.c` │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ I have completed my review of the Application Layer. My understanding of the architecture is now very concrete, as I can see how the different layers | ||
| collaborate to fulfill a use case. | ||
|
|
||
| Here is my synthesis: | ||
|
|
||
| * Clear Orchestration: This layer acts as the "manager." It doesn't contain the core business rules but instead directs the flow of a use case. For example, | ||
| the journal/writer.c service orchestrates the process of appending an edge: it calls the Domain layer to serialize the edge data into CBOR, then calls the | ||
| git_repository_port (an outbound port) to write the resulting commit and update the Git ref. | ||
| * A Perfect Use Case Example (`gm_journal_append_attributed`): | ||
| 1. The call starts at an Inbound Port (journal_command_port.c). | ||
| 2. The port calls the Application Service (journal/writer.c). | ||
| 3. The service calls a pure Domain Function (gm_edge_attributed_encode_cbor) to get the CBOR byte stream for the edge. | ||
| 4. The service then calls another pure Domain Function (gm_journal_build_commit_plan) to get a description of the Git commit that needs to be created. | ||
| 5. Finally, the service uses the Outbound Port (gm_git_repository_port) to execute the plan, writing the commit and updating the ref in Git. | ||
| * Decoupling in Action: At no point does the application or domain layer have any direct knowledge of libgit2 or the filesystem. All interactions are | ||
| mediated through the ports defined in the gm_context_t. This is a textbook hexagonal architecture. The cache_rebuild_service.c is another prime example, | ||
| using the fs_temp_port for temporary files and the git_repository_port for all Git I/O. | ||
|
|
||
| I now understand the "what" (Ports), the "why" (Domain), and the "how" (Application). The final piece of the puzzle is to see the concrete implementations | ||
| of the outbound ports—the Adapters that talk to the real world. | ||
|
|
||
| I will now proceed to the final layer of the source code: the Adapters. I'll start by listing the files in core/src/adapters. | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadFolder core/src/adapters │ | ||
| │ │ | ||
| │ Listed 6 item(s). │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ I have the list of adapter categories. Now I need to see the specific implementation files within them to understand how the abstract ports are connected to | ||
| concrete technologies like libgit2 and the filesystem. | ||
|
|
||
| I will now list the contents of each of these adapter directories. My primary focus will be on the git adapter, as it's the most critical piece of | ||
| infrastructure. | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadFolder core/src/adapters/config │ | ||
| │ │ | ||
| │ Listed 1 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/adapters/diagnostics │ | ||
| │ │ | ||
| │ Listed 1 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/adapters/fs │ | ||
| │ │ | ||
| │ Listed 1 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/adapters/git │ | ||
| │ │ | ||
| │ Listed 1 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/adapters/logging │ | ||
| │ │ | ||
| │ Listed 1 item(s). │ | ||
| │ │ | ||
| │ ✓ ReadFolder core/src/adapters/metrics │ | ||
| │ │ | ||
| │ Listed 2 item(s). │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ Excellent, I have the full list of adapter implementations. This is the final layer, where the abstract ports connect to concrete technologies. | ||
|
|
||
| My reading plan is now focused on these key adapter implementations. I will prioritize the libgit2 adapter, as it is the most complex and critical, followed | ||
| by the filesystem and logging adapters. This will complete my understanding of the entire architecture, from the application's entry points down to the | ||
| infrastructure. | ||
|
|
||
| I am now reading the selected adapter source files. | ||
| ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ | ||
| │ ✓ ReadManyFiles Will attempt to read and concatenate files using patterns: │ | ||
| │ /User… │ | ||
| │ │ | ||
| │ ### ReadManyFiles Result (Target Dir: `/Users/james/git/git-mind`) │ | ||
| │ │ | ||
| │ Successfully read and concatenated content from **6 file(s)**. │ | ||
| │ │ | ||
| │ **Processed Files:** │ | ||
| │ - `core/src/adapters/config/env_adapter.c` │ | ||
| │ - `core/src/adapters/diagnostics/stderr_diagnostics_adapter.c` │ | ||
| │ - `core/src/adapters/fs/posix_temp_adapter.c` │ | ||
| │ - `core/src/adapters/git/libgit2_repository_port.c` │ | ||
| │ - `core/src/adapters/logging/stdio_logger_adapter.c` │ | ||
| │ - `core/src/adapters/metrics/null_metrics_adapter.c` │ | ||
| ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ | ||
| ✦ I have completed my comprehensive review of the git-mind project, from the high-level documentation down to the infrastructure adapters. I have a deep and | ||
| layered understanding of how the system works. | ||
|
|
||
| Here is a summary of my understanding: | ||
|
|
||
| * Core Vision & Architecture: The project's goal is to create a Git-native knowledge graph. It achieves this through a rigorous Hexagonal Architecture. The | ||
| core of the application is a pure, side-effect-free Domain Layer that contains all business logic. This core is completely isolated from the outside world. | ||
|
|
||
| * Ports and Adapters: | ||
| * All interactions with the outside world (like Git operations, filesystem access, or logging) are defined by abstract interfaces called Ports (vtables | ||
| of function pointers). | ||
| * The Application Layer orchestrates the domain logic and interacts with these abstract ports. For example, to write to the journal, it calls the domain | ||
| to get a "commit plan," then tells the git_repository_port to execute that plan. | ||
| * Concrete implementations of these ports are called Adapters. The most critical is the libgit2_repository_port, which translates the abstract Git | ||
| operations into specific calls to the libgit2 library. This is the only layer that knows about libgit2. | ||
|
|
||
| * Data Flow (Example: Appending an Edge): | ||
| 1. An Inbound Port (e.g., journal_command_port) receives the request. | ||
| 2. It calls an Application Service (e.g., journal/writer.c). | ||
| 3. The service calls the Domain Layer to serialize the edge data into a CBOR byte stream. | ||
| 4. The service then uses an Outbound Port (git_repository_port) to create a Git commit with the CBOR data and update the appropriate journal ref | ||
| (refs/gitmind/edges/…). | ||
| 5. The call is dispatched to the `libgit2` Adapter, which executes the actual Git operations. | ||
|
|
||
| * Key Principles: | ||
| * Dependency Injection: A gm_context_t struct holds all the port implementations, which is passed to the application services, ensuring clean separation. | ||
| * Robustness: The gm_result_t type is used universally for error handling, ensuring that errors are never ignored. | ||
| * Testability: The architecture allows any adapter (e.g., the libgit2 adapter) to be swapped with a "fake" for unit testing, enabling the core logic to | ||
| be tested in complete isolation. | ||
|
|
||
| I understand the project's structure, the data storage model (Journal and Cache), the build process (make ci-local), and the overarching development | ||
| philosophy. | ||
|
|
||
| I am ready for my first assignment. No newline at end of file |
There was a problem hiding this comment.
This file should not be in the PR — it's an AI conversation transcript breaking CI.
This appears to be a raw Gemini session log accidentally committed. It:
- Breaks CI: The pipeline reports numerous markdownlint violations (MD041, MD050, MD004, MD007) originating from this file.
- Leaks local paths: Contains
/Users/james/git/git-mindfilesystem paths. - Has no documentation value: It's a conversation transcript, not a docs artifact.
Remove this file from the PR to unblock the markdown lint gate.
🧰 Tools
🪛 GitHub Actions: C-Core Gate
[error] 1-1: MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "> Please read through @AGENTS...."]
[error] 97-97: MD050/strong-style Strong style [Expected: underscore; Actual: asterisk]
[error] 99-99: MD050/strong-style Strong style [Expected: underscore; Actual: asterisk]
[error] 134-134: MD050/strong-style Strong style [Expected: underscore; Actual: asterisk]
[error] 153-153: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
[error] 153-153: MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 3]
[error] 155-155: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
[error] 155-155: MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 7]
[error] 156-156: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
[error] 156-156: MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 7]
[error] 158-158: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
[error] 158-158: MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 3]
[error] 160-160: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
[error] 160-160: MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 3]
[error] 207-207: MD050/strong-style Strong style [Expected: underscore; Actual: asterisk]
[error] 207-207: MD050/strong-style Strong style [Expected: underscore; Actual: asterisk]
[error] 209-209: MD050/strong-style Strong style [Expected: underscore; Actual: asterisk]
[error] 209-209: MD050/strong-style Strong style [Expected: underscore; Actual: asterisk]
[error] 226-226: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
[error] 226-226: MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 3]
[error] 229-229: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
[error] 229-229: MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 3]
[error] 231-231: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
[error] 231-231: MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 3]
[error] 232-232: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
[error] 232-232: MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 7]
[error] 234-234: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
[error] 234-234: MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 3]
[error] 235-235: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
[error] 235-235: MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 3]
[error] 236-236: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
[error] 236-236: MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 3]
🪛 LanguageTool
[style] ~1-~1: Consider removing “of” to be more concise
Context: ...know it's a lot, but I need you to read all of the docs/ and all of the source code lol I ...
(ALL_OF_THE)
[style] ~1-~1: Consider removing “of” to be more concise
Context: ...I need you to read all of the docs/ and all of the source code lol I really need you to ...
(ALL_OF_THE)
[style] ~126-~126: Consider removing “of” to be more concise
Context: ...ong with the key public API headers and all of the port definition headers. This will clar...
(ALL_OF_THE)
🤖 Prompt for AI Agents
In `@Gemini` Explores git-mind.md around lines 1 - 447, The file "Gemini Explores
git-mind.md" is an accidental AI transcript that breaks CI (markdownlint errors
MD041/MD050/MD004/MD007) and leaks local paths; remove it from the PR by
deleting the file from the branch (or git rm and amend/rebase the offending
commit), push the updated branch to update the PR, and re-run CI; also scan the
commit for any other transcript files or leaked local paths and remove them
before pushing so the markdownlint gate no longer fails.
| Here is my breakdown of how shiplog works. | ||
|
|
||
| High-Level Summary | ||
|
|
||
| Shiplog is a Git-native deployment ledger written entirely in Bash. Its purpose is to create an immutable, cryptographically-signed audit trail of operational | ||
| events—such as deployments, rollbacks, or hotfixes—directly within a Git repository. It requires no external databases or services. The core of the system is a | ||
| powerful CLI tool, git-shiplog, which is composed of a main executable and a library of modular shell scripts. The project places a very strong emphasis on | ||
| safety and portability, with a rigorous, Docker-based test suite that runs across multiple Linux distributions. |
There was a problem hiding this comment.
This file is causing pipeline failures and appears to be a scratch/draft conversation transcript.
The CI pipeline reports multiple markdownlint errors on this file:
- MD041: First line must be a top-level heading (line 1 is plain text).
- MD004: Unordered list style must use dashes, not asterisks (line 19+).
- MD033: Inline HTML detected.
Additionally, the file named Untitled.md at the repo root, containing what appears to be a raw AI design conversation, lacks all required doc structure (front matter, H1, ToC). Consider either:
- Removing this file from the repo if it's a scratch artifact, or
- Moving it to
docs/talk-shop/(e.g.,libgitledger_design_discussion.md) with proper front matter, a single H1, ToC, and markdownlint-compliant formatting.
🧰 Tools
🪛 GitHub Actions: C-Core Gate
[error] 1-1: MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "Here is my breakdown of how sh..."]
🪛 LanguageTool
[style] ~7-~7: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...dular shell scripts. The project places a very strong emphasis on safety and portability, w...
(EN_WEAK_ADJECTIVE)
🤖 Prompt for AI Agents
In `@Untitled.md` around lines 1 - 8, The Untitled.md file at repo root is a draft
causing markdownlint failures (MD041, MD004, MD033); either delete this scratch
file or move it to docs/talk-shop/ (e.g., libgitledger_design_discussion.md) and
then fix formatting by adding proper front matter and a single H1, generate a
Table of Contents, replace any asterisk-style unordered lists with dashes,
remove any inline HTML, and ensure the first line is a top-level heading to
satisfy MD041; confirm lint passes after changes.
| How close am I? | ||
|
|
||
| > Bingo! I have been granted an opportunity to be paid to work on Open Source Software full-time for a little while. I can work on anything I want. The overall | ||
| consensus, after discussing it with a counsil of AI haha, was the extracting that core "append-only git-native ledger" from these two applications would be an |
There was a problem hiding this comment.
Typo: "counsil" → "council".
-consensus, after discussing it with a counsil of AI haha, was the extracting that core
+consensus, after discussing it with a council of AI haha, was the extracting that core📝 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.
| consensus, after discussing it with a counsil of AI haha, was the extracting that core "append-only git-native ledger" from these two applications would be an | |
| consensus, after discussing it with a council of AI haha, was the extracting that core "append-only git-native ledger" from these two applications would be an |
🧰 Tools
🪛 LanguageTool
[grammar] ~97-~97: Ensure spelling is correct
Context: ... consensus, after discussing it with a counsil of AI haha, was the extracting that cor...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In `@Untitled.md` at line 97, Fix the typo in the sentence containing "counsil" by
replacing "counsil" with the correct spelling "council" in the line that reads
"consensus, after discussing it with a counsil of AI haha, was the extracting
that core 'append-only git-native ledger'..." so the phrase becomes "consensus,
after discussing it with a council of AI haha, was the extracting that core
'append-only git-native ledger'...".
2 participants
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests