Refactor temp repos and isolate CI workspace #178
Conversation
WalkthroughAdds a public telemetry log-format API and formatter seam, disposables for diagnostics and crypto contexts, capacity-aware journal codec/decoder and monotonic timing, fs-temp canonicalization/view safety, shared test temp-repo helpers and test support lib, CI/debug staging tooling, numerous adapters/header/test updates, and related docs. 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 stderr adapter
Diag->>Stderr: allocate state, emit_enabled = true
alt init ok
Stderr-->>CLI: provide vtbl + dispose callback
else init failed
Stderr-->>CLI: free error
end
CLI->>Logger: init stdio logger (store dispose)
CLI->>User: run subcommand
CLI->>Diag: gm_diag_reset(&diag_port) on shutdown
note right of Diag: gm_diag_reset calls vtbl.dispose if present\nand clears port pointers
CLI->>Logger: call logger_port_dispose() or fallback stdio dispose
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 pointer provided)
else error
Codec-->>Reader: error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…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", "[email protected]") < 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", "[email protected]") < 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
docs/code-reviews/PR178/1a49c6f455c33f1dcf87905f804020b6654be938.md
Outdated
Show resolved
Hide resolved
docs/code-reviews/PR178/e1215ee158d04aca9718a9e775212be3b70a15f8.md
Outdated
Show resolved
Hide resolved
…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}
| GM_ERROR(GM_ERR_IO_FAILED, "unlinkat failed for %s: %s", name, | ||
| strerror(errno))); | ||
| if (!S_ISDIR(root_stat.st_mode)) { | ||
| if (unlink(normalized) != 0 && errno != ENOENT) { |
Check failure
Code scanning / CodeQL
Time-of-check time-of-use filesystem race condition High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
General approach
The classic way to prevent TOCTOU vulnerabilities is to avoid acting on file names after a check—instead, act on open file descriptors. When deleting a non-directory file (not a directory itself), instead of calling stat() then unlink(path), you can open the file with open() using the O_NOFOLLOW and (if supported) O_PATH flags. Then, unlink through an open file descriptor using unlinkat(), making sure not to follow symlinks.
Detailed fix
In this code block:
- Instead of calling
staton the path, thenunlink, open the file withopen()usingO_NOFOLLOW(andO_PATHif available), and then callunlinkat()using the open directory file descriptor and filename (preferably). - However,
unlinkat()accepts a directory file descriptor—by opening the parent directory, you can modify the specific entry in a race-resistant way. - If possible, parse the path, open the parent directory with
O_DIRECTORY, then callunlinkat()on the filename relative to that fd, with theAT_REMOVEDIRflag unset. - Only if this fails with ENOENT does the function return OK, otherwise it issues an error.
Concrete code changes
- Introduce
#include <fcntl.h>if not present. - In the code block for removing a non-directory file (i.e., the
!S_ISDIR(root_stat.st_mode)branch), instead ofunlink(normalized), parsenormalizedinto parent directory and file name, open parent directory withopen()/O_DIRECTORY, and callunlinkat()on the file.
These changes all occur within core/src/adapters/fs/posix_temp_adapter.c, specifically in the non-directory-removal branch within remove_tree_impl.
| @@ -12,10 +12,11 @@ | ||
| #include <stdlib.h> | ||
| #include <string.h> | ||
| #include <sys/stat.h> | ||
| #include <libgen.h> | ||
| #include <sys/types.h> | ||
| #include <time.h> | ||
| #include <unistd.h> | ||
|
|
||
| #include <fcntl.h> | ||
| #include "gitmind/constants_internal.h" | ||
| #include "gitmind/error.h" | ||
| #include "gitmind/fs/path_utils.h" | ||
| @@ -518,9 +516,32 @@ | ||
| } | ||
|
|
||
| if (!S_ISDIR(root_stat.st_mode)) { | ||
| if (unlink(normalized) != 0 && errno != ENOENT) { | ||
| return errno_to_result("unlink", normalized); | ||
| // Use unlinkat to reduce TOCTOU window | ||
| char parent_dir[GM_PATH_MAX]; | ||
| char file_name[GM_PATH_MAX]; | ||
| // Copy normalized to get dirname and basename separately | ||
| if (gm_strcpy_safe(parent_dir, sizeof(parent_dir), normalized) != GM_OK || | ||
| gm_strcpy_safe(file_name, sizeof(file_name), normalized) != GM_OK) { | ||
| return gm_err_void( | ||
| GM_ERROR(GM_ERR_PATH_TOO_LONG, | ||
| "normalized path exceeds buffer while parsing path")); | ||
| } | ||
| // Use POSIX dirname and basename (thread unsafe), so save before use | ||
| char *dir = dirname(parent_dir); | ||
| char *base = basename(file_name); | ||
| int dfd = open(dir, O_DIRECTORY | O_CLOEXEC); | ||
| if (dfd == -1) { | ||
| if (errno == ENOENT) { | ||
| return gm_ok_void(); | ||
| } | ||
| return errno_to_result("open (parent dir)", dir); | ||
| } | ||
| int rc = unlinkat(dfd, base, 0); | ||
| int saved_errno = errno; | ||
| close(dfd); | ||
| if (rc != 0 && saved_errno != ENOENT) { | ||
| return errno_to_result("unlinkat", normalized); | ||
| } | ||
| return gm_ok_void(); | ||
| } | ||
|
|
| stack[depth++] = | ||
| (dir_stack_frame_t){.dir = child_dir, .path_len = current_len}; | ||
| } else { | ||
| if (unlink(current) != 0 && errno != ENOENT) { |
Check failure
Code scanning / CodeQL
Time-of-check time-of-use filesystem race condition High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
General fix:
Eliminate the window between stat (the check) and unlink (the use) by interacting with the file in an atomic fashion, using file descriptors instead of repeated name-based access. This avoids the risk of the file being replaced between the operations.
Detailed approach:
- When encountering a non-directory file (the
elseblock at line 605), instead of just callingunlink(current), open the file withopen(current, O_NONBLOCK | O_NOFOLLOW | O_RDWR)(withO_NOFOLLOWto block symlink attacks). If the open succeeds, useunlinkat(or, on Linux,unlinkatwith an open directory if possible) to remove the file via its file descriptor, ensuring it is the exact file just opened. Close the fd after. - Error handling should deal with open/unlink failures (including
ELOOPfor symlinks). - If
O_NOFOLLOWand/orunlinkatare not available, document the limitation and fall back (with a warning) to the original code; on modern POSIX, we assume both are available. - Required: Addition of
<fcntl.h>foropen/O_NOFOLLOW, potentially<unistd.h>forunlinkatif not already included.
File/region:
- Only modify the non-directory branch (lines 605–611) in
core/src/adapters/fs/posix_temp_adapter.c. - Add any required includes at the top as permitted.
| @@ -6,6 +6,7 @@ | ||
| #include <dirent.h> | ||
| #include <errno.h> | ||
| #include <inttypes.h> | ||
| #include <fcntl.h> | ||
| #include <pwd.h> | ||
| #include <stdbool.h> | ||
| #include <stdint.h> | ||
| @@ -603,9 +604,34 @@ | ||
| stack[depth++] = | ||
| (dir_stack_frame_t){.dir = child_dir, .path_len = current_len}; | ||
| } else { | ||
| if (unlink(current) != 0 && errno != ENOENT) { | ||
| close_dir_stack(stack, depth); | ||
| return errno_to_result("unlink", current); | ||
| int file_fd = open(current, O_RDWR | O_NONBLOCK | O_NOFOLLOW); | ||
| if (file_fd < 0) { | ||
| if (errno == ELOOP) { | ||
| // Refuse to unlink symlinks, treat as error or skip | ||
| close_dir_stack(stack, depth); | ||
| return gm_err_void( | ||
| GM_ERROR(GM_ERR_INVALID_PATH, | ||
| "refuse to unlink symlinked file")); | ||
| } else if (errno != ENOENT) { | ||
| close_dir_stack(stack, depth); | ||
| return errno_to_result("open", current); | ||
| } | ||
| // ENOENT: already gone, skip | ||
| } else { | ||
| #ifdef AT_FDCWD | ||
| if (unlinkat(AT_FDCWD, current, 0) != 0 && errno != ENOENT) { | ||
| close(file_fd); | ||
| close_dir_stack(stack, depth); | ||
| return errno_to_result("unlinkat", current); | ||
| } | ||
| #else | ||
| if (unlink(current) != 0 && errno != ENOENT) { | ||
| close(file_fd); | ||
| close_dir_stack(stack, depth); | ||
| return errno_to_result("unlink", current); | ||
| } | ||
| #endif | ||
| close(file_fd); | ||
| } | ||
| current[parent_len] = '\0'; | ||
| } |
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.
| static gm_result_void_t remove_tree_impl(const char *path) { | ||
| if (path == NULL) { | ||
| return gm_ok_void(); | ||
| } | ||
| // Open parent directory | ||
| int dirfd = AT_FDCWD; | ||
| // For absolute path, just use AT_FDCWD | ||
| // For relative, this will also work as AT_FDCWD | ||
| return remove_tree_at(dirfd, path); | ||
| } | ||
|
|
||
| static gm_result_void_t remove_tree_at(int dirfd, const char *name) { | ||
| struct stat st = {0}; | ||
| // Use fstatat to check stat info relative to dirfd | ||
| if (fstatat(dirfd, name, &st, AT_SYMLINK_NOFOLLOW) != 0) { | ||
| char normalized[GM_PATH_MAX]; | ||
| gm_result_void_t normalize_rc = | ||
| gm_fs_path_normalize_logical(path, normalized, sizeof(normalized)); | ||
| if (!normalize_rc.ok) { | ||
| return normalize_rc; | ||
| } | ||
| if (normalized[0] == '\0') { | ||
| return gm_ok_void(); | ||
| } | ||
|
|
||
| struct stat root_stat = {0}; | ||
| if (stat(normalized, &root_stat) != 0) { | ||
| if (errno == ENOENT) { | ||
| return gm_ok_void(); | ||
| } | ||
| return gm_err_void(GM_ERROR(GM_ERR_IO_FAILED, | ||
| "fstatat failed for %s: %s", name, | ||
| strerror(errno))); | ||
| return errno_to_result("stat", normalized); | ||
| } | ||
|
|
||
| if (!S_ISDIR(st.st_mode)) { | ||
| // Unlink regular file or symlink or device, etc. | ||
| if (unlinkat(dirfd, name, 0) != 0) { | ||
| if (errno == ENOENT) { | ||
| return gm_ok_void(); | ||
| } | ||
| return gm_err_void( | ||
| GM_ERROR(GM_ERR_IO_FAILED, "unlinkat failed for %s: %s", name, | ||
| strerror(errno))); | ||
| if (!S_ISDIR(root_stat.st_mode)) { | ||
| if (unlink(normalized) != 0 && errno != ENOENT) { | ||
| return errno_to_result("unlink", normalized); | ||
| } | ||
| return gm_ok_void(); | ||
| } | ||
|
|
||
| // Open the directory itself | ||
| int subdirfd = openat(dirfd, name, O_RDONLY | O_DIRECTORY); | ||
| if (subdirfd < 0) { | ||
| dir_stack_frame_t stack[GM_PATH_MAX]; | ||
| size_t depth = 0U; | ||
|
|
||
| char current[GM_PATH_MAX]; | ||
| if (gm_strcpy_safe(current, sizeof(current), normalized) != GM_OK) { | ||
| return gm_err_void( | ||
| GM_ERROR(GM_ERR_IO_FAILED, "openat (directory) failed for %s: %s", name, | ||
| strerror(errno))); | ||
| GM_ERROR(GM_ERR_PATH_TOO_LONG, | ||
| "normalized path exceeds buffer while removing dir")); | ||
| } | ||
| size_t current_len = strlen(current); | ||
|
|
||
| DIR *dir = fdopendir(subdirfd); | ||
| if (dir == NULL) { | ||
| close(subdirfd); // Make sure to close the fd | ||
| return gm_err_void( | ||
| GM_ERROR(GM_ERR_IO_FAILED, "fdopendir failed for %s: %s", name, | ||
| strerror(errno))); | ||
| DIR *root_dir = opendir(current); | ||
| if (root_dir == NULL) { | ||
| return errno_to_result("opendir", current); | ||
| } | ||
| stack[depth++] = (dir_stack_frame_t){.dir = root_dir, .path_len = current_len}; | ||
|
|
||
| struct dirent *entry = NULL; | ||
| while ((entry = readdir(dir)) != NULL) { | ||
| if (is_dot_entry(entry)) { | ||
| continue; | ||
| } | ||
| while (depth > 0U) { | ||
| dir_stack_frame_t *frame = &stack[depth - 1U]; | ||
| struct dirent *entry = readdir(frame->dir); | ||
| if (entry != NULL) { | ||
| if (is_dot_entry(entry)) { | ||
| continue; | ||
| } | ||
|
|
||
| // Call recursively with subdirfd as dirfd and entry->d_name | ||
| gm_result_void_t child_result = remove_tree_at(subdirfd, entry->d_name); | ||
| if (!child_result.ok) { | ||
| closedir(dir); // This closes subdirfd as well | ||
| return child_result; | ||
| } | ||
| } | ||
| size_t parent_len = frame->path_len; | ||
| bool needs_separator = | ||
| (parent_len > 0U && current[parent_len - 1U] != '/'); | ||
| size_t name_len = strlen(entry->d_name); | ||
| size_t required = parent_len + (needs_separator ? 1U : 0U) + | ||
| name_len + 1U; | ||
| if (required >= sizeof(current)) { | ||
| close_dir_stack(stack, depth); | ||
| return gm_err_void( | ||
| GM_ERROR(GM_ERR_PATH_TOO_LONG, | ||
| "child path exceeds buffer")); | ||
| } | ||
|
|
||
| closedir(dir); // This closes subdirfd | ||
| // Remove the directory itself. Use unlinkat with AT_REMOVEDIR. | ||
| if (unlinkat(dirfd, name, AT_REMOVEDIR) != 0) { | ||
| if (errno == ENOENT) { | ||
| return gm_ok_void(); | ||
| size_t append_pos = parent_len; | ||
| if (needs_separator) { | ||
| current[append_pos++] = '/'; | ||
| } | ||
| if (gm_memcpy_span(current + append_pos, | ||
| sizeof(current) - append_pos, entry->d_name, | ||
| name_len) != 0) { | ||
| close_dir_stack(stack, depth); | ||
| return gm_err_void( | ||
| GM_ERROR(GM_ERR_PATH_TOO_LONG, | ||
| "child path exceeds buffer")); | ||
| } | ||
| current_len = append_pos + name_len; | ||
| current[current_len] = '\0'; | ||
|
|
||
| struct stat entry_stat = {0}; | ||
| if (stat(current, &entry_stat) != 0) { | ||
| if (errno == ENOENT) { | ||
| current[parent_len] = '\0'; | ||
| continue; | ||
| } | ||
| close_dir_stack(stack, depth); | ||
| return errno_to_result("stat", current); | ||
| } | ||
|
|
||
| if (S_ISDIR(entry_stat.st_mode)) { | ||
| DIR *child_dir = opendir(current); | ||
| if (child_dir == NULL) { | ||
| close_dir_stack(stack, depth); | ||
| return errno_to_result("opendir", current); | ||
| } | ||
| if (depth >= (sizeof(stack) / sizeof(stack[0]))) { | ||
| closedir(child_dir); | ||
| close_dir_stack(stack, depth); | ||
| return gm_err_void( | ||
| GM_ERROR(GM_ERR_INVALID_PATH, | ||
| "directory depth exceeds stack capacity")); | ||
| } | ||
| stack[depth++] = | ||
| (dir_stack_frame_t){.dir = child_dir, .path_len = current_len}; | ||
| } else { | ||
| if (unlink(current) != 0 && errno != ENOENT) { | ||
| close_dir_stack(stack, depth); | ||
| return errno_to_result("unlink", current); | ||
| } | ||
| current[parent_len] = '\0'; | ||
| } | ||
| } else { | ||
| closedir(frame->dir); | ||
| depth--; | ||
|
|
||
| if (rmdir(current) != 0 && errno != ENOENT) { | ||
| close_dir_stack(stack, depth); | ||
| return errno_to_result("rmdir", current); | ||
| } | ||
|
|
||
| if (depth > 0U) { | ||
| dir_stack_frame_t *parent = &stack[depth - 1U]; | ||
| current[parent->path_len] = '\0'; | ||
| } | ||
| } | ||
| return gm_err_void( | ||
| GM_ERROR(GM_ERR_IO_FAILED, "unlinkat (rmdir) failed for %s: %s", name, | ||
| strerror(errno))); | ||
| } | ||
|
|
||
| return gm_ok_void(); | ||
| } |
There was a problem hiding this comment.
Fix TOCTOU vulnerabilities in file removal.
CodeQL correctly identifies time-of-check-time-of-use race conditions at lines 521 and 606. Between the stat() check and the unlink() call, an attacker could replace the file with a symlink to a critical system file, causing unintended deletion.
As per coding guidelines, fix the warnings rather than suppress them.
Recommended fix: Remove the stat() checks before unlink() and rely on error codes to differentiate files from directories:
- struct stat entry_stat = {0};
- if (stat(current, &entry_stat) != 0) {
- if (errno == ENOENT) {
- current[parent_len] = '\0';
- continue;
- }
- close_dir_stack(stack, depth);
- return errno_to_result("stat", current);
- }
-
- if (S_ISDIR(entry_stat.st_mode)) {
+ /* Try to remove as file first; if EISDIR, it's a directory */
+ if (unlink(current) == 0 || errno == ENOENT) {
+ current[parent_len] = '\0';
+ continue;
+ }
+
+ if (errno == EISDIR) {
+ /* It's a directory, traverse it */
DIR *child_dir = opendir(current);
if (child_dir == NULL) {
close_dir_stack(stack, depth);
@@ -603,11 +590,9 @@
stack[depth++] =
(dir_stack_frame_t){.dir = child_dir, .path_len = current_len};
} else {
- if (unlink(current) != 0 && errno != ENOENT) {
- close_dir_stack(stack, depth);
- return errno_to_result("unlink", current);
- }
- current[parent_len] = '\0';
+ /* unlink failed with error other than EISDIR */
+ close_dir_stack(stack, depth);
+ return errno_to_result("unlink", current);
}Similarly, for the root path handling (lines 520-525), use the same approach:
- if (!S_ISDIR(root_stat.st_mode)) {
- if (unlink(normalized) != 0 && errno != ENOENT) {
- return errno_to_result("unlink", normalized);
- }
+ if (!S_ISDIR(root_stat.st_mode)) {
+ /* Non-directory: try to unlink */
+ if (unlink(normalized) == 0 || errno == ENOENT) {
+ return gm_ok_void();
+ }
+ return errno_to_result("unlink", normalized);
- return gm_ok_void();
}This eliminates the TOCTOU window and still correctly handles both files and directories.
As per coding guidelines.
🧰 Tools
🪛 GitHub Check: CodeQL
[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.
🤖 Prompt for AI Agents
In core/src/adapters/fs/posix_temp_adapter.c around lines 497–629, the code
performs stat() checks before unlink() (root path around lines ~520–525 and
per-entry around ~606) creating TOCTOU windows; remove those pre-unlink stat()
calls and instead attempt the destructive operation and branch on error codes:
for the root path, try unlink(normalized) and if it fails with EISDIR (or
returns EISDIR when trying to unlink a directory) then open the path as a
directory (opendir) and proceed with the directory-walking stack; for child
entries, replace the stat-before-unlink logic with trying unlink(current) and if
unlink fails with EISDIR then treat it as a directory (opendir and push to
stack), if unlink fails with ENOENT just continue, and for other errors return
errno_to_result; ensure you still handle path-too-long and buffer updates as
before and close directory handles on error.
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests