fix(many): deepsource issue collection 2 - #165
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR pins CI workflow actions to specific commit SHAs, hardens server path handling and task-ID validation, changes delete endpoints to use DELETE, adds a logout route and animated theme toggle, propagates new env vars (including MAXMEM) through Docker/compose, tightens shell scripts, refactors small utilities, updates docs/tests, and tweaks badges/changelog. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: Hygiene |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Feb 24, 2026 3:01p.m. | Review ↗ |
![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: 8c5225b35b
ℹ️ 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".
Comment on lines
+10
to
+12
| "xargs -0 cat", | ||
| "> ./program.docx", | ||
| ].join(" | "); |
There was a problem hiding this comment.
The command array currently joins into find ... -print0 | xargs -0 cat | > ./program.docx, and that trailing pipe makes > ./program.docx its own (empty) pipeline stage instead of redirecting cat output. In practice this truncates/creates program.docx but writes no source content, so the script now produces an empty file even when Python files are found.
Useful? React with 👍 / 👎.
❌ 1 blocking issue (6 total)
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
| def run_gremlin(md5sum): | ||
| md5sum = _normalize_task_id(md5sum) | ||
| if md5sum is None: | ||
| return jsonify({"status": "bad_request", "message": "Invalid task id"}), 400 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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
🧹 Nitpick comments (9)
src/REvoDesign/shortcuts/dialog_hooks.py (3)
45-46:get_all_object_names()andget_all_objects()are identical.Both return
cmd.get_names("objects"). One should delegate to the other to avoid duplication.♻️ Proposed fix
def get_all_objects(): - return cmd.get_names("objects") + return get_all_object_names()Also applies to: 53-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/shortcuts/dialog_hooks.py` around lines 45 - 46, get_all_object_names() and get_all_objects() are duplicates; change one to delegate to the other to remove redundancy. For example, keep the canonical implementation using cmd.get_names("objects") in get_all_objects() (or get_all_object_names()) and make the other function simply return get_all_objects() (or get_all_object_names()); update both occurrences referenced by the symbols get_all_object_names and get_all_objects so only one contains the direct cmd.get_names call.
27-29:get_designable_chain_ids()andget_all_chain_ids()are identical.Both functions have the exact same body. One should delegate to the other, or they should be consolidated into a single function.
♻️ Proposed fix
def get_all_chain_ids() -> list[str]: - designable = ConfigBus().get_value("designable_sequences", dict, reject_none=True, cfg="runtime") - return list(designable.keys()) + return get_designable_chain_ids()Also applies to: 40-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/shortcuts/dialog_hooks.py` around lines 27 - 29, The two functions get_designable_chain_ids and get_all_chain_ids share identical bodies; remove duplication by making one delegate to the other (e.g., implement get_all_chain_ids() to return get_designable_chain_ids()) or consolidate them into a single function and update all call sites; ensure the unique logic of reading ConfigBus().get_value("designable_sequences", dict, reject_none=True, cfg="runtime") remains in only one function (referenced by the function names get_designable_chain_ids and get_all_chain_ids).
32-33: Nit: prefer unpacking over list concatenation.Per the Ruff linter hint (RUF005), consider using unpacking syntax for clarity.
♻️ Proposed fix
def get_selections() -> list[str]: - return [""] + list(cmd.get_names("selections")) + return ["", *list(cmd.get_names("selections"))]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/shortcuts/dialog_hooks.py` around lines 32 - 33, In get_selections(), replace the list concatenation that builds [""] + list(cmd.get_names("selections")) with list unpacking to satisfy RUF005; specifically, return a new list that starts with the empty string and unpacks the iterable from cmd.get_names("selections") (i.e., use unpacking with the get_selections function and cmd.get_names call).src/REvoDesign/tools/utils.py (1)
40-49: Wrappers lose type information from the original signatures.The originals in
package_manager.pyhave detailed@overloadsignatures. These*args, **kwargswrappers erase all type hints, so callers importing fromutilsget no IDE autocompletion or type checking. Consider re-exporting with type stubs or forwarding the overload signatures.🤖 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 40 - 49, The wrapper functions run_command and run_worker_thread_in_pool currently use *args/**kwargs which strips the original `@overload` type signatures from package_manager; fix by preserving and forwarding the original signatures: either re-export the functions directly (from .package_manager import run_command, run_worker_thread_in_pool) so type checkers see the original overloads, or copy the `@overload` declarations from package_manager into utils and implement thin forwarding bodies that call _run_command/_run_worker_thread_in_pool; reference the original symbols run_command and run_worker_thread_in_pool when making the change so IDEs/type checkers pick up the proper signatures.server/pssm_gremlin/pssm_gremlin.py (3)
534-538:tempfile.gettempdir()fallback is safer than the previous approach.Good improvement. Note that
except BaseExceptionon line 536 (pre-existing) is overly broad — it catchesSystemExitandKeyboardInterruptin addition toOSError. If you ever revisit this block, narrowing toOSErrorwould be more precise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/pssm_gremlin/pssm_gremlin.py` around lines 534 - 538, The try/except around computing _ROOT_MOUNT_DIRECTORY currently catches BaseException which is too broad; change the except to catch OSError (or more specific exceptions like OSError and IOError if needed) so only login/filesystem errors are handled, keep the fallback to tempfile.gettempdir() and the os.makedirs(_ROOT_MOUNT_DIRECTORY, exist_ok=True) call unchanged, and ensure the logic still assigns _ROOT_MOUNT_DIRECTORY when os.getlogin() fails.
1391-1428: Batch delete still usesPOST— inconsistent HTTP method with single-taskDELETE.The batch delete endpoint (
/PSSM_GREMLIN/api/delete) at line 1261 usesmethods=["POST"], which is reasonable since it accepts a JSON body with multiple IDs. However, havingDELETEfor single-task andPOSTfor batch-task on the same base path (/api/delete) is a minor REST design inconsistency. This isn't a bug — just a design note. If you choose to keep it as-is (which is fine for pragmatic reasons —DELETEwith a JSON body is not universally well-supported), consider documenting the rationale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/pssm_gremlin/pssm_gremlin.py` around lines 1391 - 1428, The batch-delete route at "/PSSM_GREMLIN/api/delete" currently only accepts POST while single-item delete uses DELETE; to make the HTTP methods consistent either (A) allow DELETE for the batch endpoint by changing its route decorator to accept methods=["POST","DELETE"] so clients can use DELETE with a JSON body, or (B) rename the batch endpoint to a distinct path such as "/PSSM_GREMLIN/api/delete/batch" (keeping methods=["POST"]) and add a short comment documenting why POST is used for batch; update any clients/tests that call "/PSSM_GREMLIN/api/delete" accordingly.
589-607: Good defense-in-depth for artifact deletion, with one observation.The root/home directory guard (line 595) and the
_path_is_withincheck (line 598) are good layered protections. Note that for legacy rows withresult_diroutsideRESULTS_FOLDER, the code logs a warning but still proceeds withshutil.rmtree. This is a deliberate backward-compatibility choice. Consider whether legacy paths should also be checked against a broader deny-list (e.g.,/etc,/usr,/var) to further limit blast radius.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/pssm_gremlin/pssm_gremlin.py` around lines 589 - 607, The deletion logic in _delete_task_artifacts allows legacy result_dir values outside app.config["RESULTS_FOLDER"] to be removed after only checking for root/home; extend the safety checks by rejecting additional system-critical directories (e.g., /etc, /usr, /var, /bin, /sbin) before calling shutil.rmtree. Update _delete_task_artifacts to build a deny-list of absolute paths (using os.path.abspath) and check safe_result_dir against that list (in addition to the existing root/home guards and _path_is_within check), logging and skipping deletion when safe_result_dir matches any deny-list entry.server/docker-compose.yml (1)
49-49: Inconsistent default image tag betweenwebandworkerservices.Line 49 (
web) still defaults torevodesign-pssm-gremlin-server-non-root:latest, while the shared env (line 4) and the updatedworker(line 83) both default torevodesign-pssm-gremlin-server-non-rootwithout the:latesttag. While Docker treats an untagged image as:latestimplicitly, the textual mismatch can confuse operators.Also applies to: 83-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/docker-compose.yml` at line 49, The web service image default includes a trailing :latest while the shared env and worker use the untagged form; update the image declarations for both the web and worker services to use the same default token (${SERVER_IMAGE:-revodesign-pssm-gremlin-server-non-root}) so the textual defaults match (i.e., remove the explicit :latest from the web service or make both include :latest—prefer aligning to the shared env which is untagged); modify the image lines referenced for the web and worker services accordingly.server/pssm_gremlin/templates/pssm_gremlin_dashboard.html (1)
1374-1389: Logout via HTTP Basic Auth credential override — works but is inherently browser-dependent.The technique of sending a request with bogus credentials (
"logout":"logout") to invalidate the browser's cached Basic Auth is a well-known workaround. Be aware that some browsers (notably certain Safari versions) may not reliably clear the cached credentials this way. There's no universal client-side logout for HTTP Basic Auth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/pssm_gremlin/templates/pssm_gremlin_dashboard.html` around lines 1374 - 1389, The client-side logout in function triggerLogout uses bogus HTTP Basic Auth credentials (xhr.open(..., "logout","logout")) which is unreliable across browsers; replace this approach by having triggerLogout perform a plain redirect to a server-side logout endpoint (e.g., POST/GET to "/PSSM_GREMLIN/logout" or to "/PSSM_GREMLIN/create_task" that your backend handles) and implement server-side logic to invalidate any server session or return a 401/WWW-Authenticate header to force credential re-prompt; update triggerLogout to remove the bogus credentials, send a simple fetch/redirect to the new logout endpoint, and ensure the server-side logout handler clears auth state or returns 401 so browsers will stop using cached Basic Auth.
🤖 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.test`:
- Around line 29-33: Update the incorrect section comment above the runner
environment variables: replace the "## redis settings" comment with a clear "##
Runner identity settings" (or similar) so it correctly describes the block
containing RUNNER_UID, RUNNER_GID, RUNNER_USERNAME and RUNNER_GROUP; ensure the
new comment sits immediately above those variables and remove any leftover
copy-paste text referring to Redis.
In `@server/docker/server/Dockerfile`:
- Line 38: The CMD in the Dockerfile currently runs Gunicorn via a shell wrapper
so Gunicorn is not PID 1; update the CMD so the shell execs Gunicorn (i.e.,
prefix the gunicorn invocation with exec inside the existing "sh -c" command) so
that the gunicorn process (pssm_gremlin:app) becomes PID 1 and receives signals
for proper graceful shutdown and signal handling.
In `@server/run/restart_pssm_flask.sh`:
- Line 206: The script currently forces DOMAIN="0.0.0.0", overriding any
env-provided value; change this to only set the default when DOMAIN is unset or
empty (e.g., use shell parameter expansion or a conditional assignment) so a
user-provided DOMAIN in the env file is preserved and any printed URLs reflect
the actual DOMAIN value; update the DOMAIN assignment site in
restart_pssm_flask.sh and any places that print the URL to rely on that
preserved DOMAIN variable.
In `@server/scripts/make_paired_MSA_simple.py`:
- Line 105: The diff shows creation of the variable paired_data in
make_paired_MSA_simple.py; ensure you ran the project's pre-commit hooks and
linters before pushing: run pre-commit install (once) and then pre-commit run
--all-files (or make black) to apply formatting and lint fixes, re-run tests,
and amend the commit so paired_data and surrounding code conform to the
repository style rules.
In `@src/REvoDesign/shortcuts/dialog_hooks.py`:
- Around line 36-37: The function find_all_small_molecules_in_protein currently
returns find_small_molecules_in_protein("(all)") or None which collapses an
empty list (no matches) into None and loses the distinction from the None result
when no selection was provided; update find_all_small_molecules_in_protein to
return the raw result of find_small_molecules_in_protein("(all)") (i.e., remove
the "or None") so [] is preserved, or if collapsing is intentional add a clear
comment explaining why the empty-list -> None coercion is desired and
acceptable.
- Around line 23-24: get_fasta_writer_choices currently reads the private
SeqIO._FormatToWriter dict; replace this fragile private-API access with a safe
approach: attempt to read SeqIO._FormatToWriter inside a try block but fall back
to a hardcoded list of known FASTA writers (e.g., "fasta", "fasta-2line") if
that attribute is missing or raises; update the function
get_fasta_writer_choices to prefer the dynamic list when available and return
the fallback list on any exception to avoid depending on Biopython internals.
In `@tools/copyright.js`:
- Around line 8-16: The shell pipeline string in the command variable is ending
with a stray pipe before the redirection, producing invalid syntax; fix the
command construction used by command (and thus spawnSync) so the redirection is
applied to the pipeline output rather than piped into a bare redirection token —
e.g. build the string as "find ./src/REvoDesign -type f -name '*.py' -print0 |
xargs -0 cat > ./program.docx" (remove the trailing " | " before ">
./program.docx" or move the ">" onto the last pipeline segment) so
spawnSync("bash", ["-lc", command], ...) receives a valid shell command.
---
Nitpick comments:
In `@server/docker-compose.yml`:
- Line 49: The web service image default includes a trailing :latest while the
shared env and worker use the untagged form; update the image declarations for
both the web and worker services to use the same default token
(${SERVER_IMAGE:-revodesign-pssm-gremlin-server-non-root}) so the textual
defaults match (i.e., remove the explicit :latest from the web service or make
both include :latest—prefer aligning to the shared env which is untagged);
modify the image lines referenced for the web and worker services accordingly.
In `@server/pssm_gremlin/pssm_gremlin.py`:
- Around line 534-538: The try/except around computing _ROOT_MOUNT_DIRECTORY
currently catches BaseException which is too broad; change the except to catch
OSError (or more specific exceptions like OSError and IOError if needed) so only
login/filesystem errors are handled, keep the fallback to tempfile.gettempdir()
and the os.makedirs(_ROOT_MOUNT_DIRECTORY, exist_ok=True) call unchanged, and
ensure the logic still assigns _ROOT_MOUNT_DIRECTORY when os.getlogin() fails.
- Around line 1391-1428: The batch-delete route at "/PSSM_GREMLIN/api/delete"
currently only accepts POST while single-item delete uses DELETE; to make the
HTTP methods consistent either (A) allow DELETE for the batch endpoint by
changing its route decorator to accept methods=["POST","DELETE"] so clients can
use DELETE with a JSON body, or (B) rename the batch endpoint to a distinct path
such as "/PSSM_GREMLIN/api/delete/batch" (keeping methods=["POST"]) and add a
short comment documenting why POST is used for batch; update any clients/tests
that call "/PSSM_GREMLIN/api/delete" accordingly.
- Around line 589-607: The deletion logic in _delete_task_artifacts allows
legacy result_dir values outside app.config["RESULTS_FOLDER"] to be removed
after only checking for root/home; extend the safety checks by rejecting
additional system-critical directories (e.g., /etc, /usr, /var, /bin, /sbin)
before calling shutil.rmtree. Update _delete_task_artifacts to build a deny-list
of absolute paths (using os.path.abspath) and check safe_result_dir against that
list (in addition to the existing root/home guards and _path_is_within check),
logging and skipping deletion when safe_result_dir matches any deny-list entry.
In `@server/pssm_gremlin/templates/pssm_gremlin_dashboard.html`:
- Around line 1374-1389: The client-side logout in function triggerLogout uses
bogus HTTP Basic Auth credentials (xhr.open(..., "logout","logout")) which is
unreliable across browsers; replace this approach by having triggerLogout
perform a plain redirect to a server-side logout endpoint (e.g., POST/GET to
"/PSSM_GREMLIN/logout" or to "/PSSM_GREMLIN/create_task" that your backend
handles) and implement server-side logic to invalidate any server session or
return a 401/WWW-Authenticate header to force credential re-prompt; update
triggerLogout to remove the bogus credentials, send a simple fetch/redirect to
the new logout endpoint, and ensure the server-side logout handler clears auth
state or returns 401 so browsers will stop using cached Basic Auth.
In `@src/REvoDesign/shortcuts/dialog_hooks.py`:
- Around line 45-46: get_all_object_names() and get_all_objects() are
duplicates; change one to delegate to the other to remove redundancy. For
example, keep the canonical implementation using cmd.get_names("objects") in
get_all_objects() (or get_all_object_names()) and make the other function simply
return get_all_objects() (or get_all_object_names()); update both occurrences
referenced by the symbols get_all_object_names and get_all_objects so only one
contains the direct cmd.get_names call.
- Around line 27-29: The two functions get_designable_chain_ids and
get_all_chain_ids share identical bodies; remove duplication by making one
delegate to the other (e.g., implement get_all_chain_ids() to return
get_designable_chain_ids()) or consolidate them into a single function and
update all call sites; ensure the unique logic of reading
ConfigBus().get_value("designable_sequences", dict, reject_none=True,
cfg="runtime") remains in only one function (referenced by the function names
get_designable_chain_ids and get_all_chain_ids).
- Around line 32-33: In get_selections(), replace the list concatenation that
builds [""] + list(cmd.get_names("selections")) with list unpacking to satisfy
RUF005; specifically, return a new list that starts with the empty string and
unpacks the iterable from cmd.get_names("selections") (i.e., use unpacking with
the get_selections function and cmd.get_names call).
In `@src/REvoDesign/tools/utils.py`:
- Around line 40-49: The wrapper functions run_command and
run_worker_thread_in_pool currently use *args/**kwargs which strips the original
`@overload` type signatures from package_manager; fix by preserving and forwarding
the original signatures: either re-export the functions directly (from
.package_manager import run_command, run_worker_thread_in_pool) so type checkers
see the original overloads, or copy the `@overload` declarations from
package_manager into utils and implement thin forwarding bodies that call
_run_command/_run_worker_thread_in_pool; reference the original symbols
run_command and run_worker_thread_in_pool when making the change so IDEs/type
checkers pick up the proper signatures.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.github/workflows/docker-image.yml.github/workflows/lint_badge.yml.github/workflows/schedule-update-actions.yml.github/workflows/semantic-pr-check.yml.github/workflows/unit_tests_tag.ymlREADME.mdserver/.env.exampleserver/.env.testserver/README.legacy.mdserver/README.mdserver/REvoDesign_PSSM_GREMLIN.shserver/docker-compose.ymlserver/docker/server/Dockerfileserver/pssm_gremlin/pssm_gremlin.pyserver/pssm_gremlin/templates/pssm_gremlin_dashboard.htmlserver/run/restart_pssm_flask.shserver/scripts/make_paired_MSA_simple.pysrc/REvoDesign/basic/extensions.pysrc/REvoDesign/clients/QtSocketConnector.pysrc/REvoDesign/logger/logger.pysrc/REvoDesign/shortcuts/dialog_hooks.pysrc/REvoDesign/tools/cgo_utils.pysrc/REvoDesign/tools/package_manager.pysrc/REvoDesign/tools/safe_pickle.pysrc/REvoDesign/tools/utils.pytests/server/test_pssm_gremlin.pytools/copyright.jstools/release_tag.sh
💤 Files with no reviewable changes (2)
- server/.env.example
- src/REvoDesign/tools/cgo_utils.py
Comment on lines
29
to
33
| ## redis settings | ||
| RUNNER_UID=503 | ||
| RUNNER_GID=20 | ||
| RUNNER_USERNAME=revodesign | ||
| RUNNER_GROUP=revodesign_appgroup |
There was a problem hiding this comment.
Incorrect section comment: "redis settings" should be "Runner identity settings".
Line 29 says ## redis settings but the block below defines RUNNER_UID, RUNNER_GID, RUNNER_USERNAME, and RUNNER_GROUP. Looks like a copy-paste from the actual Redis section at line 19.
Proposed fix
-## redis settings
+## Runner identity settings
RUNNER_UID=503📝 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.
Suggested change
| ## redis settings | |
| RUNNER_UID=503 | |
| RUNNER_GID=20 | |
| RUNNER_USERNAME=revodesign | |
| RUNNER_GROUP=revodesign_appgroup | |
| ## Runner identity settings | |
| RUNNER_UID=503 | |
| RUNNER_GID=20 | |
| RUNNER_USERNAME=revodesign | |
| RUNNER_GROUP=revodesign_appgroup |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 31-31: [UnorderedKey] The RUNNER_GID key should go before the RUNNER_UID key
(UnorderedKey)
[warning] 33-33: [UnorderedKey] The RUNNER_GROUP key should go before the RUNNER_UID key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/.env.test` around lines 29 - 33, Update the incorrect section comment
above the runner environment variables: replace the "## redis settings" comment
with a clear "## Runner identity settings" (or similar) so it correctly
describes the block containing RUNNER_UID, RUNNER_GID, RUNNER_USERNAME and
RUNNER_GROUP; ensure the new comment sits immediately above those variables and
remove any leftover copy-paste text referring to Redis.
| set -u | ||
|
|
||
| DOMAIN="${DOMAIN:-0.0.0.0}" | ||
| DOMAIN="0.0.0.0" |
There was a problem hiding this comment.
Avoid overriding a user-provided DOMAIN. This change ignores any DOMAIN set in the env file and can print a misleading URL.
🛠️ Suggested fix
-DOMAIN="0.0.0.0"
+DOMAIN="${DOMAIN:-0.0.0.0}"📝 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.
Suggested change
| DOMAIN="0.0.0.0" | |
| DOMAIN="${DOMAIN:-0.0.0.0}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/run/restart_pssm_flask.sh` at line 206, The script currently forces
DOMAIN="0.0.0.0", overriding any env-provided value; change this to only set the
default when DOMAIN is unset or empty (e.g., use shell parameter expansion or a
conditional assignment) so a user-provided DOMAIN in the env file is preserved
and any printed URLs reflect the actual DOMAIN value; update the DOMAIN
assignment site in restart_pssm_flask.sh and any places that print the URL to
rely on that preserved DOMAIN variable.
| # wrt += '/'.join([query[i] for i in tags])+'\n' | ||
| paired_data = [] | ||
| paired_data.append((9999, "query", "/".join([query[i] for i in tags]))) | ||
| paired_data = [(9999, "query", "/".join([query[i] for i in tags]))] |
There was a problem hiding this comment.
Please confirm pre-commit hooks and linting were run.
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 `@server/scripts/make_paired_MSA_simple.py` at line 105, The diff shows
creation of the variable paired_data in make_paired_MSA_simple.py; ensure you
ran the project's pre-commit hooks and linters before pushing: run pre-commit
install (once) and then pre-commit run --all-files (or make black) to apply
formatting and lint fixes, re-run tests, and amend the commit so paired_data and
surrounding code conform to the repository style rules.
Comment on lines
+23
to
+24
| def get_fasta_writer_choices() -> list[str]: | ||
| return [fmt for fmt in SeqIO._FormatToWriter.keys() if fmt.startswith("fas")] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Biopython SeqIO _FormatToWriter public API alternative
💡 Result:
Bio.SeqIO._FormatToWriter is a private/internal implementation detail. The supported public alternatives are:
- Use the public high-level writer API (recommended):
from Bio import SeqIO
SeqIO.write(records, out_handle, "fasta") # or "genbank", "fastq", ...This is Biopython’s documented public way to write sequence records by format string. [1]
- For a single record, use
SeqRecord.format():
text = record.format("fasta")(Or write that string to a handle yourself for simple sequential formats.) [2]
- If you specifically need a “writer class” (like
_FormatToWritergave you), import the format-specific*Writerclass directly from the relevantBio.SeqIO.*IOmodule, e.g.:
from Bio.SeqIO.FastaIO import FastaWriter
writer = FastaWriter(out_handle)
writer.write_file(records)These writer classes are part of the documented API surface (e.g., FastaWriter, GenBankWriter, etc.), and they implement the SequenceWriter interface. [3]
Bottom line: there isn’t a stable public “format string → writer class” mapping; use SeqIO.write(...) unless you have a strong reason to instantiate a specific *Writer class. [1][3]
Sources: [1] Bio.SeqIO docs (SeqIO.write), [2] Biopython Tutorial (SeqRecord.format / writing), [3] Bio.SeqIO API listing of writer classes & SequenceWriter interface.
Accessing SeqIO._FormatToWriter is relying on a private/internal API.
_FormatToWriter is a private attribute of Biopython's SeqIO module and is not part of the stable public API. Biopython does not provide a public method to enumerate available writer formats—SeqIO.write() accepts format strings but doesn't expose a way to list them programmatically. This could break without notice on a Biopython upgrade.
Consider hardcoding the known FASTA format variants (e.g., "fasta", "fasta-2line") instead, or wrap the private access in a try-except with a fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/REvoDesign/shortcuts/dialog_hooks.py` around lines 23 - 24,
get_fasta_writer_choices currently reads the private SeqIO._FormatToWriter dict;
replace this fragile private-API access with a safe approach: attempt to read
SeqIO._FormatToWriter inside a try block but fall back to a hardcoded list of
known FASTA writers (e.g., "fasta", "fasta-2line") if that attribute is missing
or raises; update the function get_fasta_writer_choices to prefer the dynamic
list when available and return the fallback list on any exception to avoid
depending on Biopython internals.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/REvoDesign/tools/measure_utils.py (5)
648-648:⚠️ Potential issue | 🟡 MinorReturn type annotation
-> Measurementis wrong; the function returnslist[Measurement].Line 673 assigns
hits = Measurement.from_session_names(...)which returnslist[Measurement], and line 718 returnshitsdirectly. The annotation should be corrected.🐛 Proposed fix
-def read_measurement(start: str | int, debug: int = 0) -> Measurement: +def read_measurement(start: str | int, debug: int = 0) -> list[Measurement]:Also applies to: 718-718
🤖 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 648, The return type annotation on read_measurement is incorrect: the function populates hits via Measurement.from_session_names(...) which returns a list[Measurement] and then returns hits; update the signature of read_measurement to return list[Measurement] (or Sequence[Measurement]) instead of Measurement, ensuring the annotation matches the actual returned value from Measurement.from_session_names and the variable hits.
691-691:⚠️ Potential issue | 🟡 MinorTypo:
"skiping"→"skipping".📝 Proposed fix
- print(f"[DEBUG] skiping {a.resi} to avoid duplicates") + print(f"[DEBUG] skipping {a.resi} to avoid duplicates")🤖 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 691, Fix the typo in the debug message that prints duplicate-skipping info: change the string in the print call that references a.resi from "[DEBUG] skiping {a.resi} to avoid duplicates" to use the correct spelling "skipping" so it reads "[DEBUG] skipping {a.resi} to avoid duplicates"; locate the print statement that uses a.resi in measure_utils.py and update only the text in that print/log call.
675-678:⚠️ Potential issue | 🟡 MinorError message always shows
[]— the list comprehension over an emptyhitsis a no-op.When
not hitsisTrue,hitsis guaranteed empty, so[m.name for m in hits]is always[].🐛 Proposed fix
if not hits: raise ValueError( - f"measurement not found in session {[m.name for m in hits]}", + "no measurements found in the current session", )🤖 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 675 - 678, The error message uses [m.name for m in hits] which is always [] when not hits; change the ValueError to report useful context by listing available measurement names from the collection you searched (e.g., session.measurements or the variable that holds all measurements) and/or the search criteria, not from hits; update the raise in the same block (the code that checks "if not hits") to include a descriptive message with the actual available measurement names and/or the requested measurement identifier instead of iterating over hits.
711-712:⚠️ Potential issue | 🟠 Major
IndexErrorif any measurement resolves to fewer than 2 atoms.
x[0]andx[1]in the list comprehensions are unconditional. If a measurement'satoms()resolves to 0 or 1 atoms (e.g., due to an(unresolved)fallback), this crashes at runtime.🐛 Proposed fix
- atom_a = [f"'{x[0]}'" for x in pairs.values()] - atom_b = [f"'{x[1]}'" for x in pairs.values()] + atom_a = [f"'{x[0]}'" for x in pairs.values() if len(x) >= 2] + atom_b = [f"'{x[1]}'" for x in pairs.values() if len(x) >= 2]🤖 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 711 - 712, The list comprehensions for atom_a and atom_b assume every entry in pairs.values() has at least two items, which raises IndexError when atoms() resolves to 0 or 1 atoms; update the logic that builds atom_a/atom_b to skip or handle short tuples (e.g., filter pairs.values() by len(x) >= 2) before accessing x[0] and x[1], or provide a safe fallback value for unresolved measurements so atom_a and atom_b only reference valid pair entries (target symbols: pairs, atom_a, atom_b, and any code that calls atoms()).
486-625: 🛠️ Refactor suggestion | 🟠 MajorRemove dead code methods
_build_uniqueid_to_atom_mapand_resolve_by_coords.Both methods are never called and appear to be remnants of an earlier design. Additionally,
_build_uniqueid_to_atom_mapcontradicts the explicit design decision at line 279 ("do NOT fall back to'id'or'serial'") by iterating through those exact attributes as fallback options. Theuidparameter in_resolve_by_coordsis also unused in the method body.🤖 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 - 625, Remove the two dead helper methods _build_uniqueid_to_atom_map and _resolve_by_coords from the class: delete their entire method definitions (they are unused), and update any code that might reference them (search for _build_uniqueid_to_atom_map and _resolve_by_coords and remove or replace calls). Also ensure you do not reintroduce the forbidden fallback behavior (do not add checks for 'id' or 'serial' anywhere) and remove the unused uid parameter consideration—if any logic depended on resolving by coords, implement a clear, intentional replacement rather than restoring these methods.server/docker-compose.yml (2)
105-119:⚠️ Potential issue | 🟡 MinorRunner image variable name mismatch (
PSSM_GREMLIN_RUNNER_IMAGEvsRUNNER_IMAGE).
x-common-envdefinesRUNNER_IMAGE, butrunner.imageusesPSSM_GREMLIN_RUNNER_IMAGE. Unless this is intentional for backwards compatibility, it’s easy to set the “wrong” var and wonder why it didn’t apply.Possible fix (use RUNNER_IMAGE consistently)
- image: ${PSSM_GREMLIN_RUNNER_IMAGE:-revodesign-pssm-gremlin-non-root} + image: ${RUNNER_IMAGE:-revodesign-pssm-gremlin-non-root}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/docker-compose.yml` around lines 105 - 119, The runner service is using image: ${PSSM_GREMLIN_RUNNER_IMAGE:-revodesign-pssm-gremlin-non-root} while x-common-env defines RUNNER_IMAGE, causing a mismatch; update the runner service (service name "runner") to use the same variable name as x-common-env (RUNNER_IMAGE) or add a consistent fallback so both names map to the same value (e.g., replace PSSM_GREMLIN_RUNNER_IMAGE with RUNNER_IMAGE or wire RUNNER_IMAGE to PSSM_GREMLIN_RUNNER_IMAGE) to ensure the expected env var controls the image.
34-38:⚠️ Potential issue | 🟠 Major
group_add: ["0"]is a meaningful privilege relaxation; document or scope it.Adding the root group can unintentionally broaden access to mounted volumes / host resources (depending on ownership/mode). If this is only to handle docker.sock ownership edge cases, consider:
- making it conditional via env (ex:
EXTRA_GROUPS), or- adding a comment explaining why it’s required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/docker-compose.yml` around lines 34 - 38, The docker-compose anchor x-docker-socket-access currently forces group_add to include "0" (root) which widens privileges; update the x-docker-socket-access anchor to avoid unconditional root group addition by either making the extra group conditional via an environment variable (e.g., read EXTRA_GROUPS or DOCKER_EXTRA_GROUPS and only include "0" when explicitly set) or add an explicit comment above x-docker-socket-access explaining why group_add: ["0"] is necessary and the security tradeoffs; reference the x-docker-socket-access anchor and the group_add key when making the change so callers of the anchor can opt in instead of receiving root group access by default.server/REvoDesign_PSSM_GREMLIN.sh (1)
11-33:⚠️ Potential issue | 🟡 MinorConda env auto-detection can re-activate multiple envs (break only exits inner loop).
Right now
breakon Line 27 exits only the inner loop (env_2), so if multiplepossible_conda_env_namesexist, the script may activate more than once. If the intent is “activate first match and stop”, consider abreak 2or a flag.Proposed fix (stop after first match)
- for env_1 in "${possible_conda_env_names[@]}"; do - for env_2 in "${existed_conda_env_names[@]}"; do + for env_1 in "${possible_conda_env_names[@]}"; do + for env_2 in "${existed_conda_env_names[@]}"; do if [[ "$env_1" == "$env_2" ]]; then echo "find ${env_1} env" conda activate "${env_1}" - break + break 2 fi done done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/REvoDesign_PSSM_GREMLIN.sh` around lines 11 - 33, The nested loop over possible_conda_env_names/env_2 can activate multiple conda envs because the plain `break` only exits the inner loop; change the control flow in the activation block to stop after the first successful activation — either replace `break` with `break 2` to exit both loops immediately, or set a flag (e.g., activated=true) after `conda activate "${env_1}"` and break the inner loop, then test the flag after the inner loop to break the outer loop; update references in the script where possible_conda_env_names, env_1, env_2, and the `break` are used.
♻️ Duplicate comments (8)
server/scripts/make_paired_MSA_simple.py (1)
105-105: Reminder to confirm pre-commit hooks and linting were run.
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 `@server/scripts/make_paired_MSA_simple.py` at line 105, Ensure pre-commit hooks and linters were executed before pushing: run `pre-commit install` and then either `pre-commit run --all-files` or `make black` (and any other configured linters) and fix any reported issues in server/scripts/make_paired_MSA_simple.py, especially around the paired_data assignment and surrounding formatting to satisfy the repo’s hooks.tools/copyright.js (1)
8-12:⚠️ Potential issue | 🔴 CriticalInvalid shell pipeline —
| > ./program.docxis still not fixed.
.join(" | ")produces:find ./src/REvoDesign -type f -name '*.py' -print0 | xargs -0 cat | > ./program.docx
| >is not valid shell syntax; bash will error out every time this runs.🛠️ Proposed fix
-const command = [ - "find ./src/REvoDesign -type f -name '*.py' -print0", - "xargs -0 cat", - "> ./program.docx", -].join(" | "); +const command = + "find ./src/REvoDesign -type f -name '*.py' -print0 | xargs -0 cat > ./program.docx";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/copyright.js` around lines 8 - 12, The current command array used to build the shell line (the variable "command") is joined with " | " causing the redirection string "> ./program.docx" to be prefixed by a pipe and produce an invalid "`| >`" token; fix it by constructing the command so redirection is appended without being joined by a pipe — e.g., join only the pipeline-producing parts (the find and xargs/cat pieces) with " | " and then append the redirect "> ./program.docx" to the resulting string (i.e., do not include the redirection as an element that gets joined with " | ").src/REvoDesign/shortcuts/dialog_hooks.py (2)
36-37:or Nonestill collapses empty-list results.This keeps the semantic change where
[]becomesNone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/shortcuts/dialog_hooks.py` around lines 36 - 37, The helper function find_all_small_molecules_in_protein currently appends "or None" which converts an empty list result from find_small_molecules_in_protein("(all)") into None; remove the "or None" so find_all_small_molecules_in_protein simply returns the list returned by find_small_molecules_in_protein("(all)") (preserving empty-list semantics) and keep the call site/function name find_small_molecules_in_protein unchanged.
23-24: Private Biopython API still used for FASTA writers.
This remains tied toSeqIO._FormatToWriter, which is not a stable public API.Biopython SeqIO._FormatToWriter public API alternatives🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/shortcuts/dialog_hooks.py` around lines 23 - 24, get_fasta_writer_choices currently depends on the private SeqIO._FormatToWriter mapping; replace that with a safe discovery approach: create a minimal SeqRecord and attempt SeqIO.write(record, io.StringIO(), fmt) for a curated list of candidate format names (e.g., "fasta", "fasta-2line", any other known fasta variants your project needs), collect formats where the write succeeds and filter those that startwith("fas"), and return that list from get_fasta_writer_choices; this removes reliance on SeqIO._FormatToWriter while keeping the function signature and behavior.server/.env.test (1)
29-34:⚠️ Potential issue | 🟡 MinorWrong section header: runner identity isn’t “redis settings”.
This looks like a copy/paste header and makes the file harder to scan.
Proposed fix
-## redis settings +## Runner identity settings RUNNER_UID=503 RUNNER_GID=20 RUNNER_USERNAME=revodesign RUNNER_GROUP=revodesign_appgroup🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/.env.test` around lines 29 - 34, The section header "## redis settings" is incorrect for the runner identity variables; change that header to something accurate like "## runner settings" or "## runner identity" so the RUNNER_UID, RUNNER_GID, RUNNER_USERNAME, and RUNNER_GROUP entries are correctly labeled and the .env.test file is easier to scan.server/run/restart_pssm_flask.sh (1)
195-210:⚠️ Potential issue | 🟡 MinorDOMAIN override + printed URL likely misleading (0.0.0.0 isn’t a “clickable” address).
Even if services bind to
0.0.0.0, users typically access via127.0.0.1, hostname, or remote IP. Consider:
- keep
DOMAIN="${DOMAIN:-0.0.0.0}", and/or- print both bind address and a suggested access URL (ex:
http://127.0.0.1:${PORT}/...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/run/restart_pssm_flask.sh` around lines 195 - 210, The current cmd_restart function sets DOMAIN="0.0.0.0" and prints a misleading clickable URL; instead preserve any existing ENV value and show both the bind address and a suggested access URL: keep DOMAIN="${DOMAIN:-0.0.0.0}" (so it respects ENV_FILE sourced earlier), keep PORT="${PORT:-8080}", and change the echo output in cmd_restart to print the bind address (DOMAIN) and a user-friendly suggested access URL using 127.0.0.1 (e.g. http://127.0.0.1:${PORT}/PSSM_GREMLIN/dashboard) so users can click or copy a working URL while still showing the actual bind address.server/docker/server/Dockerfile (1)
38-38:⚠️ Potential issue | 🟠 MajorUse
execso Gunicorn becomes PID 1 (signal handling / graceful shutdown).Current
CMD ["sh","-c","gunicorn ..."]keeps the shell as PID 1.Proposed fix
-CMD ["sh", "-c", "gunicorn -w 2 -b 0.0.0.0:${PORT:-8080} pssm_gremlin:app"] +CMD ["sh", "-c", "exec gunicorn -w 2 -b 0.0.0.0:${PORT:-8080} pssm_gremlin:app"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/docker/server/Dockerfile` at line 38, The Dockerfile CMD currently invokes a shell which stays PID 1 and prevents Gunicorn from receiving signals; update the CMD that runs "gunicorn -w 2 -b 0.0.0.0:${PORT:-8080} pssm_gremlin:app" so Gunicorn becomes PID 1—either prepend "exec" to the shell command invoked by CMD or switch to the exec/JSON form to invoke gunicorn directly (so pssm_gremlin:app is started by gunicorn as PID 1 and gets proper signal handling).server/pssm_gremlin/pssm_gremlin.py (1)
1025-1027:"Invalid task id"string literal is still duplicated five times across routesThe same inline message appears in
run_gremlin(line 1027),get_results(line 1066),download_results(line 1084),cancel_task(line 1129), anddelete_task(line 1230). Define it as a module-level constant and reference it from each site.Also applies to: 1064-1066, 1082-1084, 1127-1129, 1228-1230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/pssm_gremlin/pssm_gremlin.py` around lines 1025 - 1027, The "Invalid task id" literal is duplicated across multiple route handlers; define a module-level constant (e.g., INVALID_TASK_ID_MSG = "Invalid task id") at top of pssm_gremlin.py and replace the inline string in all places that validate _normalize_task_id (where md5sum is checked) — specifically in run_gremlin, get_results, download_results, cancel_task, and delete_task — so each return uses jsonify({"status": "bad_request", "message": INVALID_TASK_ID_MSG}), 400 instead of the hardcoded literal.
🧹 Nitpick comments (12)
src/REvoDesign/clients/QtSocketConnector.py (1)
560-561: Redundantifguard — the condition is alwaysTrueat this point.Both paths inside the preceding
if client not in self.meetingroom.current_clients:block (lines 505–558) end with an explicitreturn, so any execution that reaches line 560 is guaranteed to be the already-authenticated case. The wrappingif client in self.meetingroom.current_clients:check is tautological and can be dropped (or replaced with a plain comment/else).♻️ Proposed simplification
- if client in self.meetingroom.current_clients: - logging.debug("Client already authenticated; skip duplicate auth flow.") + else: + logging.debug("Client already authenticated; skip duplicate auth flow.")Or, since the first block always returns, a bare statement is equally clear:
- if client in self.meetingroom.current_clients: - logging.debug("Client already authenticated; skip duplicate auth flow.") + # Reaching here means client is already authenticated; nothing to do. + logging.debug("Client already authenticated; skip duplicate auth flow.")🤖 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 560 - 561, In the method inside QtSocketConnector where you handle authentication (the block that checks client not in self.meetingroom.current_clients and returns), remove the redundant "if client in self.meetingroom.current_clients:" guard and replace it with a single unconditional logging.debug call (or an else comment) since any execution reaching that point is already the authenticated case; update the code that currently logs "Client already authenticated; skip duplicate auth flow." to run directly without the tautological condition, keeping the log message and surrounding context intact.src/REvoDesign/basic/extensions.py (1)
147-147: Optional: add-> strreturn type annotation tobasename_stem.The method always returns a
str(or raises), but the signature lacks the annotation, breaking consistency with the rest of the type-annotated codebase.✏️ Proposed annotation
- def basename_stem(self, fname: str): + def basename_stem(self, fname: str) -> str:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/basic/extensions.py` at line 147, The method basename_stem is missing a return type annotation; update the function signature of basename_stem(self, fname: str) to include -> str so it reads basename_stem(self, fname: str) -> str to reflect that it always returns a string and keep type annotations consistent across the codebase.tools/copyright.js (1)
14-14: Drop the-l(login-shell) flag from the bash invocation.
-lsources/etc/profileand user profile files, making execution environment-dependent. A plain-cis sufficient for this pipeline.♻️ Proposed fix
-const result = spawnSync("bash", ["-lc", command], { stdio: "inherit" }); +const result = spawnSync("bash", ["-c", command], { stdio: "inherit" });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/copyright.js` at line 14, The bash invocation in the spawnSync call uses the login-shell flag "-l" which makes the environment dependent; update the spawnSync call that currently uses spawnSync("bash", ["-lc", command], { stdio: "inherit" }) to remove the "-l" so it becomes spawnSync("bash", ["-c", command], { stdio: "inherit" }), keeping the same stdio options and the same local variable name (result) so behavior and error handling around spawnSync remain unchanged.tools/release_tag.sh (1)
27-29: Prefer-zfor empty-string checks inside[[ ]]— consistent with this PR's quoting goals.
[[ ! $new_version ]]works in bash but is inconsistent with the quoting discipline applied elsewhere in this PR. The idiomatic form also makes intent clearer.♻️ Proposed refactor
-if [[ ! $new_version || ! $old_version || ! $new_date ]]; then +if [[ -z "$new_version" || -z "$old_version" || -z "$new_date" ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/release_tag.sh` around lines 27 - 29, Replace the empty-string checks using negation with explicit -z tests to follow the quoting/idiom used elsewhere: change the condition in release_tag.sh that currently checks new_version, old_version, and new_date via [[ ! $new_version || ! $old_version || ! $new_date ]] to use [[ -z "$new_version" || -z "$old_version" || -z "$new_date" ]], preserving the echo and exit behavior when any of new_version, old_version, or new_date is empty.src/REvoDesign/tools/measure_utils.py (1)
337-337:_atoms_cacheshould be excluded from dataclass-generated methods.As a plain dataclass field,
_atoms_cacheis exposed in__init__,__repr__, and__eq__, which is not the intent for a private mutable cache. Usefieldwithinit=False/repr=False/compare=False.♻️ Proposed fix
- _atoms_cache: list[AtomDescriptor] | None = None + _atoms_cache: list[AtomDescriptor] | None = field( + default=None, init=False, repr=False, compare=False + )🤖 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 337, _atoms_cache is currently declared as a plain dataclass field and thus appears in __init__/__repr__/__eq__; change its declaration to use dataclasses.field(default=None, init=False, repr=False, compare=False) so it is excluded from generated methods and remains a private mutable cache; import field from dataclasses if not already and keep the type annotation (list[AtomDescriptor] | None) but assign the field(...) default instead of a bare None.server/pssm_gremlin/templates/pssm_gremlin_dashboard.html (2)
1138-1184: Animation timer bookkeeping is reasonable; consider clearing pending timeouts on non-animated sync.If
syncThemeToggle(false)is called while an animation timeout is pending, the timeout still fires later and mutates state (is-animating). Not a huge deal, but you could clear any existing timer unconditionally at the start ofsyncThemeToggle()to avoid edge flicker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/pssm_gremlin/templates/pssm_gremlin_dashboard.html` around lines 1138 - 1184, The syncThemeToggle function can leave a pending animation timeout that later removes the "is-animating" class even when called without animation; to fix, at the start of syncThemeToggle (before any potential re-animation) check button.dataset.animTimer, parse it to a number, and if non-zero call window.clearTimeout(existingTimer) and clear button.dataset.animTimer (and ensure "is-animating" is removed), so pending timers are always cancelled when syncing without animate; reference syncThemeToggle, button.dataset.animTimer, is-animating, and the timer creation/clearing logic to implement this unconditional cleanup.
129-192: Theme toggle animation + icon layer looks clean; watch browser support forcolor-mix/backdrop-filter.The UI/UX improvements are good. If this dashboard is expected to run on older browsers (or embedded webviews), consider a fallback for
color-mix()andbackdrop-filterso contrast stays acceptable.Also applies to: 946-962, 766-857
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/pssm_gremlin/templates/pssm_gremlin_dashboard.html` around lines 129 - 192, Provide fallback styles for browsers that lack support for color-mix() and backdrop-filter: ensure .theme-toggle::before and .theme-toggle .theme-icon declare explicit solid background-color and contrasting color (or a simpler gradient) before using advanced properties, and add a `@supports` not (backdrop-filter: none) block to reduce or remove blur/brightness filters for unsupported browsers; update .theme-toggle.mode-light/.mode-dark/.mode-auto variants to include these fallback colors so icon contrast remains acceptable when color-mix/ backdrop-filter aren’t available.tests/server/test_pssm_gremlin.py (1)
1063-1106: Batch delete normalization test is valuable; consider adding an assertion for duplicate handling semantics.You pass
md5sumtwice (uppercased + padded + raw duplicate). It might be useful to explicitly assert the API reports the duplicate only once (which you do viadeleted == [md5sum]) and thatignored/not_founddon’t include normalized duplicates (guards future regressions).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server/test_pssm_gremlin.py` around lines 1063 - 1106, The test test_batch_delete_guards_and_normalizes_each_md5sum should explicitly assert duplicate handling: after calling client.post with duplicate representations of md5sum, add assertions that the normalized md5sum appears only once in payload["deleted"] (already present) and also verify that payload["ignored"], payload["not_found"], and payload["forbidden"] do not contain any normalized forms of the duplicated md5sum (e.g., ensure none of these lists include md5sum or its uppercased/padded variants) so duplicates aren't mistakenly categorized elsewhere; update assertions after the POST and before checking the task via module.task_store.get_task(md5sum).server/REvoDesign_PSSM_GREMLIN.sh (1)
145-226: Array-based command execution is a solid hardening step; consider consolidating log redirections.Using
local -a cmd=(...)and"${cmd[@]}"reduces injection risk and quoting bugs. One small maintainability win: build log paths as a single quoted string ("${pipline_res_dir}/log/${instance}_...") rather than mixing quoted/unquoted segments.Also applies to: 228-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/REvoDesign_PSSM_GREMLIN.sh` around lines 145 - 226, The RUN_GREMLIN function uses array-based commands correctly but mixes quoted/unquoted segments when redirecting logs; define full quoted log path variables (e.g., out_log="${pipline_res_dir}/log/${instance}_gremlin_hhblits.log" err_log="${pipline_res_dir}/log/${instance}_gremlin_hhblits.err") and use those variables for all redirections instead of concatenating quoted and unquoted pieces, then apply this pattern consistently for hhblits, hhfilter, fasta_lower_char_rm.py, and GREMLIN_TFv1 invocations (all places building "${pipline_res_dir}/log/${instance}_...") to ensure consistent quoting and easier maintenance.server/.env.test (1)
3-6: Fix dotenv-linter key ordering warnings.dotenv-linter reports 6 UnorderedKey violations in this file (lines 5, 9, 21, 26, 31, 33). While the file has good logical grouping by section, keys within each section should be alphabetically ordered: RUNNER_IMAGE before SERVER_IMAGE, LOG_DIR before SERVER_DIR, BROKER_URL before REDIS_URL, GUNICORN_WORKERS before NPROC, and RUNNER_GID/RUNNER_GROUP before RUNNER_UID. Additionally, line 29's comment says "redis settings" but contains runner configuration variables—update the comment for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/.env.test` around lines 3 - 6, Reorder the keys within each logical section of server/.env.test to satisfy dotenv-linter alphabetical rules: place RUNNER_IMAGE before SERVER_IMAGE, LOG_DIR before SERVER_DIR, BROKER_URL before REDIS_URL, GUNICORN_WORKERS before NPROC, and ensure RUNNER_GID and RUNNER_GROUP appear before RUNNER_UID; also update the comment on the runner block (currently "redis settings") to accurately describe runner configuration. Locate the variables RUNNER_IMAGE, SERVER_IMAGE, LOG_DIR, SERVER_DIR, BROKER_URL, REDIS_URL, GUNICORN_WORKERS, NPROC, RUNNER_GID, RUNNER_GROUP, and RUNNER_UID and adjust ordering and the comment accordingly to remove UnorderedKey violations.server/pssm_gremlin/pssm_gremlin.py (2)
380-384: Ruff TRY003: longValueErrormessages on lines 383 and 442 should be moved into a custom exception class (or shortened)Static analysis flags both raises as TRY003. The simplest fix is to extract a small sentinel exception or shorten the inline message.
♻️ Minimal fix – custom exception
+class PathEscapeError(ValueError): + """Raised when a path would escape its configured base directory.""" + + def _safe_join(base_dir: str, *parts: str) -> str: candidate = os.path.abspath(os.path.join(base_dir, *parts)) if not _path_is_within(base_dir, candidate): - raise ValueError(f"Path escapes configured base directory: {candidate}") + raise PathEscapeError(f"Path escapes base directory: {candidate!r}") return candidate def _task_zip_path(task: Any) -> str: raw_task_id = task if isinstance(task, str) else task["md5sum"] task_id = _normalize_task_id(raw_task_id) if task_id is None: - raise ValueError(f"Invalid task id for result archive: {raw_task_id!r}") + raise ValueError(f"Invalid task id: {raw_task_id!r}") return _safe_join(app.config["RESULTS_FOLDER"], f"{task_id}_PSSM_GREMLIN_results.zip")As per coding guidelines,
pre-commit run --all-files(ormake black) must be run before pushing to ensure formatting and linting.Also applies to: 438-443
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/pssm_gremlin/pssm_gremlin.py` around lines 380 - 384, Replace the long inline ValueError messages in _safe_join (and the similar raise in the block around lines 438-443) with a small custom exception class (e.g., PathEscapeError) or shorten the message to a brief sentinel; specifically, define a new exception (class PathEscapeError(ValueError): pass) near the top of the module, then change the raises in _safe_join and the other location to raise PathEscapeError("path escapes base") or a similarly short message, and run the repo linters/formatters (pre-commit / make black) before committing.
367-391:$anchor in_TASK_ID_PATTERNis redundant when used withfullmatch, and[A-F]is dead after.lower()Two minor issues in the pattern/normaliser pair:
_TASK_ID_PATTERN.fullmatch(...)already anchors both ends, so the trailing$inr"[a-fA-F0-9]{32}$"is redundant._normalize_task_idcalls.lower()before matching, so[A-F]is never matched.♻️ Suggested cleanup
-_TASK_ID_PATTERN = re.compile(r"[a-fA-F0-9]{32}$") +_TASK_ID_PATTERN = re.compile(r"[a-f0-9]{32}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/pssm_gremlin/pssm_gremlin.py` around lines 367 - 391, The regex _TASK_ID_PATTERN is overly specific and partly redundant given _normalize_task_id lowercases input and code uses fullmatch; update the pattern to only match lowercase hex and drop the trailing anchor (e.g., change the compiled pattern in _TASK_ID_PATTERN from r"[a-fA-F0-9]{32}$" to r"[a-f0-9]{32}") so fullmatch with the lowercased value in _normalize_task_id works correctly and the unused uppercase range and trailing '$' are removed.
🤖 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.test`:
- Around line 7-18: The .env.test currently contains hard-coded absolute local
paths (SERVER_DIR, LOG_DIR, DB_UNIREF30, DB_UNIREF90, USERS_FILE) which are
non-portable and leak local filesystem info; replace these values with
repo-relative placeholders or env-template variables (e.g.,
SERVER_DIR=./server_test, LOG_DIR=./server_test/logs,
DB_UNIREF30=./data/miniuc30 or DB_UNIREF30=${PROJECT_ROOT}/data/miniuc30,
USERS_FILE=./config/users.template.txt) and add a short comment explaining
callers should copy and fill with their local absolute paths in a non-committed
.env.local; ensure the variable names (SERVER_DIR, LOG_DIR, DB_UNIREF30,
DB_UNIREF90, USERS_FILE) remain unchanged so code loading these env vars
continues to work.
In `@server/pssm_gremlin/pssm_gremlin.py`:
- Around line 595-600: The deletion currently calls
shutil.rmtree(safe_result_dir, ...) unconditionally for any non-root-like
safe_result_dir; move the shutil.rmtree invocation so it only runs when the path
is confirmed inside the configured results folder by
_path_is_within(app.config["RESULTS_FOLDER"], safe_result_dir). Concretely,
change the control flow around safe_result_dir, so if safe_result_dir is
root-like you warn and skip, else if _path_is_within(...) is false you log a
warning and skip deletion, and only call shutil.rmtree when _path_is_within(...)
returns true; if you need to allow legacy external paths, implement an explicit
allow-list check instead of deleting by default.
In `@server/pssm_gremlin/templates/pssm_gremlin_dashboard.html`:
- Around line 1516-1531: Replace the unreliable XMLHttpRequest-based logout in
the triggerLogout function with a client redirect to a new server-driven logout
endpoint (e.g., navigate to "/PSSM_GREMLIN/logout") and implement that endpoint
server-side to respond with 401 and a WWW-Authenticate header plus a simple "You
are logged out" confirmation page; also guard the
document.getElementById("logoutBtn") usage with a null check before attaching
event listeners to avoid runtime errors. Ensure references: update triggerLogout
to perform window.location.href = "/PSSM_GREMLIN/logout" and create a server
handler for the "/PSSM_GREMLIN/logout" path that returns 401 + WWW-Authenticate
and a logout confirmation body, and add a null-guard where
getElementById("logoutBtn") is used.
In `@server/REvoDesign_PSSM_GREMLIN.sh`:
- Around line 36-37: The script currently calls readlink -f on possibly-empty
variables (e.g. REVODESIGN_RUNSCRIPT_PATH assignment and later
fasta_fp=$(readlink -f "$fasta")), which under set -e causes the script to exit
if -i (input fasta) is omitted; after parsing options with getopts validate that
required inputs (the variable parsed into fasta via -i) are present and
non-empty and call the usage/exit routine if missing, or defer set -e until
after validation; specifically, add an explicit check for the variable name used
for the -i option (fasta) right after getopts (and before fasta_fp=$(readlink -f
"$fasta")) and if empty print usage and exit, or alternatively guard readlink
calls with a test like [ -n "$fasta" ] && fasta_fp=$(readlink -f "$fasta") so
readlink is never invoked on an empty string.
In `@tools/copyright.js`:
- Around line 15-17: When handling the child spawn result in the block that
checks if (result.status !== 0), also check for spawn-level errors via
result.error and print a diagnostic before exiting; update the logic around
result.status and process.exit(result.status || 1) to detect if result.error
exists and call console.error (or the project's logger) with a clear message and
the Error object, then exit with the existing fallback exit code so the error
isn't silently swallowed.
In `@tools/release_tag.sh`:
- Line 39: The sed commands that interpolate $new_version and $old_version use
unescaped version strings so dots act as regex wildcards; update the script to
escape literal dots in those variables before passing them to sed (e.g.,
transform new_version and old_version into escaped_version by replacing '.' with
'\.'), then use the escaped variables in the existing "${SED}" -i 's/.../../'
invocations (the lines that reference $new_version and $old_version) so sed
treats dots literally.
---
Outside diff comments:
In `@server/docker-compose.yml`:
- Around line 105-119: The runner service is using image:
${PSSM_GREMLIN_RUNNER_IMAGE:-revodesign-pssm-gremlin-non-root} while
x-common-env defines RUNNER_IMAGE, causing a mismatch; update the runner service
(service name "runner") to use the same variable name as x-common-env
(RUNNER_IMAGE) or add a consistent fallback so both names map to the same value
(e.g., replace PSSM_GREMLIN_RUNNER_IMAGE with RUNNER_IMAGE or wire RUNNER_IMAGE
to PSSM_GREMLIN_RUNNER_IMAGE) to ensure the expected env var controls the image.
- Around line 34-38: The docker-compose anchor x-docker-socket-access currently
forces group_add to include "0" (root) which widens privileges; update the
x-docker-socket-access anchor to avoid unconditional root group addition by
either making the extra group conditional via an environment variable (e.g.,
read EXTRA_GROUPS or DOCKER_EXTRA_GROUPS and only include "0" when explicitly
set) or add an explicit comment above x-docker-socket-access explaining why
group_add: ["0"] is necessary and the security tradeoffs; reference the
x-docker-socket-access anchor and the group_add key when making the change so
callers of the anchor can opt in instead of receiving root group access by
default.
In `@server/REvoDesign_PSSM_GREMLIN.sh`:
- Around line 11-33: The nested loop over possible_conda_env_names/env_2 can
activate multiple conda envs because the plain `break` only exits the inner
loop; change the control flow in the activation block to stop after the first
successful activation — either replace `break` with `break 2` to exit both loops
immediately, or set a flag (e.g., activated=true) after `conda activate
"${env_1}"` and break the inner loop, then test the flag after the inner loop to
break the outer loop; update references in the script where
possible_conda_env_names, env_1, env_2, and the `break` are used.
In `@src/REvoDesign/tools/measure_utils.py`:
- Line 648: The return type annotation on read_measurement is incorrect: the
function populates hits via Measurement.from_session_names(...) which returns a
list[Measurement] and then returns hits; update the signature of
read_measurement to return list[Measurement] (or Sequence[Measurement]) instead
of Measurement, ensuring the annotation matches the actual returned value from
Measurement.from_session_names and the variable hits.
- Line 691: Fix the typo in the debug message that prints duplicate-skipping
info: change the string in the print call that references a.resi from "[DEBUG]
skiping {a.resi} to avoid duplicates" to use the correct spelling "skipping" so
it reads "[DEBUG] skipping {a.resi} to avoid duplicates"; locate the print
statement that uses a.resi in measure_utils.py and update only the text in that
print/log call.
- Around line 675-678: The error message uses [m.name for m in hits] which is
always [] when not hits; change the ValueError to report useful context by
listing available measurement names from the collection you searched (e.g.,
session.measurements or the variable that holds all measurements) and/or the
search criteria, not from hits; update the raise in the same block (the code
that checks "if not hits") to include a descriptive message with the actual
available measurement names and/or the requested measurement identifier instead
of iterating over hits.
- Around line 711-712: The list comprehensions for atom_a and atom_b assume
every entry in pairs.values() has at least two items, which raises IndexError
when atoms() resolves to 0 or 1 atoms; update the logic that builds
atom_a/atom_b to skip or handle short tuples (e.g., filter pairs.values() by
len(x) >= 2) before accessing x[0] and x[1], or provide a safe fallback value
for unresolved measurements so atom_a and atom_b only reference valid pair
entries (target symbols: pairs, atom_a, atom_b, and any code that calls
atoms()).
- Around line 486-625: Remove the two dead helper methods
_build_uniqueid_to_atom_map and _resolve_by_coords from the class: delete their
entire method definitions (they are unused), and update any code that might
reference them (search for _build_uniqueid_to_atom_map and _resolve_by_coords
and remove or replace calls). Also ensure you do not reintroduce the forbidden
fallback behavior (do not add checks for 'id' or 'serial' anywhere) and remove
the unused uid parameter consideration—if any logic depended on resolving by
coords, implement a clear, intentional replacement rather than restoring these
methods.
---
Duplicate comments:
In `@server/.env.test`:
- Around line 29-34: The section header "## redis settings" is incorrect for the
runner identity variables; change that header to something accurate like "##
runner settings" or "## runner identity" so the RUNNER_UID, RUNNER_GID,
RUNNER_USERNAME, and RUNNER_GROUP entries are correctly labeled and the
.env.test file is easier to scan.
In `@server/docker/server/Dockerfile`:
- Line 38: The Dockerfile CMD currently invokes a shell which stays PID 1 and
prevents Gunicorn from receiving signals; update the CMD that runs "gunicorn -w
2 -b 0.0.0.0:${PORT:-8080} pssm_gremlin:app" so Gunicorn becomes PID 1—either
prepend "exec" to the shell command invoked by CMD or switch to the exec/JSON
form to invoke gunicorn directly (so pssm_gremlin:app is started by gunicorn as
PID 1 and gets proper signal handling).
In `@server/pssm_gremlin/pssm_gremlin.py`:
- Around line 1025-1027: The "Invalid task id" literal is duplicated across
multiple route handlers; define a module-level constant (e.g.,
INVALID_TASK_ID_MSG = "Invalid task id") at top of pssm_gremlin.py and replace
the inline string in all places that validate _normalize_task_id (where md5sum
is checked) — specifically in run_gremlin, get_results, download_results,
cancel_task, and delete_task — so each return uses jsonify({"status":
"bad_request", "message": INVALID_TASK_ID_MSG}), 400 instead of the hardcoded
literal.
In `@server/run/restart_pssm_flask.sh`:
- Around line 195-210: The current cmd_restart function sets DOMAIN="0.0.0.0"
and prints a misleading clickable URL; instead preserve any existing ENV value
and show both the bind address and a suggested access URL: keep
DOMAIN="${DOMAIN:-0.0.0.0}" (so it respects ENV_FILE sourced earlier), keep
PORT="${PORT:-8080}", and change the echo output in cmd_restart to print the
bind address (DOMAIN) and a user-friendly suggested access URL using 127.0.0.1
(e.g. http://127.0.0.1:${PORT}/PSSM_GREMLIN/dashboard) so users can click or
copy a working URL while still showing the actual bind address.
In `@server/scripts/make_paired_MSA_simple.py`:
- Line 105: Ensure pre-commit hooks and linters were executed before pushing:
run `pre-commit install` and then either `pre-commit run --all-files` or `make
black` (and any other configured linters) and fix any reported issues in
server/scripts/make_paired_MSA_simple.py, especially around the paired_data
assignment and surrounding formatting to satisfy the repo’s hooks.
In `@src/REvoDesign/shortcuts/dialog_hooks.py`:
- Around line 36-37: The helper function find_all_small_molecules_in_protein
currently appends "or None" which converts an empty list result from
find_small_molecules_in_protein("(all)") into None; remove the "or None" so
find_all_small_molecules_in_protein simply returns the list returned by
find_small_molecules_in_protein("(all)") (preserving empty-list semantics) and
keep the call site/function name find_small_molecules_in_protein unchanged.
- Around line 23-24: get_fasta_writer_choices currently depends on the private
SeqIO._FormatToWriter mapping; replace that with a safe discovery approach:
create a minimal SeqRecord and attempt SeqIO.write(record, io.StringIO(), fmt)
for a curated list of candidate format names (e.g., "fasta", "fasta-2line", any
other known fasta variants your project needs), collect formats where the write
succeeds and filter those that startwith("fas"), and return that list from
get_fasta_writer_choices; this removes reliance on SeqIO._FormatToWriter while
keeping the function signature and behavior.
In `@tools/copyright.js`:
- Around line 8-12: The current command array used to build the shell line (the
variable "command") is joined with " | " causing the redirection string ">
./program.docx" to be prefixed by a pipe and produce an invalid "`| >`" token;
fix it by constructing the command so redirection is appended without being
joined by a pipe — e.g., join only the pipeline-producing parts (the find and
xargs/cat pieces) with " | " and then append the redirect "> ./program.docx" to
the resulting string (i.e., do not include the redirection as an element that
gets joined with " | ").
---
Nitpick comments:
In `@server/.env.test`:
- Around line 3-6: Reorder the keys within each logical section of
server/.env.test to satisfy dotenv-linter alphabetical rules: place RUNNER_IMAGE
before SERVER_IMAGE, LOG_DIR before SERVER_DIR, BROKER_URL before REDIS_URL,
GUNICORN_WORKERS before NPROC, and ensure RUNNER_GID and RUNNER_GROUP appear
before RUNNER_UID; also update the comment on the runner block (currently "redis
settings") to accurately describe runner configuration. Locate the variables
RUNNER_IMAGE, SERVER_IMAGE, LOG_DIR, SERVER_DIR, BROKER_URL, REDIS_URL,
GUNICORN_WORKERS, NPROC, RUNNER_GID, RUNNER_GROUP, and RUNNER_UID and adjust
ordering and the comment accordingly to remove UnorderedKey violations.
In `@server/pssm_gremlin/pssm_gremlin.py`:
- Around line 380-384: Replace the long inline ValueError messages in _safe_join
(and the similar raise in the block around lines 438-443) with a small custom
exception class (e.g., PathEscapeError) or shorten the message to a brief
sentinel; specifically, define a new exception (class
PathEscapeError(ValueError): pass) near the top of the module, then change the
raises in _safe_join and the other location to raise PathEscapeError("path
escapes base") or a similarly short message, and run the repo linters/formatters
(pre-commit / make black) before committing.
- Around line 367-391: The regex _TASK_ID_PATTERN is overly specific and partly
redundant given _normalize_task_id lowercases input and code uses fullmatch;
update the pattern to only match lowercase hex and drop the trailing anchor
(e.g., change the compiled pattern in _TASK_ID_PATTERN from r"[a-fA-F0-9]{32}$"
to r"[a-f0-9]{32}") so fullmatch with the lowercased value in _normalize_task_id
works correctly and the unused uppercase range and trailing '$' are removed.
In `@server/pssm_gremlin/templates/pssm_gremlin_dashboard.html`:
- Around line 1138-1184: The syncThemeToggle function can leave a pending
animation timeout that later removes the "is-animating" class even when called
without animation; to fix, at the start of syncThemeToggle (before any potential
re-animation) check button.dataset.animTimer, parse it to a number, and if
non-zero call window.clearTimeout(existingTimer) and clear
button.dataset.animTimer (and ensure "is-animating" is removed), so pending
timers are always cancelled when syncing without animate; reference
syncThemeToggle, button.dataset.animTimer, is-animating, and the timer
creation/clearing logic to implement this unconditional cleanup.
- Around line 129-192: Provide fallback styles for browsers that lack support
for color-mix() and backdrop-filter: ensure .theme-toggle::before and
.theme-toggle .theme-icon declare explicit solid background-color and
contrasting color (or a simpler gradient) before using advanced properties, and
add a `@supports` not (backdrop-filter: none) block to reduce or remove
blur/brightness filters for unsupported browsers; update
.theme-toggle.mode-light/.mode-dark/.mode-auto variants to include these
fallback colors so icon contrast remains acceptable when color-mix/
backdrop-filter aren’t available.
In `@server/REvoDesign_PSSM_GREMLIN.sh`:
- Around line 145-226: The RUN_GREMLIN function uses array-based commands
correctly but mixes quoted/unquoted segments when redirecting logs; define full
quoted log path variables (e.g.,
out_log="${pipline_res_dir}/log/${instance}_gremlin_hhblits.log"
err_log="${pipline_res_dir}/log/${instance}_gremlin_hhblits.err") and use those
variables for all redirections instead of concatenating quoted and unquoted
pieces, then apply this pattern consistently for hhblits, hhfilter,
fasta_lower_char_rm.py, and GREMLIN_TFv1 invocations (all places building
"${pipline_res_dir}/log/${instance}_...") to ensure consistent quoting and
easier maintenance.
In `@src/REvoDesign/basic/extensions.py`:
- Line 147: The method basename_stem is missing a return type annotation; update
the function signature of basename_stem(self, fname: str) to include -> str so
it reads basename_stem(self, fname: str) -> str to reflect that it always
returns a string and keep type annotations consistent across the codebase.
In `@src/REvoDesign/clients/QtSocketConnector.py`:
- Around line 560-561: In the method inside QtSocketConnector where you handle
authentication (the block that checks client not in
self.meetingroom.current_clients and returns), remove the redundant "if client
in self.meetingroom.current_clients:" guard and replace it with a single
unconditional logging.debug call (or an else comment) since any execution
reaching that point is already the authenticated case; update the code that
currently logs "Client already authenticated; skip duplicate auth flow." to run
directly without the tautological condition, keeping the log message and
surrounding context intact.
In `@src/REvoDesign/tools/measure_utils.py`:
- Line 337: _atoms_cache is currently declared as a plain dataclass field and
thus appears in __init__/__repr__/__eq__; change its declaration to use
dataclasses.field(default=None, init=False, repr=False, compare=False) so it is
excluded from generated methods and remains a private mutable cache; import
field from dataclasses if not already and keep the type annotation
(list[AtomDescriptor] | None) but assign the field(...) default instead of a
bare None.
In `@tests/server/test_pssm_gremlin.py`:
- Around line 1063-1106: The test
test_batch_delete_guards_and_normalizes_each_md5sum should explicitly assert
duplicate handling: after calling client.post with duplicate representations of
md5sum, add assertions that the normalized md5sum appears only once in
payload["deleted"] (already present) and also verify that payload["ignored"],
payload["not_found"], and payload["forbidden"] do not contain any normalized
forms of the duplicated md5sum (e.g., ensure none of these lists include md5sum
or its uppercased/padded variants) so duplicates aren't mistakenly categorized
elsewhere; update assertions after the POST and before checking the task via
module.task_store.get_task(md5sum).
In `@tools/copyright.js`:
- Line 14: The bash invocation in the spawnSync call uses the login-shell flag
"-l" which makes the environment dependent; update the spawnSync call that
currently uses spawnSync("bash", ["-lc", command], { stdio: "inherit" }) to
remove the "-l" so it becomes spawnSync("bash", ["-c", command], { stdio:
"inherit" }), keeping the same stdio options and the same local variable name
(result) so behavior and error handling around spawnSync remain unchanged.
In `@tools/release_tag.sh`:
- Around line 27-29: Replace the empty-string checks using negation with
explicit -z tests to follow the quoting/idiom used elsewhere: change the
condition in release_tag.sh that currently checks new_version, old_version, and
new_date via [[ ! $new_version || ! $old_version || ! $new_date ]] to use [[ -z
"$new_version" || -z "$old_version" || -z "$new_date" ]], preserving the echo
and exit behavior when any of new_version, old_version, or new_date is empty.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
.github/workflows/docker-image.yml.github/workflows/lint_badge.yml.github/workflows/schedule-update-actions.yml.github/workflows/semantic-pr-check.yml.github/workflows/unit_tests_tag.ymlCHANGELOG.mdREADME.mdserver/.env.exampleserver/.env.testserver/README.legacy.mdserver/README.mdserver/REvoDesign_PSSM_GREMLIN.shserver/docker-compose.ymlserver/docker/server/Dockerfileserver/pssm_gremlin/pssm_gremlin.pyserver/pssm_gremlin/templates/pssm_gremlin_dashboard.htmlserver/run/restart_pssm_flask.shserver/scripts/make_paired_MSA_simple.pysrc/REvoDesign/basic/extensions.pysrc/REvoDesign/clients/QtSocketConnector.pysrc/REvoDesign/clusters/combine_positions.pysrc/REvoDesign/logger/logger.pysrc/REvoDesign/shortcuts/dialog_hooks.pysrc/REvoDesign/tools/cgo_utils.pysrc/REvoDesign/tools/measure_utils.pysrc/REvoDesign/tools/package_manager.pysrc/REvoDesign/tools/safe_pickle.pysrc/REvoDesign/tools/utils.pytests/server/test_pssm_gremlin.pytools/copyright.jstools/release_tag.sh
💤 Files with no reviewable changes (2)
- server/.env.example
- src/REvoDesign/tools/cgo_utils.py
Comment on lines
+1516
to
+1531
| function triggerLogout() { | ||
| const targetPath = "/PSSM_GREMLIN/create_task"; | ||
| const xhr = new XMLHttpRequest(); | ||
| xhr.open("GET", targetPath, true, "logout", "logout"); | ||
| xhr.onload = () => { | ||
| window.location.href = targetPath; | ||
| }; | ||
| xhr.onerror = () => { | ||
| window.location.href = targetPath; | ||
| }; | ||
| try { | ||
| xhr.send(); | ||
| } catch (error) { | ||
| window.location.href = targetPath; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -i "pssm_gremlin_dashboard.html"Repository: YaoYinYing/REvoDesign
Length of output: 122
🏁 Script executed:
wc -l server/pssm_gremlin/templates/pssm_gremlin_dashboard.htmlRepository: YaoYinYing/REvoDesign
Length of output: 127
🏁 Script executed:
sed -n '1510,1535p' server/pssm_gremlin/templates/pssm_gremlin_dashboard.htmlRepository: YaoYinYing/REvoDesign
Length of output: 741
🏁 Script executed:
sed -n '1615,1625p' server/pssm_gremlin/templates/pssm_gremlin_dashboard.htmlRepository: YaoYinYing/REvoDesign
Length of output: 429
🌐 Web query:
XMLHttpRequest.open() with username password parameter for Basic Auth logout browser compatibility Chrome Firefox
💡 Result:
XMLHttpRequest.open(method, url, async, user, password)is still part of the web API (supported in the interface/signature), but you can’t rely on it for HTTP Basic Auth behavior to be consistent across browsers/versions. (developer.mozilla.org)- In particular, Chrome has had real-world cases where the
user/passwordarguments are ignored and the browser shows the Basic Auth prompt anyway, breaking “silent login” flows that worked elsewhere. (peteyat.es) - Don’t use
http(s)://user:pass@host/as a workaround: putting credentials in the URL is deprecated, and Chrome may ignore/reject it. (stackoverflow.com)
What to do instead (works in Chrome + Firefox)
Send the header yourself (XHR or fetch):
const token = btoa(`${user}:${pass}`);
const xhr = new XMLHttpRequest();
xhr.open("GET", "/protected");
xhr.setRequestHeader("Authorization", `Basic ${token}`);
xhr.send();(Equivalent with fetch: headers: { Authorization: "Basic " + token }.)
“Logout” with Basic Auth (Chrome/Firefox reality)
- There is no reliable, standards-based way for a web page to force-clear the browser’s cached Basic Auth credentials; “logout” is the classic Basic Auth “roach motel” problem. (stackoverflow.com)
- Common hacks (vary by browser/version):
- Return
401 UnauthorizedwithWWW-Authenticateto force a re-prompt; some browsers clear credentials if the user cancels, but it’s not dependable. (the-art-of-web.com) - Change the auth realm (e.g., per-session realm) so cached credentials no longer apply—messy but sometimes effective. (the-art-of-web.com)
- Return
Practical recommendation
If you need a predictable “Log out” button across Chrome + Firefox, don’t use browser-managed Basic Auth for an interactive web app—use cookie/session (or token) auth, where logout is under application control. (stackoverflow.com)
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/open?utm_source=openai
- 2: https://peteyat.es/blog/2012/05/21/chrome-19-xmlhttprequest-open-with-authentication-is-broken/?utm_source=openai
- 3: https://stackoverflow.com/questions/54695788/http-basic-authentication-in-url-supported-or-deprecated?utm_source=openai
- 4: https://stackoverflow.com/questions/233507/how-to-log-out-user-from-web-site-using-basic-authentication?utm_source=openai
- 5: https://www.the-art-of-web.com/system/safari-broken-logout/?utm_source=openai
- 6: https://www.the-art-of-web.com/system/safari-broken-logout/?utm_source=openai
- 7: https://stackoverflow.com/questions/233507/how-to-log-out-user-from-web-site-using-basic-authentication?utm_source=openai
Logout implementation is unreliable for Basic Auth; server-driven approach required.
The XMLHttpRequest.open("GET", targetPath, true, "logout", "logout") approach does not reliably clear cached Basic Auth credentials. Browsers (especially Chrome) ignore the username/password parameters and continue to supply cached credentials automatically. There is no standards-based way to force-clear cached Basic Auth from the browser, making the redirect to create_task likely to re-authenticate immediately and appear as if logout failed.
Instead, add a real /PSSM_GREMLIN/logout endpoint that returns 401 Unauthorized with WWW-Authenticate header and a "You are logged out" confirmation page, then navigate there.
Also, document.getElementById("logoutBtn") at line 1618 should be guarded against null to prevent a runtime error in template variants:
Suggested null-guard
- document.getElementById("logoutBtn").addEventListener("click", () => {
- triggerLogout();
- });
+ const logoutBtn = document.getElementById("logoutBtn");
+ if (logoutBtn) {
+ logoutBtn.addEventListener("click", () => {
+ triggerLogout();
+ });
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/pssm_gremlin/templates/pssm_gremlin_dashboard.html` around lines 1516
- 1531, Replace the unreliable XMLHttpRequest-based logout in the triggerLogout
function with a client redirect to a new server-driven logout endpoint (e.g.,
navigate to "/PSSM_GREMLIN/logout") and implement that endpoint server-side to
respond with 401 and a WWW-Authenticate header plus a simple "You are logged
out" confirmation page; also guard the document.getElementById("logoutBtn")
usage with a null check before attaching event listeners to avoid runtime
errors. Ensure references: update triggerLogout to perform window.location.href
= "/PSSM_GREMLIN/logout" and create a server handler for the
"/PSSM_GREMLIN/logout" path that returns 401 + WWW-Authenticate and a logout
confirmation body, and add a null-guard where getElementById("logoutBtn") is
used.
Comment on lines
+15
to
+17
| if (result.status !== 0) { | ||
| process.exit(result.status || 1); | ||
| } |
There was a problem hiding this comment.
Surface spawn-level errors (result.error).
When bash itself can't be found or the OS rejects the spawn, result.status is null and result.error is a Node.js Error. The || 1 fallback exits correctly, but the error is silently swallowed — no diagnostic is printed because stdio: "inherit" only wires the child's stdio, not Node.js-level errors.
🛠️ Proposed fix
if (result.status !== 0) {
+ if (result.error) {
+ console.error("Failed to spawn bash:", result.error.message);
+ }
process.exit(result.status || 1);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/copyright.js` around lines 15 - 17, When handling the child spawn
result in the block that checks if (result.status !== 0), also check for
spawn-level errors via result.error and print a diagnostic before exiting;
update the logic around result.status and process.exit(result.status || 1) to
detect if result.error exists and call console.error (or the project's logger)
with a clear message and the Error object, then exit with the existing fallback
exit code so the error isn't silently swallowed.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/REvoDesign_PSSM_GREMLIN.sh (1)
278-287:⚠️ Potential issue | 🟠 MajorAvoid
readlink -fbefore the output directory exists.With
set -e,readlink -ffails on non-existent paths, so the default output directory (and any new user-specified directory) will terminate the script beforemkdir -pruns.Suggested fix
if [[ -z "${output_dir:-}" ]]; then fasta_fn=$(basename "${fasta_fp}") instance=${fasta_fn%.fasta} - pipline_res_dir=$(readlink -f "${instance}"_GREMLIN_PSSM_output) + pipline_res_dir="${instance}_GREMLIN_PSSM_output" else - pipline_res_dir=$(readlink -f "$output_dir") + pipline_res_dir="$output_dir" fi -mkdir -p "$pipline_res_dir"/log +mkdir -p "$pipline_res_dir"/log +pipline_res_dir=$(readlink -f "$pipline_res_dir")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/REvoDesign_PSSM_GREMLIN.sh` around lines 278 - 287, The script calls readlink -f to canonicalize pipline_res_dir before ensuring the directory exists, which fails under set -e for non-existent paths (see variables pipline_res_dir, output_dir, fasta_fp, instance, fasta_fn); change the logic to determine the desired path string then mkdir -p that path first and only afterwards canonicalize it (or use readlink/realpath with the -m/--canonicalize-missing option if available) so the script creates the directory before attempting to resolve it and avoids early termination.
♻️ Duplicate comments (2)
server/.env.test (1)
30-34:⚠️ Potential issue | 🟡 MinorFix the runner identity section header.
This block defines RUNNER_UID/GID/USERNAME/GROUP but the header still says “redis settings,” which is misleading.
Suggested fix
-## redis settings +## Runner identity settings RUNNER_UID=503🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/.env.test` around lines 30 - 34, The header above the runner identity env vars is incorrect; replace the misleading "redis settings" header with a clear section header for the runner identity so it matches the variables RUNNER_UID, RUNNER_GID, RUNNER_USERNAME, and RUNNER_GROUP; update the comment line immediately above those variables to something like "runner identity" or "runner settings" to accurately describe the block.src/REvoDesign/shortcuts/dialog_hooks.py (1)
23-24:⚠️ Potential issue | 🟠 MajorAvoid private Biopython API usage for FASTA writers.
SeqIO._FormatToWriteris internal and can change without notice. Prefer a public API or keep a defensive fallback list to avoid breaking on Biopython upgrades.Suggested fix (fallback if private API changes)
def get_fasta_writer_choices() -> list[str]: - return [fmt for fmt in SeqIO._FormatToWriter.keys() if fmt.startswith("fas")] + try: + formats = SeqIO._FormatToWriter.keys() + except Exception: + return ["fasta", "fasta-2line"] + return [fmt for fmt in formats if fmt.startswith("fas")]Biopython SeqIO _FormatToWriter public API alternative🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/shortcuts/dialog_hooks.py` around lines 23 - 24, The function get_fasta_writer_choices currently reads the private SeqIO._FormatToWriter mapping; change it to avoid the private API by trying to access SeqIO._FormatToWriter inside a try/except and, if that attribute is missing or not mapping-like, return a defensive hard-coded fallback list of common FASTA writer formats (e.g. "fasta", "fasta-2line") so upgrades to Biopython won't break; update the function name reference get_fasta_writer_choices and the SeqIO access to implement this try/except + fallback logic.
🧹 Nitpick comments (3)
server/pssm_gremlin/pssm_gremlin.py (1)
367-392: Run pre-commit hooks before pushing.As per coding guidelines, Enable pre-commit hooks with
pre-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 `@server/pssm_gremlin/pssm_gremlin.py` around lines 367 - 392, Run the project's pre-commit hooks and format/lint the changed files: run `pre-commit install` (once) and then `pre-commit run --all-files` or `make black` to apply formatting and fix linter issues for the modified functions like _path_is_within, _safe_join, and _normalize_task_id so the commit adheres to the repository’s style checks before pushing.src/REvoDesign/shortcuts/dialog_hooks.py (1)
27-33: Run pre-commit hooks before pushing.As per coding guidelines, Enable pre-commit hooks with
pre-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/shortcuts/dialog_hooks.py` around lines 27 - 33, Pre-commit hooks were not run before pushing; run and enable them and fix any formatting/lint failures: run `pre-commit install` in the repo, execute `pre-commit run --all-files` (or `make black` if preferred), and address any reported issues in the code paths shown (functions get_designable_chain_ids and get_selections in dialog_hooks.py) until all hooks pass, then commit the fixes and push.tests/server/test_pssm_gremlin.py (1)
1-18: Run focused tests and pre-commit for this update.As per coding guidelines, Enable pre-commit hooks with
pre-commit installand runpre-commit run --all-filesormake blackbefore pushing to ensure formatting and linting; and 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/server/test_pssm_gremlin.py` around lines 1 - 18, Run the project's pre-commit hooks and focused tests before pushing: in the repository run `pre-commit install` then `pre-commit run --all-files` (or `make black` to fix formatting), and to run fast/specific tests for this test module (test_pssm_gremlin) use `make kw-test PYTEST_KW='<keyword>'` replacing <keyword> with the test marker or name you want to run; fix any lint/format/test failures reported and re-run the pre-commit/tests until clean.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/REvoDesign_PSSM_GREMLIN.sh`:
- Around line 278-287: The script calls readlink -f to canonicalize
pipline_res_dir before ensuring the directory exists, which fails under set -e
for non-existent paths (see variables pipline_res_dir, output_dir, fasta_fp,
instance, fasta_fn); change the logic to determine the desired path string then
mkdir -p that path first and only afterwards canonicalize it (or use
readlink/realpath with the -m/--canonicalize-missing option if available) so the
script creates the directory before attempting to resolve it and avoids early
termination.
---
Duplicate comments:
In `@server/.env.test`:
- Around line 30-34: The header above the runner identity env vars is incorrect;
replace the misleading "redis settings" header with a clear section header for
the runner identity so it matches the variables RUNNER_UID, RUNNER_GID,
RUNNER_USERNAME, and RUNNER_GROUP; update the comment line immediately above
those variables to something like "runner identity" or "runner settings" to
accurately describe the block.
In `@src/REvoDesign/shortcuts/dialog_hooks.py`:
- Around line 23-24: The function get_fasta_writer_choices currently reads the
private SeqIO._FormatToWriter mapping; change it to avoid the private API by
trying to access SeqIO._FormatToWriter inside a try/except and, if that
attribute is missing or not mapping-like, return a defensive hard-coded fallback
list of common FASTA writer formats (e.g. "fasta", "fasta-2line") so upgrades to
Biopython won't break; update the function name reference
get_fasta_writer_choices and the SeqIO access to implement this try/except +
fallback logic.
---
Nitpick comments:
In `@server/pssm_gremlin/pssm_gremlin.py`:
- Around line 367-392: Run the project's pre-commit hooks and format/lint the
changed files: run `pre-commit install` (once) and then `pre-commit run
--all-files` or `make black` to apply formatting and fix linter issues for the
modified functions like _path_is_within, _safe_join, and _normalize_task_id so
the commit adheres to the repository’s style checks before pushing.
In `@src/REvoDesign/shortcuts/dialog_hooks.py`:
- Around line 27-33: Pre-commit hooks were not run before pushing; run and
enable them and fix any formatting/lint failures: run `pre-commit install` in
the repo, execute `pre-commit run --all-files` (or `make black` if preferred),
and address any reported issues in the code paths shown (functions
get_designable_chain_ids and get_selections in dialog_hooks.py) until all hooks
pass, then commit the fixes and push.
In `@tests/server/test_pssm_gremlin.py`:
- Around line 1-18: Run the project's pre-commit hooks and focused tests before
pushing: in the repository run `pre-commit install` then `pre-commit run
--all-files` (or `make black` to fix formatting), and to run fast/specific tests
for this test module (test_pssm_gremlin) use `make kw-test
PYTEST_KW='<keyword>'` replacing <keyword> with the test marker or name you want
to run; fix any lint/format/test failures reported and re-run the
pre-commit/tests until clean.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.mdserver/.env.testserver/REvoDesign_PSSM_GREMLIN.shserver/docker-compose.ymlserver/pssm_gremlin/pssm_gremlin.pyserver/pssm_gremlin/templates/pssm_gremlin_dashboard.htmlsrc/REvoDesign/shortcuts/dialog_hooks.pytests/server/test_pssm_gremlin.pytools/release_tag.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- tools/release_tag.sh
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/REvoDesign_PSSM_GREMLIN.sh (1)
285-291:⚠️ Potential issue | 🟠 MajorAvoid
readlink -fon paths with non-existent parent directories whenset -eis enabled.With
set -eenabled in this script,readlink -fexits with failure if any parent directory doesn't exist. This breaks the-oflag flow when users specify nested output directories that don't yet exist (e.g.,-o outputs/exp1/results). The current code callsreadlink -fon lines 288 and 290 beforemkdir -pon line 293, triggering this failure.Create the directory first, then resolve its canonical path:
Safer output-dir resolution
if [[ -z "${output_dir:-}" ]]; then fasta_fn=$(basename "${fasta_fp}") instance=${fasta_fn%.fasta} - pipline_res_dir=$(readlink -f "${instance}"_GREMLIN_PSSM_output) + pipline_res_dir="${instance}_GREMLIN_PSSM_output" else - pipline_res_dir=$(readlink -f "$output_dir") + pipline_res_dir="$output_dir" fi +mkdir -p "$pipline_res_dir" +pipline_res_dir=$(readlink -f "$pipline_res_dir")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/REvoDesign_PSSM_GREMLIN.sh` around lines 285 - 291, The script currently calls readlink -f on pipline_res_dir (in both branches where output_dir is empty or set) before ensuring parent directories exist, which fails under set -e for nested non-existent paths; fix by creating the directory first (use mkdir -p on the target pipline_res_dir or its parent) and only then call readlink -f to canonicalize it; update the branches that set pipline_res_dir (using fasta_fp/fasta_fn/instance when output_dir is empty and using output_dir when provided) so they mkdir -p the intended directory before resolving it and ensure pipline_res_dir ends up with the absolute path after resolution.
🤖 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 24-27: Reorder the environment keys so MAXMEM appears before NPROC
in the .env.example to satisfy dotenv-linter's alphabetical/key-order rule;
update the block containing MAXMEM and NPROC by moving the "MAXMEM=64" line
above "NPROC=4" (keep surrounding comments intact) so the file shows MAXMEM
first and NPROC second.
In `@server/docker-compose.yml`:
- Around line 35-40: The x-docker-socket-access anchor currently grants both the
DOCKER_GID and the root group ("0"), which contradicts the least-privilege
comment; update the x-docker-socket-access anchor (the group_add list referenced
by services) to remove the "0" entry so only "${DOCKER_GID:-998}" is granted,
ensuring Docker socket access uses the non-root group only.
---
Outside diff comments:
In `@server/REvoDesign_PSSM_GREMLIN.sh`:
- Around line 285-291: The script currently calls readlink -f on pipline_res_dir
(in both branches where output_dir is empty or set) before ensuring parent
directories exist, which fails under set -e for nested non-existent paths; fix
by creating the directory first (use mkdir -p on the target pipline_res_dir or
its parent) and only then call readlink -f to canonicalize it; update the
branches that set pipline_res_dir (using fasta_fp/fasta_fn/instance when
output_dir is empty and using output_dir when provided) so they mkdir -p the
intended directory before resolving it and ensure pipline_res_dir ends up with
the absolute path after resolution.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.gitignoreCHANGELOG.mdserver/.env.exampleserver/.env.testserver/README.mdserver/REvoDesign_PSSM_GREMLIN.shserver/docker-compose.ymlserver/pssm_gremlin/pssm_gremlin.pytests/server/test_pssm_gremlin.py
💤 Files with no reviewable changes (1)
- server/.env.test
Comment on lines
24
to
+27
| ## Maximum number to run the server: NPROC * WORKER_CONCURRENCY | ||
| NPROC=4 | ||
| ## Maximum memory, at GB | ||
| MAXMEM=64 |
There was a problem hiding this comment.
Reorder MAXMEM ahead of NPROC to satisfy dotenv-linter.
The linter warning will persist until the key order is corrected.
♻️ Suggested reorder
-## Maximum number to run the server: NPROC * WORKER_CONCURRENCY
-NPROC=4
-## Maximum memory, at GB
-MAXMEM=64
+## Maximum memory, at GB
+MAXMEM=64
+## Maximum number to run the server: NPROC * WORKER_CONCURRENCY
+NPROC=4📝 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.
Suggested change
| ## Maximum number to run the server: NPROC * WORKER_CONCURRENCY | |
| NPROC=4 | |
| ## Maximum memory, at GB | |
| MAXMEM=64 | |
| ## Maximum memory, at GB | |
| MAXMEM=64 | |
| ## Maximum number to run the server: NPROC * WORKER_CONCURRENCY | |
| NPROC=4 |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 27-27: [UnorderedKey] The MAXMEM key should go before the NPROC 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 24 - 27, Reorder the environment keys so
MAXMEM appears before NPROC in the .env.example to satisfy dotenv-linter's
alphabetical/key-order rule; update the block containing MAXMEM and NPROC by
moving the "MAXMEM=64" line above "NPROC=4" (keep surrounding comments intact)
so the file shows MAXMEM first and NPROC second.
Comment on lines
35
to
40
| x-docker-socket-access: &docker-socket-access | ||
| # Least privilege by default: only grant Docker socket group access. | ||
| # Do not add root group access ("0") here unless you explicitly accept the security tradeoff. | ||
| group_add: | ||
| - "${DOCKER_GID:-998}" | ||
| - "0" |
There was a problem hiding this comment.
Drop root group (0) from docker socket access by default.
The comment says least-privilege, but root group is still granted. This weakens the intended hardening and expands access to the Docker socket.
🔒 Suggested change
group_add:
- "${DOCKER_GID:-998}"
- - "0"📝 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.
Suggested change
| x-docker-socket-access: &docker-socket-access | |
| # Least privilege by default: only grant Docker socket group access. | |
| # Do not add root group access ("0") here unless you explicitly accept the security tradeoff. | |
| group_add: | |
| - "${DOCKER_GID:-998}" | |
| - "0" | |
| x-docker-socket-access: &docker-socket-access | |
| # Least privilege by default: only grant Docker socket group access. | |
| # Do not add root group access ("0") here unless you explicitly accept the security tradeoff. | |
| group_add: | |
| - "${DOCKER_GID:-998}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/docker-compose.yml` around lines 35 - 40, The x-docker-socket-access
anchor currently grants both the DOCKER_GID and the root group ("0"), which
contradicts the least-privilege comment; update the x-docker-socket-access
anchor (the group_add list referenced by services) to remove the "0" entry so
only "${DOCKER_GID:-998}" is granted, ensuring Docker socket access uses the
non-root group only.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/server/test_pssm_gremlin.py (1)
1-16:⚠️ Potential issue | 🟡 MinorRun pre-commit and targeted kw-test for this test update.
Please runpre-commit installandpre-commit run --all-files(ormake black), and executemake kw-test PYTEST_KW='pssm_gremlin'(or the most relevant keyword) to validate these changes.
As per coding guidelines, "Enable pre-commit hooks withpre-commit installand runpre-commit run --all-filesormake blackbefore pushing to ensure formatting and linting" and "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/server/test_pssm_gremlin.py` around lines 1 - 16, Run the required pre-commit formatting and the targeted keyword test for the updated test file tests/server/test_pssm_gremlin.py: install pre-commit hooks with "pre-commit install" then run "pre-commit run --all-files" (or run "make black" if you prefer) to fix lint/format issues, and finally execute the focused test suite with "make kw-test PYTEST_KW='pssm_gremlin'" to validate the changes to test_pssm_gremlin.py.
♻️ Duplicate comments (1)
tools/copyright.js (1)
13-15:⚠️ Potential issue | 🟡 MinorSurface spawn failures from
spawnSync(result.error).
Ifbashcannot be spawned,result.statusis null and the failure is silent; logresult.errorbefore exiting.🛠️ Suggested fix
if (result.status !== 0) { + if (result.error) { + console.error("Failed to spawn bash:", result.error.message); + } process.exit(result.status || 1); }Node.js child_process spawnSync result.error when spawn fails🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/copyright.js` around lines 13 - 15, When using spawnSync to run the bash command in tools/copyright.js, handle the case where the child cannot be spawned by checking result.error in addition to result.status; if result.error is present, log or print the error (including context such as the command string) before calling process.exit with result.status || 1 so the spawn failure is surfaced; update the block around the spawnSync call (the result variable and its error handling) to detect result.error and include it in the exit path.
🧹 Nitpick comments (1)
server/pssm_gremlin/templates/create_task.html (1)
616-654: Respect prefers-reduced-motion for the flip animation.Consider skipping the animation when users request reduced motion.
♿ Suggested guard for reduced motion
function syncThemeToggle(animate = false) { const button = document.getElementById("themeToggle"); if (!button) return; const mode = document.documentElement.dataset.themeMode || getStoredThemeMode(); const effectiveTheme = document.documentElement.dataset.theme || resolveThemeMode(mode); const icon = themeModeIcon[mode] || themeModeIcon.auto; @@ - if (animate) { + const prefersReducedMotion = window.matchMedia && + window.matchMedia("(prefers-reduced-motion: reduce)").matches; + if (animate && !prefersReducedMotion) { button.classList.remove("is-animating"); void button.offsetWidth; button.classList.add("is-animating");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/pssm_gremlin/templates/create_task.html` around lines 616 - 654, The flip animation in syncThemeToggle currently always runs when animate=true; update syncThemeToggle (and any callers like applyThemeMode that pass animate) to respect the user's prefers-reduced-motion setting by checking window.matchMedia('(prefers-reduced-motion: reduce)').matches and treating that as animate=false; if reduced motion is requested, skip adding/removing the "is-animating" class, avoid setting/clearing dataset.animTimer, and do not call the offsetWidth forced reflow so no animation is triggered. Ensure applyThemeMode's animate argument is still supported but suppressed when reduced-motion is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/copyright.js`:
- Around line 6-11: Run the repository pre-commit hooks and formatters for the
changes to tools/copyright.js: install hooks with `pre-commit install` and then
execute `pre-commit run --all-files` (or run `make black`) so linting/formatting
will be applied to this script (notably the code that defines `command` and uses
`spawnSync`); after fixing any reported issues, re-stage the file and push the
commit.
---
Outside diff comments:
In `@tests/server/test_pssm_gremlin.py`:
- Around line 1-16: Run the required pre-commit formatting and the targeted
keyword test for the updated test file tests/server/test_pssm_gremlin.py:
install pre-commit hooks with "pre-commit install" then run "pre-commit run
--all-files" (or run "make black" if you prefer) to fix lint/format issues, and
finally execute the focused test suite with "make kw-test
PYTEST_KW='pssm_gremlin'" to validate the changes to test_pssm_gremlin.py.
---
Duplicate comments:
In `@tools/copyright.js`:
- Around line 13-15: When using spawnSync to run the bash command in
tools/copyright.js, handle the case where the child cannot be spawned by
checking result.error in addition to result.status; if result.error is present,
log or print the error (including context such as the command string) before
calling process.exit with result.status || 1 so the spawn failure is surfaced;
update the block around the spawnSync call (the result variable and its error
handling) to detect result.error and include it in the exit path.
---
Nitpick comments:
In `@server/pssm_gremlin/templates/create_task.html`:
- Around line 616-654: The flip animation in syncThemeToggle currently always
runs when animate=true; update syncThemeToggle (and any callers like
applyThemeMode that pass animate) to respect the user's prefers-reduced-motion
setting by checking window.matchMedia('(prefers-reduced-motion:
reduce)').matches and treating that as animate=false; if reduced motion is
requested, skip adding/removing the "is-animating" class, avoid setting/clearing
dataset.animTimer, and do not call the offsetWidth forced reflow so no animation
is triggered. Ensure applyThemeMode's animate argument is still supported but
suppressed when reduced-motion is true.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.mdserver/docker/server/Dockerfileserver/pssm_gremlin/templates/create_task.htmltests/server/test_pssm_gremlin.pytools/copyright.js
🚧 Files skipped from review as they are similar to previous changes (1)
- server/docker/server/Dockerfile
Comment on lines
+6
to
+11
| const { spawnSync } = require("child_process"); | ||
|
|
||
| const command = [ | ||
| "find ./src/REvoDesign -type f -name '*.py' -print0", | ||
| "xargs -0 cat > ./program.docx", | ||
| ].join(" | "); |
There was a problem hiding this comment.
Run pre-commit hooks before pushing.
Please run pre-commit install and pre-commit run --all-files (or make black) so lint/format checks cover this script.
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 `@tools/copyright.js` around lines 6 - 11, Run the repository pre-commit hooks
and formatters for the changes to tools/copyright.js: install hooks with
`pre-commit install` and then execute `pre-commit run --all-files` (or run `make
black`) so linting/formatting will be applied to this script (notably the code
that defines `command` and uses `spawnSync`); after fixing any reported issues,
re-stage the file and push the commit.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/REvoDesign/shortcuts/dialog_hooks.py (1)
23-24:⚠️ Potential issue | 🟠 Major
SeqIO._FormatToWriterstill relies on a private Biopython API.This was flagged in a previous review and remains unaddressed.
Biopython 1.81 SeqIO._FormatToWriter private attribute public alternative🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/shortcuts/dialog_hooks.py` around lines 23 - 24, The code is reading the private Biopython registry SeqIO._FormatToWriter in get_fasta_writer_choices; replace this private API usage by returning an explicit, maintained list of supported FASTA-related writer format strings (e.g. ["fasta","fastq","fastq-sanger", ...]) instead of touching SeqIO._FormatToWriter, and update get_fasta_writer_choices to return that list (or to filter a defined FASTA_FORMATS constant) so we no longer depend on the private SeqIO._FormatToWriter symbol.
🧹 Nitpick comments (4)
src/REvoDesign/shortcuts/dialog_hooks.py (3)
27-43:get_designable_chain_idsandget_all_chain_idsare identical — consolidate.Both functions have exactly the same body; having two separate names for the same logic is a DRY violation and risks divergence later.
♻️ Proposed refactor
def get_designable_chain_ids() -> list[str]: designable = ConfigBus().get_value("designable_sequences", dict, reject_none=True, cfg="runtime") return list(designable.keys()) -def get_all_chain_ids() -> list[str]: - designable = ConfigBus().get_value("designable_sequences", dict, reject_none=True, cfg="runtime") - return list(designable.keys()) +def get_all_chain_ids() -> list[str]: + return get_designable_chain_ids()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/shortcuts/dialog_hooks.py` around lines 27 - 43, Consolidate the duplicate logic in get_designable_chain_ids and get_all_chain_ids by implementing the config lookup in a single place: keep one primary function (e.g., get_designable_chain_ids) that calls ConfigBus().get_value("designable_sequences", dict, reject_none=True, cfg="runtime") and returns list(designable.keys()), and make get_all_chain_ids simply delegate to that primary function (or vice versa) so there is one source of truth for fetching chain ids and no duplicated bodies; update any references if necessary to use the delegating function name.
46-55:get_all_object_namesandget_all_objectsare identical — consolidate; also add missing return type hints.
get_all_object_namesandget_all_objectsboth returncmd.get_names("objects")with no difference. Additionally,get_all_object_names,get_all_selections, andget_all_objectsare the only functions in this file without return type annotations.♻️ Proposed refactor
-def get_all_object_names(): +def get_all_object_names() -> list[str]: return cmd.get_names("objects") -def get_all_selections(): +def get_all_selections() -> list[str]: return cmd.get_names("selections") -def get_all_objects(): - return cmd.get_names("objects") +def get_all_objects() -> list[str]: + return get_all_object_names()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/shortcuts/dialog_hooks.py` around lines 46 - 55, The two functions get_all_object_names and get_all_objects are duplicates; consolidate them by keeping a single canonical function name (choose one, e.g., get_all_object_names) and remove the other, then update any internal call-sites to use the retained name (search for get_all_objects). Also add explicit return type annotations to the three functions in this file—get_all_object_names, get_all_selections, and the removed/renamed function if you temporarily keep it—using the appropriate type (e.g., -> list[str] or Sequence[str]) to match cmd.get_names return; ensure imports/types are adjusted if necessary.
32-33: Prefer unpacking over list concatenation (Ruff RUF005).♻️ Proposed fix
def get_selections() -> list[str]: - return [""] + list(cmd.get_names("selections")) + return ["", *cmd.get_names("selections")]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/REvoDesign/shortcuts/dialog_hooks.py` around lines 32 - 33, The get_selections function uses list concatenation ([""] + list(cmd.get_names("selections"))) which Ruff flags; change it to use sequence unpacking by returning ["", *cmd.get_names("selections")] (or ["", *list(cmd.get_names("selections"))] if cmd.get_names isn't already iterable) so the empty string is prepended via unpacking in the get_selections function.tests/server/test_pssm_gremlin.py (1)
1702-1723: Minor:time.timeis not mocked, making the test timeout-sensitive.The test sets
timeout=5.0but only mockstime.sleep, nottime.time. The stub resolves on the 2nd auth attempt (after the readiness probe succeeds), so in practice the real wall-clock should stay well within 5 seconds. However, on an extremely loaded CI runner, this could theoretically flake. If you ever see intermittent failures here, mockingtime.timewith a controlled clock would fix it.Otherwise the test logic is correct and effectively validates the retry path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server/test_pssm_gremlin.py` around lines 1702 - 1723, The test test_wait_for_server_ready_retries_transient_401 is mocking time.sleep but not time.time, making the timeout calculation in _wait_for_server_ready sensitive to real wall-clock delays; update the test to monkeypatch time.time to a controlled, incrementing fake clock (used alongside the existing mocked sleep) so timeout checks in _wait_for_server_ready use the deterministic fake time; reference the test name, the _StubSession, and _wait_for_server_ready when making this change so you replace time.time with a predictable stub (e.g., a closure or generator-backed callable) that advances in step with the mocked sleep.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/REvoDesign/shortcuts/dialog_hooks.py`:
- Around line 23-24: The code is reading the private Biopython registry
SeqIO._FormatToWriter in get_fasta_writer_choices; replace this private API
usage by returning an explicit, maintained list of supported FASTA-related
writer format strings (e.g. ["fasta","fastq","fastq-sanger", ...]) instead of
touching SeqIO._FormatToWriter, and update get_fasta_writer_choices to return
that list (or to filter a defined FASTA_FORMATS constant) so we no longer depend
on the private SeqIO._FormatToWriter symbol.
---
Nitpick comments:
In `@src/REvoDesign/shortcuts/dialog_hooks.py`:
- Around line 27-43: Consolidate the duplicate logic in get_designable_chain_ids
and get_all_chain_ids by implementing the config lookup in a single place: keep
one primary function (e.g., get_designable_chain_ids) that calls
ConfigBus().get_value("designable_sequences", dict, reject_none=True,
cfg="runtime") and returns list(designable.keys()), and make get_all_chain_ids
simply delegate to that primary function (or vice versa) so there is one source
of truth for fetching chain ids and no duplicated bodies; update any references
if necessary to use the delegating function name.
- Around line 46-55: The two functions get_all_object_names and get_all_objects
are duplicates; consolidate them by keeping a single canonical function name
(choose one, e.g., get_all_object_names) and remove the other, then update any
internal call-sites to use the retained name (search for get_all_objects). Also
add explicit return type annotations to the three functions in this
file—get_all_object_names, get_all_selections, and the removed/renamed function
if you temporarily keep it—using the appropriate type (e.g., -> list[str] or
Sequence[str]) to match cmd.get_names return; ensure imports/types are adjusted
if necessary.
- Around line 32-33: The get_selections function uses list concatenation ([""] +
list(cmd.get_names("selections"))) which Ruff flags; change it to use sequence
unpacking by returning ["", *cmd.get_names("selections")] (or ["",
*list(cmd.get_names("selections"))] if cmd.get_names isn't already iterable) so
the empty string is prepended via unpacking in the get_selections function.
In `@tests/server/test_pssm_gremlin.py`:
- Around line 1702-1723: The test
test_wait_for_server_ready_retries_transient_401 is mocking time.sleep but not
time.time, making the timeout calculation in _wait_for_server_ready sensitive to
real wall-clock delays; update the test to monkeypatch time.time to a
controlled, incrementing fake clock (used alongside the existing mocked sleep)
so timeout checks in _wait_for_server_ready use the deterministic fake time;
reference the test name, the _StubSession, and _wait_for_server_ready when
making this change so you replace time.time with a predictable stub (e.g., a
closure or generator-backed callable) that advances in step with the mocked
sleep.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.mdserver/docker-compose.ymlsrc/REvoDesign/shortcuts/dialog_hooks.pytests/server/test_pssm_gremlin.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- server/docker-compose.yml
This reverts commit 6740aa8.
_wait_for_server_ready
test_wait_for_server_ready_retries_transient_401
| from collections.abc import Iterable | ||
| from dataclasses import dataclass | ||
| from pathlib import Path | ||
| from types import SimpleNamespace |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests