Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

nettacker/core/messages.py (1)

22-24: Consider specifying encoding="utf-8" when loading language YAML files.

load_yaml is used primarily to load translation files under Config.path.locale_dir, which contain non-ASCII characters (e.g., CJK, Arabic, Persian, Cyrillic). Relying on the platform default encoding means this call can fail or decode incorrectly on systems where the default is not UTF-8 (notably Windows with cp1252/cp932/etc.). Pinning the encoding makes behavior deterministic across platforms.

♻️ Proposed change

 def load_yaml(filename):
-    with open(filename, "r") as f:
+    with open(filename, "r", encoding="utf-8") as f:
         return yaml.safe_load(f)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nettacker/core/messages.py` around lines 22 - 24, The load_yaml helper opens
translation YAML files without specifying an encoding, which can mis-decode
non-ASCII locales on platforms whose default encoding isn't UTF-8; update the
load_yaml function to open the file with encoding="utf-8" (i.e., change the
open(...) call in load_yaml to include encoding="utf-8") so translation files
under Config.path.locale_dir load deterministically across platforms.

nettacker/core/arg_parser.py (1)

739-743: This open(...) was missed by the context-manager refactor.

The stated goal of the commit is to fix file-handle leaks across arg_parser.py, but the output-file writability probe at Lines 739–743 still uses a bare open(..., "w") + close(). If open itself raises, the except path is fine, but if any future change between open and close raises, the handle leaks — which is exactly the class of bug the rest of this PR is eliminating. Worth finishing the job here for consistency.

Note also that open(..., "w") truncates the target file as a side effect of the "check" (pre-existing behavior); if you touch this anyway, consider os.access(..., os.W_OK) or opening in "a" mode instead so an existing report isn't wiped just because argument parsing ran.

♻️ Proposed change (minimal, preserves existing truncate-on-check behavior)

-        # Check output file
-        try:
-            temp_file = open(options.report_path_filename, "w")
-            temp_file.close()
-        except Exception:
-            die_failure(_("file_write_error").format(options.report_path_filename))
+        # Check output file
+        try:
+            with open(options.report_path_filename, "w"):
+                pass
+        except Exception:
+            die_failure(_("file_write_error").format(options.report_path_filename))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nettacker/core/arg_parser.py` around lines 739 - 743, The writability probe
currently uses open(options.report_path_filename, "w") followed by close(),
which can leak a file handle if an exception occurs between open and close;
change this to use a context manager (e.g., with
open(options.report_path_filename, "w") as _:) so the file is always closed, and
keep the existing truncate behavior, or alternatively use os.access(...,
os.W_OK) or open in "a" mode if you want to avoid truncation; ensure you still
call die_failure(_("file_write_error").format(options.report_path_filename)) on
exceptions.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

nettacker/database/postgresql.py (1)

29-34: ⚠️ Potential issue | 🟠 Major

Keep the fallback valid and leak-free.

On Line 29, isalnum() still rejects standard names like nettacker_db, and on Line 33 the new ValueError bypasses conn.close(). Please switch to an identifier regex that matches the names you actually support, close the autocommit connection in a finally, and raise the validation error with from None so the traceback isn't reported as another OperationalError.

🔧 Suggested fix

+import re
 ...
-        conn = engine.connect()
-        conn = conn.execution_options(isolation_level="AUTOCOMMIT")
-        db_name = Config.db.name
-        if db_name.isalnum() and db_name[0].isalpha():
-            conn.execute(text(f'CREATE DATABASE "{db_name}"'))
-        else:
-            raise ValueError(f"Invalid database name: {db_name}")
-        conn.close()
+        conn = engine.connect()
+        conn = conn.execution_options(isolation_level="AUTOCOMMIT")
+        try:
+            db_name = Config.db.name
+            if re.fullmatch(r"[A-Za-z_][A-Za-z0-9_]*", db_name):
+                conn.execute(text(f'CREATE DATABASE "{db_name}"'))
+            else:
+                raise ValueError(f"Invalid database name: {db_name}") from None
+        finally:
+            conn.close()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nettacker/database/postgresql.py` around lines 29 - 34, The validation for
Config.db.name in the database creation block is too strict and the connection
may leak on error; replace the isalnum()/isalpha() checks with a regex like
r'^[A-Za-z][A-Za-z0-9_]*$' to allow underscores and standard identifiers, wrap
the conn.execute(text(...)) and any validation in a try/finally that calls
conn.close() in the finally to ensure the autocommit connection is always
closed, and when raising the validation error use raise ValueError(f"Invalid
database name: {db_name}") from None so the traceback doesn't surface as an
OperationalError; adjust the code around db_name, conn.execute, and conn.close()
accordingly.

nettacker/database/mysql.py (1)

23-29: ⚠️ Potential issue | 🟠 Major

Let invalid names fail fast instead of being printed and ignored.

On Line 23, the name check still rejects underscores, and on Line 28 the broad except Exception prints and suppresses the new ValueError. Re-raise validation errors so startup fails fast when the configured database name is invalid.

🔧 Suggested fix

+import re
 ...
-            if db_name.isalnum() and db_name[0].isalpha():
+            if re.fullmatch(r"[A-Za-z_][A-Za-z0-9_]*", db_name):
                 conn.execute(text(f"CREATE DATABASE `{db_name}` "))
             else:
-                raise ValueError(f"Invalid database name: {db_name}")
-    except Exception as e:
-        print(e)
+                raise ValueError(f"Invalid database name: {db_name}") from None
+    except ValueError:
+        raise
+    except Exception as e:
+        print(e)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nettacker/database/mysql.py` around lines 23 - 29, The database-name
validation currently rejects underscores and then a broad except prints and
suppresses the ValueError; update the validation for Config.db.name (db_name) to
allow names starting with a letter and containing letters, digits or underscores
(e.g. use a regex like ^[A-Za-z][A-Za-z0-9_]*$) and when validation fails raise
the ValueError (do not swallow it). Also remove or narrow the broad except
Exception block around the CREATE DATABASE/validation so that ValueError is not
caught and suppressed (if you must catch exceptions, log them and re-raise
rather than only printing).

nettacker/database/db.py (1)

63-72: ⚠️ Potential issue | 🟠 Major

Expand the SQLite allowlist and normalize case before comparing.

On Line 63, the journal-mode whitelist still omits TRUNCATE and PERSIST, and the direct string comparison rejects lowercase config values even though SQLite treats these PRAGMAs case-insensitively. Normalize both values before validating/executing them, and make sure the APSW handles are closed if validation fails.

🔧 Suggested fix

-            allowed_journal_modes = {"DELETE", "WAL", "MEMORY", "OFF"}
+            allowed_journal_modes = {"DELETE", "TRUNCATE", "PERSIST", "MEMORY", "WAL", "OFF"}
             allowed_sync_modes = {"OFF", "NORMAL", "FULL", "EXTRA"}
-            journal_mode = Config.db.journal_mode
-            sync_mode = Config.db.synchronous_mode
+            journal_mode = str(Config.db.journal_mode).upper()
+            sync_mode = str(Config.db.synchronous_mode).upper()
             if journal_mode not in allowed_journal_modes:
-                raise ValueError(f"Invalid journal_mode: {journal_mode}")
+                raise ValueError(f"Invalid journal_mode: {Config.db.journal_mode}")
             if sync_mode not in allowed_sync_modes:
-                raise ValueError(f"Invalid synchronous_mode: {sync_mode}")
+                raise ValueError(f"Invalid synchronous_mode: {Config.db.synchronous_mode}")
             cursor.execute(f"PRAGMA journal_mode={journal_mode}")
             cursor.execute(f"PRAGMA synchronous={sync_mode}")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nettacker/database/db.py` around lines 63 - 72, The journal/sync allowlists
need TRUNCATE and PERSIST added and comparisons should be case-normalized:
update allowed_journal_modes to include "TRUNCATE" and "PERSIST", convert
Config.db.journal_mode and Config.db.synchronous_mode to a normalized form (e.g.
.upper()) before validating and use the normalized value when calling
cursor.execute(f"PRAGMA journal_mode={...}") and cursor.execute(f"PRAGMA
synchronous={...}"); additionally, if validation fails make sure to close any
APSW handles (e.g. cursor and the connection object) before raising the
ValueError to avoid leaking resources.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes