planning: roadmap DAG styling + SVG; issue sweep; ISSUES.md #74
Conversation
flyingrobots
commented
Nov 2, 2025
flyingrobots
commented
Pulled origin/main, branched chore/issues-roadmap, and restored the Mermaid source from stash. This file is the canonical input for rendering docs/ROADMAP-DAG.svg and embedding into ROADMAP-DAG.md. No code changes or CI impacts. Next: render SVG, normalize issue fields/labels, embed per-issue dependency summaries, and refresh the DAG.
Generated docs/ROADMAP-DAG.svg from docs/ROADMAP-DAG.mmd using minlag/mermaid-cli in Docker for reliable local viewing.
Updated Mermaid source and markdown to use subgraphs for M0–M7; regenerated SVG.
Introduced classDefs for M0–M7, highlighted root nodes, and switched to '==>' for hard deps and '-.->' for soft relations. Regenerated SVG.
Introduces tools/roadmap/sweep_issues.py which parses docs/ROADMAP-DAG.mmd, sets milestones/labels, attempts to wire hard 'blocked by' edges, and posts a per-issue Dependencies comment with mini-mermaid.
Augmented existing inventory with Priority and Estimate columns and appended codename chains with mini Mermaid. Mirrors the milestone-grouped DAG.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughAdds a compliance harness (headers, runners, reporter, tests), roadmap DAG sources and tooling (parse/validate/sweep/render), CI/workflows and pre-push checks, a ledger-kernel submodule, Makefile roadmap targets, PR template and CODEOWNERS. All edits are additive configuration, docs, tools, build and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer (pre-push)
participant Hook as tools/hooks/pre-push
participant Val as tools/roadmap/validate_dag.py
participant Docker as Mermaid Lint (docker)
participant GH as GitHub CLI
Dev->>Hook: run pre-push
Hook->>Val: validate docs/ROADMAP-DAG.mmd (if present)
Val->>Val: parse DAG (nodes, hard/soft edges)
Val->>Docker: mermaid lint (non-fatal)
Docker-->>Val: ok / warn
loop per issue referenced
Val->>GH: gh issue view <num>
alt exists
GH-->>Val: 200
else missing
GH-->>Val: 404 (collect)
end
end
alt all present
Val-->>Hook: exit 0
else missing
Val-->>Hook: exit 2
end
sequenceDiagram
autonumber
participant Sweep as tools/roadmap/sweep_issues.py
participant Parser as parse_mmd()
participant REST as GitHub REST API
participant Issue as Issue(`#N`)
Sweep->>Parser: parse ROADMAP-DAG.mmd -> Dag
Parser-->>Sweep: Dag {nodes, hard_edges, soft_edges}
loop per node
Sweep->>REST: ensure_milestone(issue -> M#)
REST-->>Sweep: 200/201
Sweep->>REST: add_blocked_by(blocked, blocker)
alt idempotent ok (200/409/403)
REST-->>Sweep: ok
else error
REST-->>Sweep: warn & continue
end
Sweep->>Issue: post_dependencies_comment(hard/soft + mini-mermaid)
end
Sweep-->>Sweep: exit 0
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–75 minutes High-attention areas:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Switch from GraphQL addBlockedBy to REST dependencies endpoint and numeric issue IDs.
…ackfill Adds CODEOWNERS (@flyingrobots), PR template, roadmap targets + pre-push validation, and normalizes issue bodies. Applies Risk to project items based on Priority.
Parser now recognizes both connectors; previously skipped hard edges will now be wired as 'blocked by' via REST.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
ISSUES.mdis excluded by!*.mddocs/ROADMAP-DAG.svgis excluded by!**/*.svg
📒 Files selected for processing (21)
.github/CODEOWNERS(1 hunks).github/pull_request_template.md(1 hunks).github/workflows/compliance.yml(1 hunks).gitmodules(1 hunks)ACTIVITY.log.jsonl(1 hunks)Makefile(2 hunks)docs/DRIFT_REPORT.md(1 hunks)docs/ROADMAP-DAG.md(1 hunks)docs/ROADMAP-DAG.mmd(1 hunks)external/ledger-kernel(1 hunks)include/ledger/compliance.h(1 hunks)meson.build(1 hunks)src/compliance/checks_core.c(1 hunks)src/compliance/checks_policy.c(1 hunks)src/compliance/checks_wasm.c(1 hunks)src/compliance/report.c(1 hunks)src/compliance/suite.c(1 hunks)tools/hooks/pre-push(1 hunks)tools/roadmap/ensure_issue_template.py(1 hunks)tools/roadmap/sweep_issues.py(1 hunks)tools/roadmap/validate_dag.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
include/ledger/compliance.h (4)
src/compliance/checks_core.c (1)
lk_comp_run_core(4-16)src/compliance/checks_policy.c (1)
lk_comp_run_policy(3-6)src/compliance/checks_wasm.c (1)
lk_comp_run_wasm(3-6)src/compliance/report.c (1)
lk_comp_report_write(24-59)
tools/roadmap/ensure_issue_template.py (1)
tools/roadmap/validate_dag.py (1)
run(11-15)
src/compliance/suite.c (3)
src/compliance/checks_core.c (1)
lk_comp_run_core(4-16)src/compliance/checks_policy.c (1)
lk_comp_run_policy(3-6)src/compliance/checks_wasm.c (1)
lk_comp_run_wasm(3-6)
tools/roadmap/validate_dag.py (1)
tools/roadmap/ensure_issue_template.py (2)
run(19-24)main(40-48)
🪛 Ruff (0.14.3)
tools/roadmap/ensure_issue_template.py
1-1: Shebang is present but file is not executable
(EXE001)
6-6: typing.List is deprecated, use list instead
(UP035)
20-21: Prefer capture_output over sending stdout and stderr to PIPE
Replace with capture_output keyword argument
(UP022)
20-20: subprocess call: check for execution of untrusted input
(S603)
46-46: Do not catch blind exception: Exception
(BLE001)
tools/roadmap/validate_dag.py
1-1: Shebang is present but file is not executable
(EXE001)
12-12: Prefer capture_output over sending stdout and stderr to PIPE
Replace with capture_output keyword argument
(UP022)
12-12: subprocess call: check for execution of untrusted input
(S603)
14-14: Avoid specifying long messages outside the exception class
(TRY003)
34-34: Consider ["gh", *args] instead of concatenation
Replace with ["gh", *args]
(RUF005)
42-42: Unpacked variable edges_hard is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
44-44: Unnecessary dict comprehension (rewrite using dict())
Rewrite using dict()
(C416)
49-49: Do not catch blind exception: Exception
(BLE001)
57-57: Do not catch blind exception: Exception
(BLE001)
tools/roadmap/sweep_issues.py
21-21: typing.Dict is deprecated, use dict instead
(UP035)
21-21: typing.List is deprecated, use list instead
(UP035)
21-21: typing.Set is deprecated, use set instead
(UP035)
21-21: typing.Tuple is deprecated, use tuple instead
(UP035)
42-43: Prefer capture_output over sending stdout and stderr to PIPE
Replace with capture_output keyword argument
(UP022)
42-42: subprocess call: check for execution of untrusted input
(S603)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Ambiguous variable name: l
(E741)
🪛 YAMLlint (1.37.1)
.github/workflows/compliance.yml
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 30-30: too many blank lines (1 > 0)
(empty-lines)
…efile, sweep idempotence + milestone paging, compliance portability + docs + smoke test - compliance.yml: remove stray blank line\n- .gitmodules: drop branch=main; pin external/ledger-kernel\n- DRIFT_REPORT.md: concrete ISO-8601 date\n- validate_dag: fix docker mount f-string\n- pre-push: guard make presence\n- Makefile: add PYTHON guard; docker check; short-circuit refresh\n- sweep_issues: page milestones; upsert Dependencies comment; parse '-->' hard edges\n- compliance.h: docs, ownership, run_all proto, suite_free\n- suite/checks/report: NULL checks, propagate rc, set summary fields, portable gmtime, better JSON escaping + I/O error handling\n- meson: add compliance smoke test
coderabbitai
bot
added
area::build
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/ROADMAP-DAG.md (1)
127-237: Remove the duplicate, outdated Mermaid diagram.Lines 127-237 contain an older version of the DAG that lacks the milestone subgraphs, theme configuration, node class definitions, and root-node styling present in the new diagram (lines 6-126). This orphaned duplicate will confuse readers and cause maintenance drift. Delete lines 127-237 entirely, leaving only the enhanced diagram.
Apply this diff to remove the duplicate:
-This diagram captures the dependency graph across ALL open issues. - - -```mermaid -flowchart TD - N4["#4 CI scaffolding"] - N6["#6 Error API"] - N7["#7 Allocator hooks"] - N8["#8 Logger hooks"] - N9["#9 Context lifecycle"] - N10["#10 Git repo port interface"] - N11["#11 libgit2 adapter"] - N12["#12 Ledger lifecycle"] - N13["#13 Append path"] - N14["#14 Read path"] - N15["#15 Append/read integration tests"] - N16["#16 Policy document storage"] - N17["#17 Policy parser"] - N18["#18 Author identity port"] - N19["#19 Append enforcement"] - N20["#20 Policy enforcement tests"] - N21["#21 Trust document storage"] - N22["#22 Signature port"] - N23["#23 Commit signature validation"] - N24["#24 Attestation support"] - N25["#25 Threshold enforcement"] - N26["#26 Notes API"] - N27["#27 Tag association"] - N28["#28 CLI enhancements"] - N29["#29 Notes and tags integration tests"] - N30["#30 Indexer interface"] - N31["#31 CRoaring integration"] - N32["#32 Cache writer"] - N33["#33 Query engine"] - N34["#34 CLI query commands"] - N35["#35 Query integration tests"] - N36["#36 Deep verify"] - N37["#37 BLAKE3 checksum option"] - N38["#38 Documentation and examples"] - N39["#39 End-to-end tests"] - N47["#47 Minimal Linux CRT shim (_start) and f..."] - N50["#50 Public API v0.1 docs + visibility"] - N51["#51 Add Doxygen config + CMake target"] - N52["#52 Annotate public headers with brief/group"] - N53["#53 CI job: build + upload API docs artifact"] - N54["#54 CLI scaffold + examples"] - N55["#55 Subcommand framework + help"] - N56["#56 Implement 'version' command"] - N57["#57 Implement 'error-demo' command"] - N58["#58 Windows DLL export audit + shared bui..."] - N59["#59 Fuzzing harness for errors/version"] - N60["#60 libgit2 adapter (read-only) skeleton"] - N61["#61 Symbol-policy guard for archives (cro..."] - N62["#62 Audit GITLEDGER_API exports for DLL"] - N63["#63 Add MSVC shared build + test job"] - N64["#64 Fix printf/PRIuMAX and headers for MSVC"] - N65["#65 error_json fuzzer"] - N66["#66 version_snprintf fuzzer"] - N67["#67 CI lane (10s per target)"] - N68["#68 Define adapter interface header"] - N69["#69 Implement open repository + fixtures"] - N70["#70 List commits (iterator)"] - N71["#71 Linux archive policy (nm)"] - N72["#72 macOS archive policy (nm -U)"] - N73["#73 Windows archive policy (.lib)"] - N53 --> N51 - N56 --> N55 - N57 --> N55 - N50 --> N51 - N50 --> N52 - N50 --> N53 - N54 --> N55 - N54 --> N56 - N54 --> N57 - N58 --> N62 - N58 --> N63 - N58 --> N64 - N59 --> N65 - N59 --> N66 - N59 --> N67 - N60 --> N68 - N60 --> N69 - N60 --> N70 - N61 --> N71 - N61 --> N72 - N61 --> N73 - N13 --> N12 - N14 --> N10 - N15 --> N13 - N15 --> N14 - N11 --> N10 - N32 --> N30 - N33 --> N30 - N33 --> N31 - N33 --> N32 - N34 --> N33 - N35 --> N33 - N35 --> N34 - N36 --> N37 - N36 --> N33 - N39 --> N33 - N39 --> N34 - N39 --> N60 - N23 --> N22 - N24 --> N23 - N25 --> N17 - N25 --> N16 - N20 --> N17 - N20 --> N25 - N29 --> N26 - N29 --> N27 -</blockquote></details> </blockquote></details>♻️ Duplicate comments (1)
include/ledger/compliance.h (1)
45-47: Document the public runner contracts.
These APIs are still naked—no parameter requirements, no return-value semantics, nothing. For a public surface, that’s unacceptable. Add Doxygen-style docs explaining expected inputs (NULL handling, suite ownership), side effects, and exact success/error codes so callers know how to use them.📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/compliance.yml(1 hunks).gitmodules(1 hunks)ACTIVITY.log.jsonl(1 hunks)Makefile(2 hunks)docs/DRIFT_REPORT.md(1 hunks)docs/ROADMAP-DAG.md(2 hunks)external/ledger-kernel(1 hunks)include/ledger/compliance.h(1 hunks)src/compliance/checks_core.c(1 hunks)src/compliance/checks_policy.c(1 hunks)src/compliance/checks_wasm.c(1 hunks)src/compliance/report.c(1 hunks)src/compliance/suite.c(1 hunks)tests/compliance_smoke.c(1 hunks)tools/hooks/pre-push(1 hunks)tools/roadmap/validate_dag.py(1 hunks)🧰 Additional context used
🧬 Code graph analysis (4)
tests/compliance_smoke.c (1)
src/compliance/suite.c (2)
lk_comp_run_all(4-23)lk_comp_suite_free(25-32)src/compliance/suite.c (3)
src/compliance/checks_core.c (1)
lk_comp_run_core(4-19)src/compliance/checks_policy.c (1)
lk_comp_run_policy(3-7)src/compliance/checks_wasm.c (1)
lk_comp_run_wasm(3-7)tools/roadmap/validate_dag.py (1)
tools/roadmap/ensure_issue_template.py (2)
run(19-24)main(40-48)include/ledger/compliance.h (5)
src/compliance/checks_core.c (1)
lk_comp_run_core(4-19)src/compliance/checks_policy.c (1)
lk_comp_run_policy(3-7)src/compliance/checks_wasm.c (1)
lk_comp_run_wasm(3-7)src/compliance/report.c (1)
lk_comp_report_write(36-93)src/compliance/suite.c (2)
lk_comp_run_all(4-23)lk_comp_suite_free(25-32)🪛 GitHub Actions: CI
include/ledger/compliance.h
[error] 12-41: clang-format violations detected by clang_format_check.sh. Please run clang-format-18 to format the file (triggered by 'make format-check').
🪛 Ruff (0.14.3)
tools/roadmap/validate_dag.py
1-1: Shebang is present but file is not executable
(EXE001)
12-12: Prefer
capture_outputover sendingstdoutandstderrtoPIPEReplace with
capture_outputkeyword argument(UP022)
12-12:
subprocesscall: check for execution of untrusted input(S603)
14-14: Avoid specifying long messages outside the exception class
(TRY003)
34-34: Consider
["gh", *args]instead of concatenationReplace with
["gh", *args](RUF005)
42-42: Unpacked variable
edges_hardis never usedPrefix it with an underscore or any other dummy variable pattern
(RUF059)
44-44: Unnecessary dict comprehension (rewrite using
dict())Rewrite using
dict()(C416)
49-49: Do not catch blind exception:
Exception(BLE001)
57-57: Do not catch blind exception:
Exception(BLE001)
🔇 Additional comments (15)
external/ledger-kernel (2)
1-1: RESOLVED: .gitmodules configuration is correct and contains no conflicts.The submodule
external/ledger-kernelis explicitly pinned to commit83b81775cf027341af5f17a45646e9c40f13a267via the gitlink entry. The.gitmodulesfile correctly specifies onlypathandurl(nobranchdirective), which is the standard and intentional approach for explicit commit pinning in detached HEAD mode. This is neither contradictory nor fragile—it's a stable, predictable submodule strategy. No action required.
1-1: No circular dependency detected. Verified ledger-kernel has no references to this repository.Comprehensive analysis across all dependency manifests (npm
package.json, git submodules) confirms ledger-kernel does not reference this repository. The external submodule addition is safe..gitmodules (1)
1-3: Past issue addressed: submodule no longer tracks floating branch.The previous review correctly flagged the
branch = mainline as non-deterministic. That line has been removed, so the submodule will now properly pin to specific commit SHAs in the superproject's index.src/compliance/checks_policy.c (1)
3-7: Past issue addressed: status now explicitly set.The previous nitpick about leaving
s->summary.policyuninitialized has been resolved. Line 5 now explicitly setsLK_COMP_PARTIAL, making the stub's intent clear and consistent with the pattern used inchecks_core.c.tests/compliance_smoke.c (1)
1-8: Correct resource cleanup pattern.The test properly follows the C resource management pattern: unconditional cleanup (line 5) happens before returning the status. The
lk_comp_suite_freefunction (per suite.c:24-31) safely handles NULL and partial initialization, so calling it regardless oflk_comp_run_allsuccess is correct and leak-free.src/compliance/checks_wasm.c (1)
3-7: Past issue addressed; NA status is semantically correct.The previous comment has been resolved—line 5 now explicitly sets the status. The choice of
LK_COMP_NA(vs.LK_COMP_PARTIALinchecks_policy.c) is intentional and aligns withdocs/DRIFT_REPORT.mdline 40, which marks the WASM runtime as "not required for core" and "N/A until available."tools/hooks/pre-push (1)
1-25: Past issue addressed: hook is now resilient to missing tools.The previous major issue about environments lacking
makehas been resolved. Lines 15-22 properly guard themakeinvocation with acommand -vcheck and skip gracefully with an informative message ifmakeis absent. The same defensive pattern is applied topython3at line 6. Combined with the strict bash options (line 2), this hook will catch real failures while degrading gracefully when optional tooling is unavailable.docs/DRIFT_REPORT.md (1)
3-3: Past issue addressed: timestamp is now concrete.The previous minor issue about the un-evaluated shell substitution
$(date -u +%Y-%m-%dT%H:%M:%SZ)has been fixed. Line 3 now contains an actual ISO-8601 UTC timestamp (2025-11-03T22:48:13Z), making this a proper snapshot artifact..github/workflows/compliance.yml (2)
1-29: Past issue addressed: YAML formatting is now clean.The previous yamllint error about the extra blank line after line 29 has been resolved. The workflow file is now properly formatted.
21-25: Verify: Should the workflow invoke the actual compliance harness?The workflow builds the compliance library (lines 16-20) but then emits a hardcoded placeholder JSON (lines 22-24) with bogus timestamp
1970-01-01T00:00:00Zinstead of running it. Sincetests/compliance_smoke.cexists andlk_comp_report_writeis implemented (per context fromsrc/compliance/report.c), the workflow could compile and execute the smoke test to generate a real compliance artifact. Confirm whether the placeholder is intentional scaffolding or whether the workflow should invokemeson testor a dedicated compliance binary to produce authentic output.Makefile (1)
201-231: Roadmap targets implemented correctly.All past review feedback has been properly addressed:
- Docker availability check before
roadmap-render(lines 207-209)- Python and script existence checks before
roadmap-sweepandroadmap-validate(lines 213-218, 222-227)- Proper chaining in
roadmap-refreshusing&&operator (line 231)PYTHONvariable properly defined with?=for override capability (line 202)The implementation is robust and provides clear error messages when dependencies are missing.
ACTIVITY.log.jsonl (1)
149-165: Activity log entries are well-formed and consistent.All new entries follow the established JSONL schema with proper
who,what,where,when,why,how, andprotipfields. The entries document the roadmap DAG work, issue management automation, and compliance scaffolding in appropriate detail.tools/roadmap/validate_dag.py (2)
47-50: Past issue resolved: Docker mount path no longer has trailing space.The f-string on line 48 is now correctly formatted without the trailing space that was flagged in previous review.
1-1: Remove the shebang—file is always invoked viapython3and will never execute directly.The Makefile always invokes the script as
$(PYTHON) tools/roadmap/validate_dag.py(not as a direct executable). Since the script has no execution path where the shebang matters, it should be removed. The "choose one or the other" framing in the original review misses the actual usage pattern: this script will never be called directly.Likely an incorrect or invalid review comment.
src/compliance/suite.c (1)
25-32: Original review comment is incorrect; no action needed.The
lk_comp_casestruct explicitly documents thatclausesandnotesare non-owning pointers ("non-owning: must outlive lk_comp_case"). All actual allocations inchecks_core.cassign only static strings to these fields. The function correctly frees only the array itself—freeing the contents would be a bug. No loop or additional cleanup is required.Likely an incorrect or invalid review comment.
| @@ -0,0 +1 @@ | |||
| Subproject commit 83b81775cf027341af5f17a45646e9c40f13a267 | |||
There was a problem hiding this comment.
🧩 Analysis chain
VERIFY: Confirm developer documentation explicitly mandates git clone --recursive.
Without explicit onboarding documentation, developers will clone and encounter silent submodule failures. Verify that README and contribution guides prominently document the submodule requirement.
🏁 Script executed:
#!/bin/bash
# Description: Check developer documentation for submodule clone instructions.
echo "=== Checking README for submodule documentation ==="
for file in README.md README.rst README.txt; do
if [ -f "$file" ]; then
echo "File: $file"
if grep -qi "submodule\|clone.*recursive\|--recursive" "$file"; then
echo "✓ Contains submodule clone instructions"
echo "Relevant lines:"
grep -n -i "submodule\|clone.*recursive\|--recursive" "$file"
else
echo "⚠ WARNING: No explicit submodule instructions"
fi
echo ""
fi
done
echo "=== Checking CONTRIBUTING.md ==="
if [ -f CONTRIBUTING.md ]; then
if grep -qi "submodule\|clone.*recursive\|--recursive" CONTRIBUTING.md; then
echo "✓ Mentions submodule setup"
else
echo "⚠ WARNING: CONTRIBUTING.md does not mention submodule setup"
fi
else
echo "⚠ CONTRIBUTING.md not found"
fi
echo ""
echo "=== Checking SETUP/INSTALL documentation ==="
for file in SETUP.md INSTALL.md INSTALL.rst; do
if [ -f "$file" ]; then
echo "File: $file"
if grep -qi "submodule\|clone.*recursive" "$file"; then
echo "✓ Mentions submodule"
else
echo "⚠ No submodule mention"
fi
fi
doneLength of output: 253
UPDATE README.md and CONTRIBUTING.md with explicit git clone --recursive requirement.
Documentation verification confirms the critical gap: README.md and CONTRIBUTING.md contain no mention of the git submodule requirement. Developers cloning without --recursive will silently obtain an empty external/ledger-kernel/ directory, causing downstream build or runtime failures.
Required action: Add explicit submodule clone instructions to both README.md (Quick Start section) and CONTRIBUTING.md (Developer Setup section). Example:
git clone --recursive https://github.com/[repo].git
# or for existing clones:
git submodule update --init --recursive
🤖 Prompt for AI Agents
In README.md (Quick Start section) and CONTRIBUTING.md (Developer Setup section)
— add explicit instructions that the repo contains git submodules and must be
cloned with submodules; include the two commands shown in the review: "git clone
--recursive https://github.com/[repo].git" and the alternative for existing
clones "git submodule update --init --recursive", place them prominently in
those sections (near existing clone/setup steps), and briefly note that failing
to use --recursive will leave external/ledger-kernel empty and break builds.
🧩 Analysis chain
CRITICAL: Verify CI/CD pipelines use --recursive flag during clone.
Submodules are silently empty if cloned without --recursive. This will cause CI builds and developer clones to fail unpredictably. All clone operations must include --recursive (or initialize manually).
🏁 Script executed:
#!/bin/bash
# Description: Verify CI/CD pipelines properly initialize submodules.
echo "=== Checking GitHub Actions workflows ==="
if [ -d .github/workflows ]; then
echo "Workflows found:"
find .github/workflows -name "*.yml" -o -name "*.yaml" | while read workflow; do
echo ""
echo "File: $workflow"
if grep -q "\-\-recursive\|git submodule" "$workflow"; then
echo "✓ Contains submodule initialization"
grep -n "\-\-recursive\|git submodule" "$workflow"
else
echo "⚠ WARNING: No submodule initialization detected"
fi
done
else
echo "⚠ No .github/workflows directory found"
fi
echo ""
echo "=== Checking Makefile for clone targets ==="
if [ -f Makefile ] || [ -f makefile ]; then
MAKEFILE=$([ -f Makefile ] && echo Makefile || echo makefile)
echo "File: $MAKEFILE"
if grep -q "clone\|git.*--recursive" "$MAKEFILE"; then
echo "✓ Contains clone/recursive references"
grep -n "clone\|git.*--recursive" "$MAKEFILE"
else
echo "⚠ No recursive clone target found"
fi
fi
echo ""
echo "=== Checking README for clone instructions ==="
if [ -f README.md ] || [ -f README.rst ] || [ -f README.txt ]; then
README=$(ls README.* 2>/dev/null | head -1)
echo "File: $README"
if grep -qi "git clone.*recursive\|git.*submodule" "$README"; then
echo "✓ Contains submodule clone instructions"
else
echo "⚠ WARNING: No explicit submodule clone instructions found"
fi
fiLength of output: 529
CRITICAL: CI/CD pipelines silently skip submodule initialization—guaranteed failures in production.
Verification confirms zero submodule handling in any workflow:
.github/workflows/ci.yml: Missing--recursiveand git submodule init.github/workflows/docs-site.yml: Missing--recursiveand git submodule init.github/workflows/compliance.yml: Missing--recursiveand git submodule initMakefile: No clone target with--recursiveREADME.md: No submodule initialization instructions
Add --recursive to all git clone operations. Alternatively, add explicit git submodule update --init --recursive after clone in each workflow. Document the requirement prominently in README.md for developers.
Without this, external/ledger-kernel remains empty in CI and fresh clones, causing silent failures downstream.
🤖 Prompt for AI Agents
In external/ledger-kernel around lines 1 to 1: CI and repo docs never initialize
Git submodules so external/ledger-kernel remains empty in fresh clones and CI
jobs; update every git clone in .github/workflows/ci.yml, docs-site.yml, and
compliance.yml to include --recursive (or add a post-clone step running git
submodule update --init --recursive), add an explicit recursive clone or
submodule init target in the Makefile (e.g., clone or bootstrap target that runs
git clone --recursive or git submodule update --init --recursive), and add a
prominent note in README.md describing the required submodule init/recursive
clone command and any Makefile target to run in local developer setup.
🧩 Analysis chain
VERIFY: Confirm the pinned commit hash is reachable on the external repository.
The previous commit hash was invalid and blocked merge. This has been corrected to 83b81775cf027341af5f17a45646e9c40f13a267, but I must verify it is reachable and the submodule will not fail during clone.
🏁 Script executed:
#!/bin/bash
# Description: Verify the pinned submodule commit is reachable on the external repository.
COMMIT_HASH="83b81775cf027341af5f17a45646e9c40f13a267"
REPO_URL="https://github.com/flyingrobots/ledger-kernel"
echo "=== Verification: Pinned commit reachability ==="
# Test 1: Check if commit exists via ls-remote
echo "Test 1: Checking if commit is reachable via any ref"
git ls-remote "$REPO_URL" "$COMMIT_HASH" && echo "✓ Commit found" || echo "⚠ Commit not found in direct ls-remote"
# Test 2: Attempt fetch-pack to locate the commit
echo ""
echo "Test 2: Attempting to locate commit via fetch-pack"
git fetch-pack --all "$REPO_URL" "$COMMIT_HASH" 2>&1 | head -20
# Test 3: Get current main HEAD
echo ""
echo "Test 3: Current main branch HEAD"
MAIN_HEAD=$(git ls-remote "$REPO_URL" refs/heads/main | awk '{print $1}')
echo "Main HEAD: $MAIN_HEAD"
if [ "$MAIN_HEAD" == "$COMMIT_HASH" ]; then
echo "✓ Pinned commit matches current main HEAD"
else
echo "⚠ Pinned commit differs from main HEAD"
fi
# Test 4: List all tags
echo ""
echo "Test 4: Available tags (if any)"
git ls-remote --tags "$REPO_URL" | head -10
# Test 5: Verify submodule can be cloned
echo ""
echo "Test 5: Test cloning the submodule commit"
TMPDIR=$(mktemp -d)
git clone --depth 1 "$REPO_URL" "$TMPDIR" 2>&1 || true
if git -C "$TMPDIR" cat-file -t "$COMMIT_HASH" 2>/dev/null; then
echo "✓ Commit is present in repository history"
else
echo "⚠ Commit not found in cloned repository"
fi
rm -rf "$TMPDIR"Length of output: 561
NEXT: Verify submodule integration completeness—.gitmodules, CI/CD, and documentation.
Commit hash is valid and is current main HEAD. Now verifying the submodule is properly wired into the repository infrastructure.
🏁 Script executed:
#!/bin/bash
# Description: Verify submodule integration—.gitmodules, CI/CD, and docs.
echo "=== VERIFICATION: Submodule Integration Completeness ==="
# Test 1: Check .gitmodules for external/ledger-kernel
echo ""
echo "Test 1: Inspecting .gitmodules configuration"
if [ -f ".gitmodules" ]; then
echo "✓ .gitmodules exists"
echo "--- .gitmodules content ---"
cat .gitmodules
echo "--- end ---"
# Verify external/ledger-kernel entry
if grep -q '\[submodule "external/ledger-kernel"\]' .gitmodules; then
echo "✓ external/ledger-kernel submodule entry found"
# Extract URL and path
SUBMODULE_URL=$(sed -n '/\[submodule "external\/ledger-kernel"\]/,/\[submodule/p' .gitmodules | grep "url" | head -1 | cut -d= -f2 | xargs)
SUBMODULE_PATH=$(sed -n '/\[submodule "external\/ledger-kernel"\]/,/\[submodule/p' .gitmodules | grep "path" | head -1 | cut -d= -f2 | xargs)
echo " URL: $SUBMODULE_URL"
echo " Path: $SUBMODULE_PATH"
else
echo "✗ CRITICAL: external/ledger-kernel entry NOT found in .gitmodules"
fi
else
echo "✗ CRITICAL: .gitmodules does not exist"
fi
# Test 2: Check GitHub Actions for --recursive clone
echo ""
echo "Test 2: Checking CI/CD workflows for --recursive flag"
if [ -d ".github/workflows" ]; then
RECURSIVE_COUNT=$(find .github/workflows -name "*.yml" -o -name "*.yaml" | xargs grep -l "\-\-recursive" 2>/dev/null | wc -l)
if [ "$RECURSIVE_COUNT" -gt 0 ]; then
echo "✓ Found $RECURSIVE_COUNT workflow(s) with --recursive flag"
find .github/workflows -name "*.yml" -o -name "*.yaml" | xargs grep -n "\-\-recursive" 2>/dev/null
else
echo "⚠ WARNING: No --recursive flag found in CI/CD workflows"
echo " Checking for 'submodule' mentions:"
find .github/workflows -name "*.yml" -o -name "*.yaml" | xargs grep -n "submodule" 2>/dev/null || echo " (no mentions found)"
fi
else
echo "⚠ .github/workflows directory not found"
fi
# Test 3: Check README and developer docs
echo ""
echo "Test 3: Checking documentation for submodule guidance"
README_FILES=$(find . -maxdepth 2 -type f \( -name "README*" -o -name "DEVELOP*" -o -name "SETUP*" \) 2>/dev/null)
if [ -n "$README_FILES" ]; then
SUBMODULE_MENTIONS=$(echo "$README_FILES" | xargs grep -l "submodule\|--recursive" 2>/dev/null | wc -l)
if [ "$SUBMODULE_MENTIONS" -gt 0 ]; then
echo "✓ Found $SUBMODULE_MENTIONS file(s) mentioning submodule/--recursive"
echo "$README_FILES" | xargs grep -n "submodule\|--recursive" 2>/dev/null | head -10
else
echo "✗ CRITICAL: No documentation mentions submodule setup or --recursive clone"
fi
else
echo "⚠ No README/DEVELOP/SETUP files found"
fi
# Test 4: Verify no circular dependency (ledger-kernel does not reference this repo)
echo ""
echo "Test 4: Checking for circular dependency"
if [ -d "external/ledger-kernel" ]; then
if [ -f "external/ledger-kernel/.gitmodules" ]; then
CIRCULAR=$(grep -i "$(git config --get remote.origin.url | sed 's|.*/||; s|\.git$||')" external/ledger-kernel/.gitmodules 2>/dev/null | wc -l)
if [ "$CIRCULAR" -gt 0 ]; then
echo "✗ CRITICAL: Circular dependency detected!"
else
echo "✓ No circular dependency detected"
fi
else
echo "✓ ledger-kernel has no submodules (no circular risk)"
fi
else
echo "⚠ Cannot check—external/ledger-kernel not cloned (expected in sandbox)"
fi
# Test 5: Validate .gitmodules syntax
echo ""
echo "Test 5: Validating .gitmodules syntax"
if [ -f ".gitmodules" ]; then
if git config -f .gitmodules --list > /dev/null 2>&1; then
echo "✓ .gitmodules syntax is valid"
else
echo "✗ CRITICAL: .gitmodules syntax invalid"
git config -f .gitmodules --list 2>&1 | head -5
fi
fiLength of output: 984
CRITICAL GAPS: Submodule integration incomplete—CI/CD inconsistencies and zero documentation.
Found blocking issues that will cause silent failures and developer friction. Running final verification.
🏁 Script executed:
#!/bin/bash
# Enumerate all workflows and submodule handling; identify dependencies on ledger-kernel
echo "=== VERIFICATION: Complete CI/CD and Dependency Audit ==="
# Test 1: List all workflows with submodule handling
echo ""
echo "Test 1: All workflows and their submodule configuration"
find .github/workflows -name "*.yml" -o -name "*.yaml" 2>/dev/null | while read workflow; do
echo "Workflow: $workflow"
grep -E "submodules:|uses:.*actions/checkout" "$workflow" | head -5
echo "---"
done
# Test 2: Search codebase for references to external/ledger-kernel
echo ""
echo "Test 2: Code references to external/ledger-kernel"
REFS=$(rg -i "external/ledger-kernel|ledger-kernel|ledger_kernel" --type-list 2>/dev/null | head -1 || echo "checking manually")
echo "Searching for references in source code..."
find . -type f \( -name "*.ts" -o -name "*.js" -o -name "*.go" -o -name "*.py" -o -name "*.java" -o -name "*.rs" -o -name "*.toml" -o -name "*.gradle" -o -name "*.cmake" -o -name "CMakeLists.txt" \) ! -path "./.git/*" ! -path "./.github/*" 2>/dev/null | xargs grep -l "external/ledger-kernel\|ledger.kernel\|ledger_kernel" 2>/dev/null | head -10 || echo "(no references found in source code)"
# Test 3: Inspect docs-site.yml specifically
echo ""
echo "Test 3: docs-site.yml full inspection"
if [ -f ".github/workflows/docs-site.yml" ]; then
echo "--- docs-site.yml content ---"
cat .github/workflows/docs-site.yml
echo "--- end ---"
else
echo "docs-site.yml not found"
fi
# Test 4: Check for build/import files that reference ledger-kernel
echo ""
echo "Test 4: Build configs and dependency files"
find . -maxdepth 3 \( -name "package.json" -o -name "go.mod" -o -name "Cargo.toml" -o -name "pom.xml" -o -name "build.gradle" -o -name "CMakeLists.txt" \) ! -path "./.git/*" 2>/dev/null | xargs grep -l "ledger" 2>/dev/null || echo "(no ledger references in build files)"
# Test 5: Verify README exists and measure documentation completeness
echo ""
echo "Test 5: README setup instructions"
if [ -f "README.md" ]; then
echo "README.md exists, checking for setup section..."
sed -n '/[Ss]etup\|[Ii]nstall\|[Gg]etting [Ss]tarted/,/^##/p' README.md | head -30 || echo "(no setup section found)"
else
echo "No README.md found"
fiLength of output: 3241
🏁 Script executed:
#!/bin/bash
# Enumerate all workflows and submodule handling; identify dependencies on ledger-kernel
echo "=== VERIFICATION: Complete CI/CD and Dependency Audit ==="
# Test 1: List all workflows with submodule handling
echo ""
echo "Test 1: All workflows and their submodule configuration"
if [ -d ".github/workflows" ]; then
find .github/workflows -name "*.yml" -o -name "*.yaml" 2>/dev/null | sort | while read workflow; do
echo "Workflow: $(basename $workflow)"
grep -E "submodules:|uses:.*actions/checkout" "$workflow" 2>/dev/null | head -3 || echo " (no explicit submodule config)"
echo "---"
done
else
echo "(no workflows directory)"
fi
# Test 2: Search codebase for references to external/ledger-kernel
echo ""
echo "Test 2: Code references to external/ledger-kernel or ledger-kernel"
echo "Searching for imports, includes, requires..."
find . -type f \( -name "*.ts" -o -name "*.js" -o -name "*.go" -o -name "*.py" -o -name "*.java" -o -name "*.rs" -o -name "*.toml" -o -name "*.gradle" -o -name "CMakeLists.txt" -o -name "*.cpp" -o -name "*.h" \) ! -path "./.git/*" ! -path "./.github/*" ! -path "./node_modules/*" ! -path "./external/*" 2>/dev/null | xargs grep -i "external/ledger\|ledger.kernel\|ledger_kernel\|from.*ledger\|import.*ledger" 2>/dev/null | head -20 || echo "(no references found)"
# Test 3: Inspect docs-site.yml specifically
echo ""
echo "Test 3: docs-site.yml full inspection"
if [ -f ".github/workflows/docs-site.yml" ]; then
echo "--- docs-site.yml content ---"
cat .github/workflows/docs-site.yml
echo "--- end ---"
else
echo "(docs-site.yml not found)"
fi
# Test 4: Check package.json, go.mod, Cargo.toml for ledger deps
echo ""
echo "Test 4: Build config and dependency files"
find . -maxdepth 3 \( -name "package.json" -o -name "go.mod" -o -name "Cargo.toml" -o -name "pom.xml" -o -name "build.gradle" \) ! -path "./.git/*" 2>/dev/null | xargs grep -i "ledger" 2>/dev/null || echo "(no ledger dependencies in build files)"
# Test 5: README setup section
echo ""
echo "Test 5: README setup/install instructions"
if [ -f "README.md" ]; then
if grep -qi "setup\|install\|getting started\|clone" README.md; then
echo "Found setup-related section. Extracting first 50 lines..."
cat README.md | head -50
else
echo "No setup/install section found in README.md"
fi
else
echo "(no README.md)"
fiLength of output: 4974
CRITICAL: CI/CD wiring and documentation incomplete—add explicit submodule handling and setup docs.
Commit hash verified as valid and reachable. Submodule is correctly configured in .gitmodules. However, integration gaps create silent failure risks:
Mandatory fixes required:
-
.github/workflows/ci.yml— Add explicit submodule configuration. Currently relies on implicit default (no submodule fetch), which is ambiguous and error-prone. Either:- Add
submodules: falsewith comment explaining why CI doesn't need the kernel, OR - Add
submodules: trueif CI build/tests depend onexternal/ledger-kernel
- Add
-
README.md— "Getting Started" section — Add explicit instruction:git clone --recursive https://github.com/<repo>or document that standardgit clonerequires manualgit submodule update --init --recursivepost-checkout. -
CONTRIBUTING.mdor equivalent — Document submodule requirement in developer onboarding: whether it's optional, when it's needed, and how to initialize it.
Why this matters: Developers doing standard git clone get an empty external/ledger-kernel/ directory with zero warning. If any build step depends on it, failure will be silent/cryptic. If not, it's just sloppy infrastructure.
Current state: compliance.yml correctly enables submodules; docs-site.yml correctly disables them. The ci.yml workflow is the weak link—make its intent explicit.
🤖 Prompt for AI Agents
external/ledger-kernel (context at lines 1-1): CI and docs lack explicit
submodule handling; update .github/workflows/ci.yml to explicitly set
submodules: true if the CI build/tests require external/ledger-kernel (or set
submodules: false with a comment explaining why the kernel is not needed),
update README.md Getting Started to show cloning with submodules (git clone
--recursive ... or git submodule update --init --recursive) and clarify what
directories are populated, and add a short section in CONTRIBUTING.md (or
equivalent onboarding doc) that states whether the submodule is required, when
to initialize it, and the exact commands to do so.
There was a problem hiding this comment.
@coderabbitai thanks — addressed in the current branch. Submodule docs added; CI submodules explicit. Please double-check.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Verification: Submodule Documentation and CI Configuration ==="
echo ""
echo "1. Checking README.md for submodule clone instructions"
echo "========================================================="
if [ -f "README.md" ]; then
echo "README.md exists. Searching for submodule/recursive mentions:"
grep -n -i "submodule\|recursive\|--recursive" README.md || echo "(no matches found)"
echo ""
echo "Extracting Getting Started / Setup / Clone sections (first 80 lines):"
head -80 README.md | grep -A 10 -i "getting started\|setup\|clone\|install" || head -80 README.md
else
echo "❌ README.md not found"
fi
echo ""
echo "2. Checking CONTRIBUTING.md for submodule documentation"
echo "========================================================"
if [ -f "CONTRIBUTING.md" ]; then
echo "CONTRIBUTING.md exists. Checking for submodule mentions:"
grep -n -i "submodule\|recursive\|--recursive" CONTRIBUTING.md || echo "(no matches found)"
echo ""
echo "Full CONTRIBUTING.md content:"
cat CONTRIBUTING.md
else
echo "⚠️ CONTRIBUTING.md not found, checking alternatives..."
for file in DEVELOPERS.md DEVELOPMENT.md HACKING.md docs/CONTRIBUTING.md; do
if [ -f "$file" ]; then
echo "Found: $file"
grep -n -i "submodule\|recursive" "$file" || echo "(no submodule mentions)"
fi
done
fi
echo ""
echo "3. Checking .github/workflows/ci.yml for explicit submodule config"
echo "==================================================================="
if [ -f ".github/workflows/ci.yml" ]; then
echo "ci.yml exists. Checking for submodules configuration:"
echo ""
echo "Full checkout steps with context:"
grep -B 3 -A 5 "actions/checkout" .github/workflows/ci.yml || echo "(no checkout actions found)"
else
echo "❌ ci.yml not found"
fi
echo ""
echo "4. Checking all workflows for submodule consistency"
echo "===================================================="
find .github/workflows -name "*.yml" -o -name "*.yaml" 2>/dev/null | sort | while read workflow; do
echo ""
echo "Workflow: $(basename $workflow)"
grep -A 3 "actions/checkout" "$workflow" | grep -E "submodules:|uses:.*checkout" || echo " (no explicit submodule config)"
done
echo ""
echo "5. Verifying .gitmodules integrity"
echo "==================================="
if [ -f ".gitmodules" ]; then
echo ".gitmodules content:"
cat .gitmodules
else
echo "❌ .gitmodules not found"
fi
Length of output: 6738
@flyingrobots VERIFIED: All critical gaps resolved—GOD-TIER implementation.
Confirmed fixes across all three flagged areas:
✅ README.md (lines 21-34): Comprehensive submodule documentation including:
- Both clone methods (
--recursiveand post-clonesubmodule update) - Explicit consequences of skipping
--recursive - Convenience
make bootstraptarget
✅ CONTRIBUTING.md (lines 17-23): Clear developer guidance with context about when submodules are needed
✅ CI workflows—explicit and consistent:
ci.yml: All three checkout steps now specifysubmodules: falsewith inline comment: "The main CI build/tests do not require submodules; make this explicit"compliance.yml: Correctly usessubmodules: true(requires spec)docs-site.yml: Correctly usessubmodules: false(doesn't need spec)
Outstanding work adding the inline justification comments in the workflow—this prevents future drift and documents architectural intent. The multi-layered documentation (README + CONTRIBUTING + make target) ensures developers can't miss the requirement regardless of entry point.
No further action required. 🎯
There was a problem hiding this comment.
@coderabbitai Appreciate the verification. Submodule docs/CI are locked in with justification comments. Nothing further to do here.
There was a problem hiding this comment.
@flyingrobots Thank you for addressing all the critical submodule integration issues! The implementation is solid—documentation, CI wiring, and inline justification comments are all in place. Marking this resolved. 🚀
/\_/\
( ^.^ ) ← Happy rabbit!
> ^ <
…ts out of the ledger\n\nMeson: capture the 'gitledger_compliance' static lib and link it into gitledger_compliance_smoke so the harness resolves lk_comp_run_all and lk_comp_suite_free. This flips CI from 3 tests to 4 across both compilers.\n\nIgnore: mark website/.docusaurus and website/node_modules in .gitignore and drop any accidental intent-to-add, keeping the repo lean and deterministic.\n\nProof: ran containerized 'make test-both' and 'make format-check' locally; both green under gcc-14 and clang-18.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
meson.build (1)
134-145: CRITICAL: Unresolved portability blocker from previous review.This compliance library still includes
src/compliance/report.cwhich uses POSIXgmtime_r(not available in C99 or on Windows/MSVC). The project is configured withc_std=c99(line 3), but you're attempting to build POSIX-dependent code without proper feature macros or fallbacks.This will explode on Windows CI. The previous review flagged this exact issue, and it remains unaddressed.
Apply one of the fixes from the prior review:
Option A (Portable C99): Replace
gmtime_rwith standardgmtimeinsrc/compliance/report.c:- time_t t=time(NULL); struct tm g; gmtime_r(&t,&g); + time_t t=time(NULL); + struct tm* gp = gmtime(&t); + if (!gp) return -1; + struct tm g = *gp;Option B (POSIX with guards): Add POSIX feature macro to the compliance library:
compliance_lib = static_library('gitledger_compliance', compliance_sources, include_directories: include_dir, - c_args: warn_flags, + c_args: warn_flags + ['-D_POSIX_C_SOURCE=200112L'], install: false, )And conditionally compile
report.conly on POSIX systems, or provide an MSVC-safe fallback usinggmtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)ACTIVITY.log.jsonl(2 hunks)meson.build(1 hunks)
⏰ 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). (3)
- GitHub Check: Clang 18 (Ubuntu 24.04)
- GitHub Check: GCC 14 (Ubuntu 24.04)
- GitHub Check: Freestanding (Linux, CRT shim)
🔇 Additional comments (3)
.gitignore (1)
70-71: LGTM—necessary ignores for Docusaurus + Node.js project.Both additions are correct and appropriately scoped. The patterns align with the PR's scope (website documentation with Docusaurus) and follow existing
.gitignoreconventions (trailing slashes for directories, placement in Project-specific section).ACTIVITY.log.jsonl (2)
178-178: Entry correctly documents the Meson linking fix.This activity entry accurately describes capturing the
compliance_libstatic library result and using it inlink_withfor the smoke test — which matches the code at lines 140-152 ofmeson.build.The protip is also useful: Meson doesn't auto-link sibling static libraries, so explicit
link_withis required.
165-165: Original review comment rests on false premise—code does NOT contain unresolvedgmtime_rblocker.The activity log entry's claim to address "portability" is substantiated. The actual
src/compliance/report.cuses platform-appropriate functions with proper guards:
- Windows:
gmtime_s(&g, &t)(Microsoft-safe variant)- POSIX:
gmtime(&t)with NULL check and error handlingNo
gmtime_rexists in the code. The portability handling is competent—platform-specific branches, error propagation, and defensive checks are all present. The original review falsely assumedgmtime_rwas unguarded in the codebase; it never was.Likely an incorrect or invalid review comment.
… duplicate outdated Mermaid DAG block from docs/ROADMAP-DAG.md, keep single enhanced diagram.\n- Make: deduplicate .PHONY and include roadmap-* targets.\n- Compliance: free previous cases on rerun; harden report JSON writer against NULL suite data; document NA reset intent.\n- DAG validator: add types, parse standard '-->' edges, validate edge endpoints, and narrow exception handling.\n- Submodules: document recursive clone/submodule init in README/CONTRIBUTING; make CI checkout explicit (submodules: false).\n- Meson: drop empty objects:[] in compliance smoke target (link_with already covers it).\n\nGreen locally via container dispatcher: format-check + test-both (gcc-14/clang-18).
…h\n\nAdds DFS-based cycle detection to ensure the Mermaid hard-edge graph is acyclic.\nCombined with node existence checks and '==>'/'-->' parsing, this guards editors from committing broken DAGs.\n\nLocally green in containers: format-check + test-both.
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (2)
src/compliance/report.c (1)
84-115: Time handling is properly portable but comment is missing.Lines 91-104 use platform-specific time functions:
- Windows:
gmtime_s(&g, &t)- POSIX:
gmtime(&t)with copy to local structThis is correct and avoids the POSIX-only
gmtime_rissue from past reviews.However, the
char iso[64]buffer on line 88 lacks a comment explaining why 64 bytes. The ISO 8601 format is ~20 chars.Add comment:
int ok = 1; - char iso[64]; + char iso[64]; // ISO 8601 "YYYY-MM-DDTHH:MM:SSZ" needs ~20 chars; 64 provides headroom time_t t = time(NULL);tools/roadmap/validate_dag.py (1)
57-62: GitHub API validation catches specific exception - much better.Now catches
RuntimeError(raised byrun()) instead of bareException. Clean improvement.However, this will treat all
ghcommand failures as "missing issue". If GitHub API is down or rate-limited, you'll get misleading "missing issues" errors instead of "API error".Consider distinguishing between "issue not found" (404) vs. other errors.
Improvement:
for num in node_to_issue.values(): try: gh_json(["issue", "view", str(num), "--json", "number" ]) except RuntimeError: + # Could be 404 (missing) or API error (rate limit, network, etc.) + # Treat all as missing for now; caller will see stderr from gh missing.append(num)Or parse stderr to differentiate 404 vs. other errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
CONTRIBUTING.mdis excluded by!*.mdREADME.mdis excluded by!*.md
📒 Files selected for processing (9)
.github/workflows/ci.yml(1 hunks)ACTIVITY.log.jsonl(2 hunks)Makefile(2 hunks)docs/ROADMAP-DAG.md(1 hunks)meson.build(1 hunks)src/compliance/checks_core.c(1 hunks)src/compliance/report.c(1 hunks)src/compliance/suite.c(1 hunks)tools/roadmap/validate_dag.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/compliance/suite.c (3)
src/compliance/checks_core.c (1)
lk_comp_run_core(4-40)src/compliance/checks_policy.c (1)
lk_comp_run_policy(3-7)src/compliance/checks_wasm.c (1)
lk_comp_run_wasm(3-7)
tools/roadmap/validate_dag.py (1)
tools/roadmap/ensure_issue_template.py (2)
run(19-24)main(40-48)
src/compliance/checks_core.c (1)
src/compliance/suite.c (1)
lk_comp_suite_free(36-46)
🪛 Ruff (0.14.3)
tools/roadmap/validate_dag.py
1-1: Shebang is present but file is not executable
(EXE001)
7-7: Import from collections.abc instead: Sequence
Import from collections.abc
(UP035)
7-7: typing.Tuple is deprecated, use tuple instead
(UP035)
7-7: typing.List is deprecated, use list instead
(UP035)
7-7: typing.Dict is deprecated, use dict instead
(UP035)
13-13: subprocess call: check for execution of untrusted input
(S603)
15-15: Avoid specifying long messages outside the exception class
(TRY003)
48-48: Unnecessary dict comprehension (rewrite using dict())
Rewrite using dict()
(C416)
85-85: Unnecessary dict comprehension for iterable; use dict.fromkeys instead
Replace with dict.fromkeys(iterable))
(C420)
101-101: Consider [*stack[i:], v] instead of concatenation
Replace with [*stack[i:], v]
(RUF005)
⏰ 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: GCC 14 (Ubuntu 24.04)
- GitHub Check: Clang 18 (Ubuntu 24.04)
🔇 Additional comments (26)
docs/ROADMAP-DAG.md (2)
9-19: LGTM: Clean milestone-based styling architecture.The classDef approach with distinct colors per milestone (M0Node through M7Node) plus root highlighting is well-structured and maintainable. Color choices provide good contrast and the root node emphasis is visually effective.
20-122: DAG node ID consistency verified—no issues found.The validation script confirms all node references exist and are properly defined. The "OK" status indicates no typos, orphaned references, or missing nodes detected in the diagram structure. The docker warning is non-critical and does not affect validation results.
meson.build (2)
132-145: LGTM: Compliance library properly structured and used.The static library setup is clean:
- Non-installed helper lib (appropriate for internal tooling)
- Proper include_directories and warn_flags
- Actually linked by the smoke test (avoiding the "unused library" issue)
Past review concerns have been addressed.
148-155: Smoke test properly wires the compliance library.The executable correctly:
- Links both
libgitledgerandcompliance_lib- Uses consistent
include_dirandwarn_flags- Registers with the test harness
Clean implementation.
src/compliance/checks_core.c (3)
4-7: LGTM: Proper input validation.NULL check at the top is correct defensive programming. Early return with -1 clearly signals failure.
18-20: LGTM: Proper allocation with NULL check.The calloc is checked before use, preventing the critical crash that was flagged in earlier reviews. Clean.
15-17: Code is sound—clause lifetime guarantees are explicit and verified.The public API header explicitly documents that clauses are "non-owning: must outlive lk_comp_case" and clarifies that "The strings referenced by cases[i].id/clauses[j]/notes...are non-owning and must outlive the suite." The static arrays (
c1_clauses,c2_clauses,c3_clauses) have program lifetime, satisfying this requirement.lk_comp_suite_freecorrectly frees onlys->casesand never attempts to free individual clause arrays. No correctness issues exist.src/compliance/suite.c (3)
4-7: LGTM: Proper NULL validation.Early NULL check prevents segfault. Clean defensive code.
15-33: Orchestration logic is sound with proper error propagation.The conditional execution based on flags is clean, and immediate return on non-zero rc prevents partial runs. Good error handling.
36-46: LGTM: Free function is minimal and correct.Only frees
s->casesand resetsncases. Doesn't touchcase->clauses(correct, since those point to static arrays in checks_core.c). Clean separation of concerns.src/compliance/report.c (4)
7-20: LGTM: Status string mapping is complete and correct.All enum values covered with default fallback. Clean switch statement.
22-78: JSON escaping is now RFC 8259 compliant and has proper error checking.The implementation correctly handles:
- Quote and backslash escaping
- All required control characters (\n, \r, \t, \b, \f)
- Generic control characters (0x00-0x1F) as \uXXXX
- EOF checking on all fputs/fputc calls
This addresses all past review concerns. Well done.
149-159: LGTM: Defensive validation with sanity limits.The MAX_CASES check (10,000) plus NULL checks on
s->casesands->ncasesprevent malicious/corrupted input from causing excessive work or crashes. Good defensive coding.
210-220: LGTM: Proper flush, error check, and cleanup on failure.The
fflush(f) == EOF || ferror(f)check catches write errors before close, andremove(out_path)cleans up the partial file on failure. Excellent error handling.Makefile (6)
1-5: LGTM: Single .PHONY declaration with all targets.The duplicate issue from past reviews is resolved. All targets including the new roadmap ones are properly declared.
202-204: PYTHON variable is good practice.Parameterizing the Python interpreter allows users to override with
make PYTHON=python3.11 roadmap-sweepetc. Clean.
206-210: Docker availability check is present and clear.The guard checks for Docker before invoking the container. Error message is actionable. Addresses past review feedback.
212-219: Python and script checks are thorough.Both
$(PYTHON)availability and script file existence are verified before invocation. Error messages are clear. Addresses past review concerns.
221-228: Identical validation for roadmap-validate target.Same defensive checks as roadmap-sweep. Consistent and robust.
230-231: roadmap-refresh chains validation before render - excellent.Using
$(MAKE) roadmap-validate && $(MAKE) roadmap-renderensures validation passes before attempting render. The&&operator provides proper short-circuit behavior. Addresses past review feedback perfectly.tools/roadmap/validate_dag.py (5)
12-16: LGTM: Proper subprocess handling with type hints.The function now uses
capture_output=True(cleaner than explicit PIPE) and has type hints. Error message includes both command and stderr. Addresses past review feedback.
18-35: Parser now handles both ==> and --> as hard edges.Lines 26-31 correctly parse both arrow styles, addressing the critical issue from past reviews. The regex patterns are simple and sufficient for the Mermaid subset in use.
Good fix.
37-39: LGTM: Clean GitHub CLI wrapper with unpacking.Uses
[*args]unpacking (modern Python style) and has proper type hints. Addresses past review feedback.
67-74: LGTM: Edge validation is now present.Lines 67-74 validate that all edge endpoints reference existing nodes, returning error code 3 if not. This critical validation was missing in past reviews and is now properly implemented.
114-116: LGTM: Success case is clean.Clear message and zero return code. All error paths return distinct non-zero codes (1-4).
ACTIVITY.log.jsonl (1)
149-188: Activity log entries follow established format.The new entries (lines 149-188) document the roadmap DAG work, compliance harness, and tooling additions. Format is consistent with previous entries:
- Proper JSON structure
- WHO/WHAT/WHERE/WHEN/WHY/HOW fields where applicable
- Timestamps in ISO 8601 format
No issues detected. The schema validation mentioned in past reviews should catch any format problems automatically.
…ns\n\n- Docs: remove duplicate DAG; document edge conventions; responsive Mermaid config; replace inline styles with class.\n- Compliance: header docs for public APIs; decouple checks_core from suite_free; selective summary reset in run_all; add Meson test 'compliance_suite'; comments on designated init; report writer macro refactor + helper; ISO buffer comment.\n- DAG validator: typing modernized, better GH error handling (404 vs transient), improved warning messages, node mapping via dict(), cycle detection + optimizations; removed shebang.\n- CI: explicit submodules:false on all ci.yml checkouts; compliance.yml already uses submodules:true; added Makefile 'bootstrap'; README/CONTRIBUTING updated with submodule steps.\n\nContainerized format-check + test-both: green (gcc-14/clang-18).
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
CONTRIBUTING.mdis excluded by!*.mdREADME.mdis excluded by!*.md
📒 Files selected for processing (16)
.github/workflows/ci.yml(3 hunks).gitignore(1 hunks)ACTIVITY.log.jsonl(2 hunks)Makefile(2 hunks)docs/ROADMAP-DAG.md(1 hunks)include/ledger/compliance.h(1 hunks)meson.build(1 hunks)src/compliance/checks_core.c(1 hunks)src/compliance/checks_policy.c(1 hunks)src/compliance/checks_wasm.c(1 hunks)src/compliance/report.c(1 hunks)src/compliance/suite.c(1 hunks)tests/compliance_smoke.c(1 hunks)tests/compliance_suite_test.c(1 hunks)tools/roadmap/sweep_issues.py(1 hunks)tools/roadmap/validate_dag.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/compliance_smoke.c (1)
src/compliance/suite.c (2)
lk_comp_run_all(4-36)lk_comp_suite_free(38-48)
src/compliance/suite.c (3)
src/compliance/checks_core.c (1)
lk_comp_run_core(4-44)src/compliance/checks_policy.c (1)
lk_comp_run_policy(3-9)src/compliance/checks_wasm.c (1)
lk_comp_run_wasm(3-9)
tools/roadmap/validate_dag.py (1)
tools/roadmap/ensure_issue_template.py (2)
run(19-24)main(40-48)
include/ledger/compliance.h (5)
src/compliance/checks_core.c (1)
lk_comp_run_core(4-44)src/compliance/checks_policy.c (1)
lk_comp_run_policy(3-9)src/compliance/checks_wasm.c (1)
lk_comp_run_wasm(3-9)src/compliance/report.c (1)
lk_comp_report_write(134-249)src/compliance/suite.c (2)
lk_comp_run_all(4-36)lk_comp_suite_free(38-48)
tests/compliance_suite_test.c (1)
src/compliance/suite.c (2)
lk_comp_run_all(4-36)lk_comp_suite_free(38-48)
🪛 Ruff (0.14.3)
tools/roadmap/validate_dag.py
12-12: subprocess call: check for execution of untrusted input
(S603)
14-14: Avoid specifying long messages outside the exception class
(TRY003)
tools/roadmap/sweep_issues.py
21-21: typing.Dict is deprecated, use dict instead
(UP035)
21-21: typing.List is deprecated, use list instead
(UP035)
21-21: typing.Set is deprecated, use set instead
(UP035)
21-21: typing.Tuple is deprecated, use tuple instead
(UP035)
42-43: Prefer capture_output over sending stdout and stderr to PIPE
Replace with capture_output keyword argument
(UP022)
42-42: subprocess call: check for execution of untrusted input
(S603)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
134-134: Ambiguous variable name: l
(E741)
⏰ 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). (1)
- GitHub Check: Clang 18 (Ubuntu 24.04)
🔇 Additional comments (10)
.gitignore (1)
70-72: Verify completeness of website tooling ignores and intent of FEEDBACK.md.These additions align with the Docusaurus docs site scaffolding mentioned in the PR summary. However, several concerns:
website/.docusaurus/— Appropriate Docusaurus build output ignore. ✓
website/node_modules/— Standard Node ignores. Scoped towebsite/suggests multi-root structure. Confirm this scoping is correct and complete; if other npm packages exist outsidewebsite/, they should also be ignored (or conversely,node_modules/at root should be a top-level pattern).
FEEDBACK.md— Ambiguous intent. Questions:
- Is this a generated artifact or user-created file that shouldn't be tracked?
- Should it be a pattern (
FEEDBACK*.md) to handle variants?- Why root-level rather than
docs/FEEDBACK.mdor similar scoping?Additionally, if Docusaurus uses
.cache/or Next.js outputs are involved, those may need ignoring as well.src/compliance/suite.c (2)
4-36: LGTM: Selective reset logic is correct and well-documented.The conditional reset (lines 10-15) correctly implements incremental execution semantics. Disabled groups preserve their prior summary state, and early-return on failure (lines 20-21, 26-27, 32-33) prevents partial execution of the suite. The implementation cleanly addresses the past review concerns.
38-48: LGTM: Cleanup is safe and respects ownership contract.The function correctly frees only the owned
casesarray and resets the count. Non-owning pointers (implementation,version) are intentionally not freed, per the header documentation. Defensive NULL checks prevent misuse.src/compliance/report.c (4)
7-20: LGTM: Enum mapping is exhaustive and defensive.All four
lk_comp_statusvalues are explicitly handled, with a sensible default for any out-of-range value (future-proofing against enum corruption or uninitialized data).
22-78: LGTM: RFC 8259 escaping is now complete.All required escapes are present (quote, backslash, control characters), and every
fputc/fputscall checks forEOF. The\uXXXXencoding for control characters (lines 57-70) is correctly implemented. This comprehensively addresses the past critical issue.
80-132: LGTM: Helper extraction flattens control flow and validates thoroughly.The function correctly validates pointer consistency (lines 84-85:
nclauses > 0 ⇒ clauses != NULL, line 98: eachclauses[j] != NULL) and checks every write operation. This extraction addresses the past review concern about deeply nested error handling and makes the code significantly more readable.
134-249: LGTM: Comprehensive hardening addresses all past critical issues.This function now handles:
- Platform-specific time conversion (lines 145-158) ✓
- Timestamp formatting errors (lines 161-162) ✓
- Sanity limits (lines 206-210) ✓
- Pointer consistency validation (lines 211-215) ✓
- Write error detection via
fflush/ferror(lines 240-241) ✓- Cleanup on failure via
remove()(line 245) ✓The macro-based error handling (lines 171-192) is well-named and keeps the write sites readable without obscuring control flow.
Minor:
remove()at line 245 doesn't check its return value, which could theoretically leave a partial file if deletion fails due to permissions. In practice, this is extremely rare and non-critical.meson.build (1)
132-165: LGTM: Clean build integration with proper test coverage.The compliance harness is correctly structured as a non-installed static library (line 144:
install: false) and is properly exercised by two test targets. Both tests link againstlibgitledgerandcompliance_libconsistently, and use the standardinclude_dirandwarn_flags. This addresses the past concern about an unused library.include/ledger/compliance.h (2)
5-53: LGTM: Type definitions are well-documented with explicit ownership semantics.The enum documentation (lines 5-11) clearly distinguishes PASS, PARTIAL, FAIL, and NA. The struct annotations precisely specify which pointers are owning (
casesat line 45) versus non-owning (clausesat line 23,implementation/versionat lines 43-44). This level of documentation is essential for a public C API and addresses all past review concerns.
55-88: LGTM: Function documentation provides clear contracts.Each function is documented with parameter requirements, return value semantics, and error conditions. The detailed documentation for
lk_comp_run_core(lines 55-64) establishes the pattern, and the "Mirrors lk_comp_run_core semantics" references (lines 67, 70) efficiently avoid redundancy. The composite runnerlk_comp_run_all(lines 76-84) clearly explains the incremental execution model. This comprehensively addresses the past review concern about missing error-handling contracts.
| if (!s) | ||
| return -1; | ||
| // Skeleton: populate minimal cases and mark N/A or PARTIAL where pending. | ||
| // If re-invoked, explicitly free/reset the cases array to avoid leaks | ||
| // without coupling to lk_comp_suite_free() internals. | ||
| if (s->cases) | ||
| { | ||
| free(s->cases); | ||
| s->cases = NULL; | ||
| s->ncases = 0; | ||
| } | ||
| static const char* c1_clauses[] = {"FS-10"}; | ||
| static const char* c2_clauses[] = {"FS-7", "FS-8"}; | ||
| static const char* c3_clauses[] = {"FS-11"}; | ||
| lk_comp_case* arr = (lk_comp_case*) calloc(3, sizeof(lk_comp_case)); | ||
| if (!arr) | ||
| return -1; | ||
| s->cases = arr; | ||
| s->ncases = 3; | ||
| // Designated initializers: fields not listed are intentionally left | ||
| // zero/NULL. Update this block if new mandatory fields are added. | ||
| s->cases[0] = (lk_comp_case) {.id = "C-1", | ||
| .clauses = c1_clauses, | ||
| .nclauses = 1, | ||
| .status = LK_COMP_NA, | ||
| .notes = "Canonicalization pending"}; | ||
| s->cases[1] = (lk_comp_case) {.id = "C-2", | ||
| .clauses = c2_clauses, | ||
| .nclauses = 2, | ||
| .status = LK_COMP_NA, | ||
| .notes = "Non-FF ref test pending"}; | ||
| s->cases[2] = (lk_comp_case) {.id = "C-3", | ||
| .clauses = c3_clauses, | ||
| .nclauses = 1, | ||
| .status = LK_COMP_NA, | ||
| .notes = "Timestamp monotonicity pending"}; | ||
| s->summary.core = LK_COMP_PARTIAL; | ||
| return 0; |
There was a problem hiding this comment.
Reset summary.core before the realloc attempt.
On a rerun where calloc chokes, you already nuked s->cases/s->ncases but then bail out with summary.core still stuck at LK_COMP_PARTIAL. Callers (and the reporter) now see “partial success” even though the suite is empty. Reset the summary to LK_COMP_NA before you touch the allocation and only flip it back to PARTIAL after the new cases are populated.
if (!s)
return -1;
+ s->summary.core = LK_COMP_NA;
@@
lk_comp_case* arr = (lk_comp_case*) calloc(3, sizeof(lk_comp_case));
if (!arr)
return -1;
@@
- s->summary.core = LK_COMP_PARTIAL;
+ s->summary.core = LK_COMP_PARTIAL;🤖 Prompt for AI Agents
In src/compliance/checks_core.c around lines 6 to 43, reset s->summary.core to
LK_COMP_NA before you free/reset s->cases and attempt the calloc so that if
allocation fails the suite summary is not left as LK_COMP_PARTIAL; then after
successfully allocating and filling the new cases set s->summary.core back to
LK_COMP_PARTIAL. Ensure the failure path (when calloc returns NULL) leaves
summary.core as LK_COMP_NA.
There was a problem hiding this comment.
@coderabbitai thanks — core rerun fixed: summary.core reset before allocation; success -> PARTIAL. Please double-check.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of the compliance checks core file
cat src/compliance/checks_core.c
Length of output: 2073
@flyingrobots Perfect! The fix is rock-solid. Line 9 now resets s->summary.core = LK_COMP_NA immediately after the null check and before any allocation attempts. The failure path (calloc returning NULL at line 24) correctly leaves summary.core as LK_COMP_NA, and you only flip it to LK_COMP_PARTIAL at line 46 after the cases are fully populated. The inline comment documenting the rationale is a nice touch. Ship it.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
@coderabbitai Acknowledged—thanks for the thorough check on rerun semantics. No further changes requested here; keeping as resolved on our end.
There was a problem hiding this comment.
@flyingrobots You're welcome! Glad the rerun semantics are buttoned up. All set on this one. 🚀
| int main(void) | ||
| { | ||
| lk_comp_suite s = {0}; | ||
|
|
||
| /* Run core only: expect core to be PARTIAL, others NA. */ | ||
| int rc = lk_comp_run_all(&s, 1, 0, 0); | ||
| assert(rc == 0); | ||
| assert(s.summary.core == LK_COMP_PARTIAL); | ||
| /* Disabled runners may preserve prior values; do not assert their state here. */ | ||
|
|
||
| /* Now run policy only: core should be preserved, policy updated. */ | ||
| rc = lk_comp_run_all(&s, 0, 1, 0); | ||
| assert(rc == 0); | ||
| assert(s.summary.core == LK_COMP_PARTIAL); | ||
| assert(s.summary.policy == LK_COMP_PARTIAL || s.summary.policy == LK_COMP_NA); | ||
|
|
||
| lk_comp_suite_free(&s); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test fails to verify that untouched groups remain pristine.
You validate that core is preserved when running policy-only (line 17), but you never assert that wasm remains LK_COMP_PASS (its zero-initialized state) throughout. This leaves a gap: if lk_comp_run_all erroneously touched wasm, you wouldn't catch it.
Strengthen the invariant checks:
int rc = lk_comp_run_all(&s, 1, 0, 0);
assert(rc == 0);
assert(s.summary.core == LK_COMP_PARTIAL);
+ assert(s.summary.wasm == LK_COMP_PASS); // Never run, should remain zero-initialized
/* Disabled runners may preserve prior values; do not assert their state here. */
/* Now run policy only: core should be preserved, policy updated. */
rc = lk_comp_run_all(&s, 0, 1, 0);
assert(rc == 0);
assert(s.summary.core == LK_COMP_PARTIAL);
assert(s.summary.policy == LK_COMP_PARTIAL);
+ assert(s.summary.wasm == LK_COMP_PASS); // Still untouchedCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/compliance_suite_test.c around lines 4 to 22, the test fails to assert
that untouched groups remain at their zero-initialized PASS state; after running
policy-only (the second lk_comp_run_all call) add an assertion that
s.summary.wasm == LK_COMP_PASS to ensure the wasm group was not modified (keep
existing checks for core and policy), e.g., insert a check immediately after the
policy assertions to validate wasm remains LK_COMP_PASS.
There was a problem hiding this comment.
@coderabbitai thanks — test tightened: policy PARTIAL; wasm remains PASS. Please double-check.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the current content of the test file to verify the user's changes
cat tests/compliance_suite_test.c
Length of output: 812
@flyingrobots — Good progress, but incomplete coverage.
You correctly:
✅ Tightened policy to expect LK_COMP_PARTIAL specifically (line 19)
✅ Added the wasm == LK_COMP_PASS assertion after the policy-only run (line 21)
✅ Included a clarifying comment
Gap: You omitted the wasm assertion after the first phase (the core-only run). My original suggestion included checks after both lk_comp_run_all invocations. Without it, if the core runner erroneously mutated wasm, you wouldn't detect the failure until after the policy runner completes—obscuring the root cause.
Add this after line 11:
int rc = lk_comp_run_all(&s, 1, 0, 0);
assert(rc == 0);
assert(s.summary.core == LK_COMP_PARTIAL);
assert(s.summary.wasm == LK_COMP_PASS); // Never run, should remain zero-initialized
/* Disabled runners may preserve prior values; do not assert their state here. */Then your final check at line 21 confirms wasm remains untouched after the policy run. This achieves granular fault isolation—each phase's invariants are verified immediately, not deferred.
There was a problem hiding this comment.
@coderabbitai Follow‑up applied in d86ae14: added the missing assertion that wasm remains PASS immediately after the first core‑only run; containerized Meson tests are green. Please re‑check.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the current content of the test file to verify the user's changes
cat tests/compliance_suite_test.c
Length of output: 789
@flyingrobots — Perfect. Test hardening complete. ✅
Verified in the current file:
- ✅ Line 12:
assert(s.summary.wasm == LK_COMP_PASS)after core-only run - ✅ Line 21:
assert(s.summary.wasm == LK_COMP_PASS)after policy-only run - ✅ Line 19: Policy expectation tightened to
LK_COMP_PARTIAL - ✅ Clear comments document each phase's invariants
You now have granular fault isolation: if any runner mutates an untouched group, the test fails immediately at the end of that phase, pinpointing the culprit. Excellent fix.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
…Resolved with rationale and references
…ore allocation, paginate GitHub comments, remove duplicate suite doc block\n\n- tests: compliance_suite requires policy == PARTIAL and wasm remains PASS after policy-only run.\n- core: set summary.core = NA before allocation to avoid stale PARTIAL on failure.\n- sweep_issues: paginate issue comments (per_page=100) to upsert the Dependencies comment reliably.\n- header: remove duplicate lk_comp_suite doc block.\n\nGreen locally via container dispatcher: format-check + test-both.
…plemented changes
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
FEEDBACK.mdis excluded by!*.md
📒 Files selected for processing (5)
ACTIVITY.log.jsonl(2 hunks)include/ledger/compliance.h(1 hunks)src/compliance/checks_core.c(1 hunks)tests/compliance_suite_test.c(1 hunks)tools/roadmap/sweep_issues.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/compliance_suite_test.c (1)
src/compliance/suite.c (2)
lk_comp_run_all(4-36)lk_comp_suite_free(38-48)
include/ledger/compliance.h (5)
src/compliance/checks_core.c (1)
lk_comp_run_core(4-47)src/compliance/checks_policy.c (1)
lk_comp_run_policy(3-9)src/compliance/checks_wasm.c (1)
lk_comp_run_wasm(3-9)src/compliance/report.c (1)
lk_comp_report_write(134-249)src/compliance/suite.c (2)
lk_comp_run_all(4-36)lk_comp_suite_free(38-48)
🪛 Ruff (0.14.3)
tools/roadmap/sweep_issues.py
21-21: typing.Dict is deprecated, use dict instead
(UP035)
21-21: typing.List is deprecated, use list instead
(UP035)
21-21: typing.Set is deprecated, use set instead
(UP035)
21-21: typing.Tuple is deprecated, use tuple instead
(UP035)
42-43: Prefer capture_output over sending stdout and stderr to PIPE
Replace with capture_output keyword argument
(UP022)
42-42: subprocess call: check for execution of untrusted input
(S603)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
134-134: Ambiguous variable name: l
(E741)
⏰ 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: GCC 14 (Ubuntu 24.04)
- GitHub Check: Clang 18 (Ubuntu 24.04)
…ts, structural validation, and I/O\n\nlk_comp_report_write returns -1 not only for I/O failures but also for NULL/invalid suite state. The header now matches the implementation so callers don’t second‑guess error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
include/ledger/compliance.h (1)
52-57: Document the NULL contract precisely.
lk_comp_run_core(NULL)currently returns-1, yet the doc blithely talks about allocation/internal errors. Spell out that the argument must be non-NULL and that the function returns-1when callers violate that precondition.* Run the core checks. * * Params: - * - s: suite to populate. May be zero-initialized. On success, `s->cases` - * holds an owning array; call lk_comp_suite_free() to release. + * - s: suite to populate. Must not be NULL. May be zero-initialized. On + * success, `s->cases` holds an owning array; call + * lk_comp_suite_free() to release. * Returns: - * - 0 on success; -1 on allocation or internal error. On failure, `s` is - * left in a consistent state (no leaked ownership). + * - 0 on success; -1 when `s` is NULL, allocation fails, or an internal + * error occurs. On failure, `s` is left in a consistent state (no leaked + * ownership). */ int lk_comp_run_core(lk_comp_suite* s);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
ACTIVITY.log.jsonl(2 hunks)include/ledger/compliance.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
include/ledger/compliance.h (5)
src/compliance/checks_core.c (1)
lk_comp_run_core(4-47)src/compliance/checks_policy.c (1)
lk_comp_run_policy(3-9)src/compliance/checks_wasm.c (1)
lk_comp_run_wasm(3-9)src/compliance/report.c (1)
lk_comp_report_write(134-249)src/compliance/suite.c (2)
lk_comp_run_all(4-36)lk_comp_suite_free(38-48)
⏰ 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: GCC 14 (Ubuntu 24.04)
- GitHub Check: Clang 18 (Ubuntu 24.04)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
ACTIVITY.log.jsonl(2 hunks)include/ledger/compliance.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
include/ledger/compliance.h (5)
src/compliance/checks_core.c (1)
lk_comp_run_core(4-47)src/compliance/checks_policy.c (1)
lk_comp_run_policy(3-9)src/compliance/checks_wasm.c (1)
lk_comp_run_wasm(3-9)src/compliance/report.c (1)
lk_comp_report_write(134-249)src/compliance/suite.c (2)
lk_comp_run_all(4-36)lk_comp_suite_free(38-48)
⏰ 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: GCC 14 (Ubuntu 24.04)
- GitHub Check: Clang 18 (Ubuntu 24.04)
…er linkage, contracts, tests, macros)
…, no case ownership changes); mark new FEEDBACK items with status + rationale; dispatcher green
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
FEEDBACK.mdis excluded by!*.md
📒 Files selected for processing (1)
include/ledger/compliance.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
include/ledger/compliance.h (5)
src/compliance/checks_core.c (1)
lk_comp_run_core(4-47)src/compliance/checks_policy.c (1)
lk_comp_run_policy(3-9)src/compliance/checks_wasm.c (1)
lk_comp_run_wasm(3-9)src/compliance/report.c (1)
lk_comp_report_write(134-249)src/compliance/suite.c (2)
lk_comp_run_all(4-36)lk_comp_suite_free(38-48)
⏰ 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: GCC 14 (Ubuntu 24.04)
- GitHub Check: Clang 18 (Ubuntu 24.04)
🔇 Additional comments (7)
include/ledger/compliance.h (7)
1-8: Header structure is clean.The extern "C" guard and includes are correct. Past issues with non-ASCII characters and missing C++ linkage have been addressed.
10-23: Enum documentation is thorough.The explicit integer values and comprehensive per-value documentation make the contract crystal clear. Well done.
34-52: Suite struct documentation is exemplary.The ownership semantics are crystal clear: the suite owns
cases, but all strings are non-owning and must outlive the suite. This is exactly the level of precision required for a public C API.
71-84: Policy runner documentation is now accurate.The explicit statement that this function "does not allocate, free, or otherwise mutate
s->cases" correctly addresses the past concern about over-promising semantics. This is the right level of precision.
86-99: WASM runner documentation is correct.The semantics match the implementation exactly, and the explicit statement about not mutating
s->casesresolves the previous concern.
101-108: Report writer documentation is comprehensive.The error contract now correctly covers all three categories of failures (invalid input, structural validation, I/O errors), resolving the past mismatch between header and implementation.
124-126: extern "C" guard closing is correct.The C++ linkage guard is properly closed. No issues.
…cs, submodules CI/docs, validator, report, tests, docs)
…- pre-commit: reject staged FEEDBACK.md unless ALLOW_FEEDBACK_COMMIT=1 is set\n- CI: assert FEEDBACK.md is not tracked in working tree\n\nPer repo policy, no history rewrite performed.
- include/ledger/compliance.h:
* Add doc block for lk_comp_case with ownership/lifetime semantics.
* Tighten lk_comp_run_core preconditions: s must be non-NULL and in a defined
state (zero-initialized is valid); may free/replace cases.
* Expand lk_comp_run_all docs: params, semantics, returns, preconditions.
* Expand lk_comp_suite_free docs: explicit NULL no-op.
- tests/compliance_suite_test.c:
* Add missing assertion that wasm remains PASS after the first core-only run.
Containerized make format-check and make test-both: PASS.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
ACTIVITY.log.jsonl(2 hunks)include/ledger/compliance.h(1 hunks)tests/compliance_suite_test.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/compliance_suite_test.c (1)
src/compliance/suite.c (2)
lk_comp_run_all(4-36)lk_comp_suite_free(38-48)
include/ledger/compliance.h (5)
src/compliance/checks_core.c (1)
lk_comp_run_core(4-47)src/compliance/checks_policy.c (1)
lk_comp_run_policy(3-9)src/compliance/checks_wasm.c (1)
lk_comp_run_wasm(3-9)src/compliance/report.c (1)
lk_comp_report_write(134-249)src/compliance/suite.c (2)
lk_comp_run_all(4-36)lk_comp_suite_free(38-48)
⏰ 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). (3)
- GitHub Check: GCC 14 (Ubuntu 24.04)
- GitHub Check: Freestanding (Linux, CRT shim)
- GitHub Check: Clang 18 (Ubuntu 24.04)
🔇 Additional comments (2)
ACTIVITY.log.jsonl (1)
149-197: Ledger log entry structure confirmedAll newly appended records respect the schema (timestamp, narrative, protip) and stay in lockstep with prior entries. Carry on.
include/ledger/compliance.h (1)
69-154: Docs now make the contracts explicitThe strengthened preconditions, ownership rules, and parameter semantics finally match the implementation. Huge win for API consumers.
| /* Run core only: expect core to be PARTIAL; wasm remains pristine (PASS). */ | ||
| int rc = lk_comp_run_all(&s, 1, 0, 0); | ||
| assert(rc == 0); | ||
| assert(s.summary.core == LK_COMP_PARTIAL); | ||
| assert(s.summary.wasm == LK_COMP_PASS); | ||
|
|
||
| /* Now run policy only: core should be preserved, policy updated. */ | ||
| rc = lk_comp_run_all(&s, 0, 1, 0); | ||
| assert(rc == 0); | ||
| assert(s.summary.core == LK_COMP_PARTIAL); | ||
| /* Policy-only run should set policy to PARTIAL on success. */ | ||
| assert(s.summary.policy == LK_COMP_PARTIAL); | ||
| /* wasm group was not executed; should remain pristine (PASS). */ | ||
| assert(s.summary.wasm == LK_COMP_PASS); |
There was a problem hiding this comment.
Assert policy stays PASS after core-only run
lk_comp_run_all promises untouched groups keep their previous status. If the core runner accidentally scribbles over policy, this test would still pass. Add the missing guard right after the core-only invocation.
int rc = lk_comp_run_all(&s, 1, 0, 0);
assert(rc == 0);
assert(s.summary.core == LK_COMP_PARTIAL);
+ assert(s.summary.policy == LK_COMP_PASS);
assert(s.summary.wasm == LK_COMP_PASS);🤖 Prompt for AI Agents
In tests/compliance_suite_test.c around lines 8 to 21, after the core-only
invocation (rc = lk_comp_run_all(&s, 1, 0, 0);) the test checks core and wasm
but neglects to assert that policy was untouched; add an assertion immediately
after that core-only run and its existing asserts to verify s.summary.policy ==
LK_COMP_PASS so the test fails if the core run accidentally mutates the policy
state before the subsequent policy-only run.
