Codebase Observations

Codebase Observations — Non-Obvious Facts

Audience: Programmers | Mixed

Thread Context: N/A (audit/reference)

Platform/Backend Scope: All platforms / all backends

Overview

Non-obvious facts about SparkEngine's codebase structure, conventions, and tooling that are easy to get wrong. These are things not immediately apparent from reading CLAUDE.md or the rest of the wiki, or things that contradict common assumptions. Check these before making assumptions about how the engine builds, where subsystems live, or which files are safe to edit by hand.

---

Most Optional Systems Are OFF by Default — but Networking Is Now ON

Several major engine systems are gated behind CMake toggles. A build can succeed even if code in a disabled system is broken, because that code is never compiled.

Current default toggle state (verify with cmake --preset linux-gcc-release -N):

ENABLE_NETWORKING=ON    ← now ON by default (was OFF at the time of the original observation)
ENABLE_DXR=ON           ← on by default (SDFGI fallback when no D3D12/DXR hardware)
ENABLE_VR=OFF           ← VR is implemented and wired in, but disabled by default
ENABLE_METAL=OFF        ← experimental, off by default

The original observation flagged networking as the most common trap because it was off by default. That has since changed — ENABLE_NETWORKING is ON by default today, so networking code now compiles in standard CI builds. The general lesson still holds: if code in a disabled system silently breaks, it won't show up in CI; and if a subsystem "doesn't exist," check the CMake toggle first.

---

Use EngineContext, Not Global Variables

Legacy free-floating globals (g_graphics, g_input, g_timer, g_eventBus, g_audioEngine, g_audioBackend, g_moduleManager, g_moduleHotReload, g_physicsOwned) have been removed. Engine-lifetime ownership now lives in an EngineRuntime struct (SparkEngine/Source/Core/EngineRuntime.h), accessed via GetEngineRuntime(), intended for Core/ entry-point and lifecycle files only.

All other code accesses subsystems through the EngineContext service locator:

// WRONG — removed globals (no longer declared anywhere):
g_graphics->RenderFrame();

// CORRECT — EngineContext service locator:
auto* graphics = EngineContext::Get()->GetGraphics();
auto* input    = EngineContext::Get()->GetInput();

• EngineContext lives in SparkEngine/Source/Core/EngineContext.h.
• EngineRuntime (private ownership container) lives in SparkEngine/Source/Core/EngineRuntime.h.
• Do not introduce new file-scope g_* subsystem globals.

---

.claude/ Is Intentionally Visible to Claude

.claude/ is NOT excluded from .promptignore — Claude reads these files at session start to load persistent context. Do not add .claude/ to .promptignore.

.promptignore excludes ThirdParty/, Shaders/Compiled/, build/, IDE config dirs, .git/, binary files, and generated docs/html/ + docs/xml/. Everything else (including .claude/) is visible.

---

docs/api/ Files Are Entirely Auto-Generated

Every file under docs/api/ is generated by docs/generate-api-docs.sh. Never edit them by hand — changes are overwritten on the next run. To change API documentation, edit the Doxygen comments (@brief, @param, @return) in the header files under SparkEngine/Source/; the generation script extracts from there.

The script uses checksum-based caching (docs/generate-api-docs.sh check) and only regenerates pages whose headers changed. Note: docs/api/ is a generated artifact and may be absent from a fresh checkout until the script runs.

The same hands-off rule applies to <!-- AUTO:* --> sections in wiki files — they are written by docs/sync-wiki.sh sync.

---

Wiki Files Mix Auto and Manual Content

Wiki pages are mostly hand-written, but specific sections are auto-generated and fenced by markers:

<!-- AUTO:component-list -->
... generated content, do not edit ...
<!-- /AUTO:component-list -->

• Outside <!-- AUTO:* --> blocks: edit freely.
• Inside them: do not edit — docs/sync-wiki.sh sync will overwrite.
• During rebase conflicts in wiki files, take upstream for AUTO: sections.
• To add a new page: create the file in wiki/, then add it to wiki/_Sidebar.md.

---

Two tools/ Directories with Different Purposes

Directory	Contents	Purpose
tools/ (lowercase)	validate-prompts.sh, check-*.sh, asset generators, wine-run.sh	Validation and dev scripts
Tools/ (uppercase)	spark-cli/spark_cli.py	Developer CLI utility

CI references tools/validate-prompts.sh --ci — always lowercase. On case-insensitive filesystems (macOS) these may appear merged; they are distinct on Linux/CI.

---

CLAUDE.md Pre-Commit Step 1 Is a Partial Check

The pre-commit format step uses head -50, checking only the first 50 files by modification time. CI's check-format checks all files (including .hpp). It is possible to pass the local shortcut and fail CI. Run the full CI-matching command (no head -50) before pushing.

---

ThirdParty Is Git Submodules Plus Vendored Snapshots

ThirdParty/ mixes git submodules and vendored single-header/snapshot code. After cloning, initialize submodules before CMake configure:

git submodule update --init --recursive

If a ThirdParty/ directory looks empty or CMake reports missing files there, an uninitialized submodule is almost always the cause. There is now a single source of truth at ThirdParty/dependencies.lock plus a configure-time audit (cmake/SparkThirdPartyAudit.cmake) — see the ThirdParty Dependencies Audit page. ThirdParty/ is in .promptignore, so Claude does not see its contents (correct — it is not project code).

---

Architectural Defects Found by the 2026-07-18 Sweep (now fixed)

A multi-agent defect sweep (2026-07-18) confirmed and fixed 41 local defects across 31 files, plus three architectural ones that were fixed in follow-up commits the same day:

1. Per-client reliable-message state was server-wide (fixed). Receive-side dedup/ACK/ordered state lived per-NetworkManager, not per sender, so with ≥2 clients, client B's sequence N was dropped as a duplicate of client A's and the merged broadcast ACK silently cancelled B's retransmit. Now all reliability state (dedup windows, reorder buffers, outgoing sequences, unacked/retransmit maps) lives in a per-peer PeerState keyed by ClientID, with per-peer unicast ACKs. Regression tests: Tests/TestReliableChannel.cpp (overlapping sequence spaces, ordered independence, ACK isolation).
2. Core ECS phase systems were never registered in production (fixed). CreatePhaseSystemManager had no non-test callers. The gameplay lifecycle (GameplayLifecycleShared.cpp) now creates the manager during init and pumps UpdateAll each frame in the documented phase order; the dead StageBasedExecutor was deleted. Regression test: Tests/harden/Test_lifecycle_ecs_phase_wiring.cpp.
3. Delta snapshot ACKs were never wired (fixed). Deltas now have their own ack echo — MessageType::DeltaAck carries the delta sequence (a different sequence space than transport ACKs, which would falsely advance baselines); clients echo it for applied Unreliable entity-state updates and the server routes the trusted sender to DeltaSnapshotManager::AcknowledgeSequence (cumulative, stale acks ignored). pendingDeltas is bounded at 256/connection. Regression tests: Tests/TestNetworkReplicationIntegration.cpp.

Bloat-split campaign (same day)

Six split waves reduced the over-threshold file count from 104 to 1 (103 files split into coherent sibling TUs plus small internal headers, all moves byte-identical and build+test verified). The final wave split the 18 Windows-only files (*Windows.cpp, ProcessWin32.cpp) using the MinGW cross-build (linux-mingw-release preset) for compile verification — each split TU was checked with x86_64-w64-mingw32-g++ -fsyntax-only against the real build flags, and the full cross-build stays green. The only remaining violation is GameModules/SparkGameMMOFPS/Source/Core/TFTypes.h (301 lines, a FROZEN CONTRACT header deliberately left alone). Gotchas discovered: (1) Tests/CMakeLists.txt links some module .cpp files by explicit path (e.g. TFOutfitStore.cpp), so splitting one of those requires adding the new sibling TU there too; (2) exe-only entry files (SparkEngineWindows.cpp) are listed in SPARK_ENGINE_ENTRY_POINTS, not globbed into the lib — their split parts must be added to that list; (3) in a network-restricted container, stage DirectXMath headers into build/.dxmath-cache/extract/DirectXMath-oct2024/Inc/ (plus a placeholder oct2024.zip) — the NuGet directxmath package is a working source when GitHub is blocked.

---

Generated Docs Are Pinned to the Commit's UTC Day

docs/update-all-docs.sh check (run by the docs-health and Validate CI tooling jobs) regenerates every declared output into two isolated roots with SPARKENGINE_DOC_SOURCE_COMMITTED_AT set to the commit's committer timestamp, then compares bytes against the tracked files. wiki/advanced/Codebase-Statistics.md embeds that date as Updated YYYY-MM-DD, so a commit whose committer date lands on a new UTC day is stale by construction even when no source changed (this is what turned Working red at 9f9215a, a README-only change committed after 00:00 UTC). Run bash docs/update-all-docs.sh update and commit the regenerated statistics page on the same UTC day as the commit; the local update-codebase-stats.sh check passes regardless because it reuses the date already in the page. The check also refuses a dirty worktree, so run it after committing and amend if needed.

---

The Installed Spark::SparkEngineInterface Target Needs Dependency Include Dirs

On Linux/macOS, spark_add_game_module() links installed modules against Spark::SparkEngineInterface (no engine object code) rather than Spark::SparkEngineLib. Public engine headers still include <SDL.h> and <AL/al.h> whenever SPARK_SDL2_AVAILABLE / SPARK_OPENAL_AVAILABLE are defined, and the in-tree target inherits those directories through a $<BUILD_INTERFACE:$<TARGET_PROPERTY:SparkEngineLib,INTERFACE_INCLUDE_DIRECTORIES>> expression that is dropped from the export. cmake/SparkEngineConfig.cmake.in therefore re-attaches $<TARGET_PROPERTY:Spark::SparkEngineLib,INTERFACE_INCLUDE_DIRECTORIES> (transitive, so it carries SDL2::SDL2 and OpenAL::OpenAL) and find_dependency(OpenAL) when the exporting build linked OpenAL::OpenAL; without both, Publish Builds failed on Linux (SDL.h: No such file while building the Blank3D template) and on macOS (OpenAL::OpenAL missing from the link interface). Anything that ends up in SparkEngineLib's PUBLIC link interface must be re-imported by the config file.

---

AngelScript Bytecode Is 32-bit Packed; 64-bit Arguments Need Unaligned Accessors

AngelScript stores instruction arguments in an asDWORD stream, so 64-bit (asQWORD/pointer) arguments live at 4-byte alignment. UBSan's alignment check (-fsanitize=undefined, on in build-linux-asan) reports every *(asQWORD*)(bc+1) access as UB. The pinned submodule stays byte-identical; ThirdParty/Scripting/patches/angelscript-packed-bytecode.patch is applied to a build-tree copy at configure time and now defines packed may_alias accessor structs for both asBC_PTRARG and asBC_QWORDARG, and routes the compiler output (as_bytecode.cpp) and bytecode save/load (as_restore.cpp) through them or memcpy. Extend the patch (regenerate it with git diff from a scratch checkout and verify with git apply --check --whitespace=error-all) rather than adding -fno-sanitize=alignment; the manifest sync check requires touching ThirdParty/dependencies.lock in the same change.

Two more sanitizer-lane rules learned from the same CI run: (1) the run used to abort at the first UBSan report, so LeakSanitizer never ran; once the suite completes, LSan reports seven 128-byte realloc blocks inside the Vulkan loader / Mesa Lavapipe ICD below vkEnumeratePhysicalDevices (one per TestVulkanLavapipe case, no engine frame allocates, and VulkanDevice::Shutdown() does destroy the instance), which Tests/lsan_suppressions.txt now suppresses; (2) .github/scripts/verify-sanitizer-evidence.py classifies any private runtime log without an ERROR:/SUMMARY: signature as a verification failure, and LSan writes a "Suppressions used" summary into that log whenever a suppression matches, so the ASan lane runs with LSAN_OPTIONS=...:print_suppressions=0.

---

CMake 3.28 Needs LINK_LIBRARY_OVERRIDE for the Whole-Archive Hosts

SparkServerCore links $<LINK_LIBRARY:WHOLE_ARCHIVE,SparkEngineLib> while SparkGatewayCore links SparkEngineLib plainly. CMake 3.31+ (what the GitHub runners ship) resolves that mix in favour of WHOLE_ARCHIVE, but CMake 3.28.3 (the Ubuntu 24.04 apt package, the documented minimum is 3.25) refuses to generate with "Impossible to link target ... has already occurred with the feature 'WHOLE_ARCHIVE'". SparkServerCore, SparkServer, and SparkTests therefore set LINK_LIBRARY_OVERRIDE "WHOLE_ARCHIVE,SparkEngineLib" on POSIX, which reproduces the newer resolution explicitly.

---

Installed Linux Binaries Carry No RPATH (Known Gap)

cmake --install on Linux copies the bundled libSDL2-2.0.so.0 into lib/ but strips the build RUNPATH, and CMAKE_INSTALL_RPATH is only set for Apple. The staged bin/SparkEngine therefore starts only where a system libSDL2-2.0.so.0 happens to exist (true on the CI runners because libsdl2-dev is installed, which is why the release lane's live smoke passes). The fix is set(CMAKE_INSTALL_RPATH "$ORIGIN;$ORIGIN/../lib") for UNIX AND NOT APPLE before any target is created, but note that Tools/buildmatrix/inventory.py pins its reviewed dependency bindings (_REVIEWED_REQUIRED_TARGET_REFERENCE_CONTRACTS) to exact CMakeLists.txt line numbers, so inserting lines above ~1620 also requires re-reviewing those records; append-only edits near the end of the file are safe. Locally, run the installed engine with LD_LIBRARY_PATH=<stage>/lib until that lands.

---

The Consolidated CI Error Report Runs From Working and Separates Advisory Lanes

report-ci-errors (the CI Error Report job) checks out Working before loading .github/scripts/report-ci-errors.js because the job holds pull-requests: write; a change to the reporter or to test-report-ci-errors.js is therefore inert on a feature branch and only takes effect once merged. On push events the job fails whenever a downloaded ci-errors-* summary carries errors. Until 2026-09-02 that included lanes the workflow tolerates: build-linux-msan is continue-on-error and reports success through needs even when its summary lists MSan reports (the system libc++ is not MSan-instrumented, so those reports are expected), and clang-tidy extracts its summary under always(). The run therefore stayed red even when every required lane was green. The reporter now classifies a summary as advisory when the needs result of its job is success or skipped, prints it under an Advisory lanes line with a :warning: CI Advisory Report header, and fails the job only for required-job failures, summaries from jobs needs does not cover, or a malformed needs payload. Branch protection targets the separate Required CI Gate job, which lists the required lanes explicitly and carries no deferred failures: build-windows-shipping records its build-matrix evidence and succeeds, and only the protected build-matrix-verifier.yml workflow attests that evidence. (2026-09-06: build-linux-msan now links an MSan-instrumented libc++ built in-job; the 'expected reports' rationale no longer applies, the lane stays advisory until a run classifies clean.)

---

Publish Worker Stop Flags Under the Waiter's Mutex

build-linux-asan on 2026-09-02 timed out (exit 124 after 900 s) inside AdvancedAssetPipeline_UnsupportedTransformAndImporterFailClosed, a test that normally takes 3 ms, while the identical code passed on the neighbouring commits. The worker loop waits with a predicate (m_shouldStopProcessing || !queue.empty()) but Shutdown() set the atomic flag and called notify_all() without holding m_queueMutex. A worker that has evaluated the predicate as false but not yet blocked misses that notification, and join() then waits forever; slow sanitizer thread start-up makes the window easy to hit. An atomic flag does not help here: the standard requires the shared state to be modified under the mutex the waiter holds for the notification to be visible to it. The fix is { lock_guard lock(m_queueMutex); flag = true; } cv.notify_all();, applied to AdvancedAssetPipeline, the Linux and Windows AssetPipeline and TextureSystem streaming threads, and LevelStreamingSystem. AsyncDatabase::Close() already did this correctly. When adding a worker thread, store the stop flag inside the queue lock; a bare flag.store(true); cv.notify_all(); is a latent hang even when the wait uses a predicate.

---

HLSL Shaders Are Staged by One Target, Not by Every Consumer

SparkEngine and each of the ten in-tree game modules write to the same bin/<config>/Shaders directory. Until 2026-09-03 each target copied the full SparkEngine/Shaders/HLSL/*.hlsl set from its own POST_BUILD step, so MSBuild building modules in parallel raced on the destination and the Windows Shipping producer failed with Error copying file (if different) ... SSAO.hlsl on an otherwise clean commit. The copies now live in one SparkRuntimeShaders custom target (top-level CMakeLists.txt, section 9.4) that also stages the top-level Shaders/HLSL/ tree; SparkEngine and every module add_dependencies() on it, and a module falls back to its own copy loop only when it is built standalone against an installed SDK (SPARK_GAME_<X>_STANDALONE), where no other writer exists. Add new shader staging to that target rather than to a consumer's POST_BUILD step.

---

Static-Analysis Findings: What Was Real and What the Scanners Misread

The 2026-09-03 sweep of the code-scanning alerts (CodeQL, Codacy/cppcheck, MSVC /analyze) settled a few conventions. In tests, a pointer or container obtained from the system under test is checked with ASSERT_TRUE (fatal, throws TestAbort) before it is dereferenced; EXPECT_TRUE(p != nullptr) followed by p-> is the pattern cppcheck reports as nullPointerRedundantCheck, and it also hides the real failure behind a crash. Test helper types that own a raw allocation or FILE* delete their copy operations. Engine-side, the sweep removed always-true helper stubs and the dead branches that consumed them (MaterialLoader::RegisterMaterial, GizmoSystem::CreateGizmo*, the no-op mesh and texture processing steps, EngineInterface::ConnectToNamedPipe) and fixed one real defect: BenchmarkFrameworkBaseline could never match the }, line that closes the metrics object, so only the last baseline in a file was ever loaded. Findings that only exist in a configuration the build never uses (the no-ENABLE_NETWORKING module guards, the non-Linux/macOS peer-credential fallbacks, _WIN32-only code) were left alone. Two scanner artefacts are filtered before upload rather than fixed in code: cppcheck parses plain .h headers as C and reports syntaxError at the first namespace, and its y2038 addon warns about a 32-bit time_t that no supported target has; .github/scripts/normalize-codacy-sarif.py drops both with audit reasons. The verify_external_evidence.py locals were renamed from trusted_* to reconstructed_* because CodeQL's sensitive-data heuristic treats the word trusted as a secret marker.

The MSVC /analyze pass (NativeRecommendedRules, .github/workflows/msvc.yml) added its own conventions. 309 of its 490 findings sat in vendored code: the workflow now skips targets defined under ThirdParty/ and build/ThirdParty/ (Jolt, AngelScript), treats their headers as external, and normalize-msvc-sarif.py drops any finding whose primary location has a ThirdParty or _deps path segment, because vendored targets defined at the repository root (ImGui, Recast/Detour, tinyexr, miniz, glad) cannot be excluded by target path. C6262 reported 135 frames above its 16 KB default. The analyzer sums every local in a function, and SPARK_LOG used to expand a 4 KB char buffer at each call site, so any function with eight log calls tripped it whatever its real locals were; the macro now formats out of line in Logger::LogFormatted (printf-format attribute on GCC/Clang, _Printf_format_string_ on MSVC), the frames that really were 32 KB or more (64 KB file-hash and archive read buffers, 32K-wchar_t path buffers) moved to std::vector, and the workflow passes /analyze:stacksize 32768 so ordinary fixed-size buffers below that are not reported. Real defects the pass found: the D3D12 indirect draw and dispatch entry points handed ExecuteIndirect a null ID3D12CommandSignature (the command list now creates draw, draw-indexed and dispatch signatures), the FSR RCAS pass copied the upscaled output from a literal null source and sharpened stale intermediate contents, Serializer.h instantiated ByteSwap for single-byte types with a zero-trip unsigned count-down loop, Downloader, CollaborativeEditSession and EditorTheme ignored CoInitializeEx, WSAStartup and sscanf results, MemoryMonitor formatted an uninitialised status buffer, and DecorLayoutEditorPanel indexed a four-entry std::array with an unchecked tier. The lock findings (C26110, C26115, C26117) were analyzer limits rather than races, but each marked a lifetime that was hard to read: two sibling lock_guards named lock in one function, a lock_guard declared inside a case block, worker loops that returned from inside the queue lock, and UpdateReplication releasing a unique_lock it received by reference. They became plain scoped blocks, then one private helper per lock whose first statement constructs the guard (JobSystem::WaitForJob, AsyncDatabasePool::WaitForWorkItem, SimpleConsole::IsSensitiveCommand and RecordCommandHistory, AssetDatabase::FindMissingAssetPath and RenameAssetEntry), and UpdateReplication owns its API lock. Two findings remain on purpose and are dismissed in the code-scanning UI rather than worked again: ConsoleProcessManager's debug-only crash_test dereferences null to exercise the crash handler, and PREfast still reports C26115 at JobSystem::WorkerThread's return because it inlines the header-defined WaitForJob and loses the unique_lock destructor on the drained path; the byte-for-byte equivalent out-of-line AsyncDatabasePool::WaitForWorkItem is clean, so this is an analyzer limit, not a lock leak. Do not restructure WorkerThread for it.

---

Source & Freshness

• Original observation: .claude/knowledge/codebase-observations.md, last updated 2026-03-19.
• Re-measured against codebase 2026-06-08.
• Extended 2026-09-03 with the sanitizer-lane, installed-SDK, CMake 3.28, CI error report, worker stop-flag, and static-analysis (cppcheck, CodeQL, MSVC /analyze) notes from the stable-release CI audit.
• Extended 2026-09-02 with the docs-currentness, installed-SDK include, AngelScript alignment, and CMake 3.28 link-override notes from the stable-release audit.
• Changes since the original:
  • ENABLE_NETWORKING default OLD OFF → NEW ON (networking now compiles in standard CI). Updated the most-common-trap framing accordingly.
  • Removed-globals list, EngineContext/EngineRuntime split, .promptignore scope, two tools/ dirs, and the head -50 partial-check note all re-verified as still accurate.
  • ThirdParty note expanded: now backed by ThirdParty/dependencies.lock + cmake/SparkThirdPartyAudit.cmake (both confirmed present).
  • Noted docs/api/ may be empty on a fresh checkout (0 generated pages present in the working tree at re-measure time).

Related Pages

• Build System and CMake Modules
• ThirdParty Dependencies Audit
• Codebase Statistics
• Codebase Health