Skip to content

feat: diagnostics + drift-manifest modules, bulk hook API, and dev tooling#101

Merged
tkhquang merged 5 commits into
mainfrom
feat/audit-followups
Jun 10, 2026
Merged

feat: diagnostics + drift-manifest modules, bulk hook API, and dev tooling#101
tkhquang merged 5 commits into
mainfrom
feat/audit-followups

Conversation

@tkhquang

@tkhquang tkhquang commented Jun 10, 2026

Copy link
Copy Markdown
Owner

Summary

Follow-up batch from the independent audit: two new modules, a hook-manager convenience API, and developer tooling, on top of the prior hardening PR. Full MinGW suite passes (1389/1389).

Changes

  • diagnostics (new module): consumer-queryable counters for intentional loader-lock leak/detach events, wired into the teardown paths of logger, async_logger, config_watcher, input, memory, worker, bootstrap, and hook_manager.
  • drift_manifest (new module): durable serialize/parse of the RTTI self-heal drift report so layout drift can be saved and diffed across game versions (a read-only archive, never a heal input).
  • hook_manager: bulk enable_hooks / disable_hooks and enable_all_hooks / disable_all_hooks that toggle many hooks under one lock (ergonomics, not a perf change).
  • tooling: root .clang-format / .clang-tidy / .editorconfig, a mingw-debug-coverage preset, and a non-blocking quality.yml (ASan/UBSan probe plus advisory clang-format).
  • cleanup: narrowed the redundant sub-namespace using directives in config, scanner, and hook_manager.

Notes

The advisory quality.yml jobs are non-blocking and do not gate PRs. No public API was removed.

Summary by CodeRabbit

  • New Features

    • Added diagnostics module for tracking intentional resource cleanup events across subsystems
    • Introduced drift manifest format for serializing and persisting RTTI self-healing telemetry
    • Added batch hook management APIs for enabling/disabling multiple hooks at once
  • Documentation

    • Updated README with new diagnostics module and expanded hook manager batch operation documentation
  • Chores

    • Added code quality configuration files and automated quality checks via GitHub Actions

tkhquang added 4 commits June 11, 2026 00:36
…tizer workflow

- add root .clang-format, .clang-tidy, .editorconfig matching the existing style
- add a mingw-debug-coverage preset and a non-blocking quality.yml (ASan/UBSan probe + advisory clang-format)
- new diagnostics module exposing consumer-queryable counters for intentional loader-lock leak/detach events
- instrument the loader-lock teardown paths in logger, async_logger, config_watcher, input, memory, worker, and bootstrap
- serialize/parse the self-heal drift report (DriftEntry) to a durable, diffable manifest
- owning parsed records, file I/O, and fail-closed parsing
… usings

- add enable_hooks/disable_hooks/enable_all_hooks/disable_all_hooks (batch toggle under one lock)
- record the hook_manager loader-lock leak path via the diagnostics counters
- narrow the redundant sub-namespace using directives in config, scanner, and hook_manager
- update README and AGENTS for the new modules and APIs
@tkhquang tkhquang self-assigned this Jun 10, 2026
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@tkhquang, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 42 minutes and 57 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c09cf4a7-1f48-47d9-a2ed-5134ea6392ad

📥 Commits

Reviewing files that changed from the base of the PR and between e032871 and f7baeb0.

📒 Files selected for processing (2)
  • .clang-format
  • .github/workflows/quality.yml
📝 Walkthrough

Walkthrough

This PR introduces process-wide diagnostics instrumentation, a durable drift manifest serialization format for offline RTTI analysis, and batch hook management APIs. It includes code quality infrastructure (linting, formatting, CI workflow), comprehensive test coverage, and integration of diagnostics across shutdown paths to track intentional thread detaches under loader-lock conditions.

Changes

Diagnostics and drift manifest features with batch hook operations

Layer / File(s) Summary
Code quality infrastructure
.clang-format, .clang-tidy, .editorconfig, .github/workflows/quality.yml, CMakePresets.json
Adds C++ formatting rules (LLVM-based, Allman braces, 4-space indent), clang-tidy advisory checks scoped to public headers, editor config for UTF-8 and per-format indentation, GitHub Actions workflow for format and sanitizer probes, and new mingw-debug-coverage CMake preset.
Diagnostics API: declaration and implementation
include/DetourModKit/diagnostics.hpp, src/diagnostics.cpp, tests/test_diagnostics.cpp
Declares LeakSubsystem enum and four noexcept functions to record/count intentional leaks per subsystem. Implementation maintains per-subsystem atomic counters with relaxed memory ordering and bounds validation. Tests verify subsystem isolation, count accumulation, totaling, reset, and safe no-op handling for the Count sentinel.
Drift manifest API: serialization, parsing, file I/O
include/DetourModKit/drift_manifest.hpp, src/drift_manifest.cpp, tests/test_drift_manifest.cpp
Defines DriftRecord (owned parsed record with offsets, delta, ok flag, HealError) and ManifestError enum (MissingHeader, MalformedLine). Declares four functions: in-memory round-trip (serialize_drift_report, parse_drift_report) and file persistence (write_drift_report_to_file, read_drift_report_from_file). Implementation uses tab-separated fields, stable error tokens, strict decimal parsing, and fail-closed error handling. Tests validate round-trip preservation, negative offsets, empty reports, parsing failures, blank-line/CRLF tolerance, and file I/O.
Hook Manager batch operations
include/DetourModKit/hook_manager.hpp, src/hook_manager.cpp, tests/test_hook_manager.cpp
Adds <span> include and four public batch methods: enable_hooks/disable_hooks (by name span), enable_all_hooks/disable_all_hooks (return count toggled). Implements private helper toggle_hook_locked for shared single-hook logic under held mutator and hooks locks. Tests cover batch disable/enable, unknown-ID skipping, idempotent disable, all-hooks operations, and empty-input zero-ops.
Diagnostics integration across shutdown paths
src/async_logger.cpp, src/bootstrap.cpp, src/config_watcher.cpp, src/hook_manager.cpp, src/input.cpp, src/logger.cpp, src/memory.cpp, src/worker.cpp, src/config.cpp, src/scanner.cpp
Instruments eight translation units to record intentional-leak events via Diagnostics::record_intentional_leak(...) when worker/cleanup threads are detached under loader-lock conditions (AsyncLogger, Bootstrap, ConfigWatcher, HookManager, InputPoller, Logger, Memory, StoppableWorker). Also narrows namespace imports in config.cpp (explicit using for get_runtime_directory, trim) and scanner.cpp (using namespace DetourModKit).
Documentation and changelog
README.md, AGENTS.md, docs/misc/rtti-self-heal.md
Updates README to describe Diagnostics and drift manifest modules and batch Hook Manager APIs. Extends AGENTS with sanitizer/coverage preset commands, MinGW runtime library guidance, and test suite documentation. Adds drift manifest section to RTTI self-heal docs explaining offline manifest analysis.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • tkhquang/DetourModKit#52: Overlaps in shutdown instrumentation and AsyncLogger::shutdown() lifecycle paths where diagnostics are now recorded.
  • tkhquang/DetourModKit#69: Both PRs modify HookManager loader-lock shutdown paths and hardening behavior.
  • tkhquang/DetourModKit#10: Both touch memory cleanup shutdown paths in src/memory.cpp where diagnostics are integrated.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main changes: three new/extended modules (diagnostics, drift-manifest, bulk hook API) plus developer tooling, matching the substantive work in the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
.github/workflows/quality.yml (1)

1-69: ⚠️ Potential issue | 🟠 Major

Harden workflow security posture for quality.yml (least-privilege, disable checkout credentials, pin checkout action)

  • Add permissions: contents: read to avoid default broad GITHUB_TOKEN permissions.
  • Set with: persist-credentials: false on both actions/checkout@v4 steps to prevent credential leakage via artifacts.
  • Pin actions/checkout@v4 to a commit SHA to prevent mutable-tag supply-chain risk (34e114876b0b11c390a56381ad16ebd13914f8d5).
🔒 Proposed hardening
 name: Quality Probes

 on:
   pull_request:
     branches: [main]
   workflow_dispatch:

+# Restrict to minimal read-only permissions
+permissions:
+  contents: read
+
 # Both jobs are advisory and must never block a PR: they surface formatting
 # drift and prove the sanitizer preset still links and runs, but clang-format
 # version and MinGW ASan availability both vary by runner. The blocking gate
 # (build, tests, coverage) lives in pr-check.yml and is untouched here.

 jobs:
   format-check:
     name: clang-format (advisory)
     runs-on: windows-latest
     continue-on-error: true
     steps:
       - name: Checkout code
-        uses: actions/checkout@v4
+        uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5
+        with:
+          persist-credentials: false

       - name: Install clang-format
         run: pip install "clang-format==18.*"
         shell: bash

       - name: Check formatting (dry run, project sources only)
         run: |
           # git ls-files lists only this repo's tracked files, so submodule
           # sources under external/ are never formatted.
           git ls-files '*.cpp' '*.hpp' | xargs clang-format --dry-run --Werror
         shell: bash

   sanitizers:
     name: ASan + UBSan probe (advisory)
     runs-on: windows-latest
     continue-on-error: true
     env:
       MINGW_BIN: C:\mingw64\bin
     steps:
       - name: Checkout code
-        uses: actions/checkout@v4
+        uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5
         with:
+          persist-credentials: false
           submodules: "recursive"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/quality.yml around lines 1 - 69, The workflow grants
excessive default token rights and uses mutable checkout tags; harden it by
adding a top-level permissions: contents: read entry to restrict GITHUB_TOKEN to
repository read-only, replace both uses of actions/checkout@v4 with the pinned
commit SHA (34e114876b0b11c390a56381ad16ebd13914f8d5) and set with:
persist-credentials: false on the Checkout steps (both the Checkout code step in
the format-check job and the Checkout code step in the sanitizers job that
currently includes submodules) to prevent credential persistence/leakage.
🧹 Nitpick comments (1)
include/DetourModKit/hook_manager.hpp (1)

830-854: ⚡ Quick win

Add explicit @param tags for consistency.

Both enable_hooks and disable_hooks describe the hook_ids parameter within the @details section, but the file's established pattern (see create_inline_hook at line 500 and other methods) uses explicit @param tags for every parameter. Add @param hook_ids to each doc block for consistency.

📝 Suggested addition

For enable_hooks (after line 839):

          *          SafetyHook backend installs via a vectored exception handler and does not
          *          suspend threads, so there is no process-wide suspension to amortize.
+         * `@param` hook_ids The names of the hooks to enable.
          * `@return` The number of hooks now active.

For disable_hooks (after line 850):

          *          and skips unknown ids, and counts an already-disabled hook as a
          *          success (disable is idempotent). Ergonomics only (see
          *          `@ref` enable_hooks).
+         * `@param` hook_ids The names of the hooks to disable.
          * `@return` The number of hooks now disabled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@include/DetourModKit/hook_manager.hpp` around lines 830 - 854, Add explicit
`@param` hook_ids tags to the Doxygen blocks for enable_hooks and disable_hooks:
locate the comment above the enable_hooks declaration (function name
enable_hooks) and insert a "`@param` hook_ids The names of the hooks to enable."
line; do the same for the disable_hooks comment (function name disable_hooks)
with "`@param` hook_ids The names of the hooks to disable." to match the project's
existing doc style.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.clang-format:
- Line 6: The .clang-format file sets "Standard: c++20" while the project uses
C++23; update the Standard field in .clang-format from "c++20" to "c++23" so the
formatter recognizes C++23 syntax (locate the "Standard:" entry in the
.clang-format file and change its value to c++23 to match CMAKE_CXX_STANDARD and
project guidelines).

---

Outside diff comments:
In @.github/workflows/quality.yml:
- Around line 1-69: The workflow grants excessive default token rights and uses
mutable checkout tags; harden it by adding a top-level permissions: contents:
read entry to restrict GITHUB_TOKEN to repository read-only, replace both uses
of actions/checkout@v4 with the pinned commit SHA
(34e114876b0b11c390a56381ad16ebd13914f8d5) and set with: persist-credentials:
false on the Checkout steps (both the Checkout code step in the format-check job
and the Checkout code step in the sanitizers job that currently includes
submodules) to prevent credential persistence/leakage.

---

Nitpick comments:
In `@include/DetourModKit/hook_manager.hpp`:
- Around line 830-854: Add explicit `@param` hook_ids tags to the Doxygen blocks
for enable_hooks and disable_hooks: locate the comment above the enable_hooks
declaration (function name enable_hooks) and insert a "`@param` hook_ids The names
of the hooks to enable." line; do the same for the disable_hooks comment
(function name disable_hooks) with "`@param` hook_ids The names of the hooks to
disable." to match the project's existing doc style.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ea27e4b3-af18-4b78-ac5e-898805684da6

📥 Commits

Reviewing files that changed from the base of the PR and between 8d707a8 and e032871.

📒 Files selected for processing (26)
  • .clang-format
  • .clang-tidy
  • .editorconfig
  • .github/workflows/quality.yml
  • AGENTS.md
  • CMakePresets.json
  • README.md
  • docs/misc/rtti-self-heal.md
  • include/DetourModKit/diagnostics.hpp
  • include/DetourModKit/drift_manifest.hpp
  • include/DetourModKit/hook_manager.hpp
  • src/async_logger.cpp
  • src/bootstrap.cpp
  • src/config.cpp
  • src/config_watcher.cpp
  • src/diagnostics.cpp
  • src/drift_manifest.cpp
  • src/hook_manager.cpp
  • src/input.cpp
  • src/logger.cpp
  • src/memory.cpp
  • src/scanner.cpp
  • src/worker.cpp
  • tests/test_diagnostics.cpp
  • tests/test_drift_manifest.cpp
  • tests/test_hook_manager.cpp
💤 Files with no reviewable changes (1)
  • src/scanner.cpp

Comment thread .clang-format Outdated
- pin actions/checkout to a commit SHA, add contents: read, set persist-credentials: false
- run the ASan/UBSan probe via setup-msys2 (mingw-w64-x86_64-gcc ships the runtimes the default runner MinGW lacks)
- .clang-format: use Standard: Latest (clang-format has no c++23 enum) so C++23 parses
@tkhquang tkhquang merged commit 1255c1b into main Jun 10, 2026
2 of 4 checks passed
@tkhquang tkhquang deleted the feat/audit-followups branch June 10, 2026 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant