fix(config): refuse getSetting() before ctld.initialize(), clearly - #141
Merged
Conversation
Reading a CTLD setting before ctld.initialize() returns nil silently, then usually crashes a few lines later on ordinary arithmetic - a stack trace naming neither CTLD nor the missing init. Grilled with the user 2026-08-26 following GitHub issue #125 (davidp57/Zip): a single guard in CTLDConfig:getSetting, the deepest real choke point, rather than the issue's own suggestion of 18 per-manager getInstance() guards.
Single AFK ticket: the getSetting guard, tests extending the existing config_spec.lua seam, CHANGELOG entry. Granularity confirmed with the user.
A CTLD manager touched before ctld.initialize() used to crash on an unreadable arithmetic error deep inside src/, with a stack trace naming neither CTLD nor the missing init. Every setting read before init silently resolved to nil, and the first manager to do arithmetic on that nil (e.g. a refresh interval) is the one that crashed. CTLDConfig:getSetting - the sole real read path, everything in src/ reads config through ctld.gs, which delegates straight to it - now refuses immediately with error(msg, 3), pointing the stack at the actual caller instead of at this line. Chosen over the issue's own suggestion of a guard in each manager's getInstance() (~18 call sites) or a guard in ctld.gs: getSetting is the deepest single choke point, catching both the ctld.gs path and the documented-but-unused direct CTLDConfig.get():getSetting() API. CTLD_i18n.lua's pre-init tr() tolerance (already wraps its read in pcall, falling back to a default language) is unaffected - verified with a new test, the pcall catches the new explicit error exactly as it caught the old implicit crash. FIX-CONFIG-NOT-LOADED-GUARD, grilled with the user 2026-08-26, closes GitHub issue #125.
Reviewer's GuideThe PR changes pre-initialization configuration access from silent Sequence diagram for pre-initialization configuration accesssequenceDiagram
participant Caller
participant CTLDConfig
participant CTLD_i18n
Caller->>CTLDConfig: ctld.gs(key)
CTLDConfig->>CTLDConfig: getSetting(key)
alt configuration not loaded
CTLDConfig-->>Caller: error(..., 3)
else configuration loaded
CTLDConfig-->>Caller: setting value
end
CTLD_i18n->>CTLDConfig: ctld.gs(i18n_lang)
CTLDConfig-->>CTLD_i18n: error(..., 3)
CTLD_i18n->>CTLD_i18n: pcall(...)
CTLD_i18n-->>Caller: fallback language
Flow diagram for the configuration guard choke pointflowchart LR
A["ctld.initialize()"] --> B["CTLDConfig:load()"]
C["ctld.gs(key)"] --> D["CTLDConfig:getSetting(key)"]
E["CTLDConfig.get():getSetting(key)"] --> D
D -->|isLoaded| F["Return setting or default"]
D -->|not loaded| G["Clear error naming ctld.initialize()"]
H["ctld.tr()"] --> I["pcall around language lookup"]
I -->|error caught| J["Fallback language"]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/CTLD_config.lua" line_range="111-113" />
<code_context>
+1. `src/CTLD_config.lua`, `CTLDConfig:getSetting` (~line 106): at the top, before reading
+ `self.settings[key]`, add:
+ ```lua
+ if not self.isLoaded then
+ error("CTLD configuration is not loaded — call ctld.initialize() before reading any "
+ .. "CTLD setting or using a CTLD manager.", 3)
+ end
+ ```
</code_context>
<issue_to_address>
**issue (broader_impact):** A failed `CTLDConfig:load()` leaves `self.isLoaded` set to true because `load()` marks it loaded before parsing and validation. Subsequent `getSetting()` calls therefore bypass this guard and return incomplete or nil values instead of refusing with the promised initialization error.
**Triggers:** When `ctld.configUser` is malformed or empty, or another error occurs during configuration loading.
**Suggested fix:** Set `isLoaded` only after loading completes successfully, or reset it in an error path before rethrowing the load failure.
</issue_to_address>
### Comment 2
<location path="tests/ci/unit/i18n_spec.lua" line_range="344-346" />
<code_context>
+ -- FIX-CONFIG-NOT-LOADED-GUARD: a pre-init tr() must still degrade gracefully, not raise
+ -- the new getSetting() guard error. _activeLang()'s own pcall is what makes this work.
+ it("a call before ctld.initialize() still returns a string, never raises", function()
+ CTLDConfig._instance = nil -- fresh, never-loaded instance — no :load() call
+ assert.has_no_error(function()
+ assert.equals("hello", ctld.tr("__TESTKEY__"))
+ end)
+ end)
</code_context>
<issue_to_address>
**issue (testing):** The regression test replaces the global configuration singleton with a never-loaded instance and does not restore or load it afterward. Any later test or shared setup that calls `ctld.gs()` without reinitializing the singleton now raises the new guard error, making the suite order-dependent.
**Triggers:** When another spec runs after this test and relies on the existing loaded configuration state.
**Suggested fix:** Restore the prior singleton in an `after_each`, or call `CTLDConfig.get():load()` after the pre-init assertion.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
davidp57
approved these changes
Aug 26, 2026
Two review findings on the getSetting() not-loaded guard: - CTLDConfig:load() set isLoaded=true before validating a malformed ctld.configUser, so an aborted load left the new guard silently bypassed (settings stayed empty, but isLoaded already read true). isLoaded is now only set after validation passes. - error(msg, 3) produced a positionless error for real tail-call sites (CTLD_aasystem.lua, CTLD_troop.lua): a Lua 5.1 tail call drops the caller's own stack frame, so a fixed level either mis-points or lands on a frame with no line info. Switched to error(msg, 0) - message-only, which is what actually carries the diagnosis. Also: tools/companion/asset_check.lua now reports (via its existing outText/log style) instead of crashing uncaught when the guard fires during collection; config_spec.lua's new describe block gained the after_each the sibling i18n_spec.lua block already had, plus a red test for the load() ordering bug. FIX-CONFIG-NOT-LOADED-GUARD, review remediation on PR #141.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ctld.initialize()used to crash on an unreadable arithmetic error deep insidesrc/, with a stack trace naming neither CTLD nor the missing init. Every setting read before init silently resolved tonil, and the first manager to do arithmetic on thatnil(e.g. a refresh interval) is the one that crashed.CTLDConfig:getSetting— the sole real read path, everything insrc/reads config throughctld.gs, which delegates straight to it — now refuses immediately witherror(msg, 3), pointing the stack at the actual caller instead of at this line.getInstance()(~18 call sites) or a guard inctld.gs:getSettingis the deepest single choke point, catching both thectld.gspath and the documented-but-unused directCTLDConfig.get():getSetting()API.CTLD_i18n.lua's pre-inittr()tolerance (already wraps its read inpcall, falling back to a default language) is unaffected — verified with a new test, thepcallcatches the new explicit error exactly as it caught the old implicit crash..backlog/FIX-CONFIG-NOT-LOADED-GUARD/PRD.md).Test plan
ctld.tr()still degrades gracefully pre-init (no raise) via the existingpcallsite.busted --pattern=_spec --helper=tests/ci/helpers/init.lua tests/ci→ 1359 passed / 0 failed / 1 pending (pre-existing, DCS-live gated).luac -p CTLD.lua— Lua 5.1 syntax OK (rebuilt viamerge_CTLD.ps1).luacheck --config .luacheckrc src/CTLD_config.lua— 1 pre-existing warning (unusedconfiglocal, already counted in the known 89), no new warning introduced.Summary by Sourcery
Refuse CTLD configuration access before initialization with a clear diagnostic instead of allowing delayed, unreadable runtime failures.
Bug Fixes:
ctld.initialize()when accessed before initialization.Documentation:
Tests: