andrew/libakgl
1
0
Fork 0
Code Issues Pull Requests 1 Actions 1 Packages Projects Releases Wiki Activity

Document the checks, the copy rule and the trap that made a test lie

Browse Source 
Four gaps in the maintenance guide, each one something a maintainer will hit.

The two enforced checks were never named in it. api_surface
(scripts/check_api_surface.sh) and error_protocol
(scripts/check_error_protocol.py) fail builds and appear in ctest output, so the
guide now says what each one asserts and why -- including that the API check
strips comments before looking, because four of the nineteen symbols it found
were mentioned in header prose and that is not the same as being declared.

A new "Copying Into Fixed-Width Fields" section: never strncpy or memcpy into
one of this library's fixed-width fields, use aksl_strncpy, bound n at
sizeof(dest) - 1 to keep the truncate-not-reject contract, and know the one
place the NUL padding is load-bearing. Ten sites had the unterminated form and
one had the overreading memcpy form, and every name in this library is a
registry key.

The testing section gains the error-context ownership trap. TEST_EXPECT_* release
whatever the statement returns, and some functions return the context they were
given -- so a CLEANUP that also releases it double-releases, and a
double-released context corrupts the failure rather than reporting it. A test
written that way passes against broken code. That is not hypothetical; it is
what the first AKERR_OUTOFBOUNDS test did.

Also recorded: what ctest actually runs beyond the C suites, the gcovr
positional search path and why it must stay, and the surviving GCOV returncode 5
trap. Plus rebuild.log to .gitignore -- it had been sitting in git status long
enough to stop being noticed, which is the state a genuinely unexpected file
needs to stand out from.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Andrew Kesterson
2026-08-01 11:19:00 -04:00
parent 4d80664cd9
commit d55778e625
2 changed files with 104 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.gitignore vendored
View File

@@ -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.

99
AGENTS.md
View File

@@ -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