Conversation
Introduce a new GitHub Actions workflow (.github/workflows/deploy.yml) to copy the repo to Toolforge, fix CRLF on shell scripts, run stage-1 (tool core update) and stage-2 (deploy YAML jobs + cleanup). Add Toolforge deployment scripts: toolforge/deploy_pybot_repo.sh, toolforge/deploy_tools.sh, and toolforge/deploy_scripts/{deploy_from_local.sh,update_pybot_local.sh} to handle copy, archive, optional compile, and tfj job loading. Move previous deploy-new.yml to .github/workflows/old_scripts/deploy.yml and remove the Snorkell documentation workflow and the Sweep issue template.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
WalkthroughThe pull request adds a GitHub Actions deployment pipeline and Toolforge deployment scripts, refactors logging and Python script execution, updates scheduled jobs, and changes several data-processing helpers. ChangesDeployment automation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant Server
participant deploy_pybot_repo
participant deploy_tools
GitHubActions->>Server: SCP repository to temporary path
GitHubActions->>deploy_pybot_repo: run Stage 1 deployment
deploy_pybot_repo->>Server: update pybot files
GitHubActions->>deploy_tools: run Stage 2 job deployment
deploy_tools->>Server: flush and load changed jobs
GitHubActions->>Server: remove temporary repository
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
toolforge/c9/logger_config.py (1)
57-69: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn type contract broken for non-string input.
format_colored_textis annotated-> str, but the new early return at Line 68 returnstextmunchanged whenever it isn't astr. Any caller relying on the declaredstrreturn (e.g. concatenation,.format()) can now receive an arbitrary object and fail at runtime. Also,str | Anyin the parameter annotation is redundant (Anyalready subsumesstr).🐛 Proposed fix
-def format_colored_text(textm: str | Any) -> str: +def format_colored_text(textm: Any) -> str | Any:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@toolforge/c9/logger_config.py` around lines 57 - 69, Update format_colored_text to always return a string for non-string inputs by converting textm with the standard string conversion before returning; also simplify its parameter annotation by removing the redundant str union with Any.
🧹 Nitpick comments (1)
toolforge/c9/user_conf.py (1)
7-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the exception type caught around
load_dotenv().Static analysis flags this blind
except Exception. Since this executes before logging is configured,MemoryError,KeyboardInterruptviaBaseExceptionsubclasses aside). Consider narrowing to the exceptionsload_dotenv/file I/O can actually raise (e.g.OSError).As flagged by Ruff: "Do not catch blind exception:
Exception" (BLE001).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@toolforge/c9/user_conf.py` around lines 7 - 10, Update the exception handler around load_dotenv() to catch the narrowest expected file-loading exception, such as OSError, while preserving the existing print(e) fallback.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/md_core_helps/one_time/wikiblame/bot.py`:
- Line 86: Update the parsing logic around results so BeautifulSoup’s find_all
is invoked on the BeautifulSoup element rather than the string variable text.
Preserve the existing anchor-result parsing behavior while ensuring pages with
results no longer raise AttributeError.
In `@toolforge/deploy_pybot_repo.sh`:
- Line 17: Ensure nested deployment commands propagate failures instead of
masking them with a later successful command: update
toolforge/deploy_pybot_repo.sh line 17 to chain the copy, permission, and
update_pybot_local.sh commands with && or invoke the inner shell with sh -euc;
apply the same failure propagation to the cp, tfj flush, and tfj load commands
at toolforge/deploy_tools.sh line 36.
In `@toolforge/deploy_scripts/deploy_from_local.sh`:
- Around line 7-10: Validate positional arguments safely under set -u by
initializing the variables with default-empty expansions before dereferencing
them: update REPO_TMP_DIR and TARGET_DIR in
toolforge/deploy_scripts/deploy_from_local.sh (lines 7-10), and FULL_PATH in
toolforge/deploy_pybot_repo.sh (lines 6-8). Preserve the existing usage-message
validation after these safe initializations.
- Line 106: Update the deployment permission logic around the find/chmod command
to stop applying 770 to every file. Set directory permissions separately, then
apply executable permissions only to the required deployment scripts, leaving
configs, JSON, and other non-executable files without group write/execute
access.
In `@toolforge/deploy_scripts/update_pybot_local.sh`:
- Line 14: Update the CLEAN_INSTALL configuration used by deploy_from_local.sh
from 0 to enable clean installs, ensuring destination-only files removed or
renamed from src are deleted during deployment.
- Line 30: Quote the script path in the deploy invocation so the HOME-derived
path is passed as a single argument and remains safe when it contains spaces or
glob characters. Update the command invoking deploy_from_local.sh while
preserving its existing arguments.
In `@toolforge/deploy_tools.sh`:
- Around line 19-25: Update the filename construction in deploy_tools.sh so it
matches the deployment contract and supplied toolforge/old_mdwiki-jobs.yaml
file, using the expected old_mdwiki-jobs.yaml name. Modify the existence check
around FULL_FILE_PATH to exit with a nonzero status when the expected
configuration file is absent instead of silently succeeding.
---
Outside diff comments:
In `@toolforge/c9/logger_config.py`:
- Around line 57-69: Update format_colored_text to always return a string for
non-string inputs by converting textm with the standard string conversion before
returning; also simplify its parameter annotation by removing the redundant str
union with Any.
---
Nitpick comments:
In `@toolforge/c9/user_conf.py`:
- Around line 7-10: Update the exception handler around load_dotenv() to catch
the narrowest expected file-loading exception, such as OSError, while preserving
the existing print(e) fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f7a8e6b-d935-4a43-bda5-b7c67357c4d7
📒 Files selected for processing (19)
.github/ISSUE_TEMPLATE/sweep-template.yml.github/workflows/deploy.yml.github/workflows/old_scripts/deploy.yml.github/workflows/snorkell-auto-documentation.ymlsrc/md_core/p11143_bot/wd_helps.pysrc/md_core_helps/one_time/prior/add_old.pysrc/md_core_helps/one_time/wikiblame/bot.pysrc/td_core/copy_data/by_qid/sitelinks.pysrc/td_core/mdpages/cashwd.pytoolforge/c9/logger_config.pytoolforge/c9/pwb.pytoolforge/c9/requirements.txttoolforge/c9/user_conf.pytoolforge/deploy_pybot_repo.shtoolforge/deploy_scripts/deploy_from_local.shtoolforge/deploy_scripts/update_pybot_local.shtoolforge/deploy_tools.shtoolforge/old_mdwiki-jobs.yamltoolforge/user-config.py
💤 Files with no reviewable changes (2)
- .github/ISSUE_TEMPLATE/sweep-template.yml
- .github/workflows/snorkell-auto-documentation.yml
| self.in_first = True | ||
| # --- | ||
| results = results_div.find_all("a") | ||
| results = text.find_all("a") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Call find_all() on the BeautifulSoup element, not its text.
Line [86] invokes find_all on text, which is a str from line [81]. This raises AttributeError for every page with results and prevents parsing entirely.
Proposed fix
- results = text.find_all("a")
+ results = results_div.find_all("a")📝 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.
| results = text.find_all("a") | |
| results = results_div.find_all("a") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/md_core_helps/one_time/wikiblame/bot.py` at line 86, Update the parsing
logic around results so BeautifulSoup’s find_all is invoked on the BeautifulSoup
element rather than the string variable text. Preserve the existing
anchor-result parsing behavior while ensuring pages with results no longer raise
AttributeError.
| echo ">>> FULL_PATH: $FULL_PATH" | ||
|
|
||
| # Run deployment steps inside mdwiki's toolforge context | ||
| become mdwiki sh -c "cp -rf \"$FULL_PATH/toolforge/deploy_scripts\" /data/project/mdwiki -v;chmod +x /data/project/mdwiki/deploy_scripts/*.sh -v;/data/project/mdwiki/deploy_scripts/update_pybot_local.sh \"$FULL_PATH\"" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate failures from nested deployment shells.
The parent shell’s set -e does not make sh -c fail fast. With semicolon-separated commands, an earlier copy, flush, or permission failure can be followed by a successful final command, causing deployment to report success incorrectly.
toolforge/deploy_pybot_repo.sh#L17-L17: use&&or invoke the inner shell withsh -euc.toolforge/deploy_tools.sh#L36-L36: apply the same failure propagation tocp,tfj flush, andtfj load.
📍 Affects 2 files
toolforge/deploy_pybot_repo.sh#L17-L17(this comment)toolforge/deploy_tools.sh#L36-L36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@toolforge/deploy_pybot_repo.sh` at line 17, Ensure nested deployment commands
propagate failures instead of masking them with a later successful command:
update toolforge/deploy_pybot_repo.sh line 17 to chain the copy, permission, and
update_pybot_local.sh commands with && or invoke the inner shell with sh -euc;
apply the same failure propagation to the cp, tfj flush, and tfj load commands
at toolforge/deploy_tools.sh line 36.
| REPO_TMP_DIR="$1" # Path to uploaded tmp folder, e.g., /home/username/arwiki_repo_tmp | ||
| TARGET_DIR="$2" # Target tool folder, e.g., ARWIKI_ONE_REPO | ||
|
|
||
| if [ -z "${1:-}" ] || [ -z "${2:-}" ]; then |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate positional arguments before dereferencing them under set -u.
Both scripts currently terminate with an unbound-variable error before their intended usage message can run.
toolforge/deploy_scripts/deploy_from_local.sh#L7-L10: initialize arguments with${1:-}and${2:-}before validation.toolforge/deploy_pybot_repo.sh#L6-L8: initializeFULL_PATHwith${1:-}before validation.
📍 Affects 2 files
toolforge/deploy_scripts/deploy_from_local.sh#L7-L10(this comment)toolforge/deploy_pybot_repo.sh#L6-L8
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@toolforge/deploy_scripts/deploy_from_local.sh` around lines 7 - 10, Validate
positional arguments safely under set -u by initializing the variables with
default-empty expansions before dereferencing them: update REPO_TMP_DIR and
TARGET_DIR in toolforge/deploy_scripts/deploy_from_local.sh (lines 7-10), and
FULL_PATH in toolforge/deploy_pybot_repo.sh (lines 6-8). Preserve the existing
usage-message validation after these safe initializations.
| # Optional: Set permissions | ||
| # chmod -R 770 "$TARGET_DIR" | ||
|
|
||
| find "$TARGET_DIR" -type f ! -name "*.pyc" -exec chmod 770 {} \; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,180p' toolforge/deploy_scripts/deploy_from_local.sh | cat -nRepository: Mdwiki-TD/mdwiki-python-files
Length of output: 4313
🏁 Script executed:
printf '%s\n' "need tool output"Repository: Mdwiki-TD/mdwiki-python-files
Length of output: 184
🏁 Script executed:
rg -n "chmod|find .*type" toolforge/deploy_scripts/deploy_from_local.shRepository: Mdwiki-TD/mdwiki-python-files
Length of output: 408
🏁 Script executed:
sed -n '80,130p' toolforge/deploy_scripts/deploy_from_local.sh | cat -nRepository: Mdwiki-TD/mdwiki-python-files
Length of output: 1361
Avoid chmod 770 on every deployed file
find "$TARGET_DIR" -type f ! -name "*.pyc" -exec chmod 770 {} \; gives group write/execute to configs, JSON, and other non-executable files. Set directory perms separately and only mark the scripts that need it as executable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@toolforge/deploy_scripts/deploy_from_local.sh` at line 106, Update the
deployment permission logic around the find/chmod command to stop applying 770
to every file. Set directory permissions separately, then apply executable
permissions only to the required deployment scripts, leaving configs, JSON, and
other non-executable files without group write/execute access.
| fi | ||
|
|
||
| export SUB_DIR_COPY="src" | ||
| export CLEAN_INSTALL=0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent stale files from surviving deployments.
With CLEAN_INSTALL=0, deploy_from_local.sh overlays the source onto pybot but never removes files deleted or renamed in src. Old modules can remain importable and execute in production. Enable clean installs or use a mirror operation that removes destination-only files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@toolforge/deploy_scripts/update_pybot_local.sh` at line 14, Update the
CLEAN_INSTALL configuration used by deploy_from_local.sh from 0 to enable clean
installs, ensuring destination-only files removed or renamed from src are
deleted during deployment.
|
|
||
| # Execute the local deployment script directly without cloning from GitHub | ||
| # We copy the script to ~/deploy_scripts/ first from the github action workflow, then call it | ||
| $HOME/deploy_scripts/deploy_from_local.sh "$REPO_TMP_DIR" "$TARGET_DIR" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant lines.
git ls-files toolforge/deploy_scripts/update_pybot_local.sh
wc -l toolforge/deploy_scripts/update_pybot_local.sh
cat -n toolforge/deploy_scripts/update_pybot_local.sh | sed -n '1,120p'
# Look for any related script references that might show intended quoting style.
rg -n '\$HOME/deploy_scripts/deploy_from_local\.sh|deploy_from_local\.sh' toolforge -SRepository: Mdwiki-TD/mdwiki-python-files
Length of output: 1448
Quote the script path. $HOME/deploy_scripts/deploy_from_local.sh should be "$HOME/deploy_scripts/deploy_from_local.sh" so HOME values with spaces or glob characters don’t break the invocation.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 30-30: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@toolforge/deploy_scripts/update_pybot_local.sh` at line 30, Quote the script
path in the deploy invocation so the HOME-derived path is passed as a single
argument and remains safe when it contains spaces or glob characters. Update the
command invoking deploy_from_local.sh while preserving its existing arguments.
Source: Linters/SAST tools
| filename="${tool}-jobs.yaml" | ||
| FULL_FILE_PATH="${SOURCE_PATH}/$filename" | ||
|
|
||
| echo "Checking for configuration file: $filename" | ||
|
|
||
| # Check if the file actually exists before proceeding | ||
| if [ -f "$FULL_FILE_PATH" ]; then |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the job filename match the deployment contract.
The workflow passes FULL_PATH/toolforge, but this script searches for mdwiki-jobs.yaml while the supplied file is toolforge/old_mdwiki-jobs.yaml. The condition is therefore skipped and the script exits successfully without loading the new jobs. Rename the file or update the expected filename, and fail when the expected file is absent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@toolforge/deploy_tools.sh` around lines 19 - 25, Update the filename
construction in deploy_tools.sh so it matches the deployment contract and
supplied toolforge/old_mdwiki-jobs.yaml file, using the expected
old_mdwiki-jobs.yaml name. Modify the existence check around FULL_FILE_PATH to
exit with a nonzero status when the expected configuration file is absent
instead of silently succeeding.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores