PR Summary by Qodo

Resync CommonJS Settings exports after reload to expose ep_* plugin settings

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

• Add reusable sync for CommonJS Settings getters/setters.
• Run CJS export sync after every reloadSettings() call.
• Add regression test for ep_* plugin settings loaded via env config.

graph TD
  C[("Config/env") --> R["reloadSettings()"] --> S["settings (in-memory)"] --> X["syncCjsModuleExports()"] --> M[["module.exports (CJS mirror)"]] --> P[/"CJS plugin require()"/]

  subgraph Legend
    direction LR
    _cfg[("Config source")] ~~~ _fn["Function"] ~~~ _cjs[["CJS export"]] ~~~ _plg[/"Plugin"/]
  end

Loading

The following are alternative approaches to this PR:

1. Replace CJS mirror with Proxy-based module.exports

• ➕ Automatically reflects new keys without explicit resync calls
• ➕ Simplifies edge cases around late-added properties
• ➖ Higher runtime magic and harder debugging/stack traces
• ➖ Potential performance/compat issues in older environments
• ➖ More invasive change to a widely-used compatibility surface

2. Force consumers to use .default and deprecate direct CJS properties

• ➕ Removes the need for maintaining a mirrored export surface
• ➕ Aligns with ESM default export semantics
• ➖ Breaks documented/legacy plugin API and ecosystem expectations
• ➖ Requires deprecation/migration plan across many plugins

3. Clear require cache and re-require Settings on reload

• ➕ Keeps CJS properties in sync by re-executing module initialization
• ➖ Risky side effects (reinitialization, duplicated singletons)
• ➖ Non-local behavior and harder to reason about in tests/runtime

Recommendation: Keep the current approach: explicitly resyncing the CJS getters after reloadSettings() is minimal, predictable, and preserves the documented plugin API. The helper extraction also keeps the compatibility logic contained and makes future sync call sites straightforward if additional reload pathways are introduced.

Bug fix (1) +10 / -3

Settings.tsResync CommonJS Settings export mirror after reloadSettings() +10/-3

Resync CommonJS Settings export mirror after reloadSettings()

• Extracts the CommonJS compatibility getter/setter mirroring into a reusable syncCjsModuleExports() helper. Invokes the helper after reloadSettings() so settings keys added during reload (notably ep_* plugin hashes) are available via require('.../Settings').

src/node/utils/Settings.ts

Tests (1) +39 / -0

settings.tsAdd regression test for ep_* plugin settings exposed via CJS require +39/-0

Add regression test for ep_* plugin settings exposed via CJS require

• Adds a regression test that sets an EP__ep_test_plugin__* environment key, reloads settings from fixture files, and asserts the ep_test_plugin hash is present and consistent on the CommonJS module export. Includes cleanup to restore environment and settings state after the test.

src/tests/backend/specs/settings.ts

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Description

The new regression test’s teardown deletes ep_test_plugin from the CommonJS export object but does
not delete it from the underlying settings singleton, so the plugin hash can persist after the
test. Because reloadSettings() only overwrites keys present in the newly parsed config and never
deletes absent keys, subsequent tests can observe stale ep_test_plugin state and become
order-dependent.

Code

src/tests/backend/specs/settings.ts[R176-179]

+      } finally {
+        delete process.env[envKey];
+        if (saved === undefined) {
+          delete settingsMod.ep_test_plugin;

Relevance

●●● Strong

Team often accepts fixes preventing backend test state leakage by fully restoring global Settings in
teardown.

PR-#7798
PR-#7796

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test adds ep_test_plugin via env + reloadSettings() then attempts to clean up by deleting
only the CJS export property. reloadSettings() persists settings by calling storeSettings() on
parsed config objects, but storeSettings() only overwrites keys present in the parsed object, so
clearing the env var and calling reloadSettings() does not remove ep_test_plugin if it was
previously added. Because the singleton retains the key, the CJS mirror can also be re-populated on
reload via syncCjsModuleExports().

src/tests/backend/specs/settings.ts[154-186]
src/node/utils/Settings.ts[1189-1195]
src/node/utils/Settings.ts[962-990]
src/node/utils/Settings.ts[918-931]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new test `exposes ep_* plugin settings on CJS require after reloadSettings()` sets `EP__ep_test_plugin__allowFeature` and calls `reloadSettings()`, which adds `ep_test_plugin` onto the singleton `settings` object.

In the `finally` block, the test currently runs `delete settingsMod.ep_test_plugin;` when the key was originally absent. This deletes the accessor property on `module.exports` (the CJS mirror), not the underlying `settings` object that `reloadSettings()` mutates, so `settings.default.ep_test_plugin` can remain set and leak into later tests.

### Issue Context
- `reloadSettings()` calls `storeSettings()` to *apply* parsed settings/credentials but does not clear keys that are no longer present.
- `storeSettings()` only iterates over keys present in the parsed config object, so a previously-added `ep_*` key is not removed on subsequent reloads if the env var is cleared.

### Fix Focus Areas
- src/tests/backend/specs/settings.ts[154-186]
- src/node/utils/Settings.ts[918-931]
- src/node/utils/Settings.ts[962-990]
- src/node/utils/Settings.ts[1189-1195]

### Suggested fix
In the test teardown, when `saved === undefined`:
1. Delete the key from the underlying settings singleton (e.g., `if (settingsMod.default) delete settingsMod.default.ep_test_plugin;`).
2. Also delete the CJS mirror property (optional but keeps shapes consistent): `delete settingsMod.ep_test_plugin;`.

Optionally, if `saved` is defined, restore it on the underlying singleton as well (to avoid cases where the mirror setter doesn’t hit the expected object shape under different interop modes).

This ensures the global singleton is restored and prevents cross-test contamination.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Description

The new regression test attempts to clean up by deleting settingsMod.ep_test_plugin, but that
deletes the export property rather than reliably removing the backing settings key created by
reloadSettings(). Because reloadSettings()/storeSettings() never clears keys absent from the loaded
sources, ep_test_plugin can persist and contaminate subsequent tests; additionally, the env var is
not restored if it previously existed.

Code

src/tests/backend/specs/settings.ts[R176-181]

+      } finally {
+        delete process.env[envKey];
+        if (saved === undefined) {
+          delete settingsMod.ep_test_plugin;
+        } else {
+          settingsMod.ep_test_plugin = saved;

Evidence

The test cleanup deletes an export property, but storeSettings only adds/overwrites keys and never
clears prior keys, so a plugin key introduced during reload can remain set after the test. This
makes the test non-hermetic and potentially order-dependent.

src/tests/backend/specs/settings.ts[150-187]
src/node/utils/Settings.ts[962-990]
src/node/utils/Settings.ts[1190-1195]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new test mutates `process.env` and the Settings singleton, but its cleanup is incomplete:
- It unconditionally `delete`s the env var instead of restoring any previous value.
- It `delete`s `settingsMod.ep_test_plugin` (an export property), which does not reliably restore the underlying `settings` object state created by `reloadSettings()`. Because `storeSettings()` never deletes missing keys, the plugin key can persist across the rest of the suite.

### Issue Context
`reloadSettings()` calls `storeSettings(settingsParsed)`/`storeSettings(credentials)` and those functions only iterate over keys present in the parsed objects; they do not remove old keys.

### Fix Focus Areas
- src/tests/backend/specs/settings.ts[154-187]
- src/node/utils/Settings.ts[962-990]
- src/node/utils/Settings.ts[1190-1195]

### Suggested fix pattern
- Save the original env value: `const originalEnv = process.env[envKey]; const hadEnv = Object.prototype.hasOwnProperty.call(process.env, envKey);`
- In `finally`, restore it: if (hadEnv) process.env[envKey] = originalEnv; else delete process.env[envKey];
- Restore/remove the backing key on the actual settings object (likely `settingsMod.default ?? settingsMod`):
 - If it previously existed, set it back.
 - If it didn’t, `delete (settingsMod.default ?? settingsMod).ep_test_plugin;`
- Then call `reloadSettings()`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

The new regression test’s teardown deletes ep_test_plugin from the CommonJS export object but does
not delete it from the underlying settings singleton, so the plugin hash can persist after the
test. Because reloadSettings() only overwrites keys present in the newly parsed config and never
deletes absent keys, subsequent tests can observe stale ep_test_plugin state and become
order-dependent.

Code

src/tests/backend/specs/settings.ts[R176-179]

+      } finally {
+        delete process.env[envKey];
+        if (saved === undefined) {
+          delete settingsMod.ep_test_plugin;

Relevance

●●● Strong

Team often accepts fixes preventing backend test state leakage by fully restoring global Settings in
teardown.

PR-#7798
PR-#7796

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test adds ep_test_plugin via env + reloadSettings() then attempts to clean up by deleting
only the CJS export property. reloadSettings() persists settings by calling storeSettings() on
parsed config objects, but storeSettings() only overwrites keys present in the parsed object, so
clearing the env var and calling reloadSettings() does not remove ep_test_plugin if it was
previously added. Because the singleton retains the key, the CJS mirror can also be re-populated on
reload via syncCjsModuleExports().

src/tests/backend/specs/settings.ts[154-186]
src/node/utils/Settings.ts[1189-1195]
src/node/utils/Settings.ts[962-990]
src/node/utils/Settings.ts[918-931]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new test `exposes ep_* plugin settings on CJS require after reloadSettings()` sets `EP__ep_test_plugin__allowFeature` and calls `reloadSettings()`, which adds `ep_test_plugin` onto the singleton `settings` object.
In the `finally` block, the test currently runs `delete settingsMod.ep_test_plugin;` when the key was originally absent. This deletes the accessor property on `module.exports` (the CJS mirror), not the underlying `settings` object that `reloadSettings()` mutates, so `settings.default.ep_test_plugin` can remain set and leak into later tests.
### Issue Context
- `reloadSettings()` calls `storeSettings()` to *apply* parsed settings/credentials but does not clear keys that are no longer present.
- `storeSettings()` only iterates over keys present in the parsed config object, so a previously-added `ep_*` key is not removed on subsequent reloads if the env var is cleared.
### Fix Focus Areas
- src/tests/backend/specs/settings.ts[154-186]
- src/node/utils/Settings.ts[918-931]
- src/node/utils/Settings.ts[962-990]
- src/node/utils/Settings.ts[1189-1195]
### Suggested fix
In the test teardown, when `saved === undefined`:
1. Delete the key from the underlying settings singleton (e.g., `if (settingsMod.default) delete settingsMod.default.ep_test_plugin;`).
2. Also delete the CJS mirror property (optional but keeps shapes consistent): `delete settingsMod.ep_test_plugin;`.
Optionally, if `saved` is defined, restore it on the underlying singleton as well (to avoid cases where the mirror setter doesn’t hit the expected object shape under different interop modes).
This ensures the global singleton is restored and prevents cross-test contamination.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

To customize comments, go to the Qodo configuration screen, or learn more in the docs.