Finish renaming topsail-ng --> forge#7
Merged
kpouget merged 4 commits intoopenshift-psap:mainfrom Apr 2, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThis PR rebands the project from "TOPSAIL(-NG)" to "FORGE": renaming project metadata, container labels and paths, CI/workflow identifiers, default repo constants, environment/secrets keys, runtime/artifact paths, templates, and adds a new CLI subcommand Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
projects/core/dsl/runtime.py (1)
247-265:⚠️ Potential issue | 🔴 CriticalThe directory renaming to 'forge' is incomplete; the function will not find the expected directory.
The function now searches for a parent directory named
'forge'(line 252), but verification shows no 'forge' directory exists in the repository structure. The function will always fall through to the fallback logic since the condition on line 252 will never match.The remaining 'topsail-ng' references found in the codebase are not directory path logic and are intentional:
projects/jump_ci/toolbox/jump_ci.pyline 79 andprojects/jump_ci/testing/prepare_jump_ci.pyline 62: GitHub repository name stringsprojects/jump_ci/toolbox/jump_ci_prepare_topsail/tasks/main.ymlline 136: container image label filterEither rename the actual repository directory to 'forge' or update line 252 to match the actual directory name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/dsl/runtime.py` around lines 247 - 265, The loop that looks for the repository root checks for a parent directory named 'forge' (using filename_path.parents and parent.name) but the repo directory is actually named 'topsail-ng', so the check always fails; update the condition (the parent.name == 'forge' check inside the for parent in filename_path.parents loop) to match the real directory name (e.g., 'topsail-ng') or refactor to use a constant/setting (e.g., FORGE_HOME_NAME) and then compute forge_home, rel_path via filename_path.relative_to(forge_home) as before so the function returns the correct relative path instead of falling back.projects/jump_ci/toolbox/jump_ci_prepare_topsail/tasks/main.yml (1)
136-138:⚠️ Potential issue | 🟡 MinorImage cleanup filter still references old label
name=topsail-ng.The image pruning filter on line 136 uses
--filter label=name=topsail-ng, but the new images are built with--label owner=forge(lines 175, 186). This means:
- New images won't be cleaned up by this filter
- Only legacy images with the old label will be pruned
Update the filter to match the new label scheme.
🔧 Proposed fix
- name: List all the old images to delete # setting --filter label=preserve!=true in Podman args doesn't work shell: set -o pipefail; podman image ls --filter until=4h - --filter label=name=topsail-ng + --filter label=owner=forge --format json | jq -r '.[] | select(.Containers == 0) | select(.Labels.preserve != "true") | .Id' register: podman_images_to_delete_cmd🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/jump_ci/toolbox/jump_ci_prepare_topsail/tasks/main.yml` around lines 136 - 138, The image-cleanup command still uses the old filter string "--filter label=name=topsail-ng", so it won't match new images labeled with "--label owner=forge"; update the filter to "--filter label=owner=forge" in the pipeline that pipes to jq (the line containing the jq selection '.[] | select(.Containers == 0) | select(.Labels.preserve != "true") | .Id') — optionally include both filters (owner=forge and name=topsail-ng) if you want to retain cleanup of legacy images as well.
🧹 Nitpick comments (2)
projects/core/dsl/__init__.py (1)
9-9: Consider sorting__all__alphabetically.The static analyzer suggests applying isort-style alphabetical sorting to
__all__for consistency and maintainability. While the current order may reflect semantic grouping, alphabetical order is a common convention.♻️ Alphabetically sorted version
-__all__ = ['task', 'retry', 'when', 'always', 'execute_tasks', 'clear_tasks', 'shell'] +__all__ = ['always', 'clear_tasks', 'execute_tasks', 'retry', 'shell', 'task', 'when']🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/dsl/__init__.py` at line 9, The __all__ list is unsorted; update the module-level symbol list named __all__ to be alphabetically ordered so static analyzers and import conventions are satisfied — reorder the entries currently in __all__ ('task', 'retry', 'when', 'always', 'execute_tasks', 'clear_tasks', 'shell') into alphabetical order (e.g., 'always', 'clear_tasks', 'execute_tasks', 'retry', 'shell', 'task', 'when') while preserving the same variable name and semantics.projects/core/ci_entrypoint/run_ci.py (1)
38-38: Style: Multiple statements on one line.Per static analysis (Ruff E701), the
if not value: continueshould be on separate lines for clarity.♻️ Proposed fix
for env_name in FORGE_ENV: value = os.environ.get(env_name) - if not value: continue + if not value: + continue os.environ[env_name.replace("FORGE", "FORGE")] = value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/ci_entrypoint/run_ci.py` at line 38, The single-line conditional "if not value: continue" should be split into two lines to satisfy Ruff E701; replace it with a standard block form using the same indentation: "if not value:" on one line and "continue" on the next (indented once), preserving surrounding loop/context and behavior for the variable `value`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/core/ci_entrypoint/run_ci.py`:
- Around line 36-39: The loop in run_ci.py that iterates FORGE_ENV (for env_name
in FORGE_ENV) is dead code because env_name.replace("FORGE", "FORGE") is a
no-op; remove the entire loop block (the for env_name in FORGE_ENV: ... lines)
from run_ci.py to eliminate the redundant environment variable copy, ensuring no
other code depends on that copy behavior and updating any tests or callers if
they expect legacy TOPSAIL->FORGE mapping.
In `@projects/core/notifications/__init__.py`:
- Line 1: Update the module docstring in projects/core/notifications/__init__.py
by correcting the typo: change "FORGE Notifications moduled" to "FORGE
Notifications module" (i.e., fix the word in the module-level comment/docstring
in __init__.py).
In `@projects/jump_ci/toolbox/jump_ci_prepare_step/templates/entrypoint.sh.j2`:
- Line 9: The cleanup in prepare_jump_ci.py is still removing containers named
"topsail-on-{cluster}" causing orphaned forge containers; update the two cleanup
calls that reference the old name to use the new pattern
"forge-on-{jump_ci_prepare_step_cluster}" (or the variable used for cluster in
that module) so they match entrypoint.sh.j2; locate the removal calls in
prepare_jump_ci.py (the two places that build the container name) and replace
the "topsail-on-..." literal with "forge-on-..." using the same cluster variable
interpolation the rest of the template uses.
In `@projects/legacy/library/env.py`:
- Around line 56-57: The env variable usage is inconsistent:
env_topsail_base_dir is built from TOPSAIL_BASE_DIR but the artifact uses a
"forge_" prefix; update this to use FORGE_BASE_DIR for consistency (or
optionally support both). Change the lookup to prefer FORGE_BASE_DIR
(os.environ.get("FORGE_BASE_DIR")) and fall back to TOPSAIL_BASE_DIR if unset,
keep the artifact_dir creation using the forge_ prefix, and update the variable
name from env_topsail_base_dir to something like env_forge_base_dir (or similar)
so identifiers match the FORGE naming; ensure no other code relies on only
TOPSAIL_BASE_DIR without the fallback.
In `@pyproject.toml`:
- Around line 6-8: Update the hardcoded repository name "topsail-ng" to the new
package name "forge" wherever it's used as the repo_name parameter (e.g., the
repo_name variable/argument in jump_ci.py and prepare_jump_ci.py), and also
update the Docker image label string in the Ansible task (the docker image label
constant/value in jump_ci_prepare_topsail/tasks/main.yml) to use "forge" (or the
appropriate new image tag) so all references are consistent; ensure only the
string literal is changed and run a quick config/test to confirm no other
dependent names need adjustment.
---
Outside diff comments:
In `@projects/core/dsl/runtime.py`:
- Around line 247-265: The loop that looks for the repository root checks for a
parent directory named 'forge' (using filename_path.parents and parent.name) but
the repo directory is actually named 'topsail-ng', so the check always fails;
update the condition (the parent.name == 'forge' check inside the for parent in
filename_path.parents loop) to match the real directory name (e.g.,
'topsail-ng') or refactor to use a constant/setting (e.g., FORGE_HOME_NAME) and
then compute forge_home, rel_path via filename_path.relative_to(forge_home) as
before so the function returns the correct relative path instead of falling
back.
In `@projects/jump_ci/toolbox/jump_ci_prepare_topsail/tasks/main.yml`:
- Around line 136-138: The image-cleanup command still uses the old filter
string "--filter label=name=topsail-ng", so it won't match new images labeled
with "--label owner=forge"; update the filter to "--filter label=owner=forge" in
the pipeline that pipes to jq (the line containing the jq selection '.[] |
select(.Containers == 0) | select(.Labels.preserve != "true") | .Id') —
optionally include both filters (owner=forge and name=topsail-ng) if you want to
retain cleanup of legacy images as well.
---
Nitpick comments:
In `@projects/core/ci_entrypoint/run_ci.py`:
- Line 38: The single-line conditional "if not value: continue" should be split
into two lines to satisfy Ruff E701; replace it with a standard block form using
the same indentation: "if not value:" on one line and "continue" on the next
(indented once), preserving surrounding loop/context and behavior for the
variable `value`.
In `@projects/core/dsl/__init__.py`:
- Line 9: The __all__ list is unsorted; update the module-level symbol list
named __all__ to be alphabetically ordered so static analyzers and import
conventions are satisfied — reorder the entries currently in __all__ ('task',
'retry', 'when', 'always', 'execute_tasks', 'clear_tasks', 'shell') into
alphabetical order (e.g., 'always', 'clear_tasks', 'execute_tasks', 'retry',
'shell', 'task', 'when') while preserving the same variable name and semantics.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13c9a4e8-cdb1-41ad-9da4-284a90041bbc
📒 Files selected for processing (20)
.github/steps/prepare_minimal_ubuntu_image/action.yml.github/workflows/check_consistency.yml.gitignore.specify/memory/constitution.mdprojects/core/ci_entrypoint/github/pr_args.pyprojects/core/ci_entrypoint/prepare_ci.pyprojects/core/ci_entrypoint/run_ci.pyprojects/core/dsl/__init__.pyprojects/core/dsl/log.pyprojects/core/dsl/runtime.pyprojects/core/image/Containerfileprojects/core/image/Containerfile.lightweightprojects/core/notifications/__init__.pyprojects/core/notifications/send.pyprojects/jump_ci/testing/config.yamlprojects/jump_ci/toolbox/jump_ci_prepare_step/templates/entrypoint.sh.j2projects/jump_ci/toolbox/jump_ci_prepare_topsail/tasks/main.ymlprojects/legacy/library/env.pyprojects/legacy/library/run.pypyproject.toml
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
/test ? |
Contributor
Author
|
/test llmd-jump-ci llm_d psap_h200 |
|
@kpouget: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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)
projects/core/dsl/runtime.py (1)
246-265:⚠️ Potential issue | 🟠 MajorInconsistent
_get_forge_relative_pathimplementations cause path format mismatches.This function exists in both
log.py(line 85) andruntime.py(line 246) with incompatible implementations:
Module For /home/user/forge/projects/foo.pyReturns log.py Uses config.FORGE_HOMEprojects/foo.pyruntime.py Searches for 'forge' dir in path forge/projects/foo.pyThe inconsistency causes problems:
metadata.yamlwill containforge/...paths (from runtime.py)log_completion_bannerlogs paths likeprojects/...(from log.py)restart.shscript (line 211) will havepython "forge/..."which only works when run from the parent directory, not from forge rootAdditional issues:
- Hardcoded directory name
'forge'(line 250) fails if repo is cloned to a different directory name- Comment on line 254 claims "so it shows ../forge/..." but
relative_to()doesn't produce../prefixes- Function is duplicated across two modules instead of consolidated
Use
config.FORGE_HOMEconsistently, or consolidate into a shared utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/dsl/runtime.py` around lines 246 - 265, Replace the duplicated, hardcoded implementation of _get_forge_relative_path in runtime.py with a single shared utility that uses config.FORGE_HOME (rather than searching for a literal 'forge' directory) so both log.py and runtime.py produce the same relative path format; update _get_forge_relative_path to compute os.path.relpath(filename, start=config.FORGE_HOME) (falling back to os.path.relpath(filename) if FORGE_HOME is unset) and remove the incorrect comment about "../forge/..." and the hardcoded 'forge' string, then have both modules import the shared function to eliminate duplication and ensure restart.sh and metadata.yaml receive consistent paths.
♻️ Duplicate comments (1)
projects/core/ci_entrypoint/run_ci.py (1)
30-39:⚠️ Potential issue | 🟡 MinorThis environment variable remapping block is now dead code.
The
env_name.replace("FORGE", "FORGE")on line 39 is a no-op—it replaces "FORGE" with itself. This loop reads eachFORGE_*variable fromos.environand writes it back to the same key, achieving nothing.This was likely
replace("TOPSAIL", "FORGE")before the rename to provide backwards compatibility. Now that the rename is complete, this entire block should be removed.Additionally, line 38 violates the style rule against multiple statements on one line (
if not value: continue).🔧 Suggested fix: remove dead code
-FORGE_ENV = [ - "FORGE_LIGHT_IMAGE", "FORGE_JUMP_CI_INSIDE_JUMP_HOST", "FORGE_HOME", - "FORGE_OPENSHIFT_CI_STEP_DIR", - "PSAP_FORGE_JUMP_CI_SECRET_PATH" -] - -for env_name in FORGE_ENV: - value = os.environ.get(env_name) - if not value: continue - os.environ[env_name.replace("FORGE", "FORGE")] = value - - FORGE_HOME = Path(__file__).resolve().parent.parent.parent.parent,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/ci_entrypoint/run_ci.py` around lines 30 - 39, Remove the dead environment remapping loop that iterates over FORGE_ENV and reassigns values to the same keys (the env_name.replace("FORGE", "FORGE") no-op) — delete the FORGE_ENV definition and the for env_name in FORGE_ENV: block (including the if not value: continue single-line statement) from run_ci.py; if any backwards-compat handling is still required, implement a clear mapping with correct replace() targets or explicit keys, otherwise eliminate this dead code and ensure no multi-statement lines remain.
🧹 Nitpick comments (2)
projects/core/image/Containerfile (1)
46-47: Consider centralizing app path/user constants to reduce future rename churn.
/app/forgeandforgeare repeated in multiple instructions. Defining them once (e.g., viaARG/ENV) would lower maintenance risk for future rebrands.♻️ Example refactor
+ARG APP_NAME=forge +ARG APP_HOME=/app/forge + -WORKDIR /app/forge -ENV HOME=/app/forge +WORKDIR ${APP_HOME} +ENV HOME=${APP_HOME} ... -RUN groupadd -g 1001 forge && \ - useradd -u 1001 -g forge -s /bin/bash -d /app/forge -M forge && \ - chown -R forge:forge /app/forge +RUN groupadd -g 1001 ${APP_NAME} && \ + useradd -u 1001 -g ${APP_NAME} -s /bin/bash -d ${APP_HOME} -M ${APP_NAME} && \ + chown -R ${APP_NAME}:${APP_NAME} ${APP_HOME} ... -ENV PYTHONPATH=/app/forge \ +ENV PYTHONPATH=${APP_HOME} \ PYTHONUNBUFFERED=1 \ - FORGE_HOME=/app/forge + FORGE_HOME=${APP_HOME}Also applies to: 60-62, 72-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/image/Containerfile` around lines 46 - 47, The Dockerfile repeats the application path and user ("WORKDIR /app/forge" and "ENV HOME=/app/forge" plus other blocks using "/app/forge" and "forge"); introduce centralized build-time and/or env variables (e.g., ARG APP_DIR and ARG APP_USER then ENV APP_DIR and HOME referencing APP_DIR) at the top of the Containerfile and replace all literal occurrences used in WORKDIR, ENV HOME, USER, COPY, and any RUN steps (references: WORKDIR, ENV HOME, USER, and the COPY/RUN instructions that currently use "/app/forge" or "forge") so future renames only require updating the ARG/ENV values once.projects/core/dsl/__init__.py (1)
9-9: Optional:__all__is not sorted alphabetically.Static analysis (RUF022) flags that
__all__is not sorted. This is a minor style nit that can be addressed if you want consistency with isort conventions.♻️ Proposed fix
-__all__ = ['task', 'retry', 'when', 'always', 'execute_tasks', 'clear_tasks', 'shell'] +__all__ = ['always', 'clear_tasks', 'execute_tasks', 'retry', 'shell', 'task', 'when']🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/dsl/__init__.py` at line 9, The __all__ list in projects/core/dsl/__init__.py is not alphabetically sorted, which triggers static analysis (RUF022); reorder the entries in the __all__ variable (the list containing 'task', 'retry', 'when', 'always', 'execute_tasks', 'clear_tasks', 'shell') into alphabetical order so the names are sorted consistently (e.g., 'always', 'clear_tasks', 'execute_tasks', 'retry', 'shell', 'task', 'when').
🤖 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 `@projects/core/dsl/runtime.py`:
- Around line 246-265: Replace the duplicated, hardcoded implementation of
_get_forge_relative_path in runtime.py with a single shared utility that uses
config.FORGE_HOME (rather than searching for a literal 'forge' directory) so
both log.py and runtime.py produce the same relative path format; update
_get_forge_relative_path to compute os.path.relpath(filename,
start=config.FORGE_HOME) (falling back to os.path.relpath(filename) if
FORGE_HOME is unset) and remove the incorrect comment about "../forge/..." and
the hardcoded 'forge' string, then have both modules import the shared function
to eliminate duplication and ensure restart.sh and metadata.yaml receive
consistent paths.
---
Duplicate comments:
In `@projects/core/ci_entrypoint/run_ci.py`:
- Around line 30-39: Remove the dead environment remapping loop that iterates
over FORGE_ENV and reassigns values to the same keys (the
env_name.replace("FORGE", "FORGE") no-op) — delete the FORGE_ENV definition and
the for env_name in FORGE_ENV: block (including the if not value: continue
single-line statement) from run_ci.py; if any backwards-compat handling is still
required, implement a clear mapping with correct replace() targets or explicit
keys, otherwise eliminate this dead code and ensure no multi-statement lines
remain.
---
Nitpick comments:
In `@projects/core/dsl/__init__.py`:
- Line 9: The __all__ list in projects/core/dsl/__init__.py is not
alphabetically sorted, which triggers static analysis (RUF022); reorder the
entries in the __all__ variable (the list containing 'task', 'retry', 'when',
'always', 'execute_tasks', 'clear_tasks', 'shell') into alphabetical order so
the names are sorted consistently (e.g., 'always', 'clear_tasks',
'execute_tasks', 'retry', 'shell', 'task', 'when').
In `@projects/core/image/Containerfile`:
- Around line 46-47: The Dockerfile repeats the application path and user
("WORKDIR /app/forge" and "ENV HOME=/app/forge" plus other blocks using
"/app/forge" and "forge"); introduce centralized build-time and/or env variables
(e.g., ARG APP_DIR and ARG APP_USER then ENV APP_DIR and HOME referencing
APP_DIR) at the top of the Containerfile and replace all literal occurrences
used in WORKDIR, ENV HOME, USER, COPY, and any RUN steps (references: WORKDIR,
ENV HOME, USER, and the COPY/RUN instructions that currently use "/app/forge" or
"forge") so future renames only require updating the ARG/ENV values once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18e1b1b5-6e5e-49b0-8b43-03b13083dad8
📒 Files selected for processing (20)
.github/steps/prepare_minimal_ubuntu_image/action.yml.github/workflows/check_consistency.yml.gitignore.specify/memory/constitution.mdprojects/core/ci_entrypoint/github/pr_args.pyprojects/core/ci_entrypoint/prepare_ci.pyprojects/core/ci_entrypoint/run_ci.pyprojects/core/dsl/__init__.pyprojects/core/dsl/log.pyprojects/core/dsl/runtime.pyprojects/core/image/Containerfileprojects/core/image/Containerfile.lightweightprojects/core/notifications/__init__.pyprojects/core/notifications/send.pyprojects/jump_ci/testing/config.yamlprojects/jump_ci/toolbox/jump_ci_prepare_step/templates/entrypoint.sh.j2projects/jump_ci/toolbox/jump_ci_prepare_topsail/tasks/main.ymlprojects/legacy/library/env.pyprojects/legacy/library/run.pypyproject.toml
✅ Files skipped from review due to trivial changes (10)
- .gitignore
- projects/core/dsl/log.py
- projects/core/notifications/init.py
- .specify/memory/constitution.md
- .github/workflows/check_consistency.yml
- .github/steps/prepare_minimal_ubuntu_image/action.yml
- projects/jump_ci/testing/config.yaml
- projects/legacy/library/run.py
- pyproject.toml
- projects/jump_ci/toolbox/jump_ci_prepare_step/templates/entrypoint.sh.j2
🚧 Files skipped from review as they are similar to previous changes (6)
- projects/core/ci_entrypoint/prepare_ci.py
- projects/legacy/library/env.py
- projects/core/ci_entrypoint/github/pr_args.py
- projects/jump_ci/toolbox/jump_ci_prepare_topsail/tasks/main.yml
- projects/core/notifications/send.py
- projects/core/image/Containerfile.lightweight
Contributor
Author
the |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
projects/core/ci_entrypoint/run_ci.py (1)
30-39:⚠️ Potential issue | 🟡 MinorRemove the no-op env remap block.
This block copies each env var to itself (
replace("FORGE", "FORGE")), so it has no effect, and Line 38 also triggers Ruff E701. Please delete the block entirely.🔧 Proposed fix
-FORGE_ENV = [ - "FORGE_LIGHT_IMAGE", "FORGE_JUMP_CI_INSIDE_JUMP_HOST", "FORGE_HOME", - "FORGE_OPENSHIFT_CI_STEP_DIR", - "PSAP_FORGE_JUMP_CI_SECRET_PATH" -] - -for env_name in FORGE_ENV: - value = os.environ.get(env_name) - if not value: continue - os.environ[env_name.replace("FORGE", "FORGE")] = value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/ci_entrypoint/run_ci.py` around lines 30 - 39, Remove the entire no-op environment remapping block in run_ci.py: delete the FORGE_ENV list and the for env_name in FORGE_ENV: loop (including the os.environ.get(...) check and the os.environ[env_name.replace("FORGE", "FORGE")] = value line) since it does nothing and triggers Ruff E701; ensure no other code depends on FORGE_ENV or that loop before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/core/image/Containerfile.lightweight`:
- Line 29: The WORKDIR is hardcoded to /app/forge causing divergence when
APP_HOME is overridden; update the WORKDIR instruction to use the APP_HOME
variable (e.g., WORKDIR ${APP_HOME}/forge or WORKDIR $APP_HOME/forge) so runtime
paths follow the build-time APP_HOME value and remain consistent with the
existing APP_HOME parameterization referenced elsewhere in the Containerfile
(APP_HOME).
In `@projects/llm_d/orchestration/cli.py`:
- Around line 51-54: post_cleanup currently calls prepare_llmd.cleanup() and
passes its return directly to sys.exit, but prepare_llmd.cleanup() can return
None causing sys.exit(0) and masking failures; update prepare_llmd.cleanup() to
always return an explicit integer status (0 for success, non-zero for failure)
and modify post_cleanup to validate the returned value (e.g., if exit_code is
None or not an int, set a non-zero default such as 1) before calling
sys.exit(exit_code) so cleanup failures are not treated as success; refer to the
post_cleanup function in cli.py and the cleanup function in prepare_llmd.py when
making these changes.
---
Duplicate comments:
In `@projects/core/ci_entrypoint/run_ci.py`:
- Around line 30-39: Remove the entire no-op environment remapping block in
run_ci.py: delete the FORGE_ENV list and the for env_name in FORGE_ENV: loop
(including the os.environ.get(...) check and the
os.environ[env_name.replace("FORGE", "FORGE")] = value line) since it does
nothing and triggers Ruff E701; ensure no other code depends on FORGE_ENV or
that loop before committing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c392422-6963-4cb4-97e7-dc2b40d919a9
📒 Files selected for processing (21)
.github/steps/prepare_minimal_ubuntu_image/action.yml.github/workflows/check_consistency.yml.gitignore.specify/memory/constitution.mdprojects/core/ci_entrypoint/github/pr_args.pyprojects/core/ci_entrypoint/prepare_ci.pyprojects/core/ci_entrypoint/run_ci.pyprojects/core/dsl/__init__.pyprojects/core/dsl/log.pyprojects/core/dsl/runtime.pyprojects/core/image/Containerfileprojects/core/image/Containerfile.lightweightprojects/core/notifications/__init__.pyprojects/core/notifications/send.pyprojects/jump_ci/testing/config.yamlprojects/jump_ci/toolbox/jump_ci_prepare_step/templates/entrypoint.sh.j2projects/jump_ci/toolbox/jump_ci_prepare_topsail/tasks/main.ymlprojects/legacy/library/env.pyprojects/legacy/library/run.pyprojects/llm_d/orchestration/cli.pypyproject.toml
✅ Files skipped from review due to trivial changes (10)
- .github/steps/prepare_minimal_ubuntu_image/action.yml
- .gitignore
- projects/core/notifications/init.py
- projects/core/dsl/log.py
- projects/core/dsl/runtime.py
- .github/workflows/check_consistency.yml
- projects/core/dsl/init.py
- projects/core/ci_entrypoint/github/pr_args.py
- projects/legacy/library/env.py
- .specify/memory/constitution.md
🚧 Files skipped from review as they are similar to previous changes (7)
- projects/jump_ci/toolbox/jump_ci_prepare_step/templates/entrypoint.sh.j2
- projects/jump_ci/testing/config.yaml
- projects/core/ci_entrypoint/prepare_ci.py
- projects/core/notifications/send.py
- pyproject.toml
- projects/jump_ci/toolbox/jump_ci_prepare_topsail/tasks/main.yml
- projects/core/image/Containerfile
| ENV PYTHONPATH=$HOME | ||
|
|
||
| WORKDIR /app/topsail | ||
| WORKDIR /app/forge |
There was a problem hiding this comment.
Make WORKDIR use APP_HOME to keep the new parameterization functional.
Line 29 hardcodes /app/forge while Lines 18-25/42-44 make home path configurable via APP_HOME. If APP_HOME is overridden at build time, runtime paths diverge and later steps may break.
🔧 Proposed fix
-WORKDIR /app/forge
+WORKDIR ${APP_HOME}📝 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
| WORKDIR /app/forge | |
| WORKDIR ${APP_HOME} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/core/image/Containerfile.lightweight` at line 29, The WORKDIR is
hardcoded to /app/forge causing divergence when APP_HOME is overridden; update
the WORKDIR instruction to use the APP_HOME variable (e.g., WORKDIR
${APP_HOME}/forge or WORKDIR $APP_HOME/forge) so runtime paths follow the
build-time APP_HOME value and remain consistent with the existing APP_HOME
parameterization referenced elsewhere in the Containerfile (APP_HOME).
Comment on lines
+51
to
+54
| def post_cleanup(ctx): | ||
| """Cleanup phase - Clean up resources and finalize.""" | ||
| exit_code = prepare_llmd.cleanup() | ||
| sys.exit(exit_code) |
There was a problem hiding this comment.
Non-zero cleanup failures can be masked as success
At Line 53-54, this command exits with prepare_llmd.cleanup()’s return value. In projects/llm_d/orchestration/prepare_llmd.py (Line 13-15), cleanup() currently returns None, so sys.exit(None) exits with code 0. That can incorrectly mark failed cleanup as success in CI.
Proposed guard in this command
def post_cleanup(ctx):
"""Cleanup phase - Clean up resources and finalize."""
exit_code = prepare_llmd.cleanup()
- sys.exit(exit_code)
+ if exit_code is None:
+ logger.error("prepare_llmd.cleanup() must return an integer exit code.")
+ sys.exit(1)
+ sys.exit(exit_code)Also update prepare_llmd.cleanup() to explicitly return 0/non-zero.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/llm_d/orchestration/cli.py` around lines 51 - 54, post_cleanup
currently calls prepare_llmd.cleanup() and passes its return directly to
sys.exit, but prepare_llmd.cleanup() can return None causing sys.exit(0) and
masking failures; update prepare_llmd.cleanup() to always return an explicit
integer status (0 for success, non-zero for failure) and modify post_cleanup to
validate the returned value (e.g., if exit_code is None or not an int, set a
non-zero default such as 1) before calling sys.exit(exit_code) so cleanup
failures are not treated as success; refer to the post_cleanup function in
cli.py and the cleanup function in prepare_llmd.py when making these changes.
This was referenced Apr 3, 2026
Merged
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
Refactor
New Features
Documentation
Chores