libakgl/AGENTS.md

# 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:

```sh
cmake -S . -B build -DCMAKE_BUILD_TYPE=RelWithDebInfo
```

Build all library, utility, and test targets:

```sh
cmake --build build --parallel
```

Run the complete test suite with failure details:

```sh
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:

```elisp
((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

```sh
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:

```sh
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:

```elisp
("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.**

```c
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:

  ```c
  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 issue #5. 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_TypeName` — `akgl_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 `break`ing, 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`.**

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

## 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:

```c
	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 issue #19, which is the decision this
waits on, 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:

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

**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, 1–255 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:

```sh
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.