Skip to content

Conversation

@Nayjest
Copy link
Collaborator

@Nayjest Nayjest commented Sep 9, 2025

No description provided.

@github-actions
Copy link

github-actions bot commented Sep 9, 2025

I've Reviewed the Code

The new Config dataclass provides a clear structure for configuration management, but there are potential issues with mutable default values and falsy value checks in defaults handling that need to be addressed.

⚠️ 2 issues found across 2 files

#1 Mutable default value of 'defaults' is unsafe

snowflake_mcp/config.py L11

Using 'Config' (a mutable object) as a default value for the 'defaults' field may lead to shared state among instances. The recommended practice is to set default=None and handle default construction in post_init if necessary.
Tags: bug, maintainability
Affected code:

11:     defaults: "Config" = field(default=None)

Proposed change:

    defaults: "Config" = field(default=None, repr=False)

#2 Potential bug in checking falsy values in post_init

snowflake_mcp/config.py L15-L17

The check 'if not getattr(self, key)' conflates empty strings, None, zero, and False. This could lead to default values from 'defaults' overwriting values in config that are intentionally set to falsy (e.g., ''). This may result in unintended behavior.
Tags: bug
Affected code:

15:             for key, value in self.defaults.__dict__.items():
16:                 if not getattr(self, key):
17:                     setattr(self, key, value)

Proposed change:

            for key, value in self.defaults.__dict__.items():
                if getattr(self, key) is None:
                    setattr(self, key, value)

@Nayjest Nayjest merged commit ab78e59 into main Sep 9, 2025
1 check passed
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.

3 participants