Skip to content

feat(LuaEngine): Add manifest-based script loading system (JSON, FiveM-inspired)#388

Open
slymscfr-tech wants to merge 2 commits into
azerothcore:masterfrom
slymscfr-tech:manifest-system
Open

feat(LuaEngine): Add manifest-based script loading system (JSON, FiveM-inspired)#388
slymscfr-tech wants to merge 2 commits into
azerothcore:masterfrom
slymscfr-tech:manifest-system

Conversation

@slymscfr-tech

@slymscfr-tech slymscfr-tech commented Jun 28, 2026

Copy link
Copy Markdown

Overview

This PR adds a manifest-based script loading system to ALE, inspired by FiveM’s fxmanifest.lua resource system. It gives server administrators explicit control over which modules and files are loaded, and in what order.

Motivation

In legacy mode, files are loaded in alphabetical order by path. This breaks dependencies: handlers/player.lua loads before init.lua because h < i. The manifest system solves this by letting you define the exact load order.

How It Works

  • Auto-detected: If a manifest.json file exists in the script folder (lua_scripts/), manifest mode is activated automatically. No config option needed.
  • Retrocompatible: If no manifest.json is found, legacy recursive scanning is used (fully backward compatible).

Root manifest (lua_scripts/manifest.json):

{
    "modules": ["module_a", "module_b"]
}

Module manifest (lua_scripts/module_a/manifest.json):

{
    "files": ["init.lua", "handlers/player.lua"]
}

If a module has no manifest.json, its files are scanned recursively (fallback).

Changes

New files

  • src/LuaEngine/ALEManifest.h — Manifest structures and parser declarations
  • src/LuaEngine/ALEManifest.cpp — JSON manifest parsing using fkYAML (already a project dependency)
  • docs/MANIFEST.md — Complete documentation for the manifest system

Modified files

  • src/LuaEngine/LuaEngine.h — Added lua_manifest_mode flag and LoadScriptPathsFromManifest() declaration
  • src/LuaEngine/LuaEngine.cpp — Auto-detect manifest mode in LoadScriptPaths(), new LoadScriptPathsFromManifest(), skip sort in RunScripts() for manifest mode
  • src/LuaEngine/ALEFileWatcher.cpp — Watch .json files for auto-reload
  • conf/mod_ale.conf.dist — Document auto-detected manifest mode
  • modules/CMakeLists.txt — Link fkYAML to modules target
  • README.md — Add manifest to Key Features and docs links
  • docs/USAGE.md — Add Manifest Mode section
  • docs/IMPL_DETAILS.md — Add Manifest System section and .ext exclusion note

Key differences from legacy mode

Feature Legacy Mode Manifest Mode
File discovery Recursive scan Only files listed in manifests
Load order Alphabetical by path Order defined in manifests
.ext files Loaded first Not loaded
Detection Always Auto-detected via manifest.json

Testing

Tested with a multi-module setup:

  • Module 1 (test/): single hello_world.lua
  • Module 2 (combat_system/): nested files in utils/ and handlers/ subdirectories

Load order verified correct via print() statements in each file. The legacy alphabetical order would have loaded handlers/ before init.lua (breaking dependencies), while manifest mode loads init.lua first as intended.

Compatibility

  • No breaking changes: existing servers without manifest.json continue to work as before
  • Gradual migration supported: add a root manifest first, then add module manifests as needed
  • .ext files are excluded in manifest mode (use .lua instead)

Summary by CodeRabbit

  • New Features

    • Added manifest-based script loading for Lua modules, with automatic detection when a manifest is present.
    • Scripts now load in the order defined by the manifest, improving control and predictability.
    • Expanded documentation with a full guide, usage notes, and updated feature references.
  • Bug Fixes

    • Improved file watching to recognize JSON files, so manifest changes can trigger reloads correctly.
    • Clarified that certain legacy file types are skipped when manifest mode is active.

…M-inspired)

Add a manifest-based script loading system that auto-detects a root
manifest.json in the script folder. When found, modules are loaded in
the order specified by the root manifest, and files within each module
are loaded in the order specified by the module manifest.

Key changes:
- New ALEManifest.h/.cpp: JSON manifest parser using fkYAML
- LuaEngine.cpp: auto-detect manifest mode in LoadScriptPaths(),
  new LoadScriptPathsFromManifest(), skip sort in RunScripts()
- LuaEngine.h: add lua_manifest_mode flag and manifest loader decl
- ALEFileWatcher.cpp: watch .json files for auto-reload
- mod_ale.conf.dist: document auto-detected manifest mode
- CMakeLists.txt: link fkYAML to modules target

Behavior:
- Auto-detected: if lua_scripts/manifest.json exists -> manifest mode
- Retrocompatible: no manifest.json -> legacy recursive scanning
- .ext files are NOT loaded in manifest mode
Add dedicated docs/MANIFEST.md covering the manifest-based script loading system.
Update README.md, docs/USAGE.md, and docs/IMPL_DETAILS.md with
references to the new manifest system documentation.
@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a FiveM-inspired manifest-based Lua script loading mode to ALE. A new ALEManifest class auto-detects manifest.json, loads modules and files in manifest-specified order, excludes .ext files in manifest mode, and skips alphabetical sorting. LuaEngine gains a lua_manifest_mode flag, a new loader function, and updated RunScripts() branching. ALEFileWatcher now watches .json files. Full documentation is added in docs/MANIFEST.md and related files.

Changes

Manifest-Based Script Loading

Layer / File(s) Summary
ALEManifest API and data structures
src/LuaEngine/ALEManifest.h, src/LuaEngine/LuaEngine.h
ModuleManifest struct and ALEManifest class declared with static methods for root manifest detection, root/module manifest loading, and private helpers. ALE class gains lua_manifest_mode flag and LoadScriptPathsFromManifest() declaration.
ALEManifest implementation
src/LuaEngine/ALEManifest.cpp
Implements HasRootManifest, LoadRootManifest (parses modules array, validates directories, assembles ModuleManifest objects with requirePath/requireCPath updates), LoadModuleManifest (dual legacy/manifest-mode behavior), BuildScriptEntry, and IsSupportedExtension (excludes .ext).
LuaEngine manifest mode wiring
src/LuaEngine/LuaEngine.cpp, src/LuaEngine/ALEFileWatcher.cpp
LoadScriptPaths() auto-detects manifest mode via HasRootManifest and branches; LoadScriptPathsFromManifest() populates lua_scripts in manifest order; RunScripts() skips sorting and extension loading when lua_manifest_mode is true. IsWatchedFileType adds .json support.
Documentation and config
docs/MANIFEST.md, docs/IMPL_DETAILS.md, docs/USAGE.md, conf/mod_ale.conf.dist, README.md
New MANIFEST.md guide covers mode detection, root/module manifest formats, load order, retrocompatibility, and examples. IMPL_DETAILS.md, USAGE.md, README.md, and conf dist updated with Manifest System/Mode sections and links.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 No more alphabets ruling the load,
A manifest now charts each module's road.
manifest.json whispers who goes first,
.ext files sit out — no longer cursed.
Order is law, and the rabbit approves,
Hopping through scripts in deterministic grooves! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ 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 matches the main change: adding a manifest-based LuaEngine script loading system with JSON manifests.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@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: 6

🤖 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 `@docs/IMPL_DETAILS.md`:
- Line 20: Update the Manifest System TOC entry so its link target matches the
rendered heading anchor for the “Manifest System” section. Adjust the Markdown
link in the table of contents to point to the slug generated by the `## 📦
Manifest System` heading, ensuring the in-page navigation works correctly.

In `@docs/MANIFEST.md`:
- Around line 18-25: The TOC and back-to-top links in the manifest documentation
use incorrect Markdown fragments with a leading hyphen, so they do not match the
rendered heading anchors. Update the links in the table of contents and the
back-to-top reference to use the exact heading slugs for the sections like
Overview, How It Works, Root Manifest, Module Manifest, Load Order,
Retrocompatibility, Differences from Legacy Mode, and Examples.

In `@docs/USAGE.md`:
- Line 22: The Manifest Mode table-of-contents link is using the wrong anchor
and won’t match the rendered heading. Update the TOC entry for the Manifest Mode
section so it points to the actual generated anchor for the `## 📦 Manifest
Mode` heading, using the existing TOC item in USAGE.md as the location to fix.

In `@src/LuaEngine/ALEManifest.cpp`:
- Around line 276-279: The IsSupportedExtension check in ALEManifest is wrongly
classifying native libraries as startup scripts by returning true for .dll and
.so, which causes them to be added to lua_scripts. Update this logic so manifest
parsing only treats actual Lua script extensions as supported here, and keep
native libraries discoverable through lua_requirecpath instead of flowing into
RunScripts(). Use ALEManifest::IsSupportedExtension and the lua_scripts
population path to locate the change.
- Around line 129-163: The no-manifest fallback in ALEManifest::scanDir is
losing relative subdirectory information and returning nondeterministic order.
Update the branch that collects supported files so BuildScriptEntry receives a
path relative to modulePath, not just the basename, and preserve the legacy
stable ordering by sorting the scripts collected in this fallback path before
returning.

In `@src/LuaEngine/LuaEngine.cpp`:
- Around line 187-202: Remove the extra require-path normalization in
LuaEngine::SetupRequirePaths (the block that appends
ALEConfig::GetInstance().GetRequirePath() and GetRequireCPath() and then erases
the trailing separator), since LoadScriptPaths already handles appending config
paths and trimming once. Update the manifest-mode path assembly so it does not
duplicate ALE.RequirePath / ALE.RequireCPath or strip a real character from the
last generated entry.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5d04dfa4-b245-4c63-8a05-9cf662dbefb3

📥 Commits

Reviewing files that changed from the base of the PR and between 1cb86c9 and 35576c7.

📒 Files selected for processing (10)
  • README.md
  • conf/mod_ale.conf.dist
  • docs/IMPL_DETAILS.md
  • docs/MANIFEST.md
  • docs/USAGE.md
  • src/LuaEngine/ALEFileWatcher.cpp
  • src/LuaEngine/ALEManifest.cpp
  • src/LuaEngine/ALEManifest.h
  • src/LuaEngine/LuaEngine.cpp
  • src/LuaEngine/LuaEngine.h

Comment thread docs/IMPL_DETAILS.md

- [Configuration](#-configuration)
- [Script Management](#-script-management)
- [Manifest System](#-manifest-system)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Fix the Manifest System TOC anchor.

#-manifest-system will not match the rendered ## 📦 Manifest System heading, so the TOC link will not jump correctly.

♻️ Proposed fix
- - [Manifest System](`#-manifest-system`)
+ - [Manifest System](`#manifest-system`)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- [Manifest System](#-manifest-system)
- [Manifest System](`#manifest-system`)
🤖 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 `@docs/IMPL_DETAILS.md` at line 20, Update the Manifest System TOC entry so its
link target matches the rendered heading anchor for the “Manifest System”
section. Adjust the Markdown link in the table of contents to point to the slug
generated by the `## 📦 Manifest System` heading, ensuring the in-page
navigation works correctly.

Source: Linters/SAST tools

Comment thread docs/MANIFEST.md
Comment on lines +18 to +25
- [Overview](#-overview)
- [How It Works](#-how-it-works)
- [Root Manifest](#-root-manifest)
- [Module Manifest](#-module-manifest)
- [Load Order](#-load-order)
- [Retrocompatibility](#-retrocompatibility)
- [Differences from Legacy Mode](#-differences-from-legacy-mode)
- [Examples](#-examples)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Fix the Markdown anchors in the TOC and back-to-top link.

The fragments use a leading - (#-overview, #-how-it-works, etc.), but the rendered headings will not, so these links will not resolve.

♻️ Proposed fix
- [Overview](`#-overview`)
+ [Overview](`#overview`)
- [How It Works](`#-how-it-works`)
+ [How It Works](`#how-it-works`)
- [Root Manifest](`#-root-manifest`)
+ [Root Manifest](`#root-manifest`)
- [Module Manifest](`#-module-manifest`)
+ [Module Manifest](`#module-manifest`)
- [Load Order](`#-load-order`)
+ [Load Order](`#load-order`)
- [Retrocompatibility](`#-retrocompatibility`)
+ [Retrocompatibility](`#retrocompatibility`)
- [Differences from Legacy Mode](`#-differences-from-legacy-mode`)
+ [Differences from Legacy Mode](`#differences-from-legacy-mode`)
- [Examples](`#-examples`)
+ [Examples](`#examples`)
- [⬆ Back to Top](`#-ale-manifest-system`)
+ [⬆ Back to Top](`#ale-manifest-system`)

Also applies to: 291-291

🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 19-19: Link fragments should be valid

(MD051, link-fragments)


[warning] 24-24: Link fragments should be valid

(MD051, link-fragments)

🤖 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 `@docs/MANIFEST.md` around lines 18 - 25, The TOC and back-to-top links in the
manifest documentation use incorrect Markdown fragments with a leading hyphen,
so they do not match the rendered heading anchors. Update the links in the table
of contents and the back-to-top reference to use the exact heading slugs for the
sections like Overview, How It Works, Root Manifest, Module Manifest, Load
Order, Retrocompatibility, Differences from Legacy Mode, and Examples.

Source: Linters/SAST tools

Comment thread docs/USAGE.md
- [Your First Script](#-your-first-script)
- [Lua Basics](#-lua-basics)
- [ALE Basics](#-ale-basics)
- [Manifest Mode](#-manifest-mode)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Fix the Manifest Mode TOC anchor.

#-manifest-mode will not match the rendered ## 📦 Manifest Mode heading, so the TOC entry will not jump correctly.

♻️ Proposed fix
- [Manifest Mode](`#-manifest-mode`)
+ [Manifest Mode](`#manifest-mode`)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- [Manifest Mode](#-manifest-mode)
- [Manifest Mode](`#manifest-mode`)
🤖 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 `@docs/USAGE.md` at line 22, The Manifest Mode table-of-contents link is using
the wrong anchor and won’t match the rendered heading. Update the TOC entry for
the Manifest Mode section so it points to the actual generated anchor for the
`## 📦 Manifest Mode` heading, using the existing TOC item in USAGE.md as the
location to fix.

Source: Linters/SAST tools

Comment on lines +129 to +163
std::function<void(fs::path const&)> scanDir = [&](fs::path const& dir)
{
fs::directory_iterator endIter;
for (fs::directory_iterator it(dir); it != endIter; ++it)
{
std::string fullpath = it->path().generic_string();

// Skip hidden files/dirs
std::string name = it->path().filename().generic_string();
if (!name.empty() && name[0] == '.')
continue;

if (fs::is_directory(it->status()))
{
// Add subdirectory to require path
requirePath += fullpath + "/?.lua;" + fullpath + "/?.moon;" + fullpath + "/?.out;";
requireCPath += fullpath + "/?.dll;" + fullpath + "/?.so;";
scanDir(it->path());
}
else if (fs::is_regular_file(it->status()))
{
std::string filename = it->path().filename().generic_string();
std::size_t extDot = filename.find_last_of('.');
if (extDot == std::string::npos)
continue;

std::string ext = filename.substr(extDot);
if (IsSupportedExtension(ext))
scripts.push_back(BuildScriptEntry(filename, modulePath));
}
}
};

scanDir(fs::path(modulePath));
return scripts;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Keep the no-manifest fallback relative to modulePath and deterministic.

Line 157 passes only the basename into BuildScriptEntry(), so a nested file like handlers/foo.lua is recorded as <module>/foo.lua. Because manifest mode skips the later sort, this branch also returns filesystem iteration order instead of the legacy stable order the comment promises.

🛠️ Suggested fix
-                    std::string filename = it->path().filename().generic_string();
-                    std::size_t extDot = filename.find_last_of('.');
+                    std::string relativePath =
+                        fs::relative(it->path(), fs::path(modulePath)).generic_string();
+                    std::size_t extDot = relativePath.find_last_of('.');
                     if (extDot == std::string::npos)
                         continue;

-                    std::string ext = filename.substr(extDot);
+                    std::string ext = relativePath.substr(extDot);
                     if (IsSupportedExtension(ext))
-                        scripts.push_back(BuildScriptEntry(filename, modulePath));
+                        scripts.push_back(BuildScriptEntry(relativePath, modulePath));
                 }
             }
         };

         scanDir(fs::path(modulePath));
+        std::sort(scripts.begin(), scripts.end(), [](LuaScript const& a, LuaScript const& b) {
+            return a.filepath < b.filepath;
+        });
         return scripts;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
std::function<void(fs::path const&)> scanDir = [&](fs::path const& dir)
{
fs::directory_iterator endIter;
for (fs::directory_iterator it(dir); it != endIter; ++it)
{
std::string fullpath = it->path().generic_string();
// Skip hidden files/dirs
std::string name = it->path().filename().generic_string();
if (!name.empty() && name[0] == '.')
continue;
if (fs::is_directory(it->status()))
{
// Add subdirectory to require path
requirePath += fullpath + "/?.lua;" + fullpath + "/?.moon;" + fullpath + "/?.out;";
requireCPath += fullpath + "/?.dll;" + fullpath + "/?.so;";
scanDir(it->path());
}
else if (fs::is_regular_file(it->status()))
{
std::string filename = it->path().filename().generic_string();
std::size_t extDot = filename.find_last_of('.');
if (extDot == std::string::npos)
continue;
std::string ext = filename.substr(extDot);
if (IsSupportedExtension(ext))
scripts.push_back(BuildScriptEntry(filename, modulePath));
}
}
};
scanDir(fs::path(modulePath));
return scripts;
std::function<void(fs::path const&)> scanDir = [&](fs::path const& dir)
{
fs::directory_iterator endIter;
for (fs::directory_iterator it(dir); it != endIter; ++it)
{
std::string fullpath = it->path().generic_string();
// Skip hidden files/dirs
std::string name = it->path().filename().generic_string();
if (!name.empty() && name[0] == '.')
continue;
if (fs::is_directory(it->status()))
{
// Add subdirectory to require path
requirePath += fullpath + "/?.lua;" + fullpath + "/?.moon;" + fullpath + "/?.out;";
requireCPath += fullpath + "/?.dll;" + fullpath + "/?.so;";
scanDir(it->path());
}
else if (fs::is_regular_file(it->status()))
{
std::string relativePath =
fs::relative(it->path(), fs::path(modulePath)).generic_string();
std::size_t extDot = relativePath.find_last_of('.');
if (extDot == std::string::npos)
continue;
std::string ext = relativePath.substr(extDot);
if (IsSupportedExtension(ext))
scripts.push_back(BuildScriptEntry(relativePath, modulePath));
}
}
};
scanDir(fs::path(modulePath));
std::sort(scripts.begin(), scripts.end(), [](LuaScript const& a, LuaScript const& b) {
return a.filepath < b.filepath;
});
return scripts;
🤖 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 `@src/LuaEngine/ALEManifest.cpp` around lines 129 - 163, The no-manifest
fallback in ALEManifest::scanDir is losing relative subdirectory information and
returning nondeterministic order. Update the branch that collects supported
files so BuildScriptEntry receives a path relative to modulePath, not just the
basename, and preserve the legacy stable ordering by sorting the scripts
collected in this fallback path before returning.

Comment on lines +276 to +279
bool ALEManifest::IsSupportedExtension(std::string const& ext)
{
// In manifest mode, .ext files are NOT supported
return ext == ".lua" || ext == ".moon" || ext == ".dll" || ext == ".so" || ext == ".out";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Don't treat native libraries as startup scripts.

This marks .dll/.so entries as loadable scripts, so manifest-listed native modules get pushed into lua_scripts. RunScripts() has no native-library branch, so those files fall through the generic loader instead of just being discoverable via lua_requirecpath.

🛠️ Suggested fix
-    return ext == ".lua" || ext == ".moon" || ext == ".dll" || ext == ".so" || ext == ".out";
+    return ext == ".lua" || ext == ".moon" || ext == ".out";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
bool ALEManifest::IsSupportedExtension(std::string const& ext)
{
// In manifest mode, .ext files are NOT supported
return ext == ".lua" || ext == ".moon" || ext == ".dll" || ext == ".so" || ext == ".out";
bool ALEManifest::IsSupportedExtension(std::string const& ext)
{
// In manifest mode, .ext files are NOT supported
return ext == ".lua" || ext == ".moon" || ext == ".out";
🤖 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 `@src/LuaEngine/ALEManifest.cpp` around lines 276 - 279, The
IsSupportedExtension check in ALEManifest is wrongly classifying native
libraries as startup scripts by returning true for .dll and .so, which causes
them to be added to lua_scripts. Update this logic so manifest parsing only
treats actual Lua script extensions as supported here, and keep native libraries
discoverable through lua_requirecpath instead of flowing into RunScripts(). Use
ALEManifest::IsSupportedExtension and the lua_scripts population path to locate
the change.

Comment on lines +187 to +202
// Append custom require paths from config
const std::string& lua_path_extra = static_cast<std::string>(ALEConfig::GetInstance().GetRequirePath());
const std::string& lua_cpath_extra = static_cast<std::string>(ALEConfig::GetInstance().GetRequireCPath());

if (!lua_path_extra.empty())
lua_requirepath += lua_path_extra;

if (!lua_cpath_extra.empty())
lua_requirecpath += lua_cpath_extra;

// Erase last ;
if (!lua_requirepath.empty())
lua_requirepath.erase(lua_requirepath.end() - 1);

if (!lua_requirecpath.empty())
lua_requirecpath.erase(lua_requirecpath.end() - 1);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Remove the second require-path normalization from manifest mode.

LoadScriptPaths() already appends config paths and trims the trailing separator after this helper returns. Doing it again here always chops one real character off the last manifest-generated entry, and it also duplicates ALE.RequirePath / ALE.RequireCPath when those settings are non-empty.

🛠️ Suggested fix
-    // Append custom require paths from config
-    const std::string& lua_path_extra = static_cast<std::string>(ALEConfig::GetInstance().GetRequirePath());
-    const std::string& lua_cpath_extra = static_cast<std::string>(ALEConfig::GetInstance().GetRequireCPath());
-
-    if (!lua_path_extra.empty())
-        lua_requirepath += lua_path_extra;
-
-    if (!lua_cpath_extra.empty())
-        lua_requirecpath += lua_cpath_extra;
-
-    // Erase last ;
-    if (!lua_requirepath.empty())
-        lua_requirepath.erase(lua_requirepath.end() - 1);
-
-    if (!lua_requirecpath.empty())
-        lua_requirecpath.erase(lua_requirecpath.end() - 1);
-
     ALE_LOG_INFO("[ALE]: Loaded {} scripts from {} module(s) in {} ms",
         lua_scripts.size(), modules.size(), ALEUtil::GetTimeDiff(oldMSTime));
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Append custom require paths from config
const std::string& lua_path_extra = static_cast<std::string>(ALEConfig::GetInstance().GetRequirePath());
const std::string& lua_cpath_extra = static_cast<std::string>(ALEConfig::GetInstance().GetRequireCPath());
if (!lua_path_extra.empty())
lua_requirepath += lua_path_extra;
if (!lua_cpath_extra.empty())
lua_requirecpath += lua_cpath_extra;
// Erase last ;
if (!lua_requirepath.empty())
lua_requirepath.erase(lua_requirepath.end() - 1);
if (!lua_requirecpath.empty())
lua_requirecpath.erase(lua_requirecpath.end() - 1);
🤖 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 `@src/LuaEngine/LuaEngine.cpp` around lines 187 - 202, Remove the extra
require-path normalization in LuaEngine::SetupRequirePaths (the block that
appends ALEConfig::GetInstance().GetRequirePath() and GetRequireCPath() and then
erases the trailing separator), since LoadScriptPaths already handles appending
config paths and trimming once. Update the manifest-mode path assembly so it
does not duplicate ALE.RequirePath / ALE.RequireCPath or strip a real character
from the last generated entry.

slymscfr-tech added a commit to slymscfr-tech/mod-ale that referenced this pull request Jun 28, 2026
- Docs: fix heading anchor links (remove leading hyphens from TOC anchors
  in IMPL_DETAILS.md, MANIFEST.md, USAGE.md)
- ALEManifest: remove .dll/.so from IsSupportedExtension to prevent
  native libs being treated as startup scripts
- ALEManifest: fix scanDir fallback to preserve subdirectory path info
  and sort scripts for deterministic order
- LuaEngine: remove duplicate config require-path appending and
  trailing semicolon erasure from LoadScriptPathsFromManifest
  (caller LoadScriptPaths already handles this)
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