diff --git a/.gitignore b/.gitignore index 80efc9e..ba77ef8 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,11 @@ build*/ .aider* *~ +# rebuild.sh captures its cmake output here. A build artifact, not a record -- +# it sat untracked in `git status` long enough to become invisible, which is the +# state a genuinely unexpected untracked file needs to stand out from. +rebuild.log + # Generated by configure_file into the build tree. include/ precedes the build # tree on the include path, so a stray copy here would silently shadow the real # one and pin every consumer to whatever version it was generated at. diff --git a/AGENTS.md b/AGENTS.md index 2d352b1..3bb40b3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -81,6 +81,33 @@ Run mutation testing with `cmake --build build --target mutation`. For a quick s Generate HTML and Cobertura coverage reports with `cmake -S . -B build-coverage -DAKGL_COVERAGE=ON -DCMAKE_BUILD_TYPE=Debug`, then build and run CTest. Reports are written to `build-coverage/coverage/`; this mode requires `gcovr` and GCC or Clang. +Both `gcovr` invocations take the build tree as an explicit positional search +path. Given none, gcovr searches `--root` -- the source directory, where +developers keep their build trees -- and `--object-directory` does not narrow +that. A second instrumented tree left in the source directory therefore used to +be folded into the report, and two trees describing different line numbers for +the same function failed `coverage_reset` before any test ran. Leave the +positional path alone. + +A different trap survives: rebuilding an existing coverage tree after editing a +source leaves `.gcda` files describing the old object layout, and +`coverage_reset` -- whose whole job is to delete them -- fails with `GCOV +returncode was 5` first. `find build-coverage -name '*.gcda' -delete` clears it. + +### What `ctest` runs + +Beyond the C suites, three registered tests are not C programs and are worth +knowing about when one of them fails: + +| Test | What it checks | Script | +|---|---|---| +| `api_surface` | Every exported `akgl_*` symbol is declared in a header | `scripts/check_api_surface.sh` | +| `error_protocol` | No `*_RETURN` inside `ATTEMPT`, no `return` out of `HANDLE` | `scripts/check_error_protocol.py` | +| `headers` | Every public header compiles as the first include of a translation unit | generated from `AKGL_PUBLIC_HEADERS` | + +`api_surface` needs `nm` and the built library; it exits 2 and is reported as +skipped when it cannot run, rather than passing quietly. + ## Continuous integration `.gitea/workflows/ci.yaml` runs four jobs on every push, and each one exists @@ -362,6 +389,19 @@ because the failure mode is silent. - Validate every pointer parameter before dereferencing it, including the ones a sibling function happens not to check. +The first two rules are enforced by `scripts/check_error_protocol.py`, running +as the `error_protocol` test: it flags a `*_RETURN` inside an `ATTEMPT` block +and a `return` out of a `HANDLE` block. Both have shipped before. Neither +produces a compiler diagnostic, and neither fails a test run until the pool it +drains is empty, which is why a linter is the only thing that catches them. + +It cannot see the third hazard -- a `CATCH` inside a loop nested in an `ATTEMPT` +block, where `break` binds to the loop. When a loop is the *entire* body of an +`ATTEMPT` that is harmless, because `CLEANUP`, `PROCESS` and `FINISH` still run; +anything after the loop has to account for it. `akgl_tilemap_load_layer_objects` +carries a comment saying exactly that, and `akgl_draw_background` uses a flag +instead for the same reason. + ## API Surface Every function with external linkage must be declared in a header, and every @@ -384,8 +424,46 @@ unit is the only shape that proves anything: a second `#include` after the first proves nothing about the second, because by then the first has dragged its dependencies in. +The declaration half is enforced too, by `scripts/check_api_surface.sh` running +as the `api_surface` test. It reads the built library's dynamic symbol table, +strips comments out of every public header, and fails on an exported `akgl_*` +symbol that is declared nowhere. **Stripping the comments is the point**: four +of the nineteen symbols this found were *mentioned* in `controller.h` prose, +which is not the same as being declared, and is how they stayed missing. + Declare no-argument functions as `(void)`, not `()`. +## Copying Into Fixed-Width Fields + +**Never `strncpy` or `memcpy` into one of this library's fixed-width fields. +Use `aksl_strncpy()` from `akstdlib.h`.** + +```c +PASS(errctx, aksl_strncpy(obj->name, sizeof(obj->name), name, sizeof(obj->name) - 1)); +``` + +`strncpy(dest, src, sizeof(dest))` leaves `dest` **unterminated** whenever the +source fills it, and `memcpy(dest, src, sizeof(dest))` reads past the end of any +source shorter than the field. Every name in this library is a registry key, +handed to `strcmp`, `realpath` and SDL property calls, none of which stop at the +field. Ten sites had the first problem and one had the second; that whole class +is the same one as the savegame overread and the `akgl_get_property` overread +before it. + +`aksl_strncpy` always terminates and never NUL-pads. Two details matter: + +- **Bound `n` at `sizeof(dest) - 1`.** It raises `AKERR_OUTOFBOUNDS` when the + bytes do not fit, and capping `n` is what preserves the "truncated, not + rejected" contract those headers document. Drop the cap only when you mean to + change that contract. +- **It does not pad.** Where the padding is load-bearing -- the savegame name + tables write a fixed-width field and the reader steps that many bytes -- + `strncpy` into a pre-zeroed buffer is still correct. `write_name_field()` in + `src/game.c` is the one place that applies, and it says so. + +`-Wstringop-truncation` catches the first form at `-O2`. Nothing catches the +`memcpy` form; that one was found by reading its neighbours. + ## Fixing Defects **Read what the code does before deciding what it should do.** Every rule below @@ -452,6 +530,27 @@ cheap to check and expensive to assume. Tests use simple executable return codes and are registered through CTest in `CMakeLists.txt`; there is no declared coverage threshold. +**Know who owns the error context an assertion consumes.** `TEST_EXPECT_OK`, +`TEST_EXPECT_STATUS` and `TEST_EXPECT_ANY_ERROR` all release whatever context +the statement returns. Some functions return *the context they were given* -- +`akgl_get_json_with_default` hands its argument straight back when it does not +handle the status -- so a `CLEANUP` block that also releases that context +releases it twice, and a double-released context corrupts the failure instead of +reporting it. A test written that way **passes against the broken code**, which +is exactly what happened to the first draft of the `AKERR_OUTOFBOUNDS` test in +`tests/json_helpers.c`. When the call may hand the context back, take the result +into a local, `NULL` your own pointer, and release it yourself: + +```c +defaulted = akgl_get_json_with_default(keyerr, &defval, &dest, sizeof(int)); +keyerr = NULL; /* ownership passed either way */ +if ( defaulted != NULL ) { + defaulted->handled = true; + defaulted = akerr_release_error(defaulted); + FAIL_BREAK(e, AKGL_ERR_BEHAVIOR, "..."); +} +``` + **Every suite's `main()` must call `TEST_TRAP_UNHANDLED_ERRORS()` immediately after `akgl_error_init()`.** libakerror's default unhandled-error handler ends in `exit(errctx->status)`, `exit` keeps only the low byte, and libakgl's status