[Skeleton] Rework to be more exhaustive#26
Merged
kpouget merged 7 commits intoopenshift-psap:mainfrom Apr 14, 2026
Merged
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 |
📝 WalkthroughWalkthroughexecute_tasks() now returns the shared context with artifact_dir set; config reload now unlinks and recopies artifact config; vault init gains a strict validation flag; a new skeleton project is added with CLI, presets, orchestration, and a cluster-info toolbox integrating vault and oc calls. Changes
Sequence DiagramssequenceDiagram
participant User
participant CLI as Skeleton CLI
participant Config as Config System
participant Vault as Vault System
participant Presets as Preset Engine
User->>CLI: run ./cli.py test --presets=deep_testing
CLI->>Config: init(config, vault validation=false)
CLI->>Presets: apply preset "deep_testing"
Presets->>Config: merge preset into project config
CLI->>Vault: vault.init(vaults, strict=False)
CLI->>Config: config.get_config("skeleton")
CLI->>Vault: vault.get_vault_content_path(...)
Vault-->>CLI: file path or None
CLI->>User: logs, then invokes cluster_info toolbox if enabled
sequenceDiagram
participant Test as test_skeleton.test()
participant Toolbox as cluster_info.run()
participant Shell as oc shell
Test->>Toolbox: cluster_info(output_format="text")
Toolbox->>Shell: oc whoami -> write current_user.txt
Shell-->>Toolbox: stdout
Toolbox->>Shell: oc get nodes --output text -> write cluster_nodes.txt
Shell-->>Toolbox: stdout
Toolbox-->>Test: result with cluster_nodes_dest
Test->>Test: evaluate result -> return exit code
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 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 |
Contributor
Author
|
/test fournos skeleton collect_cluster_info deep_testing side_testing |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 28 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
projects/core/library/vault.py (1)
370-391:⚠️ Potential issue | 🟠 MajorUnhandled breaking change for
projects/fournos_launcher/orchestration/submit.py.The
strict=Truedefault will causevault.init(config.project.get_config("vaults"))insubmit.pyto perform strict validation, raisingRuntimeErroron validation failures. This differs from the previous behavior. Meanwhile,projects/skeleton/orchestration/test_skeleton.pyexplicitly passes thestrictparameter, indicating the maintainers are aware of controlling this behavior.Either update
submit.pyto explicitly passstrict=Falseif non-strict validation is required, or ensure the vaults configuration will pass strict validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/library/vault.py` around lines 370 - 391, The init signature defaulting strict=True changes behavior causing RuntimeError when existing callers relied on non-strict validation; update the caller that invokes vault.init(config.project.get_config("vaults")) to explicitly pass strict=False (i.e., call init(..., strict=False)) so it preserves previous non-strict behavior, or alternatively change the init default back to strict=False; locate the Vault init function and its use sites (references to init and _filter_and_validate_vaults / VaultManager) and apply the explicit strict=False at the submit call if you want minimal, localized change.
🧹 Nitpick comments (7)
projects/llm_d/orchestration/cli.py (1)
3-3: Align CLI wording across docstrings for consistency.Line 3 says “CLI Operations,” but Line 20 still says “CI Operations.” If this rename is intentional, update the
main()docstring too soclick --helptext stays consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/llm_d/orchestration/cli.py` at line 3, Docstring wording is inconsistent: the module header reads "CLI Operations" but the main() docstring still says "CI Operations", causing misleading help text; update the main() function's docstring (the docstring inside function main) to use "CLI Operations" (or the intended term) so click --help and other introspection show consistent wording across the module.projects/skeleton/orchestration/prepare_skeleton.py (3)
2-2: Remove unused import.The
osmodule is imported but not used anywhere in this file.♻️ Proposed fix
import logging -import os import pathlib🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/skeleton/orchestration/prepare_skeleton.py` at line 2, Remove the unused top-level import of the os module by deleting the "import os" statement (the unused symbol is the os import) from prepare_skeleton.py so there are no unused imports left in that module.
12-13: RedundantPath()wrapper and inconsistent logger usage.
config.project.config_pathis already aPathobject (seeConfig.__init__), so thepathlib.Path()wrapper is unnecessary.- Line 13 uses
logging.info()but the module definesloggerat line 6.♻️ Proposed fix
- config_file_content = pathlib.Path(config.project.config_path).read_text() - logging.info(f"Configuration file: {config_file_content}") + config_file_content = config.project.config_path.read_text() + logger.info(f"Configuration file: {config_file_content}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/skeleton/orchestration/prepare_skeleton.py` around lines 12 - 13, The code unnecessarily wraps an existing Path in pathlib.Path and uses the wrong logger; replace the Path wrapping by calling read_text() directly on config.project.config_path (e.g. set config_file_content = config.project.config_path.read_text()) and change the logging call to use the module-level logger (logger.info(...)) so the message is emitted through the configured logger; update the reference to the config_file_content variable accordingly.
26-31: Unnecessary exception handling incleanup().The try block contains only
logger.info()calls, which won't raise exceptions. The exception handler is unreachable dead code. Either remove the try/except structure or add actual cleanup logic that could fail.♻️ Proposed fix (if no real cleanup needed)
def cleanup(): logger.info("=== Skeleton Project Cleanup Phase ===") logger.info("Demonstrating basic cleanup operations") - - try: - logger.info("✅ Cleanup phase completed successfully") - return 0 - except Exception as e: - logger.error(f"❌ Error during cleanup phase: {e}") - return 1 + logger.info("✅ Cleanup phase completed successfully") + return 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/skeleton/orchestration/prepare_skeleton.py` around lines 26 - 31, The try/except around the final logger.info in the cleanup() code is dead — remove the unnecessary try/except block and simply call logger.info("✅ Cleanup phase completed successfully") followed by return 0, and delete the corresponding except that logs via logger.error; alternatively, if there is actual cleanup work to perform, move those operations into the cleanup() function before the logger.info and keep the try/except around the real cleanup steps so that logger.error(...) (and return 1) only run on genuine errors.projects/skeleton/orchestration/cli.py (1)
23-24: Unused context object setup.
ctx.ensure_object(types.SimpleNamespace)creates a context object, butctx.objis never accessed in any subcommand. Either remove this line and thetypesimport, or document the intended use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/skeleton/orchestration/cli.py` around lines 23 - 24, Remove the unused click context object setup: delete the call to ctx.ensure_object(types.SimpleNamespace) and the unused types import (or alternatively add documentation and actual usage where ctx.obj is read), ensuring no subcommand references ctx.obj; confirm that test_skeleton.init(strict_vault_validation=False) remains unchanged and that no runtime behavior depends on the removed context object.projects/core/library/vault.py (2)
370-370: Use explicitOptionaltype hint.Per PEP 484, using
Noneas default value requires an explicitOptionaltype. Static analysis flagged this as RUF013.♻️ Proposed fix
-def init(vaults: List[str] = None, strict: bool = True): +def init(vaults: Optional[List[str]] = None, strict: bool = True):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/library/vault.py` at line 370, The function signature for init currently uses a None default without an Optional type; update the signature of init(vaults: List[str] = None, strict: bool = True) to use Optional[List[str]] for the vaults parameter (e.g., init(vaults: Optional[List[str]] = None, strict: bool = True)) and add the necessary Optional import from typing at the top of the module so static analysis no longer flags RUF013; keep the existing behavior of init and the strict parameter unchanged.
324-324: Docstring is misleading.The docstring says "Don't raise exception if the vault validation fails" but when
strict=True(the default), it does raise. Consider rewording to clarify the behavior:- strict: Don't raise exception if the vault validation fails + strict: If True, raise RuntimeError when validation fails; if False, log warning and return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/library/vault.py` at line 324, The function/method docstring for the routine that accepts the strict parameter is misleading: update the docstring for the method that defines strict (the function/method with parameter named strict) to accurately state that strict=True (the default) will raise an exception on vault validation failure, while strict=False will suppress the exception and return a failure indicator (or False); mention the default value and the exact behavior for both True and False so callers aren’t confused.
🤖 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/library/vault.py`:
- Line 360: The call uses the root logging API (logging.warning(msg)) instead of
the module-level logger instance; replace that call with the module logger
(logger.warning(msg)) so it is consistent with the rest of the module (the
logger variable defined as logger); ensure any message formatting stays the same
and no other calls in the file switch to the root logger.
In `@projects/skeleton/orchestration/cli.py`:
- Around line 39-45: The safe_cli_command decorator in
projects/core/library/cli.py currently manually assigns attributes and breaks
signature preservation needed by Click (e.g., for `@click.pass_context` used on
functions like prepare); update safe_cli_command to import functools and wrap
the inner wrapper with `@functools.wraps`(command_func) (preserving the existing
wrapper implementation and behavior) so inspect.signature() sees the original
function signature; do not change decorator order (keep `@click.pass_context`
closest to the target function).
In `@projects/skeleton/orchestration/test_skeleton.py`:
- Line 53: The user-facing warning string is a typo: update the logger.warning
call in test_skeleton.py (the logger.warning(...) invocation) to a clear,
grammatically correct message such as "⚠️ Gathering cluster information to
generate the cluster node file" so logs are readable and not confusing.
- Around line 45-49: The call to cluster_info() inside test() can raise
exceptions before the falsy check, so wrap the
cluster_info(output_format="text") invocation in a try/except that catches
Exception, logs the exception (including the exception object/traceback) and
returns 1; keep the existing falsy check and logger.warning path, and ensure
test() always returns an int exit code (return 1 on exception or falsy result,
return 0 on success).
In `@projects/skeleton/README.md`:
- Around line 35-49: Add a language tag (e.g., text) to the two fenced code
blocks in the README: the directory tree block (the block containing "skeleton/
├── orchestration/ ... toolbox/") and the artifacts block (the block starting
with "$ARTIFACT_DIR/artifacts/"). Edit the opening triple-backtick of each block
to include the language identifier so they read ```text rather than ```, leaving
the content inside unchanged; this will satisfy markdownlint MD040 and improve
rendering consistency.
In `@projects/skeleton/toolbox/cluster_info/main.py`:
- Around line 45-49: The setup_directories function currently creates a relative
"artifacts" folder in the CWD; change it to create the artifacts directory under
args.artifact_dir (i.e., args.artifact_dir / "artifacts") so it matches
subsequent writes; ensure you build the full path from args.artifact_dir, call
mkdir with parents=True/ exist_ok=True (or the equivalent shell.mkdir on that
absolute/joined path), and update the return message if desired so it reflects
the actual created path; locate this change in setup_directories and reference
args.artifact_dir and "artifacts".
---
Outside diff comments:
In `@projects/core/library/vault.py`:
- Around line 370-391: The init signature defaulting strict=True changes
behavior causing RuntimeError when existing callers relied on non-strict
validation; update the caller that invokes
vault.init(config.project.get_config("vaults")) to explicitly pass strict=False
(i.e., call init(..., strict=False)) so it preserves previous non-strict
behavior, or alternatively change the init default back to strict=False; locate
the Vault init function and its use sites (references to init and
_filter_and_validate_vaults / VaultManager) and apply the explicit strict=False
at the submit call if you want minimal, localized change.
---
Nitpick comments:
In `@projects/core/library/vault.py`:
- Line 370: The function signature for init currently uses a None default
without an Optional type; update the signature of init(vaults: List[str] = None,
strict: bool = True) to use Optional[List[str]] for the vaults parameter (e.g.,
init(vaults: Optional[List[str]] = None, strict: bool = True)) and add the
necessary Optional import from typing at the top of the module so static
analysis no longer flags RUF013; keep the existing behavior of init and the
strict parameter unchanged.
- Line 324: The function/method docstring for the routine that accepts the
strict parameter is misleading: update the docstring for the method that defines
strict (the function/method with parameter named strict) to accurately state
that strict=True (the default) will raise an exception on vault validation
failure, while strict=False will suppress the exception and return a failure
indicator (or False); mention the default value and the exact behavior for both
True and False so callers aren’t confused.
In `@projects/llm_d/orchestration/cli.py`:
- Line 3: Docstring wording is inconsistent: the module header reads "CLI
Operations" but the main() docstring still says "CI Operations", causing
misleading help text; update the main() function's docstring (the docstring
inside function main) to use "CLI Operations" (or the intended term) so click
--help and other introspection show consistent wording across the module.
In `@projects/skeleton/orchestration/cli.py`:
- Around line 23-24: Remove the unused click context object setup: delete the
call to ctx.ensure_object(types.SimpleNamespace) and the unused types import (or
alternatively add documentation and actual usage where ctx.obj is read),
ensuring no subcommand references ctx.obj; confirm that
test_skeleton.init(strict_vault_validation=False) remains unchanged and that no
runtime behavior depends on the removed context object.
In `@projects/skeleton/orchestration/prepare_skeleton.py`:
- Line 2: Remove the unused top-level import of the os module by deleting the
"import os" statement (the unused symbol is the os import) from
prepare_skeleton.py so there are no unused imports left in that module.
- Around line 12-13: The code unnecessarily wraps an existing Path in
pathlib.Path and uses the wrong logger; replace the Path wrapping by calling
read_text() directly on config.project.config_path (e.g. set config_file_content
= config.project.config_path.read_text()) and change the logging call to use the
module-level logger (logger.info(...)) so the message is emitted through the
configured logger; update the reference to the config_file_content variable
accordingly.
- Around line 26-31: The try/except around the final logger.info in the
cleanup() code is dead — remove the unnecessary try/except block and simply call
logger.info("✅ Cleanup phase completed successfully") followed by return 0, and
delete the corresponding except that logs via logger.error; alternatively, if
there is actual cleanup work to perform, move those operations into the
cleanup() function before the logger.info and keep the try/except around the
real cleanup steps so that logger.error(...) (and return 1) only run on genuine
errors.
🪄 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: 0a57be2c-175e-40a0-8ebe-d6564ccfb46d
📒 Files selected for processing (12)
projects/core/dsl/runtime.pyprojects/core/library/config.pyprojects/core/library/vault.pyprojects/llm_d/orchestration/cli.pyprojects/skeleton/README.mdprojects/skeleton/orchestration/cli.pyprojects/skeleton/orchestration/config.yamlprojects/skeleton/orchestration/prepare_skeleton.pyprojects/skeleton/orchestration/presets.d/deep_testing.yamlprojects/skeleton/orchestration/presets.d/presets.yamlprojects/skeleton/orchestration/test_skeleton.pyprojects/skeleton/toolbox/cluster_info/main.py
Contributor
Author
|
/test fournos skeleton collect_cluster_info deep_testing side_testing |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 38 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
projects/skeleton/orchestration/test_skeleton.py (2)
45-49: Consider wrappingcluster_info()in try/except for consistent exit code handling.If
cluster_info()raises an exception (e.g., toolbox execution failure, subprocess error), the function will propagate the exception rather than returning the expected exit code1. This could affect CI pipeline behavior.♻️ Proposed fix
# Demonstrate calling a toolbox from orchestration logger.info("Running cluster information toolbox...") - result = cluster_info(output_format="text") - - if not result: + try: + result = cluster_info(output_format="text") + except Exception as exc: + logger.warning(f"⚠️ Cluster information gathering failed: {exc}") + return 1 + + if not result: logger.warning("⚠️ Cluster information gathering didn't work") return 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/skeleton/orchestration/test_skeleton.py` around lines 45 - 49, Wrap the call to cluster_info(output_format="text") in a try/except block so exceptions are converted into the same exit code path: call cluster_info inside try, assign to result, and in except Exception as e use logger.warning (or logger.exception) to include the exception details and then return 1; keep the existing post-call check of result to return 1 on falsy results so both error cases produce the same exit code behavior.
25-30:del client_idhas no effect.
client_idis aPathobject returned fromvault.get_vault_content_path(). Usingdelonly removes the local variable binding—it doesn't delete the underlying file or have any side effect. If the intent is to clear the reference for GC, it's unnecessary in this scope since the function continues shortly after.♻️ Proposed fix - remove the no-op del statement
else: logger.warning(f"`client_id` secret available. Size: {client_id.stat().st_size}b") - del client_id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/skeleton/orchestration/test_skeleton.py` around lines 25 - 30, The `del client_id` at the end of the block is a no-op for file removal and only removes the local binding; remove this statement to avoid misleading intent. Update the code that calls `vault.get_vault_content_path(...)` (the `client_id` variable) by deleting the `del client_id` line and leave the existing logger handling (`logger.warning(...)`, `client_id.stat().st_size`) unchanged.projects/skeleton/orchestration/prepare_skeleton.py (3)
2-4: Remove unused imports.
os,env, andrunare imported but never used in this module.♻️ Proposed fix
import logging -import os import pathlib -from projects.core.library import env, run, config +from projects.core.library import config🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/skeleton/orchestration/prepare_skeleton.py` around lines 2 - 4, Remove unused imports in prepare_skeleton.py: delete the top-level import of os and the unused names env and run from the from-projects.core.library import, leaving only pathlib and the used config import; update the import line to only bring in pathlib and config (and remove any now-empty import statements) and then run lint/tests to verify there are no remaining unused-import warnings.
22-31: Unreachable exception handler incleanup().The
tryblock only containslogger.info()calls, which won't raise exceptions under normal conditions. This makes theexceptblock dead code.♻️ Proposed fix - simplify by removing unnecessary try/except
def cleanup(): logger.info("=== Skeleton Project Cleanup Phase ===") logger.info("Demonstrating basic cleanup operations") - - try: - logger.info("✅ Cleanup phase completed successfully") - return 0 - except Exception as e: - logger.error(f"❌ Error during cleanup phase: {e}") - return 1 + logger.info("✅ Cleanup phase completed successfully") + return 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/skeleton/orchestration/prepare_skeleton.py` around lines 22 - 31, The try/except in cleanup() is redundant because the try only wraps logger.info calls; remove the try/except and simplify the function to log the start and completion messages and return 0 on success, keeping the existing logger.info("=== Skeleton Project Cleanup Phase ==="), logger.info("Demonstrating basic cleanup operations"), and logger.info("✅ Cleanup phase completed successfully") calls and delete the logger.error branch (or keep an outer-level exception only if you later add operations that can fail).
11-16:config.project.config_pathis already aPathobject.Based on the
Configclass constructor inprojects/core/library/config.py(lines 45-63),config_pathis stored as apathlib.Path. Wrapping it again withpathlib.Path()is redundant.♻️ Proposed fix
try: - config_file_content = pathlib.Path(config.project.config_path).read_text() + config_file_content = config.project.config_path.read_text() logging.info(f"Configuration file: {config_file_content}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/skeleton/orchestration/prepare_skeleton.py` around lines 11 - 16, The code redundantly wraps an already-pathlib.Path attribute; in prepare_skeleton.py replace the pathlib.Path(config.project.config_path).read_text() call with a direct call on the Path object (use config.project.config_path.read_text()) so config_file_content is read from the existing Path without re-wrapping; update the log that uses config_file_content and keep the surrounding logger.info("✅ Prepare phase completed successfully") and return 0 as-is.projects/core/library/vault.py (1)
370-370: Use explicitOptionalfor nullable parameter type hint.Per PEP 484, implicit
Optionalvia= Nonedefault is prohibited. The static analysis tool correctly flags this.♻️ Proposed fix
-def init(vaults: List[str] = None, strict: bool = True): +def init(vaults: Optional[List[str]] = None, strict: bool = True):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/library/vault.py` at line 370, The signature for init currently uses a nullable default without an explicit Optional: change the parameter type hint for vaults in the init function from List[str] = None to Optional[List[str]] = None and add the corresponding typing import (from typing import Optional) so static checkers accept the nullable parameter; ensure no other references rely on the old annotation name change.
🤖 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/skeleton/README.md`:
- Around line 242-244: Remove the orphaned command and stray closing fence
introduced after the sentence about FORGE — specifically delete the bare "
./run_ci skeleton ci --help" line and the unmatched "```" so the README resumes
properly (ensure the "Creating Your Own Project" heading and subsequent fenced
code blocks remain intact); locate the text near the "Creating Your Own Project"
section and the sentence mentioning FORGE to find and remove these orphaned
lines.
---
Nitpick comments:
In `@projects/core/library/vault.py`:
- Line 370: The signature for init currently uses a nullable default without an
explicit Optional: change the parameter type hint for vaults in the init
function from List[str] = None to Optional[List[str]] = None and add the
corresponding typing import (from typing import Optional) so static checkers
accept the nullable parameter; ensure no other references rely on the old
annotation name change.
In `@projects/skeleton/orchestration/prepare_skeleton.py`:
- Around line 2-4: Remove unused imports in prepare_skeleton.py: delete the
top-level import of os and the unused names env and run from the
from-projects.core.library import, leaving only pathlib and the used config
import; update the import line to only bring in pathlib and config (and remove
any now-empty import statements) and then run lint/tests to verify there are no
remaining unused-import warnings.
- Around line 22-31: The try/except in cleanup() is redundant because the try
only wraps logger.info calls; remove the try/except and simplify the function to
log the start and completion messages and return 0 on success, keeping the
existing logger.info("=== Skeleton Project Cleanup Phase ==="),
logger.info("Demonstrating basic cleanup operations"), and logger.info("✅
Cleanup phase completed successfully") calls and delete the logger.error branch
(or keep an outer-level exception only if you later add operations that can
fail).
- Around line 11-16: The code redundantly wraps an already-pathlib.Path
attribute; in prepare_skeleton.py replace the
pathlib.Path(config.project.config_path).read_text() call with a direct call on
the Path object (use config.project.config_path.read_text()) so
config_file_content is read from the existing Path without re-wrapping; update
the log that uses config_file_content and keep the surrounding logger.info("✅
Prepare phase completed successfully") and return 0 as-is.
In `@projects/skeleton/orchestration/test_skeleton.py`:
- Around line 45-49: Wrap the call to cluster_info(output_format="text") in a
try/except block so exceptions are converted into the same exit code path: call
cluster_info inside try, assign to result, and in except Exception as e use
logger.warning (or logger.exception) to include the exception details and then
return 1; keep the existing post-call check of result to return 1 on falsy
results so both error cases produce the same exit code behavior.
- Around line 25-30: The `del client_id` at the end of the block is a no-op for
file removal and only removes the local binding; remove this statement to avoid
misleading intent. Update the code that calls
`vault.get_vault_content_path(...)` (the `client_id` variable) by deleting the
`del client_id` line and leave the existing logger handling
(`logger.warning(...)`, `client_id.stat().st_size`) unchanged.
🪄 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: 9e1711e8-33b3-4028-9488-425aed610b8e
📒 Files selected for processing (14)
projects/core/library/ci.pyprojects/core/library/cli.pyprojects/core/library/config.pyprojects/core/library/vault.pyprojects/llm_d/orchestration/cli.pyprojects/skeleton/README.mdprojects/skeleton/orchestration/ci.pyprojects/skeleton/orchestration/cli.pyprojects/skeleton/orchestration/config.yamlprojects/skeleton/orchestration/prepare_skeleton.pyprojects/skeleton/orchestration/presets.d/deep_testing.yamlprojects/skeleton/orchestration/presets.d/presets.yamlprojects/skeleton/orchestration/test_skeleton.pyprojects/skeleton/toolbox/cluster_info/main.py
✅ Files skipped from review due to trivial changes (6)
- projects/core/library/config.py
- projects/llm_d/orchestration/cli.py
- projects/skeleton/orchestration/config.yaml
- projects/skeleton/orchestration/presets.d/presets.yaml
- projects/skeleton/orchestration/presets.d/deep_testing.yaml
- projects/skeleton/orchestration/cli.py
🚧 Files skipped from review as they are similar to previous changes (1)
- projects/skeleton/toolbox/cluster_info/main.py
Comment on lines
+242
to
244
| This project demonstrates that FORGE provides a **complete, production-ready framework** for building observable, configurable, and maintainable test harnesses. | ||
| ./run_ci skeleton ci --help | ||
| ``` |
There was a problem hiding this comment.
Malformed content: orphaned text and closing fence.
Line 242 appears to be a trailing sentence from the new content, followed by a bare command (line 243) and an orphaned closing fence (line 244). This looks like a merge/editing artifact that should be cleaned up.
♻️ Proposed fix - remove orphaned lines
This project demonstrates that FORGE provides a **complete, production-ready framework** for building observable, configurable, and maintainable test harnesses.
-./run_ci skeleton ci --help
-```
## Creating Your Own Project🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 244-244: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/skeleton/README.md` around lines 242 - 244, Remove the orphaned
command and stray closing fence introduced after the sentence about FORGE —
specifically delete the bare " ./run_ci skeleton ci --help" line and the
unmatched "```" so the README resumes properly (ensure the "Creating Your Own
Project" heading and subsequent fenced code blocks remain intact); locate the
text near the "Creating Your Own Project" section and the sentence mentioning
FORGE to find and remove these orphaned lines.
Contributor
Author
|
/test fournos skeleton collect_cluster_info deep_testing side_testing |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 06 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos skeleton collect_cluster_info deep_testing side_testing |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 06 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos skeleton collect_cluster_info deep_testing side_testing |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 00 minutes 32 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos skeleton collect_cluster_info deep_testing side_testing |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 00 minutes 51 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
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 / Behavior
Documentation
Chores