[foreign_testing] New project to support foreign testing in FORGE#24
Merged
[foreign_testing] New project to support foreign testing in FORGE#24
Conversation
|
[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 |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new foreign_testing project (CLI, orchestration, config, README) that clones external repos and maps project dirs into FORGE_HOME, refactors core CI startup/prepare behavior, and tightens GitHub notification token handling with early-exit on failure. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "foreign_testing CLI"
participant Orch as "foreign_testing module"
participant Git as "Git (clone/fetch)"
participant FS as "Filesystem (FORGE_HOME)"
participant RunCI as "run_ci.execute_project_operation"
User->>CLI: run `ci submit`
CLI->>Orch: init() / prepare()
Orch->>Git: git clone <owner>/<repo>
Orch->>Git: fetch <PR_SHA> && reset FETCH_HEAD
Git-->>Orch: repo at PR_SHA
Orch->>FS: copy mapped dirs -> ${FORGE_HOME}/projects/<name>
Orch->>RunCI: execute_project_operation("fournos_deploy","ci",["deploy"],do_prepare_ci=False,verbose=True)
RunCI-->>CLI: result / exit code
CLI-->>User: exit code
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 |
kpouget
force-pushed
the
fournos-testing
branch
from
455bfe3 to
e2526ef
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/foreign_testing/orchestration/run.py`:
- Around line 5-12: The file imports projects.core.run as run but then defines a
local function run(), shadowing the module and causing run.init() to fail;
rename the imported module on import (e.g., import as core_run or
core_run_module) and update the call sites (change run.init() to
core_run.init()) so the env.init(), core_run.init(), and
config.init(pathlib.Path(__file__).parent) calls reference the module rather
than the newly defined run() function.
In `@projects/foreign_testing/README.md`:
- Around line 1-42: Update the README examples so they reference the correct
project name and CLI commands: replace occurrences of "skeleton" with
"foreign_testing" and change the example subcommands (currently
"prepare/test/cleanup") to match the Click commands exported by the Click group
in projects/foreign_testing/orchestration/ci.py (inspect the click.Group or
`@click.command` names in that file and use those exact tokens in the Quick Start
shells and any repeated examples on lines ~266-289).
🪄 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: ec95ce86-7188-4b24-b6c0-64426d3f67c4
📒 Files selected for processing (4)
projects/foreign_testing/README.mdprojects/foreign_testing/orchestration/ci.pyprojects/foreign_testing/orchestration/run.pyprojects/foreign_testing/orchestration/test_skeleton.py
kpouget
force-pushed
the
fournos-testing
branch
5 times, most recently
from
d11d513 to
427ccc9
Compare
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)
projects/core/notifications/github/api.py (1)
25-36:⚠️ Potential issue | 🟠 MajorMove JSON parsing inside a try/except block to handle non-JSON responses gracefully.
Line 27 calls
installation_resp.json()before any error handling. If GitHub returns a non-JSON response (e.g., 404, 500, or rate-limit error with HTML), this raises an uncaught exception instead of returningNoneas the caller expects. The caller atsend.py:94checksif not user_token:to handle failures gracefully, but the unprotected JSON parse breaks that contract. Parse the response once inside a try/except that catches JSON decode errors, returnNoneon failure, and log the HTTP status and body for debugging.🛠️ Proposed fix
- if "id" not in installation_resp.json(): - logging.error("NO ID") - return None - - try: - installation_id = installation_resp.json()["id"] - except Exception as e: - msg = f"Failed to get the user token: {e}" - logging.error(msg) - raise e + try: + installation_data = installation_resp.json() + except ValueError: + logging.error( + "Failed to decode installation response (%s): %s", + installation_resp.status_code, + installation_resp.text, + ) + return None + + installation_id = installation_data.get("id") + if not installation_id: + logging.error( + "Failed to fetch GitHub installation id (%s): %s", + installation_resp.status_code, + installation_resp.text, + ) + return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/notifications/github/api.py` around lines 25 - 36, The code currently calls installation_resp.json() twice outside error handling; move the JSON parsing into a single try/except that catches JSON decode errors (e.g., ValueError/JSONDecodeError), assign the parsed body to a local variable (e.g., data), and then check for "id" in that variable; on JSON parse failure or missing "id" log the response status_code and response.text for debugging and return None so callers (like send.py) get the expected None on failure; replace references to installation_resp.json() with the parsed local variable and rethrow only for unexpected exceptions if needed.
♻️ Duplicate comments (3)
projects/foreign_testing/README.md (3)
13-13:⚠️ Potential issue | 🟡 MinorHyphenate “AI-focused” for correct grammar.
Suggested fix
-- **AI Platform Specificity**: OpenShift AI focused testing patterns +- **AI Platform Specificity**: OpenShift AI-focused testing patterns🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/foreign_testing/README.md` at line 13, Update the README line containing the phrase "AI Platform Specificity: OpenShift AI focused testing patterns" to use the grammatically correct hyphenated form "AI-focused" (i.e., change "AI focused" to "AI-focused") so the line reads "AI Platform Specificity: OpenShift AI-focused testing patterns"; locate the string in the README and make that single-word hyphenation change.
1-3:⚠️ Potential issue | 🟠 MajorREADME usage is out of sync with the actual
foreign_testingCLI.The guide still documents
skeletonandprepare/test/cleanup, but this project’s CLI wiring isforeign_testingwith theruncommand. Following current examples will fail.Suggested doc updates
-# Skeleton Project +# foreign_testing Project -./run_ci skeleton ci prepare -./run_ci skeleton ci test -./run_ci skeleton ci cleanup +./run_ci foreign_testing ci run -./run_ci skeleton ci --verbose test +./run_ci foreign_testing ci --verbose run -./run_ci skeleton ci --help +./run_ci foreign_testing ci --help -Each project should implement these standard phases: -- **prepare** ... -- **test** ... -- **cleanup** ... +This project currently exposes a single `run` command via `orchestration/ci.py`. -./run_ci skeleton ci prepare -./run_ci skeleton ci test -./run_ci skeleton ci cleanup +./run_ci foreign_testing ci runAlso applies to: 17-42, 54-61, 149-157, 266-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/foreign_testing/README.md` around lines 1 - 3, Update the README to match the actual CLI and commands: replace references to the old `skeleton` project and the `prepare`/`test`/`cleanup` commands with the real CLI name `foreign_testing` and its `run` command, and update any example usages, command snippets, and docs in the referenced sections (around the top, mid, and later blocks) so examples, flags, and workflows reflect `foreign_testing run` and its current options/arguments.
17-25:⚠️ Potential issue | 🟡 MinorAdd a language tag to the directory-tree fenced block.
Use
textfor the tree block to satisfy markdown linting.Suggested fix
-``` +```text skeleton/ ├── orchestration/ │ └── ci.py # Main CI script with Click-based CLI ... -``` +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/foreign_testing/README.md` around lines 17 - 25, The fenced directory-tree block containing "skeleton/" (showing entries like "orchestration/ci.py" and "README.md") should include a language tag to satisfy markdown linting: change the opening fence from ``` to ```text so the block becomes a ```text fenced block while keeping the same contents and closing fence.
🤖 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/notifications/send.py`:
- Line 42: The line with logging.info in projects/core/notifications/send.py has
an unterminated string literal; update the logging.info call (the
logging.info(...) statement in send.py) to use a properly quoted string and
balanced parentheses (e.g., add the missing closing quote and parenthesis) so
the module can be parsed and imports will succeed.
---
Outside diff comments:
In `@projects/core/notifications/github/api.py`:
- Around line 25-36: The code currently calls installation_resp.json() twice
outside error handling; move the JSON parsing into a single try/except that
catches JSON decode errors (e.g., ValueError/JSONDecodeError), assign the parsed
body to a local variable (e.g., data), and then check for "id" in that variable;
on JSON parse failure or missing "id" log the response status_code and
response.text for debugging and return None so callers (like send.py) get the
expected None on failure; replace references to installation_resp.json() with
the parsed local variable and rethrow only for unexpected exceptions if needed.
---
Duplicate comments:
In `@projects/foreign_testing/README.md`:
- Line 13: Update the README line containing the phrase "AI Platform
Specificity: OpenShift AI focused testing patterns" to use the grammatically
correct hyphenated form "AI-focused" (i.e., change "AI focused" to "AI-focused")
so the line reads "AI Platform Specificity: OpenShift AI-focused testing
patterns"; locate the string in the README and make that single-word hyphenation
change.
- Around line 1-3: Update the README to match the actual CLI and commands:
replace references to the old `skeleton` project and the
`prepare`/`test`/`cleanup` commands with the real CLI name `foreign_testing` and
its `run` command, and update any example usages, command snippets, and docs in
the referenced sections (around the top, mid, and later blocks) so examples,
flags, and workflows reflect `foreign_testing run` and its current
options/arguments.
- Around line 17-25: The fenced directory-tree block containing "skeleton/"
(showing entries like "orchestration/ci.py" and "README.md") should include a
language tag to satisfy markdown linting: change the opening fence from ``` to
```text so the block becomes a ```text fenced block while keeping the same
contents and closing fence.
🪄 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: eeeda460-286e-40c6-9761-a4c075599c20
📒 Files selected for processing (6)
projects/core/notifications/github/api.pyprojects/core/notifications/send.pyprojects/foreign_testing/README.mdprojects/foreign_testing/orchestration/ci.pyprojects/foreign_testing/orchestration/config.yamlprojects/foreign_testing/orchestration/foreign_testing.py
✅ Files skipped from review due to trivial changes (1)
- projects/foreign_testing/orchestration/config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- projects/foreign_testing/orchestration/ci.py
kpouget
force-pushed
the
fournos-testing
branch
12 times, most recently
from
12fb768 to
2f109f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/llm_d/orchestration/cli.py (1)
27-31:⚠️ Potential issue | 🟠 MajorReplace
sys.exit(exit_code)withreturn exit_codein all four command handlers.The
safe_cli_commanddecorator expects command functions to return an exit code, which it then handles viasys.exit(). Callingsys.exit()directly inside the command bypasses the decorator's wrapper entirely, preventing it from owning the exit path and applying consistent error handling.Update
prepare(lines 27-31),test(36-40),pre_cleanup(45-49), andpost_cleanup(54-58) to return the exit code instead of callingsys.exit()directly.🤖 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 27 - 31, The four CLI handlers decorated with safe_cli_command (prepare, test, pre_cleanup, post_cleanup) currently call sys.exit(exit_code) directly; change each to return exit_code instead so the safe_cli_command wrapper can manage exiting and consistent error handling—locate the functions prepare, test, pre_cleanup, and post_cleanup and replace their sys.exit(...) calls with return exit_code.
♻️ Duplicate comments (1)
projects/foreign_testing/README.md (1)
1-42:⚠️ Potential issue | 🟡 MinorREADME still documents the skeleton project and the wrong CLI.
This file lives under
projects/foreign_testing/, but the guide still tells users to runskeletonwithprepare/test/cleanup. The actual entrypoint inprojects/foreign_testing/orchestration/ci.pyexposessubmit, so anyone following these examples will invoke the wrong project and non-existent commands.Also applies to: 54-61, 149-157, 266-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/foreign_testing/README.md` around lines 1 - 42, The README references the wrong project name and CLI commands—update all examples that mention running "./run_ci skeleton ci prepare/test/cleanup" to use the actual project entrypoint and command exposed by the orchestration CLI (replace "skeleton" with "foreign_testing" and the "ci prepare/test/cleanup" sequence with the "submit" command as implemented in orchestration/ci.py); ensure every occurrence (including the quick-start examples and the other noted blocks) consistently documents "./run_ci foreign_testing submit" (and any required args) so docs match the actual CLI symbol `submit`.
🧹 Nitpick comments (1)
projects/core/ci_entrypoint/run_ci.py (1)
204-225: Remove the duplicatefind_ci_script()definition.This second copy overwrites the earlier
find_ci_script()on Lines 180-202. They’re identical now, but keeping both makes future edits easy to desynchronize and hides which implementation is actually live.🤖 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 204 - 225, There are two identical definitions of find_ci_script() in the file which causes the later one to overwrite the earlier; remove the duplicate function block so only a single canonical find_ci_script() remains, keeping the original implementation (the function that checks project_dir / "orchestration" / f"{operation}.py" and tests exists + os.access(..., os.X_OK)) and ensure no other references rely on the removed duplicate; after deletion, run tests/lint to confirm no unresolved references.
🤖 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/github/pr_args.py`:
- Around line 72-75: The code is masking a missing project by setting
project_name = "project_not_set"; instead, when parts is empty set project_name
to None (or omit the field) or raise a dedicated validation error where
project-less directives are not allowed so callers can distinguish "absent" from
a real literal name. Update the branch that currently reads parts and assigns
project_name (the parts variable and project_name identifier in pr_args.py) to
either assign None when parts is falsy or raise a clear ValidationError (or
similar) at parse time, and ensure downstream code that expects a required
project (e.g., jump-ci handling) checks for None/raises rather than relying on
the sentinel string.
In `@projects/foreign_testing/orchestration/ci.py`:
- Around line 2-8: Replace the generic skeleton/template module docstring at the
top of the file with a concise description of this module's purpose for the
foreign_testing project: update the module-level docstring in
projects/foreign_testing/orchestration/ci.py to state that it contains CI
orchestration helpers and entrypoint logic for the foreign_testing project
(mentioning "foreign_testing" by name and summarizing its role), so stack
traces, editor hovers, and maintenance reflect the correct project.
In `@projects/foreign_testing/orchestration/foreign_testing.py`:
- Around line 29-32: The current checkout uses a fixed tmp_dest (tmp_dest =
f"/tmp/{repo_name}") which causes collisions and stale clones; change the
checkout to create an isolated temp directory (use tempfile.mkdtemp() or
tempfile.TemporaryDirectory()) and assign that to tmp_dest, then use the same
run.run calls (git clone, git -C "{tmp_dest}" fetch, reset) against that
generated tmp_dest and ensure the temp directory is removed/cleaned up after use
(or use TemporaryDirectory as a context manager) so each run gets its own
workspace for repo_name and pull_pull_sha.
---
Outside diff comments:
In `@projects/llm_d/orchestration/cli.py`:
- Around line 27-31: The four CLI handlers decorated with safe_cli_command
(prepare, test, pre_cleanup, post_cleanup) currently call sys.exit(exit_code)
directly; change each to return exit_code instead so the safe_cli_command
wrapper can manage exiting and consistent error handling—locate the functions
prepare, test, pre_cleanup, and post_cleanup and replace their sys.exit(...)
calls with return exit_code.
---
Duplicate comments:
In `@projects/foreign_testing/README.md`:
- Around line 1-42: The README references the wrong project name and CLI
commands—update all examples that mention running "./run_ci skeleton ci
prepare/test/cleanup" to use the actual project entrypoint and command exposed
by the orchestration CLI (replace "skeleton" with "foreign_testing" and the "ci
prepare/test/cleanup" sequence with the "submit" command as implemented in
orchestration/ci.py); ensure every occurrence (including the quick-start
examples and the other noted blocks) consistently documents "./run_ci
foreign_testing submit" (and any required args) so docs match the actual CLI
symbol `submit`.
---
Nitpick comments:
In `@projects/core/ci_entrypoint/run_ci.py`:
- Around line 204-225: There are two identical definitions of find_ci_script()
in the file which causes the later one to overwrite the earlier; remove the
duplicate function block so only a single canonical find_ci_script() remains,
keeping the original implementation (the function that checks project_dir /
"orchestration" / f"{operation}.py" and tests exists + os.access(..., os.X_OK))
and ensure no other references rely on the removed duplicate; after deletion,
run tests/lint to confirm no unresolved references.
🪄 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: 42bf4de9-5304-40e4-a895-471b832d4718
📒 Files selected for processing (12)
projects/core/ci_entrypoint/github/pr_args.pyprojects/core/ci_entrypoint/run_ci.pyprojects/core/notifications/github/api.pyprojects/core/notifications/send.pyprojects/foreign_testing/README.mdprojects/foreign_testing/orchestration/ci.pyprojects/foreign_testing/orchestration/config.yamlprojects/foreign_testing/orchestration/foreign_testing.pyprojects/fournos_launcher/orchestration/ci.pyprojects/llm_d/orchestration/ci.pyprojects/llm_d/orchestration/cli.pyprojects/skeleton/orchestration/ci.py
✅ Files skipped from review due to trivial changes (3)
- projects/llm_d/orchestration/ci.py
- projects/skeleton/orchestration/ci.py
- projects/fournos_launcher/orchestration/ci.py
🚧 Files skipped from review as they are similar to previous changes (2)
- projects/core/notifications/github/api.py
- projects/foreign_testing/orchestration/config.yaml
kpouget
force-pushed
the
fournos-testing
branch
3 times, most recently
from
4b3c590 to
8316115
Compare
Contributor
Author
|
/test fournos llm_d cks |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 00 minutes 30 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos skeleton |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 37 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos skeleton |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 17 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
kpouget
force-pushed
the
fournos-testing
branch
from
8316115 to
652a460
Compare
Contributor
Author
|
/test fournos skeleton |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 28 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos skeleton |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 00 minutes 27 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
I merge this, the foreign testing work will continue on FOURNOS side |
This was referenced Apr 16, 2026
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
Documentation
New Features
Bug Fixes
/testdirective now tolerates a missing project token.Chores
Behavior