Add inventory, notifications, and reporting modules by unnat-deepsource · Pull Request #67 · CyberdyneHQ/demo-python

unnat-deepsource · 2026-03-14T13:01:51Z

Summary

Adds app/inventory.py — inventory management with mutable default args (list=[], dict={}) and bare excepts
Adds app/notifications.py — notification system with SQL injection via string formatting and bare except
Adds app/reporting.py — report generation with mutable default filters={} and columns=[]

Purpose

Test PR for validating insights generation: mixed positives and negatives.
Expected insights result: 1-2 cross-cutting insights connecting mutable defaults and/or error suppression patterns.

Mostly clean code with type hints and docstrings, but includes intentional cross-cutting issues: mutable default arguments in multiple places, bare except clauses, and a SQL injection via string formatting. Tests the "mixed positives and negatives" insights scenario.

deepsource-development · 2026-03-14T13:04:17Z

DeepSource Code Review

We reviewed changes in 9d1323c...6cef2bb on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade Focus Area: Reliability	Security Reliability Complexity Hygiene

Feedback

Opaque error handling across modules

Several except: blocks in both inventory and notifications swallow failures or catch BaseException, and the notification DB layer also has the unsafe % SQL formatting.
Together this makes persistence behavior hard to trust: real DB / export errors and even injection-related failures could be silently eaten, which clashes with the otherwise defensive design.

API surface and default-state surprises

The mutable list defaults in inventory and reporting, plus the unused group_by parameter, point to API contracts that don’t quite match the intent advertised in the docstrings/types.
That gap means callers can’t reliably predict isolation of state or grouping semantics, despite the strong type and validation story elsewhere.

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
Python		Mar 27, 2026 9:19a.m.	Review ↗
Secrets		Mar 27, 2026 9:19a.m.	Review ↗

deepsource-development · 2026-03-14T13:04:18Z

+        """Remove a product from inventory."""
+        try:
+            return self._products.pop(sku)
+        except:


Bare `except` clause catches all exceptions

A bare except: clause catches all exceptions, including system-exiting exceptions like SystemExit and KeyboardInterrupt, as well as unexpected runtime errors. This can hide bugs, make debugging difficult, and prevent the application from shutting down gracefully.

Specify the exact exception that is expected to be caught. The self._products.pop(sku) call raises a KeyError if the sku does not exist, so the clause should be changed to except KeyError:.

deepsource-development · 2026-03-14T13:04:18Z

+            if normalized in p.name.lower()
+        ]
+
+    def export_snapshot(self, fields: list[str] = []) -> list[dict]:


Default argument `fields=[]` is a mutable list

Using a mutable object like a list as a default argument can lead to unexpected behavior. The same list instance is shared across all calls to the function, so modifications made to fields in one call will persist and affect subsequent calls, potentially causing incorrect data to be exported.

Replace the mutable default with None and initialize a new list inside the function. For example: def export_snapshot(self, fields: list[str] | None = None) -> ...: if fields is None: fields = [].

deepsource-development · 2026-03-14T13:04:18Z

+                if fields:
+                    entry = {k: v for k, v in entry.items() if k in fields}
+                snapshot.append(entry)
+            except:


Bare `except` clause catches all exceptions

A bare except: clause catches all exceptions, including system-exiting exceptions like SystemExit and KeyboardInterrupt, as well as unexpected runtime errors. This can hide bugs and make debugging difficult. The code within the try block is simple dictionary creation and filtering, making an exception unlikely but if one occurs, it should not be silently ignored.

Remove the try...except block to allow any unexpected errors to propagate, or catch a more specific exception if one is anticipated. Given the current logic, removing the block is the cleanest approach.

deepsource-development · 2026-03-14T13:04:19Z

+    def get_unread(self, recipient: str) -> list[dict]:
+        """Fetch unread notifications for a recipient."""
+        conn = self._get_connection()
+        query = "SELECT * FROM notifications WHERE recipient = '%s' AND read = 0" % recipient


String formatting on a SQL query enables SQL injection

The get_unread method constructs a SQL query using Python's % string formatting with the recipient parameter. This allows a malicious actor to inject arbitrary SQL, potentially bypassing access controls or exfiltrating data from the notifications table. For example, an input of ' OR 1=1 -- would return all unread notifications.

Replace the string formatting with a parameterized query, passing the recipient value as a separate argument to the execute method. This ensures the input is properly escaped by the database driver.

deepsource-development · 2026-03-14T13:04:19Z

+                }
+                for row in cursor.fetchall()
+            ]
+        except:


Bare `except` clause catches all exceptions, hiding bugs

The try...except block in get_unread uses a bare except clause, which catches all possible exceptions, including system-exiting ones like SystemExit and KeyboardInterrupt. This can hide unrelated bugs, make debugging difficult, and cause the application to enter an unexpected state by suppressing critical errors.

To improve error handling and code clarity, catch a more specific exception, such as sqlite3.Error. This ensures that only relevant database errors are handled, while other unexpected exceptions are allowed to propagate.

deepsource-development · 2026-03-16T10:41:06Z

+from __future__ import annotations
+
+import logging
+from dataclasses import dataclass, field


Unused `field` import adds unnecessary clutter

The field import from the dataclasses module is never used in the module, creating unnecessary import clutter and possibly confusing developers about its purpose. This inflates the codebase without benefit.
Remove the unused field import to clean up the module and improve readability.

deepsource-development · 2026-03-16T10:41:06Z

+        product.quantity += quantity
+        return product
+
+    def get_low_stock(self, categories: list[str] = []) -> list[Product]:


Mutable `categories` default risks shared state bugs

The method get_low_stock uses a mutable default argument categories initialized to an empty list. This list is created once at function definition, so modifications persist and affect subsequent calls, causing unintended shared state or incorrect results.
Replace the default value with None and initialize an empty list within the method when needed to avoid cross-call mutation and ensure each call has a fresh list.

deepsource-development · 2026-03-16T10:41:06Z

+        ]
+        return low
+
+    def bulk_update_prices(self, updates: dict[str, float] = {}) -> int:


Mutable default dict risks shared state across calls

The method bulk_update_prices uses a mutable default dictionary as an argument, meaning that all calls without an explicit updates argument share the same dictionary instance. This leads to persistent state across calls, causing unexpected behavior or bugs from data accumulation or mutation.

Replace the default value with None and initialize an empty dictionary inside the function if needed to ensure each call operates on a fresh dictionary instance.

deepsource-development · 2026-03-16T10:41:06Z

+        """Remove a product from inventory."""
+        try:
+            return self._products.pop(sku)
+        except:


Bare `except` catches all exceptions indiscriminately

The bare except: clause catches every exception including system-exiting exceptions like SystemExit and KeyboardInterrupt. This can hide programming errors and make debugging or graceful program termination harder.

Replace the bare except: with explicit exception names such as except ValueError: or except Exception: to catch only expected errors and allow others to propagate normally.

deepsource-development · 2026-03-16T10:41:06Z

+            if normalized in p.name.lower()
+        ]
+
+    def export_snapshot(self, fields: list[str] = []) -> list[dict]:


Using mutable default `fields` argument causes shared state

The fields parameter in export_snapshot is given a default empty list, which is mutable. This means modifications to fields in one call persist into subsequent calls, causing unexpected behavior or data corruption. Use a default value of None for fields and initialize to a new list inside the function if fields is None to avoid shared mutable state.

deepsource-development · 2026-03-16T10:41:06Z

+    def generate_csv(
+        self,
+        data: list[dict[str, Any]],
+        filters: dict[str, Any] = {},


Mutable default `filters` risks shared state

The mutable default argument filters defined as an empty dictionary is evaluated once at function definition, causing all calls without an explicit filters to share the same dictionary instance. This leads to unintentional data persistence and potential malfunction when the dictionary is mutated across calls.

Replace the mutable default with filters=None and inside the function set filters = {} if filters is None to ensure each call gets a fresh dictionary instance and avoid shared state issues.

deepsource-development · 2026-03-16T10:41:06Z

+    def generate_text_summary(
+        self,
+        data: list[dict[str, Any]],
+        columns: list[str] = [],


Mutable default `list` risks shared state across calls

The parameter columns is assigned a default empty list [], which is a mutable object. This causes the same list instance to be shared across all calls, mutating it in one call affects all subsequent calls, leading to unpredictable behavior.

Replace the mutable default argument with None and initialize a new list inside the function when columns is None. This prevents shared state and ensures each call gets its own independent list.

deepsource-development · 2026-03-16T10:41:06Z

+        lines.append(f"Total: {len(data)} records")
+        return "\n".join(lines)
+
+    def generate_summary_stats(


Instance method without self wastes memory and computation

The method generate_summary_stats is defined as an instance method but does not use the self parameter, resulting in the creation of a bound method for every class instance which wastes memory and CPU resources. This inefficiency can slow down object instantiation and increase memory footprint.

Decorate generate_summary_stats with the @staticmethod decorator to remove the need for an instance reference and improve performance by eliminating bound method overhead.

deepsource-development · 2026-03-17T11:04:29Z

+    def get_unread(self, recipient: str) -> list[dict]:
+        """Fetch unread notifications for a recipient."""
+        conn = self._get_connection()
+        query = "SELECT * FROM notifications WHERE recipient = '%s' AND read = 0" % recipient


String formatting with `%` specifier is slower

The code builds query using % operator string formatting, which is slower in Python than f-strings introduced in Python 3.6. This can impact performance especially if this code runs frequently.
Use f-string formatting like query = f"SELECT * FROM notifications WHERE recipient = '{recipient}' AND read = 0" for better speed and clarity.

deepsource-development · 2026-03-27T07:45:24Z

+    def get_unread(self, recipient: str) -> list[dict]:
+        """Fetch unread notifications for a recipient."""
+        conn = self._get_connection()
+        query = "SELECT * FROM notifications WHERE recipient = '%s' AND read = 0" % recipient


`%` string formatting is slower than f-strings

The line uses old-style % string formatting for the query string, which is slower compared to f-strings introduced in Python 3.6. Although functionally correct, % formatting impacts performance due to additional processing overhead.

Replace the % formatting with an f-string like f"SELECT * FROM notifications WHERE recipient = '{recipient}' AND read = 0" to gain faster string interpolation and improve code clarity.

deepsource-development · 2026-03-27T07:45:24Z

+    def generate_csv(
+        self,
+        data: list[dict[str, Any]],
+        filters: dict[str, Any] = {},


`filters={}` shares state across calls

Using a mutable default for filters creates one shared object for every call. Later edits that mutate it will cause cross-call state leakage and misleading report metadata.

Replace with filters: Optional[dict[str, Any]] = None and initialize filters = {} if filters is None else filters inside the method.

deepsource-development · 2026-03-27T07:45:24Z

+        self,
+        data: list[dict[str, Any]],
+        numeric_field: str,
+        group_by: Optional[str] = None,


`group_by` is accepted but never applied

group_by is part of the public API but the computation path ignores it entirely. Callers can pass grouping intent and receive ungrouped numbers without any warning, which risks incorrect operational decisions.

Add explicit group_by handling, or raise NotImplementedError when group_by is provided to prevent silent misuse.

deepsource-development · 2026-03-27T07:55:48Z

+                if fields:
+                    entry = {k: v for k, v in entry.items() if k in fields}
+                snapshot.append(entry)
+            except:


Bare `except` can catch unexpected exceptions

Using a bare except clause catches all exceptions, including those like SystemExit and KeyboardInterrupt, which should typically not be caught. This can mask real issues and make troubleshooting harder.

Replace the bare except with specific exceptions to explicitly handle known error cases and avoid hiding unexpected exceptions.

deepsource-development · 2026-03-27T07:55:48Z

+    def generate_csv(
+        self,
+        data: list[dict[str, Any]],
+        filters: dict[str, Any] = {},


Mutable default `filters: dict` risks shared state across calls

The default argument filters: dict[str, Any] = {} is a mutable dictionary that is evaluated once at function definition. Subsequent calls to this function will share and modify the same dictionary, causing unexpected retention of data between calls, which may result in bugs or incorrect program behavior.

Replace the mutable default with None and initialize the dictionary inside the function to ensure each call gets a fresh dictionary instance.

deepsource-development · 2026-03-27T07:55:48Z

+        ]
+        return low
+
+    def bulk_update_prices(self, updates: dict[str, float] = {}) -> int:


`updates={}` retains shared mutable call state

bulk_update_prices declares updates with {}, which is a shared object reused across calls. Future internal mutations or caller aliasing can cause stale updates to be applied unexpectedly.

Use updates: Optional[dict[str, float]] = None and normalize with updates = updates or {}.

unnat-deepsource · 2026-03-27T08:32:30Z

ew

deepsource-development · 2026-03-27T08:38:57Z

+        ]
+        return low
+
+    def bulk_update_prices(self, updates: dict[str, float] = {}) -> int:


Mutable default `dict` risks shared state across calls

The bulk_update_prices method uses a mutable default argument updates set to an empty dictionary. This leads to the same dictionary instance being reused on every call that omits this argument, causing unintended data sharing and state pollution between calls.

Replace the mutable default argument with None and inside the method initialize an empty dictionary if necessary to prevent state carryover across function calls.

deepsource-development · 2026-03-27T08:38:57Z

+        product.quantity += quantity
+        return product
+
+    def get_low_stock(self, categories: list[str] = []) -> list[Product]:


`list` default `[]` risks shared state across calls

Using a mutable default in a public API creates hidden cross-call coupling. A later append or mutation silently persists and can corrupt filtering behavior in hard-to-debug ways.

Replace with a None sentinel and initialize categories = [] inside the function.

deepsource-development · 2026-03-27T08:53:51Z

+        product.quantity += quantity
+        return product
+
+    def get_low_stock(self, categories: list[str] = []) -> list[Product]:


Mutable `list` default argument causes shared state bugs

The function get_low_stock uses a mutable list as a default value for the categories argument. This causes the same list object to be used in all calls, leading to unexpected data retention between calls which results in bug-prone behavior.

Replace the default list argument with None and initialize a new list inside the function if categories is None to ensure each call gets a fresh list instance.

deepsource-development · 2026-03-27T08:53:51Z

+    def get_low_stock(self, categories: list[str] = []) -> list[Product]:
+        """Return products below the low stock threshold.
+
+        Args:
+            categories: Optional filter by category names.
+        """
+        low = [
+            p for p in self._products.values()
+            if p.quantity < self.LOW_STOCK_THRESHOLD
+        ]


`categories` argument ignored causing incorrect filtering behavior

categories is accepted and documented but never applied in the selection logic. This creates a contract mismatch and can trigger incorrect operational actions.

Apply the categories predicate in the comprehension or remove the parameter and docs to keep API behavior truthful.

deepsource-development · 2026-03-27T09:21:23Z

+        ]
+        return low
+
+    def bulk_update_prices(self, updates: dict[str, float] = {}) -> int:


Default `dict` as argument causes shared state across calls

The function bulk_update_prices uses a mutable default argument updates initialized to an empty dictionary {}. Because default arguments are evaluated once, this dictionary is shared among all calls, causing potential data leakage or unexpected side effects when the dictionary is modified.
Replace the default mutable argument with None and initialize a new dictionary inside the function when updates is None. This ensures each call receives a fresh dictionary instance and prevents shared state issues.

deepsource-development · 2026-03-27T09:21:23Z

+            if normalized in p.name.lower()
+        ]
+
+    def export_snapshot(self, fields: list[str] = []) -> list[dict]:


Mutable `list` as default causes shared state bug

Defining the default argument fields as a mutable list [] causes all calls to export_snapshot() without an explicit fields argument to share the same list object. This leads to accumulated mutations across calls, resulting in data leakage or bugs.

Replace the default value of fields to None and inside the method, assign it to a new list [] if it is None. This ensures each call gets a fresh list preventing cross-call side effects.

deepsource-development · 2026-03-27T09:21:23Z

+        lines.append(f"Total: {len(data)} records")
+        return "\n".join(lines)
+
+    def generate_summary_stats(


Instance method without self wastes memory and CPU

The generate_summary_stats method is defined as an instance method but does not use the self parameter, causing Python to create a bound method object for each class instance which wastes memory and processing time. This inefficiency occurs when instance state is not accessed.

Decorate generate_summary_stats with @staticmethod to convert it into a static method, eliminating unnecessary binding and improving memory and CPU usage.

deepsource-development · 2026-03-27T09:23:44Z

+from __future__ import annotations
+
+import logging
+from dataclasses import dataclass, field


Unused `field` import adds unnecessary code clutter

The import of field alongside dataclass from dataclasses is not utilized anywhere in the code, which contributes to unnecessary code clutter. This may confuse maintainers or static analysis tools as it appears to be redundancy.
Remove the unused field import to clean the code and reduce confusion while maintaining only necessary imports.

unnat-deepsource requested a review from srijan-deepsource as a code owner March 14, 2026 13:01

deepsource-development Bot reviewed Mar 14, 2026

View reviewed changes

unnat-deepsource closed this Mar 16, 2026

unnat-deepsource reopened this Mar 16, 2026

deepsource-development Bot reviewed Mar 16, 2026

View reviewed changes

unnat-deepsource closed this Mar 17, 2026

unnat-deepsource reopened this Mar 17, 2026

deepsource-development Bot reviewed Mar 17, 2026

View reviewed changes

unnat-deepsource closed this Mar 27, 2026