Files
libakgl/AGENTS.md
Andrew Kesterson 75da766724
Some checks failed
libakgl CI Build / cmake_build (push) Failing after 21s
libakgl CI Build / performance (push) Failing after 20s
libakgl CI Build / memory_check (push) Failing after 17s
libakgl CI Build / mutation_test (push) Failing after 19s
Record how to find out whether a tutorial teaches
docs_examples proves every listing in docs/ compiles and still matches the file
it was quoted from. It cannot prove a chapter teaches anything -- a document can
be made entirely of verified excerpts and still be unfollowable, because what a
reader needs is the glue between them.

So AGENTS.md gains "Testing a Tutorial": hand the chapter to a subagent on a
weaker model with the library, its headers and the art but not the finished
program, and make them build and run the game. The weaker model is the point; it
will not paper over a gap with knowledge the document did not give it.

The section carries the rules that make the result mean something -- copy the
tree with examples/ and docs/ removed rather than asking them not to look, let
them read the public headers because a real reader has them, do not make them
hand-author a .tmj, give them a screenshot helper, and verify their binary
yourself -- plus the sandbox recipe, and the warning to prove the build path
works before handing it over.

It also records how to read the report, because a weaker model states its own
mistakes as documentation defects with total confidence. Reproduce before
believing; and a rejected finding usually still leaves something behind.

The evidence for doing any of this is in the table at the end: four read-only
reviews of chapters 20 and 21 found real gaps and missed all three of the
defects the first build-and-run found, including a published example output the
chapter could not produce.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KzBDV2fqgnUAcqCKqKvc71
2026-08-02 09:45:28 -04:00

42 KiB
Raw Blame History

Repository Guidelines

Project Structure & Module Organization

This is a C library intended to support the development of video games. Public C headers live in include/akgl/; keep declarations there aligned with their implementations in src/. Tests are standalone C programs under tests/, with JSON, image, and map fixtures in tests/assets/. The util/ directory contains the charviewer utility and its sample assets. Third-party and companion libraries are vendored in deps/. Treat build/ and generated akgl.pc files as build outputs rather than hand-maintained source; include/akgl/SDL_GameControllerDB.h is generated too, but is tracked on purpose — see below.

Generated and Vendored Sources

include/akgl/SDL_GameControllerDB.h is generated, and is tracked deliberately

mkcontrollermappings.sh regenerates this header by fetching the community controller database from raw.githubusercontent.com. It is committed to the repository on purpose: it is the offline fallback that keeps the library buildable if upstream is renamed, rate-limited, taken down, or simply unreachable from the build machine. Do not delete it, do not add it to .gitignore, and do not "clean up" the fact that a generated file is tracked.

Working rules:

  • Never hand-edit it. It is machine-written; changes belong in mkcontrollermappings.sh.
  • An ordinary build does not touch it. Regeneration is cmake --build build --target controllerdb, and nothing else runs the script. Until 0.5.0 an add_custom_command re-ran it on every build, which made every build need network access and left the file modified with a fresh $(date) stamp and nothing of substance changed.
  • Update it as a deliberate, standalone commit when you actually want newer mappings, so the diff is reviewable and bisectable.
  • A failed fetch cannot reach this file. The script assembles the header in a temporary directory and moves it into place only after checking curl's exit status, a plausible minimum mapping count, and that no mapping contains a character that would break a C string literal. Any of those failing leaves the tracked copy untouched and exits non-zero.
  • Formatting tooling ignores it: scripts/reindent.sh and the pre-commit hook both skip it.

Fixed in 0.5.0. The safety net used to be self-defeating: mkcontrollermappings.sh had no set -e and never checked curl, so a failed fetch wrote AKGL_SDL_GAMECONTROLLER_DB_LEN 0 and an empty array over the good tracked copy, and exited 0 — and because CMake re-ran the generator on every build, an offline build silently destroyed the very fallback the file exists to provide. Both halves are closed: the script refuses rather than overwrites, and a build does not run it.

Build, Test, and Development Commands

Configure an out-of-tree development build:

cmake -S . -B build -DCMAKE_BUILD_TYPE=RelWithDebInfo

Build all library, utility, and test targets:

cmake --build build --parallel

Run the complete test suite with failure details:

ctest --test-dir build --output-on-failure

Run one test while iterating, for example ctest --test-dir build -R sprite --output-on-failure. The rebuild.sh script also installs into a developer-specific /home/andrew/local prefix and removes existing outputs; prefer the portable commands above unless that exact workflow is intended.

Run only the performance suites with ctest --test-dir build -L perf --output-on-failure, or leave them out of an ordinary run with -LE perf. They take about 30 seconds together, print a table of nanoseconds per operation, and fail only when a measurement exceeds a budget set at roughly ten times the recorded baseline. AKGL_BENCH_SCALE scales every iteration count — AKGL_BENCH_SCALE=0.1 ctest --test-dir build -L perf for a quick look — and below 1.0 the budgets are reported but not enforced. The recorded baseline and what it means are in PERFORMANCE.md.

Check for memory defects with cmake --build build --target memcheck, or scripts/memcheck.sh directly — it takes ctest's selection flags, so scripts/memcheck.sh -R tilemap and scripts/memcheck.sh -LE perf both work. It runs the suites that already exist under valgrind (ctest -T memcheck) with the headless drivers forced, and exits non-zero when valgrind finds a definite leak, an invalid access, or a read of uninitialised memory — which ctest -T memcheck on its own will not do. The whole run takes about thirty seconds because the perf suites scale themselves down under valgrind. Suppressions for third-party findings live in scripts/valgrind.supp; add one only when the finding genuinely cannot be fixed from this repository.

Run mutation testing with cmake --build build --target mutation. For a quick smoke run, use scripts/mutation_test.py --target src/tilemap.c --max-mutants 10; the harness mutates only a scratch copy and runs every suite.

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 because the others cannot do its work:

Job Build What it is for
cmake_build Debug + AKGL_COVERAGE=ON The unit suites and the coverage report. The perf suites run here too, at AKGL_BENCH_SCALE=0.02, for path coverage only — a timing taken under gcov is a measurement of gcov.
performance RelWithDebInfo ctest -L perf. The only job where budgets are enforced, because tests/benchutil.h enforces them only when compiled optimized at full scale. Keeps the tables as an artifact.
memory_check RelWithDebInfo scripts/memcheck.sh, every suite under valgrind. Gates: a definite leak, an invalid access or a branch on uninitialised memory fails the build. Keeps the valgrind logs as an artifact.
mutation_test Debug One focused source file, so CI stays bounded.

Every suite runs in every job. The character suite used to be excluded from the coverage and memory-check jobs on the grounds that it failed deliberately. It did not: it was reporting success while running one of its four tests, for two independent reasons, and both are fixed. See TODO.md, "Test suites that could not fail".

Coding Style

The canonical style is Emacs cc-mode stroustrup, with tabs enabled. This is not advisory — new and edited code must match what cc-mode produces, so that reindenting a region never shows up as a diff.

Emacs setup

.dir-locals.el in the repository root already applies the style to every c-mode buffer, so nothing needs configuring by hand:

((c-mode . ((c-file-style . "stroustrup")
            (indent-tabs-mode . t)
            (tab-width . 8)
            (fill-column . 100)
            (require-final-newline . t))))

Interactively: C-x h C-M-\ (mark-whole-buffer + indent-region) reindents the buffer. A correctly formatted file is a fixed point of that operation — reindenting must produce no change.

Reindenting from the command line

scripts/reindent.sh                 # reindent every tracked C source in place
scripts/reindent.sh src/tilemap.c   # reindent only the named files
scripts/reindent.sh --check         # list non-conforming files, change nothing

--check exits 1 when something needs reindenting and 2 if Emacs is missing, so it is usable from CI. The script drives Emacs in batch mode through scripts/reindent.el, which reindents, normalises leading whitespace to the canonical tab/space mix, strips trailing whitespace, and ensures a final newline. include/akgl/SDL_GameControllerDB.h is generated and always skipped.

Note that reindent.el deliberately does not use Emacs' tabify: that function's tabify-regexp is " [ \t]+", which is not anchored to the start of a line, so it rewrites runs of spaces anywhere and destroys the hand-aligned value columns in the bit-flag tables in actor.h and iterator.h. Only leading whitespace is ever rewritten.

The pre-commit hook

scripts/hooks/pre-commit reindents staged C sources before the commit is written. Enable it once per clone:

git config core.hooksPath scripts/hooks

It inspects the staged content rather than the working tree, so what lands in the commit is what was checked. When a file needs reindenting it is fixed in the working tree and re-staged — but only if the index and working tree agree for that file. If they differ, re-staging would sweep unstaged work into the commit, so the hook stops and tells you to reindent and stage it yourself. It steps aside during a merge, and warns rather than blocking if Emacs is unavailable. git commit --no-verify bypasses it.

For reference, cc-mode defines the style as:

("stroustrup"
 (c-basic-offset . 4)
 (c-comment-only-line-offset . 0)
 (c-offsets-alist . ((statement-block-intro . +)
                     (substatement-open      . 0)
                     (substatement-label     . 0)
                     (label                  . 0)
                     (statement-cont         . +))))

Indentation and whitespace

  • 4 columns per level (c-basic-offset 4).

  • Tabs are 8 columns wide and are used for indentation (indent-tabs-mode t, tab-width 8). Emacs emits the largest possible run of tabs and pads the remainder with spaces, which produces this ladder. Editors that expand tabs, or that assume a 4-column tab, will silently corrupt it:

    Depth Columns Bytes
    1 4 4 spaces
    2 8 TAB
    3 12 TAB + 4 spaces
    4 16 TAB TAB
    5 20 TAB TAB + 4 spaces
  • No trailing whitespace. Files end with a single newline.

  • case labels sit at the same column as their switch (label . 0).

  • Continuation lines of a wrapped expression indent one level (statement-cont . +).

The whole tree conforms: scripts/reindent.sh --check is clean, and the pre-commit hook keeps it that way. Convert a file to the canonical style in its own commit, not mixed into a behavioral change.

Braces

  • Function bodies open on their own line, in column 0.
  • Control statements keep the brace on the same line, one space before it.
  • else, else if, and while of a do/while stay on the closing-brace line: } else {, not a bare else on the next line. Note that this is a house convention rather than something cc-mode enforces — the stroustrup style governs indentation only and will not move a brace or an else for you.
  • Always brace, even single-statement bodies.
akerr_ErrorContext *akgl_actor_update(akgl_Actor *obj)
{
    if ( obj->curSpriteFrameId == 0 ) {
        obj->curSpriteReversing = false;
    } else if ( obj->parent != NULL ) {
        obj->curSpriteFrameId -= 1;
    } else {
        obj->curSpriteFrameId += 1;
    }
}

Spacing

  • Spaces inside control-flow parentheses: if ( x == y ) {, while ( done == false ) {, for ( i = 0; i < len; i++ ) {. This is the dominant existing convention (140 sites to 7) and cc-mode will not add or remove it.

  • No space between a function name and its argument list, at both call and definition sites, and no padding inside call parentheses: SDL_Log("x %d", n).

  • Binary operators and assignment are surrounded by single spaces; unary operators are not separated from their operand.

  • The pointer * binds to the identifier, not the type: char *name, akgl_Actor **dest. Never char* name.

  • When a call is too long for one line, put each argument on its own line indented one level past the callee, with the closing ) on its own line. This is the established shape for the FAIL_* and CATCH macros:

    FAIL_ZERO_RETURN(
        errctx,
        SDL_SetPointerProperty(AKGL_REGISTRY_ACTOR, name, (void *)obj),
        AKERR_KEY,
        "Unable to add actor to registry"
        );
    

Compiler warnings

-Wall is on for every target this project owns -- the library, the test suites and charviewer -- and the tree builds clean under it at every optimization level.

-Werror is not on by default. It is option(AKGL_WERROR ... OFF), turned on only by the cmake_build and performance CI jobs. Two reasons, and both are measured rather than assumed:

  • libakgl is consumed with add_subdirectory(); akbasic does exactly that. A target-level -Werror makes every diagnostic a newer compiler invents into a broken build for somebody else's project.
  • The warning set is not stable across build types. An -O2 build reports ten -Wstringop-truncation warnings that -O0 and -fsyntax-only do not, because they need the middle end. The two CI jobs that set AKGL_WERROR are deliberately one of each: coverage at -O0, performance at -O2.

If you are checking this by hand, compile for real at the optimization level being shipped. -fsyntax-only reported 6 of the 16 findings in src/.

-Wextra is deliberately not adopted. It adds 22 findings, 17 of which are inherent to the design rather than defects: 13 -Wunused-parameter, because a backend vtable entry or an SDL callback must match a signature whether it reads every argument or not, and 4 -Wimplicit-fallthrough from libakerror's own PROCESS/HANDLE/HANDLE_GROUP, which fall through between case labels by design. Adopting it today would mean disabling both permanently to gain 5 -Wsign-compare. The fallthrough half is filed upstream as deps/libakerror/TODO.md item 8; revisit when that lands.

Vendored code is exempt, not fixed. deps/semver/semver.c is listed directly in add_library(akgl ...), so the target's PRIVATE options reach it; set_source_files_properties(... COMPILE_OPTIONS "-w") keeps a future semver update from failing this build. The other vendored projects are separate targets and are unaffected either way.

Never silence a warning with a cast. Three -Wpointer-sign findings in src/sprite.c were real signedness mismatches -- uint32_t * passed where an int * was expected -- and a cast would have hidden every one. That is the whole argument of TODO.md item 37. Read into a correctly typed local, check the range, and assign.

No repository-wide formatter

There is no clang-format or linter wired into the build, and cc-mode's indentation engine is not exactly reproducible by clang-format. Do not introduce one without discussion, and do not reformat code you are not otherwise changing — unrelated whitespace churn makes review harder and is the reason the style drifted in the first place.

Naming Conventions

  • Public functions: akgl_<subsystem>_<verb>, all lower snake_case — akgl_actor_set_character, akgl_heap_next_string. No camelCase, and never embed a type name (akgl_Actor_cmhf_left_on is wrong; it should be akgl_actor_cmhf_left_on).

  • Public types: akgl_TypeNameakgl_Actor, akgl_SpriteSheet, akgl_PhysicsBackend. Every type exported from a header takes the prefix. point and RectanglePoints shipped bare until 0.5.0; they are akgl_Point and akgl_RectanglePoints now, and neither is precedent.

  • Constants and macros: AKGL_UPPER_SNAKE_CASE. A constant belongs to the subsystem it describes: a character limit is AKGL_CHARACTER_MAX_*, not AKGL_SPRITE_MAX_CHARACTER_*. Name a constant for what its value is — a nanoseconds-per-millisecond scale factor is AKGL_TIME_ONEMS_NS.

  • Exported globals take the akgl_ prefix like any other public symbol. Do not add bare names (renderer, camera, window), leading-underscore names (reserved at file scope), or SCREAMING_SNAKE names for mutable objects — AKGL_UPPER_SNAKE_CASE is for constants.

    This one is not cosmetic and there is a scar to prove it. Until 0.5.0 the library exported renderer, and tests/character.c defined a SDL_Renderer *renderer of its own. Both had external linkage and the same spelling, so the executable's definition preempted the shared library's: the library read a SDL_Renderer * through an akgl_RenderBackend *, every texture load in that suite failed, and the suite reported success anyway. A consuming game with a variable called renderer would have hit the same thing with no test to notice.

  • static helpers drop the akgl_ prefix, which exists only to avoid external collisions.

  • Include guards: _AKGL_<FILE>_H_, matching the file name. Guard names without the project prefix risk colliding with system headers.

  • Parameter names must match between the declaration and the definition — Doxygen publishes the header spelling. Conventional names: dest for an output parameter (not dst), self for a backend receiver, obj for the instance being initialized or inspected, e for an incoming error context to be inspected.

  • The local error context is named errctx (92 sites to 45). New code uses errctx; convert e when you touch a function for other reasons.

  • Header/source pairs are named by feature: include/akgl/sprite.h and src/sprite.c.

Error-Handling Protocol

The akerror control-flow macros have a shape that must be followed exactly, because the failure mode is silent.

  • Inside an ATTEMPT block use the _BREAK variants (FAIL_ZERO_BREAK, FAIL_NONZERO_BREAK, FAIL_BREAK) and CATCH. Outside it use the _RETURN variants.
  • Never use a *_RETURN macro inside an ATTEMPT block. It returns past the CLEANUP block, so every release, fclose, and free in CLEANUP is skipped. This has already caused a heap-string leak on the success path of akgl_get_json_tilemap_property.
  • CLEANUP must precede PROCESS. Transposing them moves the cleanup body into the PROCESS switch, where it runs only when an error context exists.
  • CATCH reports failure by breaking, which binds to the innermost enclosing loop or switch. A CATCH written directly inside a while exits the loop rather than the function — put the ATTEMPT block inside the loop.
  • Never return from inside a HANDLE block either. FINISH ends with RELEASE_ERROR, so leaving before it means the handled context is never given back to AKERR_ARRAY_ERROR — one leaked slot per call, and the 129th call aborts the process with "Unable to pull an error context from the array!". SUCCEED_RETURN is safe because it releases first; a bare return, or a return f(...) that tail-calls the fallback, is not. Set a flag in the HANDLE block and act on it after FINISH. This has already cost a process-killing leak in akgl_path_relative; see TODO.md, "Performance".
  • 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 declaration must have a definition. A non-static function that appears in no header is still in the ABI but is unreachable by callers; a declaration with no definition is a link error for anyone compiling against the header alone. If a function is exposed only so tests can reach it, declare it under the existing "part of the internal API" comment block in the relevant header.

Headers must be self-contained: include what you use, so that a translation unit including a single akgl header compiles without a particular include order. Within the project use the angled form, #include <akgl/sibling.h>.

This is enforced. AKGL_PUBLIC_HEADERS in CMakeLists.txt lists every header installed into include/akgl/, and drives two things from that one list: what install() ships, and a generated translation unit per header — each including exactly that header and nothing before it — linked into the headers suite. A new header is covered by adding it to that list. One header per translation 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.

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.

Reaching For libakstdlib

deps/libakstdlib wraps most of libc so a failure arrives as an akerr_ErrorContext * with context instead of a return value the caller has to remember to check. Use the wrapper wherever the underlying call can actually fail, which in practice means anything touching a file, a format string, or a fixed-width destination:

Instead of Use Because
fclose aksl_fclose The flush happens here, so a full disk or an exceeded quota is reported only here
fgetc aksl_fgetc fgetc(3) returns EOF for both end-of-file and a read error; this raises AKERR_EOF and AKERR_IO separately
snprintf aksl_snprintf Truncation becomes AKERR_OUTOFBOUNDS naming both lengths, instead of a silently short string
strncpy into a fixed field aksl_strncpy See Copying Into Fixed-Width Fields
fopen/fread/fwrite/realpath/atof/atoi the aksl_ form Already the convention in this tree

fclose(3) disassociates the stream whether or not it succeeds. Drop the FILE * before you look at the status, or CLEANUP closes it a second time:

	tmpfp = fp;
	fp = NULL;
	CATCH(errctx, aksl_fclose(tmpfp));

Written the other way round this aborts in glibc with double free detected in tcache, and only on the path where a close actually fails -- which is to say, never during development. /dev/full is how tests/game.c reaches it: writes to that device are accepted and the ENOSPC surfaces at the flush.

Do not convert the pure-arithmetic calls. memset, memcpy, strlen, strcmp and friends can only fail on a NULL argument, and the tree is inconsistent about them already; see TODO.md, "libakstdlib wrappers not yet adopted", before starting a sweep.

Never silence -Wformat-truncation. include/akgl/error.h still exports DISABLE_GCC_WARNING_FORMAT_TRUNCATION and nothing uses it. Both sites that did were genuinely truncating, and both reported the length problem as something else -- one as a missing texture file, with SDL naming a path the caller never wrote. aksl_snprintf is the answer.

Fixing Defects

Read what the code does before deciding what it should do. Every rule below is here because it was broken in this repository, and every one of them was cheap to check and expensive to assume.

  • Check the surrounding code before designing around a hazard. The savegame name-width fix first grew four AKGL_GAME_SAVE_*_NAME_WIDTH aliases, so the on-disk format's field widths could diverge from the object model's. They were deleted: akgl_game_load compares the save's libversion against AKGL_VERSION and refuses a mismatch in the same function, sixteen lines above the tables being edited, so the divergence they defended against cannot happen quietly. An abstraction defending an impossible case is worse than none, because the next reader has to work out what it is for.

  • Abstract when the second consumer is real, not before. A #define with one use, a wrapper with one caller, a parameter only ever passed one value: delete it and inline the thing. Those four aliases had exactly one use per side and each expanded to exactly one existing constant.

  • A test that has not failed has not been tested. Break the fix, watch the new test go red, put the fix back. Two tests in the 0.5.0 defect work passed against the unfixed library on the first attempt:

    • the savegame roundtrip asserted the load succeeded, and a reader stepping the wrong field width does not fail — it finds a run of zeros mid-entry, stops early, and reports success with silently wrong maps;
    • the akgl_get_json_with_default test used TEST_EXPECT_OK, which releases whatever context the statement returns — and that function hands back the context it was given when it does not handle the status, so the CLEANUP block released it a second time and the double release corrupted the failure instead of reporting it.

    Neither was discovered by reasoning. Both were discovered by reverting the fix and being surprised.

  • Pick the test case that distinguishes the two behaviours, not the obvious one. !AKGL_BITMASK_HAS(mask, bit) misparses to !(mask & bit) == bit without the parentheses — but for a bit that is set that is 0 == bit, false, which is the same answer the correct parse gives. It only diverges for a bit that is not set whose value is not 1. The obvious test passes either way.

  • Do not trust a comment, a TODO entry, or a CI exclusion that states a premise. Verify it. tests/character.c was excluded from two CI jobs and from the mutation harness because it "fails deliberately". It did not: it was exiting 0 while running one of its four tests. TODO.md carried eleven entries describing code that had already changed. A premise nobody has re-checked is where the next defect is hiding.

  • When a measurement moves, say what you measured. Absolute benchmark numbers drift with the machine. The akgl_game_update fix is recorded as the gap between two rows of the same run — 92 µs before, noise after — because a later run read every row 15% high, including rows nothing had touched. Do not re-baseline a whole table to record one change.

Rules

  • Add yourself (agent program name, model name and version) as a co-author on every commit message
  • Avoid dynamic memory allocation. Use the akgl_heap_* methods to retrieve necessary objects at runtime, and to release them when done. If the akgl heap facilities don't provide the kind of object needed, create a new heap layer to support that type of object.

Testing Guidelines

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:

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, "...");
}

Never exit(3) on an akerr status — call akerr_exit(). An exit status is one byte wide and libakgl's status band starts at 256, so exit(AKGL_ERR_SDL) is a wait status of 0 and a shell sees a clean run. Every suite in this directory once reported success on the most common failure a library built on SDL can have: tests/character.c aborted at its second of four tests on a bad renderer and was green until 0.5.0. akerr_exit() (libakerror 2.0.1) maps 0 to 0, 1255 to themselves, and anything else to AKERR_EXIT_STATUS_UNREPRESENTABLE (125). The default unhandled-error handler calls it, so an ordinary suite needs no trap of its own — tests/testutil.h carried one until 0.6.0 and it is gone. A suite that installs its own handler still has to call akerr_exit() from it; tests/actor.c does. Add focused tests as tests/<feature>.c, create a matching test_<feature> target, and register it with add_test. Put reusable fixtures in tests/assets/ and keep paths compatible with tests launched from the build tree. Coverage mode wraps the suite in a CTest fixture so counters are reset before tests and reports are generated afterward.

Physics simulations

tests/physics_sim.c is not a unit suite. tests/physics.c checks that akgl_physics_simulate does the arithmetic it says it does; this one runs the arcade backend for a second or two the way a game does and asks whether the result is what a player would expect. It carries a Mario-esque jump and fall, Zelda-style top-down walking, and a run reversed at full speed, because those are the three shapes most 2D games are made of.

It found three defects that every existing unit test passed straight over, so the distinction is worth keeping:

  • The first step was however long the level took to load. gravity_time was never initialized and dt was unbounded, so a 250 ms load produced a 250 ms step -- 100 px of fall where a 60 Hz frame is 0.44 px. Both initializers seed the clock now, and akgl_physics_simulate bounds dt to max_timestep (physics.max_timestep, default 0.05 s).
  • Releasing a vertical key cancelled gravity. The _off handlers zeroed ay, ey, ty and vy together, and ey is where the arcade backend accumulates gravity, so tapping down mid-jump stopped the character in the air. The handlers clear ax/tx (or ay/ty) and nothing else. Velocity was never theirs to clear either -- simulate recomputes v as e + t every step.
  • Diagonal movement was 41% too fast. Thrust was capped per axis, so an actor holding two directions got both caps at once and travelled their diagonal. It is capped as a vector against the sx/sy/sz ellipse now.

Two rules for working on this suite:

  • Drive the clock, do not sleep on it. sim_step() sets gravity_time to now - dt and calls simulate once, so a simulated second is 60 steps and takes no wall-clock time. Only test_sim_first_step_is_not_a_leap uses a real SDL_Delay, because a real stall is the thing it is measuring.
  • main must SDL_Init. Without it, SDL sets its clock epoch on first use, SDL_GetTicksNS() returns something around 130 ns, and every dt-dependent assertion passes for a reason that has nothing to do with the code. The first-step test passed exactly that way before the SDL_Init call was added, and only started failing -- correctly -- once it was there.

Each simulation prints what it measured whether it passes or fails, and main exits with the number that failed. Read the numbers when changing the backend; a change that keeps every assertion green while moving the apex of a jump by 30 px is a change worth noticing.

Performance suites

tests/perf.c (nothing that draws) and tests/perf_render.c (everything that does) are benchmarks, built and registered like any other suite but listed in AKGL_PERF_SUITES so they carry the perf label and a longer timeout. The harness is tests/benchutil.h. Three rules keep the numbers honest, and all three were learned by getting them wrong first:

  • Nothing that checks an error goes inside the clock. PASS and CATCH call akerr_valid_error_address, which walks AKERR_ARRAY_ERROR — more work than several of the calls being measured. Use BENCH_LOOP, which stashes the context and stops at the first failure, and hand it to PASS afterwards.
  • Flush the renderer before stopping the clock. SDL batches: a draw_* call queues a command and returns. BENCH_FLUSH_STOP in tests/perf_render.c is there because the first version of that suite measured queueing, reported a tilemap frame 250 times faster than it is, and paid the real cost at teardown inside SDL_DestroyTexture.
  • A drawing benchmark needs a raw-SDL control that does the same pixel work. Without one there is no way to separate what libakgl costs from what the rasterizer costs, and the answer is not the one you would guess — see PERFORMANCE.md.

Budgets are per-operation ceilings at roughly ten times the recorded baseline; they are enforced only in an optimized build at full scale. When a change moves a number for a good reason, re-record the baseline in PERFORMANCE.md in the same commit and say why it moved.

These two suites are also the memory-check vehicle. tests/benchutil.h detects valgrind from LD_PRELOAD and drops the scale to AKGL_BENCH_VALGRIND_SCALE, so the same binaries walk every path once instead of a hundred thousand times, and the timings a checked run prints are labelled as meaningless. Do not add memory-check suites: a new path worth checking belongs in a benchmark, where it gets both.

Testing a Tutorial

ctest -R docs_examples proves every listing in docs/ still compiles and still matches the file it was quoted from. It cannot prove the chapter teaches anything: a document can be composed entirely of verified excerpts and still be unfollowable, because what a reader needs is the glue between them.

So test a tutorial by making somebody follow it. Hand it to a subagent on a weaker model with no context beyond the chapter, and have them build the thing it describes. The weaker model is the point -- it will not paper over a gap with knowledge the document did not give it, which is exactly the failure mode a capable reviewer has.

The rules that make the result mean something

  • Give them what a real reader has, and nothing more. The chapter, the library source, its public headers, and the art. Not the finished program. Copy the tree with examples/ and docs/ removed rather than telling them not to look -- a rule they can break is not a control.
  • Headers are fair game. The manual defers signatures to Doxygen and a real user has include/akgl/*.h in front of them. Forbidding those tests a reader who does not exist.
  • They must compile it and run it. This is the whole thing. A design nobody built proves nothing, and the defects that matter most do not fail to compile.
  • Verify their work yourself. Run the binary they produced. Look at the screenshot they took. Do not take a report's word for what it built -- see "Reading the report" below.
  • Do not make them author assets by hand. A real reader draws a map in Tiled; hand-writing a .tmj tests nothing about the chapter and eats the whole session. Give them docs/tutorials/assets/. The asset formats are already covered by docs_examples, which runs the chapter's own JSON blocks through akgl_sprite_load_json and akgl_character_load_json.
  • Give them a screenshot helper, marked as scaffolding and not part of the tutorial, so there is a picture to check. Screen output is evidence; an exit status of 0 is not.

Setting the sandbox up

One directory per reader, because two of them building at once in the same tree collide:

rsync -a --exclude='.git' --exclude='build' --exclude='examples' --exclude='docs' \
      . "$SB/reader/libakgl/"
cp docs/20-tutorial-sidescroller.md "$SB/reader/TUTORIAL.md"
cp -r docs/tutorials/assets/sidescroller "$SB/reader/art"
mkdir -p "$SB/reader/game"

A consumer using the CMake the chapter itself teaches -- add_subdirectory(../libakgl libakgl) plus the documented target_link_libraries line -- configures and builds in about a minute from cold, and seconds after that. Tell them to use cmake --build build --target <theirs> --parallel, or they will also build every test suite in the tree.

Prove the path works before you hand it over. Write a throwaway consumer that opens a window and takes a screenshot, build it, run it, and delete it. A reader who cannot build is a reader who finds nothing, and the fault will be yours -- the screenshot helper's first draft included akgl/renderer.h for akgl_renderer, which is declared in akgl/game.h, and would have cost them the session.

Reading the report

Every finding is a hypothesis until you check it. A weaker model reports its own mistakes as documentation defects with total confidence, and both kinds are worth having -- but only one is worth acting on.

  • Reproduce the failure before believing it. One reader reported the dialogue freeze broken; it had drawn its own map with the NPC out of reach and never used the one it was given. Running the reference showed the player moving zero pixels through the whole freeze window.
  • A rejected finding usually still leaves something. That same map failure was not a code defect, but it did show the chapter stated TALK_RANGE without saying what it means when you are placing NPCs -- and that a too-distant NPC produces success, not an error.
  • Fix the document, not the reader. If they guessed a header name, the chapter never named it.

What this catches that review does not

Four read-only passes over the two tutorials in docs/ found real gaps and missed all three of these, which the first build-and-run found immediately:

Defect Why reading missed it
libakstdlib's header was never named -- aksl_* functions, <akstdlib.h> file Every reviewer already knew, or did not have to write the #include
The asset directory defaulted to "." instead of the compiled-in macro Prose said "falls back", and nobody had to run it from another directory
A published example output the chapter could not produce The capture happened after teardown released the pool. Nothing fails to compile

The third is the shape to remember: a tutorial that prints an expected result is making a claim, and a claim nobody executed is a claim that is probably wrong.

Commit & Pull Request Guidelines

Recent commits use concise, imperative summaries such as Fix a scale bug... and often explain related changes in one sentence. Keep each commit scoped and describe user-visible behavior. Pull requests should summarize the change, identify affected modules, list the CMake/CTest commands run, and link relevant issues. Include screenshots only for rendering, map, or charviewer changes.