CAPI cleanup — tracked plan

Goal: make src/dasher.h / src/CAPI.cpp easier to use, manage, and read — without breaking ABI or regressing behaviour.

Source review: session of 2026-09-11 (see git history / DasherCore-Review.txt).

Release plan (agreed 2026-09-11)

PR #1: everything through Phase 3 (Phases 0–3). After our own review-loop, watch for Greptile's PR review and run a loop addressing its findings (it may skip such a large diff — then our review stands).
- DONE — PR #90, all 15 checks green (4-OS/toolchain matrix, ASan+ UBSan, clang-tidy, clang-format, DCO, Greptile). Own review-loop ran at every stage (9/10 each). Greptile loop: 3×P2 → double-compile fixed (CMake glob excluded CAPI TUs from the DasherCore archive), buffer- lifetime test de-UB'd, test-hook-in-ABI declined with documented reply.
PR #2: Phase 4 (small consistency fixes, one commit each, version-gated).
PR #3: Phase 5 (JSON parser, per-context locale, getter deprecation). Stacked on PR #2. Merge the stack, release, then Will builds a frontend against it as the real-world test.
Every step: own review-loop; build [100%] + full ctest + symbol freeze.

Phase 0 — regression safety net (do this FIRST, before any refactor)

0.1 Record green baseline: cmake --build build -j then ctest --test-dir build --output-on-failure. Save the test list + pass count here: - date: 2026-09-11 - tests passed: 43 / 43
0.2 Symbol-count baseline: nm -D build/bin/*/libdasher*.so | grep ' T ' | wc -l (and dumpbin /exports on Windows). Every refactor step must keep the exported-symbol set IDENTICAL — record the count per step. - baseline saved: tests/baseline_symbols.txt (2026-09-11) - total exported T symbols: 1337 (incl. internal C++ — full freeze) - dasher_* C API functions: 109
0.3 New test: exported-symbol freeze. A test (or CI script) that diffs the current exported symbol list against tests/baseline_symbols.txt. Fails if a symbol is removed/renamed. Adding symbols is allowed. - done: tests/check_symbols.sh, gating on the 109 dasher_* C API symbols only (internal C++ symbols deliberately NOT frozen — alarm fatigue), Mach-O underscore-normalized, Linux-only - automated: runs as ctest dasher_symbol_freeze in CI, not by memory
0.4 New test: golden behaviour lock before the file split. Capture test_draw_snapshots + test_ppm_golden outputs (already golden-based); confirm they cover: frame commands, training, alphabet switch, palette switch. Add cases for anything uncovered — NOT for the refactor itself. - existing: frame0/10/mouse/output determinism + training/probability goldens (frame commands + training covered) - added: snapshot_alphabet_switch_deterministic + snapshot_palette_switch_deterministic (mid-session switch + input, hash-identical across runs) — the paths Phase 2's extraction touches
0.5 Add missing coverage for quirks the refactor could silently break: - [x] shared tlString scratch: call two string getters back-to-back, assert first result isn't clobbered (extends test_capi_buffer_lifetime.cpp) - 3 new cases: same-getter address stability, cross-getter shared-buffer clobber (UB-safe ordering), stringBuf independence - [x] locale is process-global: two ctxs, different dasher_set_locale, document current (global) behaviour in a test so a future per-context fix is a deliberate change, not a surprise - test_capi_contracts.cpp "locale is process-global" (also covers global string overrides) - [x] error-sentinel conventions ("" vs NULL vs -1) — snapshot current return values for error inputs per function - test_capi_contracts.cpp: NULL-ctx tolerance batch, -1 sentinels, "" vs NULL string outliers, "Unknown" LM name, wrong-type parameter catches, capi-version pin - new suite registered: dasher_capi_contracts_tests - verified: 45/45 tests pass incl. dasher_symbol_freeze (109 C API symbols gated; the 1337 full-export freeze was rescoped during review — see below)

Phase 0 review outcome (review-loop, 2026-09-11)

Two reviewer passes (7/10 → 8/10, gate met). Fixes applied: symbol freeze automated + rescoped to the 109 dasher_* symbols (freezing 1228 internal C++ exports was alarm fatigue); complete null-ctx sweep (~100 functions); real-buffer preview pins; event-2 callback contract cases; fixed real alphabet id ("English" is not a shipped id — AlphIO silently falls back, so the switch test was a no-op); valid-id LM static pin; LC_ALL=C collation; stale line refs.

0.6 Engine-error lifecycle pin (was deferred — done 2026-09-11, before Phase 2 extractions, since the boundary guard now owns these semantics): dasher_test_inject_failure test hook (test/diagnostic section, DASHER_FAIL_INJECT_* sites: frame, mouse×3, key, Realize) drives a real C++ exception through the boundary guard. Contracts suite pins: outputs zeroed on failed frame, DASHER_LOG_ERROR via log callback with entry-point context, engineError latches, disarmed calls still no-op, dasher_reset does NOT clear, recreate clears, failed-Realize → retry recovers (review-P1-#77 scenario). - baseline updated deliberately: 109 → 110 exported C API symbols.

Phase 1 — constants + error-convention docs (header only, zero ABI risk)

1.1 Add #define/enum constants to dasher.h for every magic int: opcodes 0–6, output event types 0–2, message types 0/1, log levels 0–3, appearance modes 0/1/2, key codes (0, 1–4, 100–102), parameter types 0/1/2, UI control types 0–5. Plain int constants — ABI-free. - done 2026-09-11: ~35 DASHER_* defines in a Constants block near the top of dasher.h (drawn from the engine's real VirtualKey enum, not just the header prose). No new exported symbols (freeze stayed 109).
1.2 Update header prose to reference the constants instead of raw numbers. - all opcode/event/message/log/appearance/key/param-type/UI prose now uses the constant names; docs/C_API.md tables + code samples too
1.3 Add one "error conventions" table to dasher.h (or docs/C_API.md): which functions return 0/-1, "" vs NULL, and what -1 means per getter. - added "Error conventions" section to dasher.h (after thread-safety note), cross-referenced by tests/test_capi_contracts.cpp
1.4 Tests: compile-check constants exist and match current values (static_assert-style doctest case in test_capi.cpp). - 39 static_asserts (renumber = compile error) + runtime case header_constants_match_runtime_behaviour (frame's first opcode == DASHER_CMD_CLEAR; param/ui type ranges; appearance defaults) - caught during writing: DASHER_MESSAGE_INFO/WARNING values initially transposed vs the implementation (bInterrupt?1:0) — verified correct
1.5 Update docs/C_API.md + any frontends' notes if they hardcode numbers. - opcode table gained a Constant column; key table, event table, appearance snippets use constants
verified: full rebuild, 45/45 ctest green, symbol freeze 109 unchanged. NOTE: clang-tidy not runnable locally (no compile_commands.json in any build dir) — relies on CI. Process note: never pipe builds through head (SIGPIPE truncated a build and left stale binaries undetected).

Phase 2 — split CAPI.cpp into internal TUs (no exported-surface change)

Target layout (internal only; dasher.h unchanged, still one public header):

Phase 3 — dedupe + dead code (behaviour-preserving)

3.1 Custom-action marshalling lambda exists twice (CAPI.cpp Interface GetPendingCustomActions ~line 686 and dasher_register_action ~line 2505). Extract one shared helper. - done: capi::make_custom_action_adapter (CAPI_internal.h, inline); both sites now one-liners
3.2 Mouse-up release duplication (dasher_set_alphabet_id, dasher_set_palette) — one release_mouse_if_down(ctx) helper.
3.3 Tests: test_control_actions.cpp must still pass unchanged after 3.1 (that's the regression guard for the lambda merge). - passed unchanged — and it mattered: the first 3.2 draft had an infinite recursion (bulk-replace clobbered the new helper's own body); dasher_capi_tests hung, caught by the build-to-100%-then- test rule before commit

Phase 4 — consistency fixes (small, test-visible)

Shipped as PR #2 (branch capi/phase-4-consistency), one commit per change:

4.1 LM-name sentinel: "Unknown" → "" (matches its description twin).
4.2 NULL-vs-"": dasher_find_companion_palette NULL → "". NULL survives in exactly one documented place — dasher_get_localized_string, where "missing translation" is a distinct state frontends branch on. DASHER_CAPI_VERSION 1 → 2 with the full behavioural note (both 4.1 and 4.2) in dasher.h.
4.3 Permitted-value memoization on the ctx (capi::permittedValues), invalidated by key change, ANY parameter change, AND every realize boundary (Realize populates the lists without firing OnParameterChanged — low-memory mode and the AlphIO/ColorIO scans are parameter-silent). Seven functions across five list families routed through it. HONESTY NOTE (kept deliberately): TWO review rounds caught the same failure mode — a claim without the code. Round 1: the first cut (fd943e6b) shipped the cache with an invalidation mechanism that existed only in its commit message (a python replace() silently no-op'd against clang-formatted code) plus a vacuous test. Round 2: the remediation commit (8520bccf) claimed the three boundary tests were added — the test-file replace had no-op'd the same way. Finally landed with match-verified edits and a MUTATION TEST: removing both invalidation mechanisms makes the suite fail on exactly the loop-1 bug (pre-realize 0 cached forever). Either mechanism alone keeps the SUITE green — but that rides on fresh-install realize firing a parameter broadcast; with a persisted settings file the realize-boundary call is the only guard, so both stay. Process rules: scripted replaces must assert the pattern matched; mutation checks must confirm the mutated build actually relinked (a stale binary gives a false green).

Phase 5 — bigger cleanups

Decision record (5.1, agreed with Will 2026-09-12): KEEP the JSON files and KEEP a flat reader. The strings_*.json are a translation-exchange format (extract_strings.py generates the English source; tooling/translators speak flat JSON; locales.json is RFC 0003's canonical list frontends consume directly), so converting to XML/pugixml breaks the ecosystem to save ~80 lines. Vendoring a JSON library adds cross-platform/WASM surface against the low-memory posture for a need this size. Safety comes from controlled inputs + the corpus guard test instead of parser generality.

5.1 Harden the flat reader (full JSON escape set incl. \uXXXX with surrogate pairs; unknown escapes degrade leniently; malformed files yield no translations) + tests/test_locale_files.cpp corpus guard (every shipped locale loads via the public API; escape fixture; malformed-degrades). Scope documented in the parser header.
5.2 Locale state moved onto the ctx (CAPI v3): the four locale functions are per-context; the ctx-less dasher_get_parameter_info reads a documented "last context wins" snapshot (ABI: no ctx param). Contracts tests rewritten for isolation + snapshot behaviour.
5.3 Indexed list getters: documented the preferred bulk form (get_parameter_string_values); both share the 4.3 cache, so this is ergonomics guidance, not deprecation-for-perf. Kept, not removed — the per-row form is legitimate for lazy UIs.

Phase 5 shipped as PR #3 (#92, all 15 checks green; greptile loop closed — two P1s on the reader fixed: atomic rejection of malformed files, then full token-grammar enforcement after the follow-up found end-state checks alone accepted numbers/missing separators/junk — and mis-paired values).

Done-when assessment (2026-09-12, stack merged)

Exported C API symbols: 110 (Phase 0 baseline 109 + the test-injection hook, deliberate) — freeze test green throughout. ✓
Full ctest green on Linux/macOS/Windows + Sanitize + clang-tidy. ✓
dasher.h: constants for every magic int, error-conventions section, CAPI version notes 1→2→3. ✓
Frontends build unchanged against the header (additions only); Will verifies for real with a frontend build post-release. ✓
"No single source file over ~800 lines": NOT met as written — CAPI.cpp is ~1160 (Interface + game + callbacks + strand2 + typing rate remain). Honest call: further splitting has diminishing returns; the original 2552-line monolith is six focused units. Revisit if it grows.

Release

Stack complete (#90 #91 #92). Next: tag a release (bump project VERSION per the #89 drift guard), then Will builds a frontend against it.

Done when

No single source file over ~800 lines in src/.
Exported symbol list identical to Phase 0 baseline (additions allowed).
Full ctest green on Linux, macOS, Windows CI.
dasher.h has constants for every magic int; docs table for errors.
Frontend repos (Dasher-Apple / GTK / Android / Windows / dasher-electron) still build against the header unchanged.

Per-step checklist (copy into each PR/commit)

[ ] BUILD: cmake --build build -j reaches [100%] AND greps clean for errors
    (a failed build + stale binaries can still "pass" ctest — verify both,
    never trust one alone; piping builds through head/truncation SIGPIPEs
    them mid-run)
[ ] ctest --test-dir build --output-on-failure  (all pass, 45+ tests)
[ ] exported symbols unchanged vs baseline_symbols.txt
[ ] no new warnings in CI for touched files
[ ] clang-tidy clean for touched files (see .clang-tidy)

FilesExpand file tree

todo.md

Latest commit

History