ci: add GitHub Actions build/test + ASan/UBSan (fix cmake test build) - #43
Conversation
There is currently no build/test CI running on PRs (CircleCI does not report a
status; only Hound and Cursor Bugbot run), so regressions in the library or the
unit tests can land unnoticed.
- Add .github/workflows/ci.yml:
* build-and-test: cmake build + ctest on gcc and clang.
* sanitizers: build td_test with -fsanitize=address,undefined and run it
(leak detection on; allocator_may_return_null=1 so an oversized capacity
request exercises the allocation-failure path instead of aborting).
- Fix the cmake unit-test build: minunit.h uses clock_gettime(), which the
top-level "-std=c99" hides unless _POSIX_C_SOURCE is defined on the command
line before any header. Without this, `ctest` fails to build td_test with
"unknown type name 'clockid_t'". Add the define via target_compile_definitions.
Both jobs verified green locally (gcc+clang ctest pass; ASan+UBSan clean).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gabsow
left a comment
gabsow
left a comment
There was a problem hiding this comment.
The build/test and sanitizer jobs look good, but I found one workflow-security blocker before approval.
This repository currently has default_workflow_permissions: write. The new workflow runs pull-request-controlled code without overriding that default, and actions/checkout@v4 persists its token credentials by default. A test or build step could therefore access a write-capable token in same-repository PR runs.
Please scope the workflow explicitly:
permissions:
contents: readAnd preferably avoid leaving even the read token in the checkout after the fetch:
- uses: actions/checkout@v4
with:
persist-credentials: falseOnce that is added, the PR looks ready for approval.
Address review: the repo default is default_workflow_permissions: write, and actions/checkout@v4 persists its token, so pull-request-controlled build/test steps could reach a write-capable token in same-repo PR runs. Scope the workflow to `permissions: contents: read` and set `persist-credentials: false` on checkout. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks — addressed in facdfb3. Scoped the workflow to |
gabsow
left a comment
gabsow
left a comment
There was a problem hiding this comment.
Re-reviewed facdfb3. The previous blocker is resolved: the workflow now uses read-only permissions and neither checkout step persists credentials. The GCC, Clang, and sanitizer jobs are green. Approved.
|
You can merge this one |
The sanitizer job compiled and ran only td_test, so other test targets (e.g. td_capacity_test, td_sort_complexity_test) were not covered by ASan/UBSan. Build the suite with sanitizer flags through CMake and run it via CTest, so every registered test target is covered automatically. A non-Release build avoids the top-level -std=c99 that hides POSIX clock_gettime() in minunit.h. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 6054d44. Configure here.
The project defaults CMAKE_BUILD_TYPE to Release, whose top-level -std=c99 hides POSIX clock_gettime() in minunit.h, so a default-type sanitizer build would only compile targets that carry a _POSIX_C_SOURCE define. Pin the sanitizer job to Debug so every registered test target compiles and runs under ASan/UBSan. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
2 participants
What
Adds real build/test CI, since none currently runs on PRs (CircleCI reports no status; only Hound + Cursor Bugbot run), and fixes the cmake unit-test build so
ctestworks..github/workflows/ci.ymlbuild-and-test: cmake build +cteston gcc and clang.sanitizers: buildtd_testwith ASan + UBSan and run it (leak detection on;allocator_may_return_null=1so an oversized capacity request exercises the allocation-failure path instead of aborting).tests/CMakeLists.txt):minunit.hcallsclock_gettime(), which the top-level-std=c99hides unless_POSIX_C_SOURCEis defined on the command line before any header. Without it, buildingtd_testfails withunknown type name 'clockid_t', soctestnever runs. Fixed viatarget_compile_definitions(td_test PRIVATE _POSIX_C_SOURCE=200809L).Why
Complements #41 (the capacity-narrowing guard): with ASan/UBSan CI in place, integer-narrowing / overflow regressions like the one #41 fixes are caught automatically. Kept intentionally separate and small so #41 stays a focused change.
Verified locally
ctestpasses on gcc and clang (was previously failing to build).🤖 Generated with Claude Code
Note
Low Risk
CI and test-target compile-definition changes only; no runtime library or security logic is modified.
Overview
Introduces GitHub Actions CI on push to
masterand on pull requests, with read-only workflow permissions and checkout without persisted credentials.The
build-and-testjob configures a Release build with tests enabled, builds with gcc and clang, and runsctest. A separatesanitizersjob uses clang with a Debug build, ASan/UBSan compile/link flags, and runs the full test suite viactest(leak detection plusallocator_may_return_nullfor large allocation failure paths).tests/CMakeLists.txtadds_POSIX_C_SOURCE=200809Lon thetd_testtarget sominunit.hcan useclock_gettime()when the project’s-std=c99would otherwise break the unit test build.Reviewed by Cursor Bugbot for commit 8bf0dd7. Bugbot is set up for automated code reviews on this repo. Configure here.