Skip to content

fix: add setters to CJS compatibility layer in Settings #7481

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JohnMcLear merged 1 commit into from
Apr 6, 2026
+1 −0
Merged

fix: add setters to CJS compatibility layer in Settings #7481

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/node/utils/Settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,7 @@ if (typeof module !== 'undefined' && module.exports) {
if (!(key in currentExports)) {
Object.defineProperty(currentExports, key, {
get: () => (settings as any)[key],
set: (v: any) => { (settings as any)[key] = v; },
enumerable: true,
configurable: true,
});
Comment on lines 670 to 676
Copy link
Copy Markdown

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects bot Apr 6, 2026

Choose a reason for hiding this comment

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

Action required

1. No regression test for setter 📘 Rule violation ☼ Reliability

This bug fix changes module.exports property descriptors to add a setter, but the PR does not
include any accompanying regression test to ensure CJS consumers can mutate settings. Without an
automated test, the issue can silently reoccur if this change is reverted or refactored.
Agent Prompt 
## Issue description
A bug fix was made to allow CJS consumers to mutate settings (by adding a `set` handler in the `module.exports` compatibility block), but no regression test was added.

## Issue Context
Without a test, a future refactor (or revert) could reintroduce `TypeError: Cannot set property which has only a getter` for CJS consumers.

## Fix Focus Areas
- src/node/utils/Settings.ts[670-676]

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

Copy link
Copy Markdown
Member Author

@JohnMcLear JohnMcLear Apr 6, 2026

Choose a reason for hiding this comment

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

Addressed in the latest push. Thanks for catching this!

Expand Down
Loading