fix(many): issues reported from deepsource - #164
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR hardens archive extraction and unpickling, introduces soft-delete task semantics in the server, converts many instance methods to static where appropriate, updates test ordering to pytest-dependency, and adds/rewrites server docs and env configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server as PSSM Server
participant DB as Database
participant FS as File System
User->>Server: DELETE /tasks/{id}
Server->>DB: SELECT task row
DB-->>Server: task (status, files)
Server->>Server: compute deleted status ("deleted:finshed"/"deleted:cancel"), finished_at, walltime
Server->>DB: UPDATE task status (soft-delete)
DB-->>Server: update OK
Server->>FS: retain task files (no removal)
FS-->>Server: files retained
Server-->>User: 204 No Content
sequenceDiagram
participant Client
participant Extractor as extract_archive()
participant Archive as Archive File
participant Validator as _safe_member_target()
participant FS as File System
participant Err as Error Handler
Client->>Extractor: extract(archive, dest)
Extractor->>Archive: iterate members
Archive-->>Extractor: member path "../evil.txt"
Extractor->>Validator: validate(member path)
Validator->>Validator: check absolute / "../" / canonicalize under dest
alt Unsafe path
Validator-->>Err: raise ValueError("Unsafe archive member path")
Err-->>Client: extraction failed (exception)
else Safe path
Validator->>FS: create dirs & write file
FS-->>Extractor: written
Extractor-->>Client: extraction success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
 |
|
Overall Grade Focus Area: Reliability |
Security Reliability  Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Feb 24, 2026 6:54a.m. | Review ↗ |
❌ 3 blocking issues (16 total)
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
|
|
||
| def _build_uniqueid_to_atom_map(self, cmd_module) -> dict[int, AtomDescriptor]: | ||
| @staticmethod | ||
| def _build_uniqueid_to_atom_map(cmd_module) -> dict[int, AtomDescriptor]: |
|
|
||
| import importlib | ||
| import io | ||
| import pickle |
| "set": set, | ||
| "slice": slice, | ||
| "str": str, | ||
| "tuple": tuple, |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 535e7f2f21
ℹ️ About Codex in GitHub
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".
| task_store.update_task( | ||
| md5sum, | ||
| status=deleted_status, |
There was a problem hiding this comment.
Switching deletion to task_store.update_task(... status=deleted:...) leaves the row in place, but run_gremlin_task still performs unconditional update_task writes to packing results/finished later in its execution. If a user deletes a running task and Celery revoke arrives too late (or revoke fails), those later writes overwrite the deleted status and bring the task back with inconsistent state (artifacts were already removed), which is a regression from the previous hard-delete behavior where late worker writes became no-ops.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/REvoDesign/shortcuts/tools/esm2.py (1)
331-331:⚠️ Potential issue | 🟡 MinorPre-existing off-by-one in
compute_ppplsilently drops the last two residues from the PPPL sum.
batch_tokenshas shape(1, len(sequence) + 2)— BOS at index 0, EOS at indexlen(sequence) + 1, and amino-acid tokens at indices1 … len(sequence)inclusive. The current rangerange(1, len(sequence) - 1)stops atlen(sequence) - 2, omitting the last two residue positions from the log-probability accumulation. This causes the pseudo-perplexity score to be systematically underestimated for every sequence.🐛 Proposed fix
- for i in range(1, len(sequence) - 1): + for i in range(1, len(sequence) + 1):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/shortcuts/tools/esm2.py` at line 331, The loop in compute_pppl incorrectly iterates with range(1, len(sequence) - 1), which omits the last two residue tokens when accumulating log-probabilities from batch_tokens (shape (1, len(sequence)+2), residues at indices 1..len(sequence)); change the loop to iterate over the full residue index range (e.g., range(1, len(sequence)+1)) so you include the final residue but still exclude EOS at len(sequence)+1, and keep the rest of the PPPL accumulation logic intact in compute_pppl.src/REvoDesign/magician/designers/cart_ddg.py (1)
90-95:⚠️ Potential issue | 🟠 Major
self.initializedis never set toTruewheninitialize()returns early.When
self.relaxed_pdbalready exists (line 90–91), the method returns before line 95. Any caller that guards onself.initialized(which defaults toFalseat line 57) will incorrectly conclude that initialization has not occurred, potentially triggering a redundant re-initialization cycle.🐛 Proposed fix
# skip relax if it has been done if isinstance(self.relaxed_pdb, str) and os.path.isfile(self.relaxed_pdb) and not self.reload: + self.initialized = True return logging.info(f"Relaxing {self.molecule} ...") self.relaxed_pdb = self.ddg_runner.relax(nstruct_relax=self.relax_nstruct) self.initialized = True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/magician/designers/cart_ddg.py` around lines 90 - 95, The initialize() method exits early when the existing relaxed PDB is found, but never sets self.initialized to True; update initialize() so that when the condition checking isinstance(self.relaxed_pdb, str) and os.path.isfile(self.relaxed_pdb) and not self.reload is true, you set self.initialized = True before returning (or set it immediately after the early-return check), ensuring callers that check self.initialized see initialization completed; locate this logic in the initialize() implementation of the CartDDG designer (references: self.relaxed_pdb, self.reload, self.initialized, and self.ddg_runner.relax) and make the minimal change to mark initialization complete on the early-return path.src/REvoDesign/common/mutant_tree.py (1)
39-52:⚠️ Potential issue | 🟡 MinorUse explicit
Nonecheck to preserve caller-provided empty dicts.
mutant_tree or {}replaces any falsy value (including an empty dict{}passed by the caller) with a new instance, breaking reference identity. Use an explicitNonecheck instead.💡 Suggested change
- self.mutant_tree = mutant_tree or {} + self.mutant_tree = mutant_tree if mutant_tree is not None else {}Additionally, ensure
pre-commit run --all-filesormake blackis executed before pushing to satisfy the project's pre-commit hook requirements for Python files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/common/mutant_tree.py` around lines 39 - 52, The constructor MutantTree.__init__ currently uses "mutant_tree or {}" which replaces caller-provided empty dicts and breaks identity; change the assignment to explicitly check for None (e.g., if mutant_tree is None then set self.mutant_tree = {} else self.mutant_tree = mutant_tree) so callers' empty dicts are preserved, and then run the project's pre-commit/black (pre-commit run --all-files or make black) before pushing.src/REvoDesign/tools/package_manager.py (2)
1496-1512:⚠️ Potential issue | 🟡 MinorStale
selfreference inget_existing_directorydocstring.The docstring at line 1503 still documents
selfas a parameter, but the method is now a@staticmethod.📝 Proposed fix
`@staticmethod` def get_existing_directory(): """ Opens a dialog for the user to select an existing directory. - Parameters: - - self: The instance of the class this method is called on. - Returns: - str: The path of the selected directory. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/tools/package_manager.py` around lines 1496 - 1512, The get_existing_directory static method's docstring incorrectly documents a self parameter; update the docstring for get_existing_directory to remove any mention of self and adjust the "Parameters" section (or delete it) so it reflects that the method is static and returns a str path of the selected directory, keeping the description and return info accurate for the static method.
2399-2412:⚠️ Potential issue | 🟡 Minor
json.JSONDecodeErrorandTypeError/KeyErrorare unhandled after the_read_https_urlrefactor.The
exceptclauses only catchHTTPErrorandURLError. Two gaps remain:
json.loads(response_data)raisesjson.JSONDecodeErrorif a proxy or intermediary returns a non-JSON body with a 200 status code.[tag["name"] for tag in tags]raisesTypeErrorif the API returns a JSON object ({"message": "..."}) instead of a list, orKeyErrorif an entry is missing the"name"key.🛡️ Proposed fix
try: response_data = _read_https_url(api_url).decode() - # Parse JSON response data - tags = json.loads(response_data) - # Extract the name of each tag - tag_names = [tag["name"] for tag in tags] - return tag_names + tags = json.loads(response_data) + if not isinstance(tags, list): + logging.warning("Unexpected GitHub tags response format") + return [] + return [tag["name"] for tag in tags if isinstance(tag, dict) and "name" in tag] except HTTPError as e: logging.warning(f"GitHub API returned status code {e.code}") return [] except URLError as e: logging.error(f"Failed to reach the server. Reason: {e.reason}") return [] + except (json.JSONDecodeError, ValueError) as e: + logging.error(f"Failed to parse GitHub API response: {e}") + return []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/tools/package_manager.py` around lines 2399 - 2412, The code calling _read_https_url decodes and JSON-parses response_data and then builds tag_names but only catches HTTPError and URLError; update the try/except around json.loads(response_data) and the list comprehension that builds tag_names to also handle json.JSONDecodeError, TypeError, and KeyError by logging an informative message and returning an empty list. Specifically, after obtaining response_data from _read_https_url, validate that json.loads(response_data) produces a list (assign to tags), and when building tag_names (the list comprehension using tag["name"]) either guard for missing "name" keys or catch KeyError/TypeError and log the failure; ensure all new exception handlers use the existing logging style and return [] on error.tests/cases/tabs/test_mutate.py (1)
215-216:⚠️ Potential issue | 🟡 MinorWrong variable in
os.remove— pre-existing correctness bug.The guard checks
ddg_design_psebut then removesddg_design_case. The PSE cleanup never fires; instead, the test inadvertently deletes the design-case file. Every analogous cleanup block in this file follows the consistent pattern of checking and removing the same variable.🐛 Proposed fix
if os.path.exists(test_worker.test_data.ddg_design_pse): - os.remove(test_worker.test_data.ddg_design_case) + os.remove(test_worker.test_data.ddg_design_pse)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cases/tabs/test_mutate.py` around lines 215 - 216, The cleanup guard is checking test_worker.test_data.ddg_design_pse but erroneously calls os.remove on test_worker.test_data.ddg_design_case; change the os.remove call to remove the same variable being checked (test_worker.test_data.ddg_design_pse) so the PSE file is removed and the design-case file is not deleted by this block; update the os.remove call in the block that references ddg_design_pse to use ddg_design_pse.src/REvoDesign/shortcuts/tools/exports.py (1)
104-116:⚠️ Potential issue | 🟡 MinorStale docstring —
chain_idsdefault changed from[]toNonebut the description was not updated.📝 Proposed fix
- chain_ids (list[str]): List of chain IDs to dump. Defaults to []. + chain_ids (list[str] | None): List of chain IDs to dump. Defaults to None; falls back to the chain selected in the UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/shortcuts/tools/exports.py` around lines 104 - 116, The docstring for the function (parameters: chain_ids, output_dir, drop_missing_residue, suffix) is stale: it still says chain_ids defaults to [] while the signature uses None; update the docstring to reflect the actual default and behavior (e.g., "chain_ids (list[str] | None): List of chain IDs to dump; if None, all chains will be processed. Defaults to None."). Edit the docstring near the function that calls dump_fasta_from_struct to replace the incorrect default description and, if relevant, note that None means "use all chains" or the actual behavior implemented in dump_fasta_from_struct.
🧹 Nitpick comments (9)
src/REvoDesign/magician/__init__.py (1)
115-115: Consider calling the static method through the class, not the instance.After the
@staticmethodconversion,self.magician_assistant.get(...)works but is misleading — it implies an instance is required. The idiomatic form is:♻️ Proposed refactor
- self.gimmick = self.magician_assistant.get(name=name, **kwargs) + self.gimmick = MagicianAssistant.get(name=name, **kwargs)Additionally, the
self.magician_assistantinstance created at line 71 appears to be unnecessary—theinstalled_workerfield is never accessed, and the only usage is the static method call which doesn't require an instance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/magician/__init__.py` at line 115, Replace the instance call to the static method with a class call and remove the unused instance: change the call site that uses self.magician_assistant.get(name=..., **kwargs) to call the static method on the class (MagicianAssistant.get(...)) and delete the now-unused instance field (the self.magician_assistant creation and the unused installed_worker field referenced at the creation around line 71), ensuring any imports or references to MagicianAssistant are present; confirm no other code paths rely on the instance before removing it.src/REvoDesign/tools/customized_widgets.py (1)
614-616: Unused parameters in@staticmethod– consider prefixing with_to silence ARG004 warnings.
col_name,row, andcolare never referenced in the body; they exist only to define the interface for subclass overrides. Ruff flags all three as ARG004. Additionally,QButtonMatrixGremlin.get_WT_label(line 785) overrides this as a regular instance method (withself), creating a base-class/subclass inconsistency: the base is@staticmethod, the subclass is an instance method. Python's descriptor protocol keeps it functional at runtime (self.get_WT_label(...)dispatches correctly via MRO), but the asymmetry is a maintainability concern.Prefix the unused params with
_to silence the linter without removing them (they still define the expected override signature):♻️ Proposed fix
`@staticmethod` -def get_WT_label(row_name: str, col_name: str, row: int, col: int) -> str: +def get_WT_label(row_name: str, _col_name: str, _row: int, _col: int) -> str: return row_name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/tools/customized_widgets.py` around lines 614 - 616, The base-class method get_WT_label is declared as `@staticmethod` and has unused params causing ARG004 and a signature mismatch with QButtonMatrixGremlin.get_WT_label (an instance method); change the base method to an instance method (remove `@staticmethod` and add self) and rename unused parameters to _col_name, _row, and _col so the linter is satisfied while preserving the override signature with QButtonMatrixGremlin.get_WT_label; keep the body returning row_name unchanged.src/REvoDesign/magician/designers/colabdesign.py (1)
57-62: Optional: encapsulate the error message to satisfy Ruff TRY003.If Ruff TRY003 is enforced, prefer moving the message into a custom exception class to avoid the lint warning.
♻️ Suggested refactor
+class MissingPdbFileError(FileNotFoundError): + def __init__(self, path: str): + super().__init__(f"Input pdb file does not exist: {path}") + class ColabDesigner_MPNN(ExternalDesignerAbstract): @@ - if not os.path.exists(self.pdb_filename): - raise FileNotFoundError(f"Input pdb file does not exist: {self.pdb_filename}") + if not os.path.exists(self.pdb_filename): + raise MissingPdbFileError(self.pdb_filename)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/magician/designers/colabdesign.py` around lines 57 - 62, Create a custom exception class (e.g., InputPDBNotFound) in the same module that accepts the missing filename and implements __str__ (or a class-level message) to produce the error text, then replace the direct FileNotFoundError raise with raise InputPDBNotFound(self.pdb_filename); locate the check around make_temperal_input_pdb / self.pdb_filename and the mpnn_model.prep_inputs call and update the raise there to use the new exception to satisfy Ruff TRY003.src/REvoDesign/magician/designers/cart_ddg.py (1)
134-134: Uselogging.warning()instead ofprint().This is inconsistent with the rest of the file (e.g.,
logging.infoon line 92) and bypasses any configured log routing or suppression.- print(f"Warning: No ddG value found for {ddg_mut_id}") + logging.warning(f"No ddG value found for {ddg_mut_id}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/magician/designers/cart_ddg.py` at line 134, Replace the plain print call that emits "Warning: No ddG value found for {ddg_mut_id}" with a logging.warning call so the message goes through the module's logging configuration; locate the occurrence that references ddg_mut_id in cart_ddg.py and change print(f"Warning: No ddg value found for {ddg_mut_id}") to logging.warning("No ddG value found for %s", ddg_mut_id) (or equivalent formatted logging) to preserve structured logging and avoid bypassing configured handlers.src/REvoDesign/tools/package_manager.py (1)
70-88: Rename unused*args/**kwargsto*_args/**_kwargsto silence ARG004 warnings.All five
MockLoggerstatic methods accept*args, **kwargsfor API compatibility withlogging, but Ruff ARG004 flags them as unused. Using a leading underscore suppresses the warning without changing behavior.♻️ Proposed fix
- def debug(msg: str, *args, **kwargs): + def debug(msg: str, *_args, **_kwargs): print(f"[DEBUG]: {msg}") if LOGGER_LEVEL < 10 else None - def info(msg: str, *args, **kwargs): + def info(msg: str, *_args, **_kwargs): print(f"[INFO]: {msg}") if LOGGER_LEVEL < 20 else None - def warning(msg: str, *args, **kwargs): + def warning(msg: str, *_args, **_kwargs): print(f"[WARNING]: {msg}") if LOGGER_LEVEL < 30 else None - def error(msg: str, *args, **kwargs): + def error(msg: str, *_args, **_kwargs): print(f"[ERROR]: {msg}") if LOGGER_LEVEL < 40 else None - def critical(msg: str, *args, **kwargs): + def critical(msg: str, *_args, **_kwargs): print(f"[CRITICAL]: {msg}") if LOGGER_LEVEL < 50 else None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/tools/package_manager.py` around lines 70 - 88, The MockLogger static methods (debug, info, warning, error, critical) currently accept unused parameters named *args and **kwargs which trigger ARG004; rename those parameters to *_args and **_kwargs in each method signature to suppress the linter while keeping API compatibility and behavior unchanged, ensuring each method still references LOGGER_LEVEL exactly as before.pyproject.toml (1)
179-179:pytest-dependencyis unpinned — minor reproducibility concern.Other entries in the same block use version specifiers (
pytest<=8.3.3,pytest-cov<=6.0.0, etc.). Whilepytest-orderandpytest-emojiare also unpinned, adding a lower-bound or exact pin forpytest-dependencywould improve reproducibility given it now gates the entire dependency-ordered test chain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 179, Pin the pytest-dependency package in the pyproject.toml dependencies block (the same section that contains pytest<=8.3.3, pytest-cov<=6.0.0, pytest-order, pytest-emoji) to improve reproducibility; update the "pytest-dependency" entry to a version specifier consistent with the file's style (e.g., add a compatible upper-bound or exact pin like == or <=/>= range) so that test ordering behavior is deterministic across environments. Ensure you modify the existing "pytest-dependency" token rather than adding a duplicate entry.src/REvoDesign/tools/utils.py (1)
614-618: Self-caughtraisein_safe_member_target— the explicitraiseon line 616 is immediately caught by theexcept ValueErroron line 617 and re-raised, producing an unnecessary exception chain where both__cause__and the outer exception carry the same message.Split the
os.path.commonpathcall to separate the "platform-level ValueError" case from the "traversal detected" case:♻️ Proposed refactor
- try: - if os.path.commonpath([base_path, target_path]) != base_path: - raise ValueError(f"Unsafe archive member path: {member_name}") - except ValueError as exc: - raise ValueError(f"Unsafe archive member path: {member_name}") from exc + try: + common = os.path.commonpath([base_path, target_path]) + except ValueError as exc: + # os.path.commonpath raises ValueError on Windows when paths are on different drives + raise ValueError(f"Unsafe archive member path: {member_name}") from exc + if common != base_path: + raise ValueError(f"Unsafe archive member path: {member_name}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/tools/utils.py` around lines 614 - 618, The current _safe_member_target implementation raises a ValueError when detecting path traversal and then immediately catches and re-raises it, creating an unnecessary self-caused exception chain; change the logic to call os.path.commonpath(base_path, target_path) inside a try/except that only handles platform-level ValueError (store that as a distinct error), and separately check the traversal condition (if commonpath != base_path) to raise a fresh ValueError with the traversal message using member_name — this removes the pointless except-from-self chaining and preserves clear, distinct errors for platform issues vs. unsafe member paths.src/REvoDesign/tools/safe_pickle.py (1)
1-10: Reminder to run pre-commit/black on Python changes.
Please ensure hooks are installed and lint/formatting are run before pushing.
As per coding guidelines, Enable pre-commit hooks withpre-commit installand runpre-commit run --all-filesormake blackbefore pushing to ensure formatting and linting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/tools/safe_pickle.py` around lines 1 - 10, The file safe_pickle.py needs to be formatted/linted per project hooks; install and run the pre-commit hooks and black formatter before pushing: run `pre-commit install` (once), then `pre-commit run --all-files` or `make black` to format imports and code in module safe_pickle.py (and any other Python files), fix any reported lint errors, and re-commit the results; ensure CI/local checks pass before updating the PR.src/REvoDesign/clients/QtSocketConnector.py (1)
1-5: Reminder to run pre-commit/black on Python changes.
Please ensure hooks are installed and lint/formatting are run before pushing.
As per coding guidelines, Enable pre-commit hooks withpre-commit installand runpre-commit run --all-filesormake blackbefore pushing to ensure formatting and linting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/clients/QtSocketConnector.py` around lines 1 - 5, Run the project's pre-commit hooks and formatter on your Python changes (specifically the new/modified src/REvoDesign/clients/QtSocketConnector.py) before pushing: install hooks with `pre-commit install` if not already installed, then run `pre-commit run --all-files` (or `make black`) to apply linting/formatting fixes and commit the resulting changes; ensure the updated, formatted QtSocketConnector.py is included in the commit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/.env.example`:
- Around line 8-16: The .env example contains developer-specific absolute paths;
replace the concrete values for SERVER_DIR, LOG_DIR, DB_UNIREF30, DB_UNIREF90
and USERS_FILE with clearly labeled placeholders (e.g., REPLACE_WITH_SERVER_DIR,
REPLACE_WITH_LOG_DIR, REPLACE_WITH_DB_UNIREF30, REPLACE_WITH_DB_UNIREF90,
REPLACE_WITH_USERS_FILE or <PROJECT_ROOT>/...) so the template is generic and
does not leak local filesystem or usernames; update the variable values in
server/.env.example accordingly and keep the variable names unchanged.
In `@server/README.legacy.md`:
- Line 189: Replace the misspelled word "ocuppied" with "occupied" in the README
sentence that begins "If `8080` is not ocuppied, this command returns nothing,
otherwise the process name and PID will be shown." Update that exact string so
it reads "If `8080` is not occupied, ..." to correct the typo.
In `@server/README.md`:
- Around line 27-31: The README prerequisites are missing the download tool used
later — add aria2 (provides aria2c) to the install list so fresh hosts can run
UniRef downloads; update the apt install command shown (the block containing
"sudo apt-get install -y docker.io docker-compose-plugin ncbi-blast+") to
include aria2 (or aria2c) and add a verification line like "aria2c --version"
similar to the existing "makeblastdb -version" to ensure it's installed; search
for the install command block in the README and update it and any prerequisite
section accordingly.
In `@src/REvoDesign/clusters/combine_positions.py`:
- Around line 250-255: The numeric comparison on self.combi can raise TypeError
if it's a string; ensure self.combi is coerced to an int before checking >=1
(e.g., in Combinations.__init__ or setup() where self.combi is first
assigned/validated). Parse self.combi = int(self.combi) inside setup()/__init__
(or before the checks in combine_positions.py) wrapped in try/except ValueError
to raise a clear ValueError like "Invalid combination size: {self.combi}.
Expected integer >= 1." and then perform the existing if self.combi < 1 check;
also update any callers (run_combinations) to pass-through or validate strings
consistently.
In `@src/REvoDesign/tools/package_manager.py`:
- Around line 418-429: The validation raise inside the broad except swallows the
error—change control flow so invalid JSON structure returns early (or use
try/else) instead of raising; specifically in the function using
_read_https_url(...) and json.loads(...) replace the raise ValueError("Fetched
data is not a dictionary.") with an explicit return {} (or move the JSON
structure check into a try: ... else: ... block so exceptions are only used for
network/parse errors) and keep the logging calls (logging.debug/logging.error)
intact so callers can observe the failure via the returned empty dict rather
than silently catching the ValueError.
- Around line 156-162: The S310 (Ruff) warnings need suppressing in the
_read_https_url function: add "# noqa: S310" to the urllib.request.Request line
(in addition to current code) and replace the existing "# nosec B310" on the
urllib.request.urlopen context manager line with a combined comment "# noqa:
S310 # nosec B310" so both Ruff and Bandit suppressions are present.
In `@src/REvoDesign/tools/safe_pickle.py`:
- Around line 70-74: importlib.import_module can raise
ModuleNotFoundError/ImportError which should be normalized to
pickle.UnpicklingError so callers see a consistent error type; wrap the
importlib.import_module(module) call in a try/except that catches
ModuleNotFoundError and ImportError and re-raises pickle.UnpicklingError (e.g.
"Missing module in pickle payload: {module}") from the original exception, while
keeping the existing getattr(...) except AttributeError handling intact.
---
Outside diff comments:
In `@src/REvoDesign/common/mutant_tree.py`:
- Around line 39-52: The constructor MutantTree.__init__ currently uses
"mutant_tree or {}" which replaces caller-provided empty dicts and breaks
identity; change the assignment to explicitly check for None (e.g., if
mutant_tree is None then set self.mutant_tree = {} else self.mutant_tree =
mutant_tree) so callers' empty dicts are preserved, and then run the project's
pre-commit/black (pre-commit run --all-files or make black) before pushing.
In `@src/REvoDesign/magician/designers/cart_ddg.py`:
- Around line 90-95: The initialize() method exits early when the existing
relaxed PDB is found, but never sets self.initialized to True; update
initialize() so that when the condition checking isinstance(self.relaxed_pdb,
str) and os.path.isfile(self.relaxed_pdb) and not self.reload is true, you set
self.initialized = True before returning (or set it immediately after the
early-return check), ensuring callers that check self.initialized see
initialization completed; locate this logic in the initialize() implementation
of the CartDDG designer (references: self.relaxed_pdb, self.reload,
self.initialized, and self.ddg_runner.relax) and make the minimal change to mark
initialization complete on the early-return path.
In `@src/REvoDesign/shortcuts/tools/esm2.py`:
- Line 331: The loop in compute_pppl incorrectly iterates with range(1,
len(sequence) - 1), which omits the last two residue tokens when accumulating
log-probabilities from batch_tokens (shape (1, len(sequence)+2), residues at
indices 1..len(sequence)); change the loop to iterate over the full residue
index range (e.g., range(1, len(sequence)+1)) so you include the final residue
but still exclude EOS at len(sequence)+1, and keep the rest of the PPPL
accumulation logic intact in compute_pppl.
In `@src/REvoDesign/shortcuts/tools/exports.py`:
- Around line 104-116: The docstring for the function (parameters: chain_ids,
output_dir, drop_missing_residue, suffix) is stale: it still says chain_ids
defaults to [] while the signature uses None; update the docstring to reflect
the actual default and behavior (e.g., "chain_ids (list[str] | None): List of
chain IDs to dump; if None, all chains will be processed. Defaults to None.").
Edit the docstring near the function that calls dump_fasta_from_struct to
replace the incorrect default description and, if relevant, note that None means
"use all chains" or the actual behavior implemented in dump_fasta_from_struct.
In `@src/REvoDesign/tools/package_manager.py`:
- Around line 1496-1512: The get_existing_directory static method's docstring
incorrectly documents a self parameter; update the docstring for
get_existing_directory to remove any mention of self and adjust the "Parameters"
section (or delete it) so it reflects that the method is static and returns a
str path of the selected directory, keeping the description and return info
accurate for the static method.
- Around line 2399-2412: The code calling _read_https_url decodes and
JSON-parses response_data and then builds tag_names but only catches HTTPError
and URLError; update the try/except around json.loads(response_data) and the
list comprehension that builds tag_names to also handle json.JSONDecodeError,
TypeError, and KeyError by logging an informative message and returning an empty
list. Specifically, after obtaining response_data from _read_https_url, validate
that json.loads(response_data) produces a list (assign to tags), and when
building tag_names (the list comprehension using tag["name"]) either guard for
missing "name" keys or catch KeyError/TypeError and log the failure; ensure all
new exception handlers use the existing logging style and return [] on error.
In `@tests/cases/tabs/test_mutate.py`:
- Around line 215-216: The cleanup guard is checking
test_worker.test_data.ddg_design_pse but erroneously calls os.remove on
test_worker.test_data.ddg_design_case; change the os.remove call to remove the
same variable being checked (test_worker.test_data.ddg_design_pse) so the PSE
file is removed and the design-case file is not deleted by this block; update
the os.remove call in the block that references ddg_design_pse to use
ddg_design_pse.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 179: Pin the pytest-dependency package in the pyproject.toml dependencies
block (the same section that contains pytest<=8.3.3, pytest-cov<=6.0.0,
pytest-order, pytest-emoji) to improve reproducibility; update the
"pytest-dependency" entry to a version specifier consistent with the file's
style (e.g., add a compatible upper-bound or exact pin like == or <=/>= range)
so that test ordering behavior is deterministic across environments. Ensure you
modify the existing "pytest-dependency" token rather than adding a duplicate
entry.
In `@src/REvoDesign/clients/QtSocketConnector.py`:
- Around line 1-5: Run the project's pre-commit hooks and formatter on your
Python changes (specifically the new/modified
src/REvoDesign/clients/QtSocketConnector.py) before pushing: install hooks with
`pre-commit install` if not already installed, then run `pre-commit run
--all-files` (or `make black`) to apply linting/formatting fixes and commit the
resulting changes; ensure the updated, formatted QtSocketConnector.py is
included in the commit.
In `@src/REvoDesign/magician/__init__.py`:
- Line 115: Replace the instance call to the static method with a class call and
remove the unused instance: change the call site that uses
self.magician_assistant.get(name=..., **kwargs) to call the static method on the
class (MagicianAssistant.get(...)) and delete the now-unused instance field (the
self.magician_assistant creation and the unused installed_worker field
referenced at the creation around line 71), ensuring any imports or references
to MagicianAssistant are present; confirm no other code paths rely on the
instance before removing it.
In `@src/REvoDesign/magician/designers/cart_ddg.py`:
- Line 134: Replace the plain print call that emits "Warning: No ddG value found
for {ddg_mut_id}" with a logging.warning call so the message goes through the
module's logging configuration; locate the occurrence that references ddg_mut_id
in cart_ddg.py and change print(f"Warning: No ddg value found for {ddg_mut_id}")
to logging.warning("No ddG value found for %s", ddg_mut_id) (or equivalent
formatted logging) to preserve structured logging and avoid bypassing configured
handlers.
In `@src/REvoDesign/magician/designers/colabdesign.py`:
- Around line 57-62: Create a custom exception class (e.g., InputPDBNotFound) in
the same module that accepts the missing filename and implements __str__ (or a
class-level message) to produce the error text, then replace the direct
FileNotFoundError raise with raise InputPDBNotFound(self.pdb_filename); locate
the check around make_temperal_input_pdb / self.pdb_filename and the
mpnn_model.prep_inputs call and update the raise there to use the new exception
to satisfy Ruff TRY003.
In `@src/REvoDesign/tools/customized_widgets.py`:
- Around line 614-616: The base-class method get_WT_label is declared as
`@staticmethod` and has unused params causing ARG004 and a signature mismatch with
QButtonMatrixGremlin.get_WT_label (an instance method); change the base method
to an instance method (remove `@staticmethod` and add self) and rename unused
parameters to _col_name, _row, and _col so the linter is satisfied while
preserving the override signature with QButtonMatrixGremlin.get_WT_label; keep
the body returning row_name unchanged.
In `@src/REvoDesign/tools/package_manager.py`:
- Around line 70-88: The MockLogger static methods (debug, info, warning, error,
critical) currently accept unused parameters named *args and **kwargs which
trigger ARG004; rename those parameters to *_args and **_kwargs in each method
signature to suppress the linter while keeping API compatibility and behavior
unchanged, ensuring each method still references LOGGER_LEVEL exactly as before.
In `@src/REvoDesign/tools/safe_pickle.py`:
- Around line 1-10: The file safe_pickle.py needs to be formatted/linted per
project hooks; install and run the pre-commit hooks and black formatter before
pushing: run `pre-commit install` (once), then `pre-commit run --all-files` or
`make black` to format imports and code in module safe_pickle.py (and any other
Python files), fix any reported lint errors, and re-commit the results; ensure
CI/local checks pass before updating the PR.
In `@src/REvoDesign/tools/utils.py`:
- Around line 614-618: The current _safe_member_target implementation raises a
ValueError when detecting path traversal and then immediately catches and
re-raises it, creating an unnecessary self-caused exception chain; change the
logic to call os.path.commonpath(base_path, target_path) inside a try/except
that only handles platform-level ValueError (store that as a distinct error),
and separately check the traversal condition (if commonpath != base_path) to
raise a fresh ValueError with the traversal message using member_name — this
removes the pointless except-from-self chaining and preserves clear, distinct
errors for platform issues vs. unsafe member paths.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
CHANGELOG.mdMakefiledocs/dev_trick/codex_deepsource.mdplayground/plotly_qt.pypyproject.tomlserver/.env.exampleserver/README.legacy.mdserver/README.mdserver/pssm_gremlin/pssm_gremlin.pyserver/pssm_gremlin/templates/create_task.htmlserver/pssm_gremlin/templates/pssm_gremlin_dashboard.htmlsrc/REvoDesign/REvoDesign.pysrc/REvoDesign/application/font/font_manager.pysrc/REvoDesign/basic/abc_singleton.pysrc/REvoDesign/basic/server_monitor.pysrc/REvoDesign/clients/QtSocketConnector.pysrc/REvoDesign/clusters/cluster_sequence.pysrc/REvoDesign/clusters/combine_positions.pysrc/REvoDesign/common/multi_mutant_designer.pysrc/REvoDesign/common/mutant_tree.pysrc/REvoDesign/editor/monaco/monaco.pysrc/REvoDesign/logger/logger.pysrc/REvoDesign/magician/__init__.pysrc/REvoDesign/magician/designers/cart_ddg.pysrc/REvoDesign/magician/designers/colabdesign.pysrc/REvoDesign/phylogenetics/gremlin_tools.pysrc/REvoDesign/shortcuts/tools/esm2.pysrc/REvoDesign/shortcuts/tools/exports.pysrc/REvoDesign/shortcuts/tools/mutation_effect_predictors.pysrc/REvoDesign/shortcuts/tools/rfdiffusion_tasks.pysrc/REvoDesign/shortcuts/utils.pysrc/REvoDesign/sidechain/sidechain_solver.pysrc/REvoDesign/tools/customized_widgets.pysrc/REvoDesign/tools/measure_utils.pysrc/REvoDesign/tools/mutant_tools.pysrc/REvoDesign/tools/package_manager.pysrc/REvoDesign/tools/safe_pickle.pysrc/REvoDesign/tools/utils.pytests/cases/tabs/test_cluster.pytests/cases/tabs/test_config.pytests/cases/tabs/test_evaluate.pytests/cases/tabs/test_interact.pytests/cases/tabs/test_mutate.pytests/cases/tabs/test_prepare.pytests/cases/tabs/test_run_ui.pytests/cases/tabs/test_translate.pytests/cases/tabs/test_visualize.pytests/server/test_pssm_gremlin.pytests/tools/test_utils.py
💤 Files with no reviewable changes (3)
- src/REvoDesign/tools/mutant_tools.py
- src/REvoDesign/application/font/font_manager.py
- src/REvoDesign/logger/logger.py
| SERVER_DIR=/Users/yyy/Documents/protein_design/REvoDesign/playground/server_test | ||
| LOG_DIR=/Users/yyy/Documents/protein_design/REvoDesign/playground/server_test/revodesign/server/logs | ||
|
|
||
| ## Databases for MSA searching | ||
| DB_UNIREF30=/Users/yyy/Documents/protein_design/REvoDesign/playground/miniuc/uc30/miniuc30 | ||
| DB_UNIREF90=/Users/yyy/Documents/protein_design/REvoDesign/playground/miniuc/uc90/uniref90 | ||
|
|
||
| ## Users for accessing the server via basic authentication | ||
| USERS_FILE=/Users/yyy/Documents/protein_design/REvoDesign/server/pssm_gremlin/users.template.txt |
There was a problem hiding this comment.
Developer-specific absolute paths committed in the example file leak local filesystem structure (including username) and render the template unusable for any other user.
Replace every concrete path with a clearly labelled placeholder:
📝 Proposed fix
-SERVER_DIR=/Users/yyy/Documents/protein_design/REvoDesign/playground/server_test
-LOG_DIR=/Users/yyy/Documents/protein_design/REvoDesign/playground/server_test/revodesign/server/logs
+SERVER_DIR=/srv/revodesign/workdir
+LOG_DIR=/srv/revodesign/logs
## Databases for MSA searching
-DB_UNIREF30=/Users/yyy/Documents/protein_design/REvoDesign/playground/miniuc/uc30/miniuc30
-DB_UNIREF90=/Users/yyy/Documents/protein_design/REvoDesign/playground/miniuc/uc90/uniref90
+DB_UNIREF30=/data/uniref30/uniref30
+DB_UNIREF90=/data/uniref90/uniref90
## Users for accessing the server via basic authentication
-USERS_FILE=/Users/yyy/Documents/protein_design/REvoDesign/server/pssm_gremlin/users.template.txt
+USERS_FILE=/srv/revodesign/users.txt📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SERVER_DIR=/Users/yyy/Documents/protein_design/REvoDesign/playground/server_test | |
| LOG_DIR=/Users/yyy/Documents/protein_design/REvoDesign/playground/server_test/revodesign/server/logs | |
| ## Databases for MSA searching | |
| DB_UNIREF30=/Users/yyy/Documents/protein_design/REvoDesign/playground/miniuc/uc30/miniuc30 | |
| DB_UNIREF90=/Users/yyy/Documents/protein_design/REvoDesign/playground/miniuc/uc90/uniref90 | |
| ## Users for accessing the server via basic authentication | |
| USERS_FILE=/Users/yyy/Documents/protein_design/REvoDesign/server/pssm_gremlin/users.template.txt | |
| SERVER_DIR=/srv/revodesign/workdir | |
| LOG_DIR=/srv/revodesign/logs | |
| ## Databases for MSA searching | |
| DB_UNIREF30=/data/uniref30/uniref30 | |
| DB_UNIREF90=/data/uniref90/uniref90 | |
| ## Users for accessing the server via basic authentication | |
| USERS_FILE=/srv/revodesign/users.txt |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 9-9: [UnorderedKey] The LOG_DIR key should go before the SERVER_DIR key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/.env.example` around lines 8 - 16, The .env example contains
developer-specific absolute paths; replace the concrete values for SERVER_DIR,
LOG_DIR, DB_UNIREF30, DB_UNIREF90 and USERS_FILE with clearly labeled
placeholders (e.g., REPLACE_WITH_SERVER_DIR, REPLACE_WITH_LOG_DIR,
REPLACE_WITH_DB_UNIREF30, REPLACE_WITH_DB_UNIREF90, REPLACE_WITH_USERS_FILE or
<PROJECT_ROOT>/...) so the template is generic and does not leak local
filesystem or usernames; update the variable values in server/.env.example
accordingly and keep the variable names unchanged.
| ```bash | ||
| sudo apt-get update | ||
| sudo apt-get install -y docker.io docker-compose-plugin ncbi-blast+ | ||
| makeblastdb -version | ||
| ``` |
There was a problem hiding this comment.
Add the download tool dependency used below.
aria2c is used for UniRef downloads but isn’t listed in prerequisites, so fresh hosts will fail at the first download step.
🔧 Suggested doc fix
-sudo apt-get install -y docker.io docker-compose-plugin ncbi-blast+
+sudo apt-get install -y docker.io docker-compose-plugin ncbi-blast+ aria2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```bash | |
| sudo apt-get update | |
| sudo apt-get install -y docker.io docker-compose-plugin ncbi-blast+ | |
| makeblastdb -version | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/README.md` around lines 27 - 31, The README prerequisites are missing
the download tool used later — add aria2 (provides aria2c) to the install list
so fresh hosts can run UniRef downloads; update the apt install command shown
(the block containing "sudo apt-get install -y docker.io docker-compose-plugin
ncbi-blast+") to include aria2 (or aria2c) and add a verification line like
"aria2c --version" similar to the existing "makeblastdb -version" to ensure it's
installed; search for the install command block in the README and update it and
any prerequisite section accordingly.
| if not os.path.exists(self.inputfile): | ||
| raise FileNotFoundError(f"Input mutation file does not exist: {self.inputfile}") | ||
| if not os.path.exists(self.fastafile): | ||
| raise FileNotFoundError(f"Input fasta file does not exist: {self.fastafile}") | ||
| if self.combi < 1: | ||
| raise ValueError(f"Invalid combination size: {self.combi}. Expected >= 1.") |
There was a problem hiding this comment.
Coerce self.combi before numeric comparison.
If self.combi is still a string (e.g., from CLI args), the new < 1 check can raise TypeError before setup() casts it. Consider parsing it up front and throwing a controlled error.
✅ Suggested fix
- if self.combi < 1:
- raise ValueError(f"Invalid combination size: {self.combi}. Expected >= 1.")
+ try:
+ self.combi = int(self.combi)
+ except (TypeError, ValueError) as exc:
+ raise ValueError(
+ f"Invalid combination size: {self.combi}. Expected an integer >= 1."
+ ) from exc
+ if self.combi < 1:
+ raise ValueError(f"Invalid combination size: {self.combi}. Expected >= 1.")#!/bin/bash
# Verify how Combinations.combi is assigned and whether it can be a string.
rg -nP --type=py '\.combi\s*=' -C3
rg -nP --type=py 'Combinations\s*\(' -C3
rg -nP --type=py '\brun_combinations\s*\(' -C3🧰 Tools
🪛 Ruff (0.15.2)
[warning] 251-251: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 253-253: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 255-255: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/REvoDesign/clusters/combine_positions.py` around lines 250 - 255, The
numeric comparison on self.combi can raise TypeError if it's a string; ensure
self.combi is coerced to an int before checking >=1 (e.g., in
Combinations.__init__ or setup() where self.combi is first assigned/validated).
Parse self.combi = int(self.combi) inside setup()/__init__ (or before the checks
in combine_positions.py) wrapped in try/except ValueError to raise a clear
ValueError like "Invalid combination size: {self.combi}. Expected integer >= 1."
and then perform the existing if self.combi < 1 check; also update any callers
(run_combinations) to pass-through or validate strings consistently.
| def _read_https_url(url: str, timeout: float = 10.0) -> bytes: | ||
| parsed = urlparse(url) | ||
| if parsed.scheme != "https" or not parsed.netloc: | ||
| raise ValueError(f"Only https URLs are allowed: {url}") | ||
| request = urllib.request.Request(url, headers={"User-Agent": "REvoDesign-PackageManager"}) | ||
| with urllib.request.urlopen(request, timeout=timeout) as response: # nosec B310 | ||
| return response.read() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/REvoDesign/tools/package_manager.py | sed -n '156,162p'Repository: YaoYinYing/REvoDesign
Length of output: 532
🏁 Script executed:
# Search for any other instances of similar URL/request handling in the codebase
rg "urllib.request" --type py -B 2 -A 2Repository: YaoYinYing/REvoDesign
Length of output: 4926
🌐 Web query:
Ruff S310 urllib request scheme check explicit https mitigations
💡 Result:
Ruff S310 (suspicious-url-open-usage) flags urllib.request.urlopen(...) (and similar “URL open” APIs) when the URL’s scheme isn’t explicitly constrained, because urllib can open unexpected schemes (e.g., file: or other handler-based/custom schemes), which can lead to SSRF/local file access or other unintended behavior. Mitigation is to audit each callsite and allowlist schemes (e.g., only http/https, or only https). [1]
Recommended mitigation (explicit https only)
Do a real scheme parse and enforce an allowlist (don’t rely only on “block file:” blacklists; historically there have been surprising scheme behaviors/bypasses such as local_file variants in some contexts). [3]
from urllib.parse import urlsplit
from urllib.request import urlopen
def open_https(url: str):
u = url.strip()
p = urlsplit(u)
if p.scheme.lower() != "https":
raise ValueError("Only https URLs are allowed")
return urlopen(u)If you already enforce schemes and still get S310
If your code already constrains the scheme (e.g., startswith(("https:", "http:"))), the underlying Bandit-style check can still warn; in such cases it may be reasonable to suppress at the line with # nosec (Bandit) / the equivalent Ruff suppression for your setup. [2]
Sources: [1] Ruff S310 rule docs, [2] Bandit “nosec” example for urlopen with scheme checks, [3] Python security issue discussion showing why allowlisting schemes is safer than trying to block only file:.
🌐 Web query:
Ruff noqa S310 suppression directive syntax
💡 Result:
To suppress Ruff’s S310 on a specific line, add a noqa with the code at the end of the line:
value = eval(user_input) # noqa: S310[1]
Other common suppression forms:
-
Multiple codes on the same line
x = 1 # noqa: E741, S310
[1]
-
Suppress
S310for an entire file# ruff: noqa: S310[1]
-
Suppress
S310for a block (range suppression)# ruff: disable[S310] ... # ruff: enable[S310]
[1]
Source: Ruff “Error suppression” docs. [1]
🏁 Script executed:
# Check for Ruff configuration files
fd -e toml | xargs grep -l "S310" 2>/dev/null || echo "No S310 config found"Repository: YaoYinYing/REvoDesign
Length of output: 85
🏁 Script executed:
# Check for ruff.toml or pyproject.toml
ls -la | grep -E "(ruff|pyproject)" && cat ruff.toml pyproject.toml 2>/dev/null | head -50Repository: YaoYinYing/REvoDesign
Length of output: 1827
🏁 Script executed:
# Search for other urllib.request patterns with suppression comments
rg "urllib.request\.(Request|urlopen)" --type py -B 2 -A 2 | grep -E "(nosec|noqa)"Repository: YaoYinYing/REvoDesign
Length of output: 189
Add # noqa: S310 to lines 160–161 to suppress Ruff S310 errors.
Ruff S310 is enabled in this project (see ruff.toml). The existing # nosec B310 on line 161 only suppresses bandit, not Ruff. Line 160 has no suppression at all, and both lines will fail pre-commit checks. Use # noqa: S310 on line 160, and combine it with the existing Bandit comment on line 161 as # noqa: S310 # nosec B310.
Fix
- request = urllib.request.Request(url, headers={"User-Agent": "REvoDesign-PackageManager"})
- with urllib.request.urlopen(request, timeout=timeout) as response: # nosec B310
+ request = urllib.request.Request(url, headers={"User-Agent": "REvoDesign-PackageManager"}) # noqa: S310
+ with urllib.request.urlopen(request, timeout=timeout) as response: # noqa: S310 # nosec B310
return response.read()🧰 Tools
🪛 Ruff (0.15.2)
[warning] 159-159: Avoid specifying long messages outside the exception class
(TRY003)
[error] 160-160: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[error] 161-161: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/REvoDesign/tools/package_manager.py` around lines 156 - 162, The S310
(Ruff) warnings need suppressing in the _read_https_url function: add "# noqa:
S310" to the urllib.request.Request line (in addition to current code) and
replace the existing "# nosec B310" on the urllib.request.urlopen context
manager line with a combined comment "# noqa: S310 # nosec B310" so both Ruff
and Bandit suppressions are present.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/tools/customized_widgets/test_dialog_wrapper_registry.py (1)
79-89:⚠️ Potential issue | 🟡 MinorRemove the unused dict literal at lines 79-89.
This bare dict literal is a no-op statement with no assignment or usage, likely leftover from refactoring. It should be removed to keep the test clean and avoid linter warnings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tools/customized_widgets/test_dialog_wrapper_registry.py` around lines 79 - 89, Remove the unused dict literal that creates a dict with an AskedValue instance (the bare {"value": AskedValue(...), "index": 0} block) from the test in tests/tools/customized_widgets/test_dialog_wrapper_registry.py; delete that no-op statement entirely (or replace it with an assigned variable or used fixture if the test actually needs the AskedValue) so the test no longer contains an unreferenced literal and linter warnings are resolved.tests/sidechain/test_sidechain_solvers.py (1)
58-58:⚠️ Potential issue | 🟡 MinorTypo in parametrize ID:
"PIPPack-ensumble"→"PIPPack-ensemble".The misspelling will appear in CI test reports and break
-k PIPPack-ensemblekeyword filtering.🐛 Proposed fix
- ["PIPPack-ensumble", PIPPack_worker, {"pdb_file": WT_PDB}], + ["PIPPack-ensemble", PIPPack_worker, {"pdb_file": WT_PDB}],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sidechain/test_sidechain_solvers.py` at line 58, Update the pytest parameter ID that is misspelled: replace the string "PIPPack-ensumble" with the correct "PIPPack-ensemble" in the pytest.mark.parametrize (the ID list used in tests/sidechain/test_sidechain_solvers.py) so CI reports and `-k` filtering use the correct name.tests/tools/customized_widgets/test_asked_value_dynamic.py (1)
55-61:⚠️ Potential issue | 🟡 Minor
test_asked_value_dynamic_type_checkis a guaranteed-pass no-op and provides no test value.
TypedDictreturns a normal dict object at runtime — it does not define a new runtime type, and constructing or assigning one is equivalent to constructing a dictionary directly. A plain variable assignment (_ = input_dict) can never raise anException, so thetry/exceptblock will never be triggered regardless of the dict's content or structure. The test will always pass, even with a completely wrong dict shape.Consider replacing it with a structural assertion that actually validates the keys/values at runtime:
🛠️ Suggested replacement
`@pytest.mark.parametrize`("input_dict", dynamic_asked_values_with_index) def test_asked_value_dynamic_type_check(input_dict: dict[str, Any]): - """Ensure that AskedValueDynamic accepts valid dicts.""" - try: - _: AskedValueDynamic = input_dict # type: ignore - except Exception as e: - pytest.fail(f"Unexpected exception: {e}") + """Ensure that AskedValueDynamic dicts have the expected keys and types.""" + assert "value" in input_dict + assert "index" in input_dict + assert isinstance(input_dict["value"], AskedValue) + assert isinstance(input_dict["index"], int)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tools/customized_widgets/test_asked_value_dynamic.py` around lines 55 - 61, The test_asked_value_dynamic_type_check is a no-op because assigning input_dict to _: AskedValueDynamic does not perform runtime validation; replace the try/except assignment with explicit structural checks that validate the runtime dict shape for AskedValueDynamic (check required keys exist, their types and nested structures) or call a runtime validator (e.g., a constructor or function that enforces AskedValueDynamic schema) and assert expected failures/successes accordingly; locate this logic around test_asked_value_dynamic_type_check and use the unique name AskedValueDynamic to guide where to add key presence and type assertions or to call the schema validator.
🧹 Nitpick comments (5)
tests/evo/test_gremlin_pytorch.py (1)
20-20: Incomplete fix —msastill uses a bare f-string with no interpolation.Line 20 has the same deepsource lint issue that motivated line 21's fix but was left unaddressed in this PR.
♻️ Proposed fix
- msa = f"../tests/data/msa/2KL8.i90c75_aln.fas" + msa = "../tests/data/msa/2KL8.i90c75_aln.fas"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/evo/test_gremlin_pytorch.py` at line 20, The variable msa is assigned using a bare f-string with no interpolation (msa = f"../tests/data/msa/2KL8.i90c75_aln.fas"); replace the f-string with a normal string literal or add the intended interpolation. Update the msa assignment (variable name: msa) to use "../tests/data/msa/2KL8.i90c75_aln.fas" (no leading f) or convert to a proper formatted f-string only if you actually intend to inject variables.tests/sidechain/test_sidechain_solvers.py (2)
21-24:setup_pymolfixture never tears down PyMOL state.
cmd.load(WT_PDB)accumulates objects in the global PyMOL session across test runs with no correspondingcmd.reinitialize()orcmd.delete(MOLECULE). Add a yield-based teardown to prevent state leakage between tests.♻️ Proposed fixture teardown
`@pytest.fixture`(autouse=True) def setup_pymol(self): """Auto-used fixture to load PDB file before each test""" cmd.load(WT_PDB) + yield + cmd.reinitialize()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sidechain/test_sidechain_solvers.py` around lines 21 - 24, The fixture setup_pymol calls cmd.load(WT_PDB) but never cleans up, causing PyMOL state to accumulate; change setup_pymol into a yield-based fixture that loads WT_PDB before the yield and performs teardown after the yield by deleting the loaded object (e.g., use cmd.delete(...) with the WT_PDB/molecule name or call cmd.reinitialize()) so each test starts with a clean PyMOL session; update references in the fixture to ensure the correct object name is deleted.
12-15: Module-level relative paths and eagerMutant.from_pdbcall are fragile.
WT_PDBandMUT_PDBare relative paths resolved against the process CWD at collection time.MUTANTSis also populated at module import — if the path doesn't resolve (e.g., pytest is invoked from a different directory), the entire collection fails with an obscure error rather than a skipped test.Consider using
pathlib.Path(__file__).parent.parent / "data" / "..."for robust path resolution, and lazy-evaluatingMUTANTSinside a fixture.♻️ Proposed fix for path resolution and lazy evaluation
-WT_PDB = "../tests/data/3fap_hf3_A_short.pdb" -MUT_PDB = "../tests/data/3fap_hf3_A_RFD.pdb" +from pathlib import Path +_DATA_DIR = Path(__file__).parent.parent / "data" +WT_PDB = str(_DATA_DIR / "3fap_hf3_A_short.pdb") +MUT_PDB = str(_DATA_DIR / "3fap_hf3_A_RFD.pdb") MOLECULE = "3fap_hf3_A_short" -MUTANTS: list[Mutant] = [Mutant(**m.__dict__) for m in Mutant.from_pdb(WT_PDB, [MUT_PDB])] -mutant_string = MUTANTS[0].full_mutant_idThen expose
MUTANTSvia a session-scoped fixture so failures appear as test errors rather than collection errors:`@pytest.fixture`(scope="session") def mutants(): return [Mutant(**m.__dict__) for m in Mutant.from_pdb(WT_PDB, [MUT_PDB])]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sidechain/test_sidechain_solvers.py` around lines 12 - 15, The module currently defines WT_PDB, MUT_PDB as relative strings and eagerly constructs MUTANTS via Mutant.from_pdb at import time, which breaks collection if CWD differs; change WT_PDB and MUT_PDB to be resolved with pathlib using Path(__file__).parent.parent / "data" / "<filename>" and remove the top-level MUTANTS construction, exposing a session-scoped pytest fixture (e.g., def mutants():) that calls Mutant.from_pdb and returns [Mutant(**m.__dict__) for m in ...] so the PDB loading happens lazily during fixture setup rather than at import/collection time.src/REvoDesign/shortcuts/tools/vina_tools.py (1)
92-96: Consider hoisting thethreadingimport to the module level.Importing inside a method body is an antipattern that DeepSource (and most linters) flag. Since the PR's goal is to resolve DeepSource findings, this pre-existing issue is worth addressing here.
♻️ Proposed refactor
At the top of the file alongside the other stdlib imports:
from dataclasses import dataclass +import threading from random import randintThen in
PutCenterCallback.__call__:if self.name not in cmd.get_names("objects"): # Remove the callback if the object no longer exists - import threading - threading.Thread(None, cmd.delete, args=(self.cb_name,)).start() return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/shortcuts/tools/vina_tools.py` around lines 92 - 96, Hoist the local import of threading to the module level and remove the inline "import threading" inside PutCenterCallback.__call__; keep the behavior that spawns a background thread to call cmd.delete(self.cb_name) (the Thread invocation and args can remain unchanged), so update the top-of-file imports to include "import threading" and leave the body of PutCenterCallback.__call__ to simply call threading.Thread(None, cmd.delete, args=(self.cb_name,)).start() before returning.tests/tools/customized_widgets/test_asked_value_dynamic.py (1)
19-27: Remove staledummy_functionfixture and unusedassert_asked_value_equalhelper.Both are orphaned by the removal of
dialog_wrapper:
dummy_function's docstring still referencesdialog_wrapper, and no test in this file uses the fixture.assert_asked_value_equalis defined but never called.🧹 Proposed cleanup
-@pytest.fixture -def dummy_function(): - """Returns a mock function to be wrapped by dialog_wrapper.""" - return MagicMock() - - -def assert_asked_value_equal(av1: AskedValue, av2: AskedValue): - """Helper to compare two AskedValue objects.""" - assert vars(av1) == vars(av2)With these removed, the
MagicMockandAnyimports can also be cleaned up if nothing else uses them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tools/customized_widgets/test_asked_value_dynamic.py` around lines 19 - 27, Remove the stale pytest fixture dummy_function and the unused helper assert_asked_value_equal from the test file (they reference dialog_wrapper and are never used); also delete any now-unnecessary imports like MagicMock and Any if nothing else in the file depends on them, ensuring no dead references remain and tests still run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.qlty/configs/.hadolint.yaml:
- Around line 1-2: Remove the global DL3008 suppression from .hadolint.yaml and
instead address the concrete occurrences: either pin the apt packages in
./server/docker/server/Dockerfile (the install of build-essential curl procps)
to specific versions, or keep the rule enabled and add an inline hadolint ignore
comment only on that apt RUN line (use "# hadolint ignore=DL3008") so DL3008
remains enforced globally but ignored only where pinning is infeasible.
In `@Makefile`:
- Around line 14-15: The bootstrap tests are being matched in both
PYTEST_NON_DIST_SERIAL_ARGS and PYTEST_NON_DIST_SLOW_SERIAL_ARGS so they run
twice; update the Makefile so bootstrap only matches the slow bucket by making
the slow expression require very_slow as well (i.e., change the
PYTEST_NON_DIST_SLOW_SERIAL_ARGS marker expression from 'or bootstrap' to 'or
(bootstrap and very_slow)') so tests like
test_plugin_gui_visibility/test_load_molecule/test_pocket only execute once.
In `@server/REvoDesign_PSSM_GREMLIN.sh`:
- Around line 168-171: The script builds command strings for hhblits, hhfilter,
remove_inserts, GREMLIN_TFv1 and psiblast and then runs them with eval "$cmd",
which allows word-splitting and injection; change each to build and run a bash
array (e.g., cmd=(which hhblits; or better: cmd=(hhblits -i "$fasta" -oa3m
"${instance}.a3m" ...)) so every argument is quoted and preserved, then execute
with "${cmd[@]}" and redirect stdout/stderr to the same
"${pipline_res_dir}/log/${instance}_... .log" and .err files; update all five
locations (hhblits, hhfilter, remove_inserts, GREMLIN_TFv1, psiblast) to use
this array-based construction and execution to eliminate eval and unquoted
variable expansion.
In `@src/REvoDesign/application/font/font_manager.py`:
- Line 15: This change modifies the imports in font_manager.py (the line
importing REvoDesign.Qt as QtGui, QtWidgets) but the pre-commit/formatting hooks
must be run before pushing; run pre-commit install (to enable hooks) and then
run pre-commit run --all-files (or make black) and fix any formatting/lint
errors reported, then re-run the tests/lint until clean and re-commit the
corrected file.
In `@tools/translate.sh`:
- Around line 23-31: The for-loop uses command substitution with ls which breaks
on filenames with spaces; replace the loop "for i in $(ls
src/REvoDesign/UI/language/*.ts); do ... done" with direct glob iteration (e.g.
"for i in src/REvoDesign/UI/language/*.ts; do") while keeping the quoted "$i"
inside the loop (and optionally enable nullglob behavior if you need to handle
zero matches), ensuring the rest of the body (lupdate invocation) remains
unchanged and still references "$i".
---
Outside diff comments:
In `@tests/sidechain/test_sidechain_solvers.py`:
- Line 58: Update the pytest parameter ID that is misspelled: replace the string
"PIPPack-ensumble" with the correct "PIPPack-ensemble" in the
pytest.mark.parametrize (the ID list used in
tests/sidechain/test_sidechain_solvers.py) so CI reports and `-k` filtering use
the correct name.
In `@tests/tools/customized_widgets/test_asked_value_dynamic.py`:
- Around line 55-61: The test_asked_value_dynamic_type_check is a no-op because
assigning input_dict to _: AskedValueDynamic does not perform runtime
validation; replace the try/except assignment with explicit structural checks
that validate the runtime dict shape for AskedValueDynamic (check required keys
exist, their types and nested structures) or call a runtime validator (e.g., a
constructor or function that enforces AskedValueDynamic schema) and assert
expected failures/successes accordingly; locate this logic around
test_asked_value_dynamic_type_check and use the unique name AskedValueDynamic to
guide where to add key presence and type assertions or to call the schema
validator.
In `@tests/tools/customized_widgets/test_dialog_wrapper_registry.py`:
- Around line 79-89: Remove the unused dict literal that creates a dict with an
AskedValue instance (the bare {"value": AskedValue(...), "index": 0} block) from
the test in tests/tools/customized_widgets/test_dialog_wrapper_registry.py;
delete that no-op statement entirely (or replace it with an assigned variable or
used fixture if the test actually needs the AskedValue) so the test no longer
contains an unreferenced literal and linter warnings are resolved.
---
Nitpick comments:
In `@src/REvoDesign/shortcuts/tools/vina_tools.py`:
- Around line 92-96: Hoist the local import of threading to the module level and
remove the inline "import threading" inside PutCenterCallback.__call__; keep the
behavior that spawns a background thread to call cmd.delete(self.cb_name) (the
Thread invocation and args can remain unchanged), so update the top-of-file
imports to include "import threading" and leave the body of
PutCenterCallback.__call__ to simply call threading.Thread(None, cmd.delete,
args=(self.cb_name,)).start() before returning.
In `@tests/evo/test_gremlin_pytorch.py`:
- Line 20: The variable msa is assigned using a bare f-string with no
interpolation (msa = f"../tests/data/msa/2KL8.i90c75_aln.fas"); replace the
f-string with a normal string literal or add the intended interpolation. Update
the msa assignment (variable name: msa) to use
"../tests/data/msa/2KL8.i90c75_aln.fas" (no leading f) or convert to a proper
formatted f-string only if you actually intend to inject variables.
In `@tests/sidechain/test_sidechain_solvers.py`:
- Around line 21-24: The fixture setup_pymol calls cmd.load(WT_PDB) but never
cleans up, causing PyMOL state to accumulate; change setup_pymol into a
yield-based fixture that loads WT_PDB before the yield and performs teardown
after the yield by deleting the loaded object (e.g., use cmd.delete(...) with
the WT_PDB/molecule name or call cmd.reinitialize()) so each test starts with a
clean PyMOL session; update references in the fixture to ensure the correct
object name is deleted.
- Around line 12-15: The module currently defines WT_PDB, MUT_PDB as relative
strings and eagerly constructs MUTANTS via Mutant.from_pdb at import time, which
breaks collection if CWD differs; change WT_PDB and MUT_PDB to be resolved with
pathlib using Path(__file__).parent.parent / "data" / "<filename>" and remove
the top-level MUTANTS construction, exposing a session-scoped pytest fixture
(e.g., def mutants():) that calls Mutant.from_pdb and returns
[Mutant(**m.__dict__) for m in ...] so the PDB loading happens lazily during
fixture setup rather than at import/collection time.
In `@tests/tools/customized_widgets/test_asked_value_dynamic.py`:
- Around line 19-27: Remove the stale pytest fixture dummy_function and the
unused helper assert_asked_value_equal from the test file (they reference
dialog_wrapper and are never used); also delete any now-unnecessary imports like
MagicMock and Any if nothing else in the file depends on them, ensuring no dead
references remain and tests still run.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
.qlty/.gitignore.qlty/configs/.hadolint.yaml.qlty/configs/.shellcheckrc.qlty/qlty.tomlCHANGELOG.mdMakefileplayground/plotly_qt.pypyproject.tomlserver/REvoDesign_PSSM_GREMLIN.shsrc/REvoDesign/application/font/font_manager.pysrc/REvoDesign/bootstrap/set_config.pysrc/REvoDesign/editor/monaco/monaco.pysrc/REvoDesign/shortcuts/tools/vina_tools.pysrc/REvoDesign/shortcuts/utils.pysrc/REvoDesign/shortcuts/wrappers/represents.pysrc/REvoDesign/tools/cgo_utils.pysrc/REvoDesign/tools/measure_utils.pysrc/REvoDesign/tools/rosetta_utils.pytests/basic/test_designer_abc.pytests/bootstrap/test_set_config.pytests/cases/tabs/test_cluster.pytests/cases/tabs/test_config.pytests/cases/tabs/test_evaluate.pytests/cases/tabs/test_interact.pytests/cases/tabs/test_mutate.pytests/cases/tabs/test_prepare.pytests/cases/tabs/test_run_ui.pytests/cases/tabs/test_translate.pytests/cases/tabs/test_visualize.pytests/evo/test_gremlin_pytorch.pytests/menu/shortcuts/test_pick_design_from_profile.pytests/menu/shortcuts/test_represents.pytests/menu/shortcuts/test_vina_tools.pytests/menu/test_window_pops.pytests/sidechain/test_sidechain_solvers.pytests/tools/customized_widgets/test_asked_value_dynamic.pytests/tools/customized_widgets/test_dialog_wrapper_registry.pytests/tools/test_cgo_utils.pytools/ci-runner.admin.shtools/translate.sh
💤 Files with no reviewable changes (8)
- tests/bootstrap/test_set_config.py
- src/REvoDesign/shortcuts/wrappers/represents.py
- tests/menu/shortcuts/test_vina_tools.py
- tools/ci-runner.admin.sh
- tests/menu/shortcuts/test_represents.py
- tests/basic/test_designer_abc.py
- tests/tools/test_cgo_utils.py
- tests/menu/test_window_pops.py
✅ Files skipped from review due to trivial changes (4)
- src/REvoDesign/tools/cgo_utils.py
- src/REvoDesign/bootstrap/set_config.py
- .qlty/configs/.shellcheckrc
- .qlty/.gitignore
🚧 Files skipped from review as they are similar to previous changes (10)
- pyproject.toml
- tests/cases/tabs/test_interact.py
- src/REvoDesign/editor/monaco/monaco.py
- tests/cases/tabs/test_translate.py
- tests/cases/tabs/test_mutate.py
- tests/cases/tabs/test_cluster.py
- tests/cases/tabs/test_run_ui.py
- tests/cases/tabs/test_evaluate.py
- tests/cases/tabs/test_config.py
- playground/plotly_qt.py
| ignored: | ||
| - DL3008 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where DL3008 would apply and whether inline ignores are needed.
fd -0 -i '(Dockerfile|.*\.Dockerfile)$' -t f | while IFS= read -r -d '' f; do
echo "== $f =="
rg -n "apt-get\s+install" "$f"
rg -n "hadolint ignore=DL3008" "$f"
doneRepository: YaoYinYing/REvoDesign
Length of output: 227
🏁 Script executed:
# Also check the current hadolint configuration
cat .qlty/configs/.hadolint.yamlRepository: YaoYinYing/REvoDesign
Length of output: 84
🏁 Script executed:
# Check if there are any other hadolint configs in the repo
find . -name "*hadolint*" -type fRepository: YaoYinYing/REvoDesign
Length of output: 95
🏁 Script executed:
# Get context around the apt-get install on line 21
sed -n '15,30p' ./server/docker/server/DockerfileRepository: YaoYinYing/REvoDesign
Length of output: 555
🏁 Script executed:
# Check the full Dockerfile to understand its purpose
wc -l ./server/docker/server/Dockerfile
head -30 ./server/docker/server/DockerfileRepository: YaoYinYing/REvoDesign
Length of output: 912
🏁 Script executed:
# Check the runner Dockerfile to see why it has no apt-get
head -30 ./server/docker/runner/DockerfileRepository: YaoYinYing/REvoDesign
Length of output: 1092
Avoid disabling DL3008 globally; prefer targeted ignores.
The global DL3008 disable suppresses version-pinning enforcement for the unversioned apt packages in ./server/docker/server/Dockerfile (line 21: build-essential curl procps). This reduces reproducibility and supply-chain hygiene. Either pin versions for these packages or use an inline # hadolint ignore=DL3008 comment if pinning is infeasible.
🧩 Suggested change (re‑enable DL3008)
-ignored:
- - DL3008📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ignored: | |
| - DL3008 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.qlty/configs/.hadolint.yaml around lines 1 - 2, Remove the global DL3008
suppression from .hadolint.yaml and instead address the concrete occurrences:
either pin the apt packages in ./server/docker/server/Dockerfile (the install of
build-essential curl procps) to specific versions, or keep the rule enabled and
add an inline hadolint ignore comment only on that apt RUN line (use "# hadolint
ignore=DL3008") so DL3008 remains enforced globally but ignored only where
pinning is infeasible.
| PYTEST_NON_DIST_SERIAL_ARGS=-m "(serial and not very_slow) or bootstrap" --cov-append | ||
| PYTEST_NON_DIST_SLOW_SERIAL_ARGS=-m "(serial and very_slow) or bootstrap" --cov-append |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all test files and search for bootstrap marker usage
rg "@pytest\.mark\.bootstrap" --type py -A 2Repository: YaoYinYing/REvoDesign
Length of output: 918
🏁 Script executed:
#!/bin/bash
# Find all test files and search for very_slow marker usage
rg "@pytest\.mark\.very_slow" --type pyRepository: YaoYinYing/REvoDesign
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Check for tests marked with both bootstrap and very_slow
rg -B 5 "@pytest\.mark\.bootstrap" --type py | rg -A 5 "@pytest\.mark\.very_slow"Repository: YaoYinYing/REvoDesign
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check the full Makefile context to understand how these variables are used
cat -n Makefile | head -30Repository: YaoYinYing/REvoDesign
Length of output: 1693
Bootstrap tests execute twice across serial stages.
The 3 bootstrap tests in the codebase (test_plugin_gui_visibility, test_load_molecule, test_pocket) are not marked with very_slow. With or bootstrap in both PYTEST_NON_DIST_SERIAL_ARGS and PYTEST_NON_DIST_SLOW_SERIAL_ARGS, they match both marker expressions and run in both PYTEST_RUN_SECOND_ARGS and PYTEST_RUN_THIRD_ARGS, doubling runtime and risking flakiness. Route bootstrap tests into only one bucket by conditionally adding the very_slow marker.
Example fix (single execution by slow/fast bucket)
-PYTEST_NON_DIST_SERIAL_ARGS=-m "(serial and not very_slow) or bootstrap" --cov-append
-PYTEST_NON_DIST_SLOW_SERIAL_ARGS=-m "(serial and very_slow) or bootstrap" --cov-append
+PYTEST_NON_DIST_SERIAL_ARGS=-m "(serial and not very_slow) or (bootstrap and not very_slow)" --cov-append
+PYTEST_NON_DIST_SLOW_SERIAL_ARGS=-m "(serial and very_slow) or (bootstrap and very_slow)" --cov-append📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| PYTEST_NON_DIST_SERIAL_ARGS=-m "(serial and not very_slow) or bootstrap" --cov-append | |
| PYTEST_NON_DIST_SLOW_SERIAL_ARGS=-m "(serial and very_slow) or bootstrap" --cov-append | |
| PYTEST_NON_DIST_SERIAL_ARGS=-m "(serial and not very_slow) or (bootstrap and not very_slow)" --cov-append | |
| PYTEST_NON_DIST_SLOW_SERIAL_ARGS=-m "(serial and very_slow) or (bootstrap and very_slow)" --cov-append |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 14 - 15, The bootstrap tests are being matched in both
PYTEST_NON_DIST_SERIAL_ARGS and PYTEST_NON_DIST_SLOW_SERIAL_ARGS so they run
twice; update the Makefile so bootstrap only matches the slow bucket by making
the slow expression require very_slow as well (i.e., change the
PYTEST_NON_DIST_SLOW_SERIAL_ARGS marker expression from 'or bootstrap' to 'or
(bootstrap and very_slow)') so tests like
test_plugin_gui_visibility/test_load_molecule/test_pocket only execute once.
| from immutabledict import immutabledict | ||
|
|
||
| from REvoDesign.Qt import QtCore, QtGui, QtWidgets | ||
| from REvoDesign.Qt import QtGui, QtWidgets |
There was a problem hiding this comment.
Confirm pre-commit hooks/linting were run.
Please ensure pre-commit install and pre-commit run --all-files (or make black) were executed before pushing this Python change.
As per coding guidelines, Enable pre-commit hooks with pre-commit install and run pre-commit run --all-files or make black before pushing to ensure formatting and linting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/REvoDesign/application/font/font_manager.py` at line 15, This change
modifies the imports in font_manager.py (the line importing REvoDesign.Qt as
QtGui, QtWidgets) but the pre-commit/formatting hooks must be run before
pushing; run pre-commit install (to enable hooks) and then run pre-commit run
--all-files (or make black) and fix any formatting/lint errors reported, then
re-run the tests/lint until clean and re-commit the corrected file.
| if [ "$stage" == 'compile' ]; then echo Done with "$stage";exit 0; fi | ||
|
|
||
| # update translation files | ||
| for i in $(ls src/REvoDesign/UI/language/*.ts); do | ||
| echo "Updating $i" | ||
| lupdate src/REvoDesign/UI/REvoDesign.ui -ts $i | ||
| lupdate src/REvoDesign/UI/REvoDesign.ui -ts "$i" | ||
| done | ||
| echo "Translation files updated." | ||
| if [ $stage == 'translate' ]; then echo Done with $stage;exit 0; fi | ||
| if [ "$stage" == 'translate' ]; then echo Done with "$stage";exit 0; fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine the relevant lines
if [ -f tools/translate.sh ]; then
echo "=== File exists, examining lines 23-31 ==="
sed -n '23,31p' tools/translate.sh
echo ""
echo "=== Full file context (first 40 lines) ==="
head -40 tools/translate.sh
else
echo "File tools/translate.sh not found"
fiRepository: YaoYinYing/REvoDesign
Length of output: 1622
🏁 Script executed:
# Check for translation files in the expected directory
echo "=== Checking for translation files ==="
if [ -d src/REvoDesign/UI/language ]; then
ls -la src/REvoDesign/UI/language/
echo ""
echo "=== File names (with potential issues highlighted) ==="
find src/REvoDesign/UI/language -name '*.ts' -print0 | while IFS= read -r -d '' f; do
if [[ "$f" =~ [[:space:]] ]]; then
echo "⚠️ WHITESPACE: $f"
else
echo "✓ OK: $f"
fi
done
else
echo "Directory src/REvoDesign/UI/language not found"
fiRepository: YaoYinYing/REvoDesign
Length of output: 670
Replace for i in $(ls ...) with glob iteration.
Using ls in a for loop is fragile and breaks with whitespace or special characters in filenames. Replace with direct glob expansion, keeping the quoted "$i" usage you already added.
🔧 Suggested fix
-# update translation files
-for i in $(ls src/REvoDesign/UI/language/*.ts); do
- echo "Updating $i"
- lupdate src/REvoDesign/UI/REvoDesign.ui -ts "$i"
-done
+# update translation files
+for i in src/REvoDesign/UI/language/*.ts; do
+ [ -e "$i" ] || continue
+ echo "Updating $i"
+ lupdate src/REvoDesign/UI/REvoDesign.ui -ts "$i"
+done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ "$stage" == 'compile' ]; then echo Done with "$stage";exit 0; fi | |
| # update translation files | |
| for i in $(ls src/REvoDesign/UI/language/*.ts); do | |
| echo "Updating $i" | |
| lupdate src/REvoDesign/UI/REvoDesign.ui -ts $i | |
| lupdate src/REvoDesign/UI/REvoDesign.ui -ts "$i" | |
| done | |
| echo "Translation files updated." | |
| if [ $stage == 'translate' ]; then echo Done with $stage;exit 0; fi | |
| if [ "$stage" == 'translate' ]; then echo Done with "$stage";exit 0; fi | |
| if [ "$stage" == 'compile' ]; then echo Done with "$stage";exit 0; fi | |
| # update translation files | |
| for i in src/REvoDesign/UI/language/*.ts; do | |
| [ -e "$i" ] || continue | |
| echo "Updating $i" | |
| lupdate src/REvoDesign/UI/REvoDesign.ui -ts "$i" | |
| done | |
| echo "Translation files updated." | |
| if [ "$stage" == 'translate' ]; then echo Done with "$stage";exit 0; fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 26-26: Iterating over ls output is fragile. Use globs.
(SC2045)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/translate.sh` around lines 23 - 31, The for-loop uses command
substitution with ls which breaks on filenames with spaces; replace the loop
"for i in $(ls src/REvoDesign/UI/language/*.ts); do ... done" with direct glob
iteration (e.g. "for i in src/REvoDesign/UI/language/*.ts; do") while keeping
the quoted "$i" inside the loop (and optionally enable nullglob behavior if you
need to handle zero matches), ensuring the rest of the body (lupdate invocation)
remains unchanged and still references "$i".
| class TaskDatabase: | ||
| """Minimal SQLite-based task tracker for GREMLIN jobs.""" | ||
|
|
||
| DELETED_STATUSES = {"deleted:finshed", "deleted:cancel"} |
| logging.error(f"Failed to decode GitHub tags response as JSON: {e}") | ||
| return [] | ||
| except TypeError as e: | ||
| logging.error(f"Failed to parse GitHub tags response: {e}") |
|
|
||
| try: | ||
| imported_module = importlib.import_module(module) | ||
| except (ModuleNotFoundError, ImportError) as exc: |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/REvoDesign/tools/measure_utils.py (1)
648-719:⚠️ Potential issue | 🟡 MinorReturn type annotation
-> Measurementis wrong — function returnslist[Measurement].
hitsis assigned fromMeasurement.from_session_names(...), which returnslist["Measurement"], and is directlyreturn-ed at line 718. The annotation misleads callers and will fail any strict type-checking pass.🛠️ Proposed fix
-def read_measurement(start: str | int, debug: int = 0) -> Measurement: +def read_measurement(start: str | int, debug: int = 0) -> list[Measurement]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/tools/measure_utils.py` around lines 648 - 719, The return annotation on read_measurement is incorrect: hits is a list produced by Measurement.from_session_names and the function returns that list; update the signature of read_measurement to return a sequence type such as list[Measurement] (or list["Measurement"] if forward refs are needed) instead of -> Measurement, keeping the rest of the function unchanged and ensuring imports/typing compatibility for list[...] annotations.
♻️ Duplicate comments (2)
src/REvoDesign/clusters/combine_positions.py (1)
246-251:self.combi < 1still unsafe if the attribute is set to a string before callingrun_combinations.If a caller assigns
obj.combi = "2"(e.g., from CLI args) and then callsrun_combinations()directly (bypassingsetup()which doesint()), the comparison on line 250 raises aTypeErrorinstead of a cleanValueError. The fix proposed in the previous review — coercingself.combitointbefore the comparison — is still applicable here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/clusters/combine_positions.py` around lines 246 - 251, The comparison self.combi < 1 can raise TypeError if self.combi is a string; in the run_combinations method coerce self.combi to an int up front (e.g., attempt to set self.combi = int(self.combi)) inside a try/except and if conversion fails raise a ValueError with a clear message; then perform the existing check (self.combi < 1) and raise the current ValueError if needed so callers who passed string CLI args get a clean ValueError instead of a TypeError.src/REvoDesign/tools/package_manager.py (1)
160-161:⚠️ Potential issue | 🟡 MinorAdd
# noqa: S310to suppress Ruff S310 errors that will fail pre-commit.Lines 160–161 still lack the Ruff suppression (
# noqa: S310). Line 161 only has# nosec B310(Bandit), and line 160 has no suppression at all.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/tools/package_manager.py` around lines 160 - 161, The two urllib calls creating the Request and opening the URL (the line that assigns request = urllib.request.Request(url, headers=...) and the with urllib.request.urlopen(request, timeout=...) as response block) need Ruff suppression for S310 to avoid pre-commit failures; add "# noqa: S310" to the end of both those lines (keeping the existing "# nosec B310" on the urlopen line) so both the Request creation and the urlopen call include the Ruff suppression.
🧹 Nitpick comments (6)
tests/menu/shortcuts/test_vina_tools.py (1)
8-8: Run pre-commit and a targeted pytest keyword run for this test change.Please run
pre-commit installpluspre-commit run --all-files(ormake black), and a focused test likemake kw-test PYTEST_KW='vina_tools'to validate this change quickly.As per coding guidelines, Enable pre-commit hooks with
pre-commit installand runpre-commit run --all-filesormake blackbefore pushing to ensure formatting and linting; Runmake kw-test PYTEST_KW='<keyword>'for fast and specified testing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/menu/shortcuts/test_vina_tools.py` at line 8, Run the project's pre-commit hooks and formatters and then run a targeted pytest for the vina_tools tests: run "pre-commit install" followed by "pre-commit run --all-files" (or run "make black") to satisfy formatting/linting, then run the focused test suite with "make kw-test PYTEST_KW='vina_tools'" to validate the changes affecting the enlargebox/get_pca_box/getbox/movebox/rmhet/showaxes imports in the test; fix any lint/format/test failures and re-run the commands until clean.src/REvoDesign/clusters/combine_positions.py (1)
234-243: Inconsistent error-message style — string concatenation vs. f-strings used everywhere else.All other
ValueErrormessages added in this PR use f-strings (lines 63, 82, 247, 249, 251); this one uses+concatenation, which Ruff TRY003 also flags on the changed lines. Aligning to an f-string keeps the style consistent and may satisfy the pre-commit Ruff hook.♻️ Proposed refactor to use an f-string
- if aa != self.fastasequence[pos]: - raise ValueError( - "WT = " - + self.fastasequence[pos] - + str(pos + 1) - + " input AA: " - + aa - + " mut file contains: " - + eval_wt - ) + if aa != self.fastasequence[pos]: + raise ValueError( + f"WT mismatch at position {pos + 1}: expected {aa}, " + f"found {self.fastasequence[pos]}; mut file entry: {eval_wt}" + )As per coding guidelines,
pre-commit run --all-files(ormake black) should be run before pushing to ensure formatting and linting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/clusters/combine_positions.py` around lines 234 - 243, Replace the string concatenation in the ValueError inside combine_positions.py with a single f-string to match the project's style: construct the message using f"WT = {self.fastasequence[pos]}{pos + 1} input AA: {aa} mut file contains: {eval_wt}" (referencing self.fastasequence, pos, aa, eval_wt) so the error message is consistent with other ValueErrors and avoids + concatenation flagged by Ruff.src/REvoDesign/tools/package_manager.py (2)
418-430: Movereturn json_datato anelseblock to satisfy Ruff TRY300.
return json_dataat line 427 is still inside thetrybody. Ruff TRY300 prefers returns that are only reachable when no exception occurred to live in theelseclause, making the success path explicit.♻️ Proposed refactor
def fetch_gist_json(url: str) -> dict[str, Any]: try: data = _read_https_url(url, timeout=10).decode("utf-8") json_data = json.loads(data) - logging.debug("Extras table is fetched and parsed: \n" f"{json_data}") - - # Validate the structure of the fetched data - if not isinstance(json_data, dict): - logging.error("Error fetching or validating the JSON data: Fetched data is not a dictionary.") - return {} - return json_data except Exception as e: logging.error(f"Error fetching or validating the JSON data: {e}: ") return {} + else: + logging.debug("Extras table is fetched and parsed: \n%s", json_data) + if not isinstance(json_data, dict): + logging.error("Error fetching or validating the JSON data: Fetched data is not a dictionary.") + return {} + return json_data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/tools/package_manager.py` around lines 418 - 430, The return json_data is inside the try block which triggers Ruff TRY300; restructure the try/except so the successful-return path is in an else clause: keep the call to _read_https_url(...).decode and json.loads inside try, perform your isinstance(json_data, dict) validation and logging inside the try or after parsing but move the final successful return json_data into an else attached to the try (while retaining the logging.error and return {} in the except and the non-dict error path), referencing the parsed variable json_data and the helper _read_https_url to locate the code to change.
2404-2423: Uselogging.exceptioninstead oflogging.errorinsideexceptblocks (Ruff TRY400).All three newly added error-logging calls inside
exceptclauses uselogging.error, which discards the stack trace.logging.exceptionis idiomatic here and automatically includes the traceback without needing to embed{e}in the message string.♻️ Proposed refactor
try: tag_names = [tag["name"] for tag in tags] except (KeyError, TypeError) as e: - logging.error(f"Failed to extract tag names from GitHub response: {e}") + logging.exception("Failed to extract tag names from GitHub response") return [] return tag_names except HTTPError as e: logging.warning(f"GitHub API returned status code {e.code}") return [] except URLError as e: logging.error(f"Failed to reach the server. Reason: {e.reason}") return [] except json.JSONDecodeError as e: - logging.error(f"Failed to decode GitHub tags response as JSON: {e}") + logging.exception("Failed to decode GitHub tags response as JSON") return [] except TypeError as e: - logging.error(f"Failed to parse GitHub tags response: {e}") + logging.exception("Failed to parse GitHub tags response") return []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/tools/package_manager.py` around lines 2404 - 2423, The exception handlers in the function that extracts GitHub tag names use logging.error and lose tracebacks; replace logging.error(...) in the except blocks for (KeyError, TypeError) as e, for URLError as e, for json.JSONDecodeError as e, and for the final except TypeError as e with logging.exception(...) (remove the explicit "{e}" from the message since logging.exception adds the traceback), leaving the logging.warning for HTTPError unchanged; update the calls around the tag_names extraction block and the subsequent except handlers (the try/except surrounding tag parsing and the handlers for URLError, json.JSONDecodeError, and TypeError) to use logging.exception so full stack traces are recorded.src/REvoDesign/tools/measure_utils.py (2)
435-435:Optional["Measurement"]is inconsistent with the| Nonestyle adopted throughout this file.All other changed type annotations in this file (lines 155–162, 230–236, 332–337, 570) use the
X | Noneunion syntax; line 435 still usesOptional.♻️ Proposed fix
- def from_names_entry(cls, entry: Sequence[Any]) -> Optional["Measurement"]: + def from_names_entry(cls, entry: Sequence[Any]) -> "Measurement | None":🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/tools/measure_utils.py` at line 435, Update the return type annotation for the classmethod from_names_entry from Optional["Measurement"] to "Measurement | None" to match the file's modern union style; locate the method definition (def from_names_entry(cls, entry: Sequence[Any]) -> Optional["Measurement"]: ) and replace the Optional[...] with the PEP 604 union form Measurement | None (keeping the existing forward-reference quotes if used elsewhere in the file) so the annotation style is consistent with other functions like the ones around lines 155–162 and 332–337.
486-562:_build_uniqueid_to_atom_mapis dead code;_resolve_by_coordshas an unused parameter.
_build_uniqueid_to_atom_mapis never called anywhere in the codebase. TheMeasurement.atoms()method (lines 354–432) builds its unique_id map directly from_build_scene_atom_list, which it calls at line 368. While both methods traverse PyMOL objects similarly, they differ in unique_id extraction:_build_scene_atom_listonly uses the explicitunique_idattribute, whereas_build_uniqueid_to_atom_maptries multiple fallback attributes ("unique_id","uniq","id","serial"). If the more permissive fallback behavior is intentional,_build_uniqueid_to_atom_mapshould be integrated into the resolution flow; otherwise, remove it.Similarly,
_resolve_by_coords(lines 568–625) accepts auidparameter but never uses it. The method always constructs the returnedAtomDescriptorwithunique_id=Noneinstead ofunique_id=uid. If this method were integrated into the atom resolution pipeline, callers would receive descriptors without the queried unique_id.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/tools/measure_utils.py` around lines 486 - 562, _build_uniqueid_to_atom_map is dead and _resolve_by_coords ignores its uid parameter; either remove the unused function or integrate it and fix the coord resolver: update Measurement.atoms (which currently calls _build_scene_atom_list) to call _build_uniqueid_to_atom_map if you want the permissive fallback lookup (so unique_id extraction tries "unique_id","uniq","id","serial"), or delete _build_uniqueid_to_atom_map and its tests/usages; and in _resolve_by_coords ensure the uid parameter is used — set the returned AtomDescriptor.unique_id to uid (or remove the uid parameter if not needed) and delete the unused variable to avoid confusion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/REvoDesign/magician/designers/cart_ddg.py`:
- Around line 46-47: Remove the unused **kwargs parameter from the __init__
method of the designer class so its signature matches the base class; change def
__init__(self, molecule: str, **kwargs): to only accept molecule, call
super().__init__(molecule) unchanged, and ensure no other code relies on passing
keyword args into this constructor (update callers if any). This aligns the
class __init__ with ExternalDesignerAbstract.__init__ and removes the dead
parameter.
In `@src/REvoDesign/tools/package_manager.py`:
- Around line 70-88: The five static methods (debug, info, warning, error,
critical) in MockLogger currently declare unused variadic parameters as *args
and **kwargs which triggers Ruff ARG004; rename them to *_args and **_kwargs in
each method signature (debug, info, warning, error, critical) to mark them as
intentionally unused and keep the logging-compatible signature, then run
pre-commit to ensure ARG004 is resolved and there are no other references
depending on the original parameter names.
In `@tests/server/test_pssm_gremlin.py`:
- Line 1596: Remove the unused parameter by editing the test function signature
for test_server_image_handles_authenticated_requests to drop the tmp_path
argument (keep miniuc_databases, runner_image_tag, server_image_tag), so the
function no longer declares tmp_path which triggers Ruff linting; update only
the def line for test_server_image_handles_authenticated_requests to remove
tmp_path and ensure no other references to tmp_path exist in that test.
---
Outside diff comments:
In `@src/REvoDesign/tools/measure_utils.py`:
- Around line 648-719: The return annotation on read_measurement is incorrect:
hits is a list produced by Measurement.from_session_names and the function
returns that list; update the signature of read_measurement to return a sequence
type such as list[Measurement] (or list["Measurement"] if forward refs are
needed) instead of -> Measurement, keeping the rest of the function unchanged
and ensuring imports/typing compatibility for list[...] annotations.
---
Duplicate comments:
In `@src/REvoDesign/clusters/combine_positions.py`:
- Around line 246-251: The comparison self.combi < 1 can raise TypeError if
self.combi is a string; in the run_combinations method coerce self.combi to an
int up front (e.g., attempt to set self.combi = int(self.combi)) inside a
try/except and if conversion fails raise a ValueError with a clear message; then
perform the existing check (self.combi < 1) and raise the current ValueError if
needed so callers who passed string CLI args get a clean ValueError instead of a
TypeError.
In `@src/REvoDesign/tools/package_manager.py`:
- Around line 160-161: The two urllib calls creating the Request and opening the
URL (the line that assigns request = urllib.request.Request(url, headers=...)
and the with urllib.request.urlopen(request, timeout=...) as response block)
need Ruff suppression for S310 to avoid pre-commit failures; add "# noqa: S310"
to the end of both those lines (keeping the existing "# nosec B310" on the
urlopen line) so both the Request creation and the urlopen call include the Ruff
suppression.
---
Nitpick comments:
In `@src/REvoDesign/clusters/combine_positions.py`:
- Around line 234-243: Replace the string concatenation in the ValueError inside
combine_positions.py with a single f-string to match the project's style:
construct the message using f"WT = {self.fastasequence[pos]}{pos + 1} input AA:
{aa} mut file contains: {eval_wt}" (referencing self.fastasequence, pos, aa,
eval_wt) so the error message is consistent with other ValueErrors and avoids +
concatenation flagged by Ruff.
In `@src/REvoDesign/tools/measure_utils.py`:
- Line 435: Update the return type annotation for the classmethod
from_names_entry from Optional["Measurement"] to "Measurement | None" to match
the file's modern union style; locate the method definition (def
from_names_entry(cls, entry: Sequence[Any]) -> Optional["Measurement"]: ) and
replace the Optional[...] with the PEP 604 union form Measurement | None
(keeping the existing forward-reference quotes if used elsewhere in the file) so
the annotation style is consistent with other functions like the ones around
lines 155–162 and 332–337.
- Around line 486-562: _build_uniqueid_to_atom_map is dead and
_resolve_by_coords ignores its uid parameter; either remove the unused function
or integrate it and fix the coord resolver: update Measurement.atoms (which
currently calls _build_scene_atom_list) to call _build_uniqueid_to_atom_map if
you want the permissive fallback lookup (so unique_id extraction tries
"unique_id","uniq","id","serial"), or delete _build_uniqueid_to_atom_map and its
tests/usages; and in _resolve_by_coords ensure the uid parameter is used — set
the returned AtomDescriptor.unique_id to uid (or remove the uid parameter if not
needed) and delete the unused variable to avoid confusion.
In `@src/REvoDesign/tools/package_manager.py`:
- Around line 418-430: The return json_data is inside the try block which
triggers Ruff TRY300; restructure the try/except so the successful-return path
is in an else clause: keep the call to _read_https_url(...).decode and
json.loads inside try, perform your isinstance(json_data, dict) validation and
logging inside the try or after parsing but move the final successful return
json_data into an else attached to the try (while retaining the logging.error
and return {} in the except and the non-dict error path), referencing the parsed
variable json_data and the helper _read_https_url to locate the code to change.
- Around line 2404-2423: The exception handlers in the function that extracts
GitHub tag names use logging.error and lose tracebacks; replace
logging.error(...) in the except blocks for (KeyError, TypeError) as e, for
URLError as e, for json.JSONDecodeError as e, and for the final except TypeError
as e with logging.exception(...) (remove the explicit "{e}" from the message
since logging.exception adds the traceback), leaving the logging.warning for
HTTPError unchanged; update the calls around the tag_names extraction block and
the subsequent except handlers (the try/except surrounding tag parsing and the
handlers for URLError, json.JSONDecodeError, and TypeError) to use
logging.exception so full stack traces are recorded.
In `@tests/menu/shortcuts/test_vina_tools.py`:
- Line 8: Run the project's pre-commit hooks and formatters and then run a
targeted pytest for the vina_tools tests: run "pre-commit install" followed by
"pre-commit run --all-files" (or run "make black") to satisfy
formatting/linting, then run the focused test suite with "make kw-test
PYTEST_KW='vina_tools'" to validate the changes affecting the
enlargebox/get_pca_box/getbox/movebox/rmhet/showaxes imports in the test; fix
any lint/format/test failures and re-run the commands until clean.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
CHANGELOG.mdplayground/print_mut.pyserver/README.legacy.mdserver/README.mdserver/pssm_gremlin/pssm_gremlin.pysrc/REvoDesign/application/__init__.pysrc/REvoDesign/clusters/combine_positions.pysrc/REvoDesign/common/mutant_tree.pysrc/REvoDesign/data/__init__.pysrc/REvoDesign/driver/__init__.pysrc/REvoDesign/editor/monaco/__init__.pysrc/REvoDesign/magician/designers/cart_ddg.pysrc/REvoDesign/tools/__init__.pysrc/REvoDesign/tools/measure_utils.pysrc/REvoDesign/tools/package_manager.pysrc/REvoDesign/tools/safe_pickle.pytests/__init__.pytests/basic/__init__.pytests/basic/test_designer_abc.pytests/bootstrap/__init__.pytests/cases/__init__.pytests/cases/tabs/__init__.pytests/citations/__init__.pytests/conftest.pytests/editor/__init__.pytests/menu/shortcuts/__init__.pytests/menu/shortcuts/test_vina_tools.pytests/server/test_pssm_gremlin.pytests/tools/__init__.pytests/tools/test_pm_ui.pytests/tools/test_utils.pytools/license_notice.py
💤 Files with no reviewable changes (17)
- tests/citations/init.py
- tests/menu/shortcuts/init.py
- tests/tools/init.py
- tests/editor/init.py
- tools/license_notice.py
- tests/basic/init.py
- tests/cases/init.py
- playground/print_mut.py
- src/REvoDesign/tools/init.py
- tests/bootstrap/init.py
- src/REvoDesign/data/init.py
- tests/init.py
- src/REvoDesign/driver/init.py
- src/REvoDesign/editor/monaco/init.py
- tests/basic/test_designer_abc.py
- tests/cases/tabs/init.py
- src/REvoDesign/application/init.py
✅ Files skipped from review due to trivial changes (2)
- tests/conftest.py
- tests/tools/test_pm_ui.py
🚧 Files skipped from review as they are similar to previous changes (2)
- server/README.legacy.md
- src/REvoDesign/common/mutant_tree.py
| def __init__(self, molecule: str, **kwargs): | ||
| super().__init__(molecule) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate and examine ExternalDesignerAbstract class definition
find . -type f -name "*.py" | xargs rg -l "class ExternalDesignerAbstract"Repository: YaoYinYing/REvoDesign
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Examine ExternalDesignerAbstract class and its __init__ method
rg -n -A10 "class ExternalDesignerAbstract" src/REvoDesign/basic/designer.py
echo "---"
rg -n -A5 "def __init__" src/REvoDesign/basic/designer.py | head -20Repository: YaoYinYing/REvoDesign
Length of output: 986
🏁 Script executed:
#!/bin/bash
# Check cart_ddg.py __init__ implementation
rg -n -A5 "def __init__" src/REvoDesign/magician/designers/cart_ddg.pyRepository: YaoYinYing/REvoDesign
Length of output: 229
Remove **kwargs from the signature—the base class ExternalDesignerAbstract.__init__ only accepts molecule, not kwargs.
The __init__ method accepts **kwargs but the base class doesn't. These kwargs are silently ignored, creating dead parameters.
💡 Suggested change
- def __init__(self, molecule: str, **kwargs):
+ def __init__(self, molecule: str):
super().__init__(molecule)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __init__(self, molecule: str, **kwargs): | |
| super().__init__(molecule) | |
| def __init__(self, molecule: str): | |
| super().__init__(molecule) |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 46-46: Unused method argument: kwargs
(ARG002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/REvoDesign/magician/designers/cart_ddg.py` around lines 46 - 47, Remove
the unused **kwargs parameter from the __init__ method of the designer class so
its signature matches the base class; change def __init__(self, molecule: str,
**kwargs): to only accept molecule, call super().__init__(molecule) unchanged,
and ensure no other code relies on passing keyword args into this constructor
(update callers if any). This aligns the class __init__ with
ExternalDesignerAbstract.__init__ and removes the dead parameter.
| @staticmethod | ||
| def debug(msg: str, *args, **kwargs): | ||
| print(f"[DEBUG]: {msg}") if LOGGER_LEVEL < 10 else None | ||
|
|
||
| def info(self, msg: str, *args, **kwargs): | ||
| @staticmethod | ||
| def info(msg: str, *args, **kwargs): | ||
| print(f"[INFO]: {msg}") if LOGGER_LEVEL < 20 else None | ||
|
|
||
| def warning(self, msg: str, *args, **kwargs): | ||
| @staticmethod | ||
| def warning(msg: str, *args, **kwargs): | ||
| print(f"[WARNING]: {msg}") if LOGGER_LEVEL < 30 else None | ||
|
|
||
| def error(self, msg: str, *args, **kwargs): | ||
| @staticmethod | ||
| def error(msg: str, *args, **kwargs): | ||
| print(f"[ERROR]: {msg}") if LOGGER_LEVEL < 40 else None | ||
|
|
||
| def critical(self, msg: str, *args, **kwargs): | ||
| @staticmethod | ||
| def critical(msg: str, *args, **kwargs): | ||
| print(f"[CRITICAL]: {msg}") if LOGGER_LEVEL < 50 else None |
There was a problem hiding this comment.
Rename *args, **kwargs to *_args, **_kwargs to suppress ARG004 pre-commit failures.
Ruff ARG004 is active in this project and will flag all five MockLogger static methods for unused *args/**kwargs. The arguments are intentionally kept to match the real logging module's interface, but the underscore-prefixed convention signals that explicitly.
🛠️ Proposed fix
`@staticmethod`
-def debug(msg: str, *args, **kwargs):
+def debug(msg: str, *_args, **_kwargs):
print(f"[DEBUG]: {msg}") if LOGGER_LEVEL < 10 else None
`@staticmethod`
-def info(msg: str, *args, **kwargs):
+def info(msg: str, *_args, **_kwargs):
print(f"[INFO]: {msg}") if LOGGER_LEVEL < 20 else None
`@staticmethod`
-def warning(msg: str, *args, **kwargs):
+def warning(msg: str, *_args, **_kwargs):
print(f"[WARNING]: {msg}") if LOGGER_LEVEL < 30 else None
`@staticmethod`
-def error(msg: str, *args, **kwargs):
+def error(msg: str, *_args, **_kwargs):
print(f"[ERROR]: {msg}") if LOGGER_LEVEL < 40 else None
`@staticmethod`
-def critical(msg: str, *args, **kwargs):
+def critical(msg: str, *_args, **_kwargs):
print(f"[CRITICAL]: {msg}") if LOGGER_LEVEL < 50 else NoneAs per coding guidelines, pre-commit run --all-files must pass before pushing; Ruff ARG004 is currently active and will fail on all five methods.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @staticmethod | |
| def debug(msg: str, *args, **kwargs): | |
| print(f"[DEBUG]: {msg}") if LOGGER_LEVEL < 10 else None | |
| def info(self, msg: str, *args, **kwargs): | |
| @staticmethod | |
| def info(msg: str, *args, **kwargs): | |
| print(f"[INFO]: {msg}") if LOGGER_LEVEL < 20 else None | |
| def warning(self, msg: str, *args, **kwargs): | |
| @staticmethod | |
| def warning(msg: str, *args, **kwargs): | |
| print(f"[WARNING]: {msg}") if LOGGER_LEVEL < 30 else None | |
| def error(self, msg: str, *args, **kwargs): | |
| @staticmethod | |
| def error(msg: str, *args, **kwargs): | |
| print(f"[ERROR]: {msg}") if LOGGER_LEVEL < 40 else None | |
| def critical(self, msg: str, *args, **kwargs): | |
| @staticmethod | |
| def critical(msg: str, *args, **kwargs): | |
| print(f"[CRITICAL]: {msg}") if LOGGER_LEVEL < 50 else None | |
| `@staticmethod` | |
| def debug(msg: str, *_args, **_kwargs): | |
| print(f"[DEBUG]: {msg}") if LOGGER_LEVEL < 10 else None | |
| `@staticmethod` | |
| def info(msg: str, *_args, **_kwargs): | |
| print(f"[INFO]: {msg}") if LOGGER_LEVEL < 20 else None | |
| `@staticmethod` | |
| def warning(msg: str, *_args, **_kwargs): | |
| print(f"[WARNING]: {msg}") if LOGGER_LEVEL < 30 else None | |
| `@staticmethod` | |
| def error(msg: str, *_args, **_kwargs): | |
| print(f"[ERROR]: {msg}") if LOGGER_LEVEL < 40 else None | |
| `@staticmethod` | |
| def critical(msg: str, *_args, **_kwargs): | |
| print(f"[CRITICAL]: {msg}") if LOGGER_LEVEL < 50 else None |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 71-71: Unused static method argument: args
(ARG004)
[warning] 71-71: Unused static method argument: kwargs
(ARG004)
[warning] 75-75: Unused static method argument: args
(ARG004)
[warning] 75-75: Unused static method argument: kwargs
(ARG004)
[warning] 79-79: Unused static method argument: args
(ARG004)
[warning] 79-79: Unused static method argument: kwargs
(ARG004)
[warning] 83-83: Unused static method argument: args
(ARG004)
[warning] 83-83: Unused static method argument: kwargs
(ARG004)
[warning] 87-87: Unused static method argument: args
(ARG004)
[warning] 87-87: Unused static method argument: kwargs
(ARG004)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/REvoDesign/tools/package_manager.py` around lines 70 - 88, The five
static methods (debug, info, warning, error, critical) in MockLogger currently
declare unused variadic parameters as *args and **kwargs which triggers Ruff
ARG004; rename them to *_args and **_kwargs in each method signature (debug,
info, warning, error, critical) to mark them as intentionally unused and keep
the logging-compatible signature, then run pre-commit to ensure ARG004 is
resolved and there are no other references depending on the original parameter
names.
| def test_server_image_handles_authenticated_requests( | ||
| miniuc_databases, runner_image_tag, server_image_tag, tmp_path | ||
| ): | ||
| def test_server_image_handles_authenticated_requests(miniuc_databases, runner_image_tag, server_image_tag, tmp_path): |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Verify the unused argument in the test signature and its usage
rg -A 30 "def test_server_image_handles_authenticated_requests" tests/server/test_pssm_gremlin.pyRepository: YaoYinYing/REvoDesign
Length of output: 1275
🏁 Script executed:
# Get the complete function by searching for the next function definition
# This will help us see the entire test_server_image_handles_authenticated_requests function
rg -n "def test_server_image_handles_authenticated_requests" tests/server/test_pssm_gremlin.py -A 100 | head -120Repository: YaoYinYing/REvoDesign
Length of output: 4396
Remove unused tmp_path argument from test function signature.
Line 1596: tmp_path is unused in the function body; remove it to satisfy Ruff linting rules.
🧹 Suggested fix
-def test_server_image_handles_authenticated_requests(miniuc_databases, runner_image_tag, server_image_tag, tmp_path):
+def test_server_image_handles_authenticated_requests(miniuc_databases, runner_image_tag, server_image_tag):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_server_image_handles_authenticated_requests(miniuc_databases, runner_image_tag, server_image_tag, tmp_path): | |
| def test_server_image_handles_authenticated_requests(miniuc_databases, runner_image_tag, server_image_tag): |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 1596-1596: Unused function argument: tmp_path
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/server/test_pssm_gremlin.py` at line 1596, Remove the unused parameter
by editing the test function signature for
test_server_image_handles_authenticated_requests to drop the tmp_path argument
(keep miniuc_databases, runner_image_tag, server_image_tag), so the function no
longer declares tmp_path which triggers Ruff linting; update only the def line
for test_server_image_handles_authenticated_requests to remove tmp_path and
ensure no other references to tmp_path exist in that test.
[ci skip]
| RUN_PSSM() { | ||
| local fasta=$(readlink -f $1) | ||
| local fasta_fn=$(basename $1) | ||
| local fasta=$(readlink -f "$1") |
| local fasta=$(readlink -f $1) | ||
| local fasta_fn=$(basename $1) | ||
| local fasta=$(readlink -f "$1") | ||
| local fasta_fn=$(basename "$1") |
[ci skip]
* save * save * save * save * save * save * save [ci skip] * lint [ci skip] * save * Update test_pssm_gremlin.py * save * save * save * save [ci skip] * Update dialog_hooks.py * save * revert logout * revert server tests * Revert "revert server tests" This reverts commit 6740aa8. * Update test_pssm_gremlin.py * server test back to #164 _wait_for_server_ready * drop server test helper case test_wait_for_server_ready_retries_transient_401 * save [ci skip]
1 participant
Summary by CodeRabbit