[forge] Keep working on the framework#18
Merged
kpouget merged 19 commits intoopenshift-psap:mainfrom Apr 8, 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 |
openshift-ci
bot
added
the
needs-rebase
|
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 18 minutes and 4 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 (5)
📝 WalkthroughWalkthroughAdds a YAML-driven VaultManager for secrets, integrates vaults into launcher/config/CI, introduces task ReadOnlyArgs/TaskContext and Jinja2 template rendering, refactors toolbox/tooling to use shared task context, and applies logging/notification and CLI safety wrapper changes. Documentation added; one project vault removed. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / main()
participant Launcher as Fournos Launcher
participant Config as Config System
participant Vault as VaultManager
participant Env as Environment
participant Job as Fournos Job
CLI->>Launcher: init()
Launcher->>Config: read project config (includes `vaults`)
Config-->>Launcher: config + vaults list
Launcher->>Vault: init(vaults=[...])
Vault->>Vault: load & validate vault YAMLs from FORGE_HOME/vaults
Vault-->>Launcher: VaultManager ready
Launcher->>Vault: get_vault_content_path(vault, key)
Vault-->>Launcher: /resolved/path/to/secret
Launcher->>Env: set env var (e.g. KUBECONFIG) from path
Launcher->>Job: submit_job(owner, display_name, pipeline_name, env)
Job->>Env: read KUBECONFIG + provided env
Env-->>Job: kubeconfig path + env vars
Job-->>CLI: submission result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
openshift-ci
bot
removed
the
needs-rebase
Contributor
Author
|
/test fournos llm_d hello world |
|
• Link to the test results. • No reports index generated... Test configuration: • Failure indicator: Empty. |
Contributor
Author
|
/test fournos llm_d hello world |
|
• Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos llm_d hello world |
|
• Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos llm_d hello world |
|
• Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos llm_d hello world |
|
• 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: 3
🧹 Nitpick comments (9)
projects/core/dsl/task.py (1)
132-144: Annotation check conditions are likely ineffective.The check
params[0].annotation in (inspect.Parameter.empty, 'func', callable)at line 138 has issues:
'func'is a string literal, but annotations are typically types orinspect.Parameter.emptycallableis the built-in function, not a type annotation likeCallableIn practice this works because most decorator parameters are unannotated (
inspect.Parameter.empty), but the extra conditions are misleading. Consider simplifying or using proper type checks.♻️ Suggested simplification
# Check if this is a simple decorator that takes a single function argument - if (len(params) == 1 and - params[0].annotation in (inspect.Parameter.empty, 'func', callable) and - len(args) == 1 and len(kwargs) == 0 and - callable(args[0]) and hasattr(args[0], '__name__')): + if (len(params) == 1 and + params[0].annotation is inspect.Parameter.empty and + len(args) == 1 and len(kwargs) == 0 and + callable(args[0]) and hasattr(args[0], '__name__')):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/dsl/task.py` around lines 132 - 144, The annotation check in the decorator detection is wrong/misleading: replace the tuple check on params[0].annotation (params[0].annotation in (inspect.Parameter.empty, 'func', callable)) with a simple proper test that the parameter is unannotated (params[0].annotation is inspect.Parameter.empty) or otherwise remove annotation-based checks entirely; keep the surrounding logic (len(params) == 1, len(args) == 1, len(kwargs) == 0, callable(args[0]) and hasattr(args[0], '__name__')) and then call _ensure_is_task(func, decorator_func.__name__) as before so decorator_func, params, args, kwargs and _ensure_is_task remain the referenced symbols to locate the change.docs/orchestration/vaults.md (3)
83-91: Add language specifiers to code blocks for consistency.The code blocks showing file path resolution patterns are missing language specifiers. While these are simple text examples, adding a language identifier improves markdown linting compliance and rendering consistency.
📝 Proposed fix
### File Path Resolution Vault content paths are resolved as: -``` +```text {env_var_value}/{filename}For example, with
PSAP_FORGE_FOURNOS_CI_SECRET_PATH=/secrets/ciand content namedkubeconfig:
-+text
/secrets/ci/kubeconfig🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/orchestration/vaults.md` around lines 83 - 91, The markdown code blocks in docs/orchestration/vaults.md that show the vault path pattern and example should include a language specifier to satisfy linting; update the two triple-backtick blocks surrounding `{env_var_value}/{filename}` and `/secrets/ci/kubeconfig` to use ```text (i.e., add "text" after the opening backticks) so both fenced code blocks explicitly declare the language.
154-165: Add language specifier to directory structure example.📝 Proposed fix
### Directory Structure Vault directories should contain only the files defined in the vault specification: -``` +```text /path/to/ci/secrets/ ├── kubeconfig # Defined in vault spec ├── server_url # Defined in vault spec └── api_token # Defined in vault spec</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/orchestration/vaults.mdaround lines 154 - 165, The code block showing
the vault directory structure in docs/orchestration/vaults.md lacks a language
specifier; update the fenced code block markers around the directory example
(the triple backticks surrounding the tree under "Directory Structure") to
include "text" (i.e., changetotext) so the block is rendered with the
correct language hint.</details> --- `265-291`: **Add language specifiers to error message examples.** The error message examples in the Error Handling section are missing language specifiers. <details> <summary>📝 Proposed fix</summary> ```diff **Missing environment variable:** -``` +```text Vault 'my-vault' requires environment variable MY_VAULT_SECRET_PATH to be set ``` Solution: Export the required environment variable **Missing description:** -``` +```text Vault 'my-vault' content 'api_key' is missing description ``` Solution: Add description to vault definition **Extra files:** -``` +```text Vault 'my-vault' contains extra file 'backup.txt' at '/path/vault/backup.txt' not defined in specification ``` Solution: Either remove the file or add it to the vault definition **Missing content file:** -``` +```text Vault 'my-vault' missing content 'api_key' at: /path/vault/api_key ``` Solution: Create the missing secret file ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/orchestration/vaults.mdaround lines 265 - 291, Update the Error
Handling examples in docs/orchestration/vaults.md to add language specifiers
(use ```text) for each code block containing the example error messages (the
four blocks under "Missing environment variable", "Missing description", "Extra
files", and "Missing content file") so the fenced code blocks explicitly declare
the language; ensure you replace the plain triple-backtick blocks with
triple-backticks followed by text for the examples in that section.</details> </blockquote></details> <details> <summary>projects/core/library/vault.py (5)</summary><blockquote> `360-360`: **Use explicit `Optional[List[str]]` type hint.** Same as line 68 - the default `None` requires an explicit `Optional` annotation per PEP 484. <details> <summary>📝 Proposed fix</summary> ```diff -def init(vaults: List[str] = None): +def init(vaults: Optional[List[str]] = None): ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@projects/core/library/vault.py` at line 360, The function signature for init currently uses a default None without an explicit Optional type; update its annotation to use Optional[List[str]] for the vaults parameter (i.e., change def init(vaults: List[str] = None): to use Optional[List[str]]), and ensure typing.Optional is imported (or already used elsewhere, e.g., as on line 68) so the annotation is consistent with PEP 484 across the module. ``` </details> --- `113-118`: **Add explicit check for required `env_key` field.** If `env_key` is missing from the YAML file, this will raise a `KeyError` with an unclear message. Consider validating required fields explicitly for better error reporting. <details> <summary>📝 Proposed fix</summary> ```diff + if 'env_key' not in data: + raise ValueError(f"Vault definition missing required 'env_key' field: {vault_file}") + vault_def = VaultDefinition( name=vault_name, env_key=data['env_key'], description=data.get('description', ''), content=content ) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@projects/core/library/vault.py` around lines 113 - 118, Before constructing VaultDefinition, explicitly validate that the required key 'env_key' exists and is non-empty in the parsed YAML dict (data); if missing or empty, raise a clear ValueError (or custom exception) with a descriptive message referencing the vault name or source file so callers know which vault is malformed. Update the block around vault_def creation (the code that builds VaultDefinition and assigns to vault_def) to perform this check on data['env_key'] (or data.get('env_key')) and only proceed to call VaultDefinition(...) when the check passes. ``` </details> --- `18-19`: **Remove unused `TYPE_CHECKING` block.** The `TYPE_CHECKING` import is used but the conditional block only contains `pass`, making it dead code. Either remove it entirely or add the forward reference imports that justify its existence. <details> <summary>🧹 Proposed fix</summary> ```diff -from typing import Dict, List, Optional, Set, TYPE_CHECKING +from typing import Dict, List, Optional, Set from dataclasses import dataclass -if TYPE_CHECKING: - pass - import projects.core.library.env as env ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@projects/core/library/vault.py` around lines 18 - 19, The conditional TYPE_CHECKING block in vault.py is dead code (it only contains "pass"); remove the import of TYPE_CHECKING and the empty "if TYPE_CHECKING: pass" block, or alternatively populate that block with the intended forward-reference imports used only for typing; specifically update references to the TYPE_CHECKING symbol and the conditional block around it so there are no unused imports or empty conditionals (e.g., remove TYPE_CHECKING import and the "if TYPE_CHECKING" block unless you add forward-only type imports for classes used elsewhere in this module). ``` </details> --- `68-71`: **Use explicit `Optional[Path]` type hint.** PEP 484 prohibits implicit `Optional`. The default `None` requires an explicit `Optional` annotation. <details> <summary>📝 Proposed fix</summary> ```diff - def __init__(self, vault_definitions_dir: Path = None): + def __init__(self, vault_definitions_dir: Optional[Path] = None): self.vault_definitions_dir = vault_definitions_dir or env.FORGE_HOME / "vaults" ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@projects/core/library/vault.py` around lines 68 - 71, The __init__ signature currently uses an implicit None default for vault_definitions_dir without an Optional annotation; update the parameter type to Optional[Path] (i.e., change def __init__(self, vault_definitions_dir: Optional[Path] = None)) and add the corresponding import for Optional from typing at the top of the module, keeping the rest of the logic (assigning self.vault_definitions_dir and _vault_cache and calling _load_vault_definitions) unchanged. ``` </details> --- `79-86`: **Consider narrowing the exception type or re-raising after logging.** Catching a broad `Exception` during vault loading is reasonable for robustness (one malformed file shouldn't prevent loading others), but this can mask unexpected errors (e.g., permission issues, disk I/O errors). Consider catching more specific exceptions like `yaml.YAMLError` and `OSError`, or at minimum logging the full traceback for debugging. <details> <summary>📝 Proposed fix with traceback</summary> ```diff try: vault_def = self._load_vault_definition(vault_file) self._vault_cache[vault_def.name] = vault_def logger.debug(f"Loaded vault definition: {vault_def.name}") - except Exception as e: - logger.error(f"Failed to load vault definition from {vault_file}: {e}") + except (yaml.YAMLError, OSError, KeyError) as e: + logger.error(f"Failed to load vault definition from {vault_file}: {e}", exc_info=True) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@projects/core/library/vault.py` around lines 79 - 86, The loop that loads vault defs currently catches all Exception in projects/core/library/vault.py (around the self.vault_definitions_dir.glob("*.yaml") loop calling self._load_vault_definition) which can mask real errors; change it to catch specific expected errors (e.g., yaml.YAMLError and OSError) and use logger.exception(...) to record the full traceback when those occur, and for any other unexpected exceptions re-raise after logging so they aren’t silently swallowed (keep storing results in self._vault_cache as before for successful loads). ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@projects/core/library/config.py:
- Around line 289-303: In filter_out_used_overrides, get_config("overrides", {})
can return None when overrides is explicitly null, causing overrides.items() to
fail; update the function to treat a None (or non-dict) overrides as an empty
dict before iterating (e.g., if not isinstance(overrides, dict): overrides =
{}), then proceed with the existing loop and call to set_config; reference
filter_out_used_overrides, get_config, has_config, and set_config when making
the change.In
@projects/core/library/vault.py:
- Around line 382-397: The convenience functions get_vault_manager,
validate_project_vaults, and get_vault_content_path currently assume
_vault_manager is initialized and will raise AttributeError if init() wasn't
called; update get_vault_manager to check if _vault_manager is None and raise a
clear RuntimeError (or ValueError) like "Vault manager not initialized; call
init() first" (or alternatively change its return type to Optional[VaultManager]
and propagate Optional typing), and update validate_project_vaults and
get_vault_content_path to either call the guarded get_vault_manager (so they
inherit the clear error) or explicitly guard for None before calling methods on
the manager.In
@projects/fournos_launcher/orchestration/submit.py:
- Around line 16-17: config.project.get_config("vaults") can raise KeyError when
the key is absent; change the retrieval to provide a safe default before calling
vault.init. Replace the direct call with a guarded fetch (e.g., call get_config
with a default like None or wrap in try/except KeyError) so that vault.init
receives None/[] when "vaults" is missing; update the code around
config.project.get_config("vaults") and the subsequent vault.init(...) call to
use that default value.
Nitpick comments:
In@docs/orchestration/vaults.md:
- Around line 83-91: The markdown code blocks in docs/orchestration/vaults.md
that show the vault path pattern and example should include a language specifier
to satisfy linting; update the two triple-backtick blocks surrounding
{env_var_value}/{filename}and/secrets/ci/kubeconfigto use ```text (i.e.,
add "text" after the opening backticks) so both fenced code blocks explicitly
declare the language.- Around line 154-165: The code block showing the vault directory structure in
docs/orchestration/vaults.md lacks a language specifier; update the fenced code
block markers around the directory example (the triple backticks surrounding the
tree under "Directory Structure") to include "text" (i.e., change ``` to- Around line 265-291: Update the Error Handling examples in docs/orchestration/vaults.md to add language specifiers (use ```text) for each code block containing the example error messages (the four blocks under "Missing environment variable", "Missing description", "Extra files", and "Missing content file") so the fenced code blocks explicitly declare the language; ensure you replace the plain triple-backtick blocks with triple-backticks followed by text for the examples in that section. In `@projects/core/dsl/task.py`: - Around line 132-144: The annotation check in the decorator detection is wrong/misleading: replace the tuple check on params[0].annotation (params[0].annotation in (inspect.Parameter.empty, 'func', callable)) with a simple proper test that the parameter is unannotated (params[0].annotation is inspect.Parameter.empty) or otherwise remove annotation-based checks entirely; keep the surrounding logic (len(params) == 1, len(args) == 1, len(kwargs) == 0, callable(args[0]) and hasattr(args[0], '__name__')) and then call _ensure_is_task(func, decorator_func.__name__) as before so decorator_func, params, args, kwargs and _ensure_is_task remain the referenced symbols to locate the change. In `@projects/core/library/vault.py`: - Line 360: The function signature for init currently uses a default None without an explicit Optional type; update its annotation to use Optional[List[str]] for the vaults parameter (i.e., change def init(vaults: List[str] = None): to use Optional[List[str]]), and ensure typing.Optional is imported (or already used elsewhere, e.g., as on line 68) so the annotation is consistent with PEP 484 across the module. - Around line 113-118: Before constructing VaultDefinition, explicitly validate that the required key 'env_key' exists and is non-empty in the parsed YAML dict (data); if missing or empty, raise a clear ValueError (or custom exception) with a descriptive message referencing the vault name or source file so callers know which vault is malformed. Update the block around vault_def creation (the code that builds VaultDefinition and assigns to vault_def) to perform this check on data['env_key'] (or data.get('env_key')) and only proceed to call VaultDefinition(...) when the check passes. - Around line 18-19: The conditional TYPE_CHECKING block in vault.py is dead code (it only contains "pass"); remove the import of TYPE_CHECKING and the empty "if TYPE_CHECKING: pass" block, or alternatively populate that block with the intended forward-reference imports used only for typing; specifically update references to the TYPE_CHECKING symbol and the conditional block around it so there are no unused imports or empty conditionals (e.g., remove TYPE_CHECKING import and the "if TYPE_CHECKING" block unless you add forward-only type imports for classes used elsewhere in this module). - Around line 68-71: The __init__ signature currently uses an implicit None default for vault_definitions_dir without an Optional annotation; update the parameter type to Optional[Path] (i.e., change def __init__(self, vault_definitions_dir: Optional[Path] = None)) and add the corresponding import for Optional from typing at the top of the module, keeping the rest of the logic (assigning self.vault_definitions_dir and _vault_cache and calling _load_vault_definitions) unchanged. - Around line 79-86: The loop that loads vault defs currently catches all Exception in projects/core/library/vault.py (around the self.vault_definitions_dir.glob("*.yaml") loop calling self._load_vault_definition) which can mask real errors; change it to catch specific expected errors (e.g., yaml.YAMLError and OSError) and use logger.exception(...) to record the full traceback when those occur, and for any other unexpected exceptions re-raise after logging so they aren’t silently swallowed (keep storing results in self._vault_cache as before for successful loads).🪄 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:
65147250-82cc-4618-a1a4-315e48424293📒 Files selected for processing (12)
docs/orchestration/vaults.mdprojects/core/dsl/task.pyprojects/core/library/ci.pyprojects/core/library/config.pyprojects/core/library/vault.pyprojects/core/notifications/send.pyprojects/fournos_launcher/orchestration/ci.pyprojects/fournos_launcher/orchestration/config.yamlprojects/fournos_launcher/orchestration/submit.pyprojects/llm_d/orchestration/ci.pyprojects/llm_d/orchestration/vaults.yamlprojects/skeleton/orchestration/ci.py💤 Files with no reviewable changes (1)
- projects/llm_d/orchestration/vaults.yaml
Contributor
Author
|
/test fournos llm_d hello world |
|
• Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos llm_d hello world |
|
• Link to the test results. • No reports index generated... Test configuration: |
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 (1)
projects/core/dsl/runner.py (1)
38-64:⚠️ Potential issue | 🟠 MajorKeep the entire command trace on a single output path.
Line 38 uses
logger.info(), but lines 57-64 still useprint()directly. Unlikeprojects/core/dsl/shell.py, this module doesn't calllogging.basicConfig(), so the command banner may not appear while the output lines still print—splitting the trace across output channels. Consolidate to use onlylogger.info()throughout, as shown inshell.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/dsl/runner.py` around lines 38 - 64, The command trace is split between logger.info and print calls in projects/core/dsl/runner.py (around the subprocess.run / CommandResult handling); replace the direct print(...) calls that output stdout, stderr and exit code with logger.info(...) calls (preserving the same formatted strings and .strip() usage) and also replace the trailing print() with logger.info("") so the entire banner and command output go through the same logging path (use the existing logger variable and keep constructing CommandResult as-is).
♻️ Duplicate comments (1)
projects/core/library/vault.py (1)
382-397:⚠️ Potential issue | 🟡 MinorGuard against uninitialized vault manager in convenience functions.
If
init()hasn't been called,get_vault_manager()returnsNone, and calling methods likevalidate_project_vaults()orget_vault_content_path()will raise anAttributeError.🛡️ Proposed fix
def get_vault_manager() -> VaultManager: """Get the global vault manager instance""" + if _vault_manager is None: + raise RuntimeError("VaultManager not initialized. Call vault.init() first.") return _vault_managerAlternatively, update the return type to
Optional[VaultManager]if callers are expected to handleNone.🤖 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 382 - 397, The convenience functions validate_project_vaults and get_vault_content_path currently call get_vault_manager() which can return None if init() wasn't called; update both functions to guard against a None vault manager by retrieving vm = get_vault_manager(), checking if vm is None and raising a clear runtime error (e.g., "Vault manager not initialized; call init() first") instead of calling methods on None, then delegate to vm.validate_project_vaults(...) and vm.get_vault_content_path(...) when present; ensure the error message names the function (validate_project_vaults/get_vault_content_path) and suggests calling init() to help debugging.
🧹 Nitpick comments (7)
projects/core/library/vault.py (1)
68-68: Use explicitOptionalfor type hints.PEP 484 prohibits implicit
Optional. The parametervault_definitions_dir: Path = Noneshould use explicit optional typing.♻️ Proposed fix
- def __init__(self, vault_definitions_dir: Path = None): + def __init__(self, vault_definitions_dir: Optional[Path] = None):Similarly for line 360:
-def init(vaults: List[str] = None): +def init(vaults: Optional[List[str]] = None):🤖 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 68, The type hints use implicit Optional (e.g., vault_definitions_dir: Path = None in Vault.__init__) which violates PEP 484; update the signatures to use explicit Optional[Path] and import Optional from typing (add "from typing import Optional" if missing), and apply the same change to the other occurrence referenced around line 360 (replace the parameter currently annotated as Path = None with Optional[Path]). Ensure imports are updated and any type references use Optional[Path] consistently.docs/orchestration/vaults.md (3)
158-165: Add language identifier to directory structure code block.📝 Proposed fix
-``` +```text /path/to/ci/secrets/ ├── kubeconfig # Defined in vault spec ├── server_url # Defined in vault spec └── api_token # Defined in vault spec</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/orchestration/vaults.mdaround lines 158 - 165, The fenced code block
showing the directory structure is missing a language identifier; update the
triple-backtick fence around the directory tree in the vaults.md content to
include a language tag (use "text") so the block becomestext ...which
ensures correct rendering and preserves spacing for the directory structure.</details> --- `84-91`: **Add language identifiers to fenced code blocks.** The code blocks showing path resolution examples should have a language identifier (e.g., `text` or `plaintext`) for consistency and to satisfy linters. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text {env_var_value}/{filename} ``` For example, with `PSAP_FORGE_FOURNOS_CI_SECRET_PATH=/secrets/ci` and content named `kubeconfig`: -``` +```text /secrets/ci/kubeconfig ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/orchestration/vaults.mdaround lines 84 - 91, Add language identifiers
to the two fenced code blocks that show path resolution examples in the
docs/orchestration/vaults.md snippet: change the opening triple-backtick fences
for the examples containing "{env_var_value}/{filename}" and
"/secrets/ci/kubeconfig" to include a language identifier such as "text" (i.e.,
265-291: Add language identifiers to error example code blocks.The error message examples at lines 270-291 should have language identifiers.
📝 Proposed fix
-``` +```text Vault 'my-vault' requires environment variable MY_VAULT_SECRET_PATH to be set-
+text
Vault 'my-vault' content 'api_key' is missing description-``` +```text Vault 'my-vault' contains extra file 'backup.txt' at '/path/vault/backup.txt' not defined in specification-
+text
Vault 'my-vault' missing content 'api_key' at: /path/vault/api_key🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/orchestration/vaults.md` around lines 265 - 291, In the "Error Handling" section (the four error example blocks under the header), add explicit language identifiers to each fenced code block by changing the opening triple backticks to ```text for the examples: "Vault 'my-vault' requires environment variable MY_VAULT_SECRET_PATH to be set", "Vault 'my-vault' content 'api_key' is missing description", "Vault 'my-vault' contains extra file 'backup.txt' at '/path/vault/backup.txt' not defined in specification", and "Vault 'my-vault' missing content 'api_key' at: /path/vault/api_key" so they render with the text language tag and consistent formatting.projects/fournos_launcher/toolbox/submit_and_wait/main.py (2)
28-28: Use explicitOptionalfor type hint.PEP 484 prohibits implicit
Optional. The parameterenv: dict = Noneshould use explicit optional typing.♻️ Proposed fix
- env: dict = None, + env: Optional[dict] = None,And add
Optionalto imports:-from typing import Any +from typing import Any, Optional🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py` at line 28, The parameter declaration using a nullable default (env: dict = None) violates PEP 484; change the annotation to use explicit Optional by updating the parameter to env: Optional[dict] = None and add Optional to your typing imports (e.g., include Optional in the existing from typing import ... list or add from typing import Optional); ensure you update the signature where the env parameter is defined so tools and linters recognize the optional type.
90-90: Remove unnecessary f-string prefix.The string
f"Inputs validated"has no placeholders and should be a regular string.♻️ Proposed fix
- return f"Inputs validated" + return "Inputs validated"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py` at line 90, Replace the unnecessary f-string prefix on the return statement that currently reads return f"Inputs validated" with a plain string literal (return "Inputs validated") so no formatting is used; locate the return f"Inputs validated" in the function that performs input validation and remove the leading f character.projects/core/dsl/template.py (1)
76-84: Consider removing misleading autoescape setting.The comment says "Enable auto-escaping for security (though we're generating YAML, not HTML)" but
autoescapeis configured for['html', 'xml'], which won't affect YAML output. Since this generates YAML, the autoescape setting is effectively a no-op but the configuration and comment may confuse future maintainers.♻️ Proposed simplification
# Set up Jinja2 environment env = jinja2.Environment( loader=jinja2.FileSystemLoader(templates_dir), - # Enable auto-escaping for security (though we're generating YAML, not HTML) - autoescape=jinja2.select_autoescape(['html', 'xml']), + # No autoescape needed for YAML output + autoescape=False, # Raise errors for undefined variables instead of silently ignoring undefined=jinja2.StrictUndefined, # Keep line endings consistent keep_trailing_newline=True )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/dsl/template.py` around lines 76 - 84, The autoescape parameter in the jinja2.Environment call is misleading for YAML generation; update the env construction (the jinja2.Environment call that currently references templates_dir and autoescape) by removing the autoescape argument and its misleading comment (or set autoescape=None) so the configuration and comment accurately reflect that auto-escaping is not used for YAML; keep the other options (undefined=jinja2.StrictUndefined, keep_trailing_newline=True) unchanged.
🤖 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/dsl/context.py`:
- Around line 16-18: ReadOnlyArgs currently stores the original args_dict by
reference (object.__setattr__(self, '_args_dict', args_dict)) and
create_task_parameters() copies shared_context attributes by reference, allowing
in-place mutations like args.env[...] or context.some_list.append(...) to leak
across tasks; fix by performing defensive deep copies of any mutable values when
constructing ReadOnlyArgs (copy.deepcopy on values placed into _args_dict) and
when building task contexts in create_task_parameters(), and likewise replace
the direct setattr(shared_context, ...) assignment in runtime.py with a
defensive copy of the value before assigning so task-local modifications cannot
affect the shared objects.
In `@projects/core/dsl/toolbox.py`:
- Around line 107-109: The code yields serialized task_context without guarding
for None and may leak secrets; update the block that calls
clean_args(e.task_context) and yaml.dump (around the generator yielding "~~
CONTEXT:" in toolbox.py) to first check if e.task_context is None and yield a
clear "no context" line instead, and when task_context is present pass only a
safe, allowlisted subset (or a redacted copy) to clean_args before yaml.dump;
ensure clean_args is only called with a dict and that sensitive keys are
filtered/masked prior to serialization.
In `@projects/fournos_launcher/orchestration/submit.py`:
- Around line 22-28: prepare_env currently blindly sets os.environ["KUBECONFIG"]
= str(kubeconfig_path) which will set the literal "None" if
vault.get_vault_content_path returns None; change prepare_env to check the
result of vault.get_vault_content_path (the kubeconfig_path variable) and if
it's None either raise a clear exception or log an error and exit, otherwise set
os.environ["KUBECONFIG"] to the valid path (converted to string). Reference
prepare_env, vault.get_vault_content_path, and the os.environ["KUBECONFIG"]
assignment when implementing the guard and error handling.
In `@projects/llm_d/toolbox/capture_isvc_state/main.py`:
- Around line 14-18: The change introduced a keyword-only parameter by adding
"*" to the run signature which breaks existing callers that call
capture_isvc_state(_cfg.name, _cfg.ns) positionally; either remove the "*" from
the run(llmisvc_name: str, *, namespace: str = "") signature so namespace
remains positional, or update the callers of capture_isvc_state to pass
namespace as a keyword (capture_isvc_state(_cfg.name, namespace=_cfg.ns));
locate the function named run and the call sites named capture_isvc_state in the
tests and apply one consistent fix across the repo.
- Around line 63-65: The current use of f-string interpolation into
shell.run(...) (e.g., the oc invocations around shell.run calls that build
commands like 'oc get llminferenceservice {args.llmisvc_name} -n
{context.target_namespace} -oyaml') is vulnerable to command injection because
shell=True is used by default in projects/core/dsl/shell.py::run; change each
interpolated call to pass the command as a list (argv form) and set shell=False
and explicit check=False where appropriate (for example the shell.run call that
fetches llminferenceservice and all other similar oc commands around lines 64,
75, 86, 97) so arguments like args.llmisvc_name and context.target_namespace are
passed as separate list elements instead of being interpolated into a shell
string.
---
Outside diff comments:
In `@projects/core/dsl/runner.py`:
- Around line 38-64: The command trace is split between logger.info and print
calls in projects/core/dsl/runner.py (around the subprocess.run / CommandResult
handling); replace the direct print(...) calls that output stdout, stderr and
exit code with logger.info(...) calls (preserving the same formatted strings and
.strip() usage) and also replace the trailing print() with logger.info("") so
the entire banner and command output go through the same logging path (use the
existing logger variable and keep constructing CommandResult as-is).
---
Duplicate comments:
In `@projects/core/library/vault.py`:
- Around line 382-397: The convenience functions validate_project_vaults and
get_vault_content_path currently call get_vault_manager() which can return None
if init() wasn't called; update both functions to guard against a None vault
manager by retrieving vm = get_vault_manager(), checking if vm is None and
raising a clear runtime error (e.g., "Vault manager not initialized; call init()
first") instead of calling methods on None, then delegate to
vm.validate_project_vaults(...) and vm.get_vault_content_path(...) when present;
ensure the error message names the function
(validate_project_vaults/get_vault_content_path) and suggests calling init() to
help debugging.
---
Nitpick comments:
In `@docs/orchestration/vaults.md`:
- Around line 158-165: The fenced code block showing the directory structure is
missing a language identifier; update the triple-backtick fence around the
directory tree in the vaults.md content to include a language tag (use "text")
so the block becomes ```text ... ``` which ensures correct rendering and
preserves spacing for the directory structure.
- Around line 84-91: Add language identifiers to the two fenced code blocks that
show path resolution examples in the docs/orchestration/vaults.md snippet:
change the opening triple-backtick fences for the examples containing
"{env_var_value}/{filename}" and "/secrets/ci/kubeconfig" to include a language
identifier such as "text" (i.e., ```text) so linters and formatters recognize
them as plaintext code blocks.
- Around line 265-291: In the "Error Handling" section (the four error example
blocks under the header), add explicit language identifiers to each fenced code
block by changing the opening triple backticks to ```text for the examples:
"Vault 'my-vault' requires environment variable MY_VAULT_SECRET_PATH to be set",
"Vault 'my-vault' content 'api_key' is missing description", "Vault 'my-vault'
contains extra file 'backup.txt' at '/path/vault/backup.txt' not defined in
specification", and "Vault 'my-vault' missing content 'api_key' at:
/path/vault/api_key" so they render with the text language tag and consistent
formatting.
In `@projects/core/dsl/template.py`:
- Around line 76-84: The autoescape parameter in the jinja2.Environment call is
misleading for YAML generation; update the env construction (the
jinja2.Environment call that currently references templates_dir and autoescape)
by removing the autoescape argument and its misleading comment (or set
autoescape=None) so the configuration and comment accurately reflect that
auto-escaping is not used for YAML; keep the other options
(undefined=jinja2.StrictUndefined, keep_trailing_newline=True) unchanged.
In `@projects/core/library/vault.py`:
- Line 68: The type hints use implicit Optional (e.g., vault_definitions_dir:
Path = None in Vault.__init__) which violates PEP 484; update the signatures to
use explicit Optional[Path] and import Optional from typing (add "from typing
import Optional" if missing), and apply the same change to the other occurrence
referenced around line 360 (replace the parameter currently annotated as Path =
None with Optional[Path]). Ensure imports are updated and any type references
use Optional[Path] consistently.
In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py`:
- Line 28: The parameter declaration using a nullable default (env: dict = None)
violates PEP 484; change the annotation to use explicit Optional by updating the
parameter to env: Optional[dict] = None and add Optional to your typing imports
(e.g., include Optional in the existing from typing import ... list or add from
typing import Optional); ensure you update the signature where the env parameter
is defined so tools and linters recognize the optional type.
- Line 90: Replace the unnecessary f-string prefix on the return statement that
currently reads return f"Inputs validated" with a plain string literal (return
"Inputs validated") so no formatting is used; locate the return f"Inputs
validated" in the function that performs input validation and remove the leading
f character.
🪄 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: c61be165-bf61-487d-aa18-faed30df58ea
📒 Files selected for processing (19)
docs/orchestration/vaults.mdprojects/core/dsl/__init__.pyprojects/core/dsl/context.pyprojects/core/dsl/runner.pyprojects/core/dsl/runtime.pyprojects/core/dsl/shell.pyprojects/core/dsl/template.pyprojects/core/dsl/toolbox.pyprojects/core/library/ci.pyprojects/core/library/config.pyprojects/core/library/vault.pyprojects/fournos_launcher/orchestration/ci.pyprojects/fournos_launcher/orchestration/config.yamlprojects/fournos_launcher/orchestration/submit.pyprojects/fournos_launcher/toolbox/submit_and_wait/main.pyprojects/llm_d/orchestration/ci.pyprojects/llm_d/orchestration/vaults.yamlprojects/llm_d/toolbox/capture_isvc_state/main.pyprojects/skeleton/orchestration/ci.py
💤 Files with no reviewable changes (1)
- projects/llm_d/orchestration/vaults.yaml
✅ Files skipped from review due to trivial changes (2)
- projects/fournos_launcher/orchestration/ci.py
- projects/core/dsl/init.py
🚧 Files skipped from review as they are similar to previous changes (3)
- projects/skeleton/orchestration/ci.py
- projects/llm_d/orchestration/ci.py
- projects/fournos_launcher/orchestration/config.yaml
Contributor
Author
|
/test fournos llm_d hello world |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
projects/core/dsl/toolbox.py (1)
107-109:⚠️ Potential issue | 🟠 Major
clean_args()will crash whentask_contextisNone.The
clean_argsfunction iterates overvalue.items()(line 93), bute.task_contextcan beNonewhen task context is unavailable (seeTaskExecutionError.__init__default). This will raiseAttributeError: 'NoneType' object has no attribute 'items'during error reporting.Also, the f-string on line 107 has no placeholders.
🐛 Proposed fix
- yield f"~~ CONTEXT:" - for line in yaml.dump(clean_args(e.task_context), default_flow_style=False, sort_keys=False).splitlines(): - yield f"~~ {line}" + yield "~~ CONTEXT:" + if e.task_context is not None: + for line in yaml.dump(clean_args(e.task_context), default_flow_style=False, sort_keys=False).splitlines(): + yield f"~~ {line}" + else: + yield "~~ (no context available)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/dsl/toolbox.py` around lines 107 - 109, The code currently calls clean_args(e.task_context) which will throw if e.task_context is None because clean_args expects a mapping; change to pass a safe default (e.g. clean_args(e.task_context or {})) or explicitly check if e.task_context is None and yield a placeholder like "~~ <no context available>" before dumping; also remove the unnecessary f-string on the "~~ CONTEXT:" line (use a plain string) so the yield reads consistently and avoid the AttributeError when TaskExecutionError.__init__ leaves task_context as None.projects/llm_d/toolbox/capture_isvc_state/main.py (1)
14-18:⚠️ Potential issue | 🟠 MajorUpdate all callers to pass
namespaceas a keyword argument.The
*in the function signature makesnamespacekeyword-only. Both test_llmd.py files pass it positionally at line 27:capture_isvc_state(_cfg.name, _cfg.ns) # ❌ Will failMust be:
capture_isvc_state(_cfg.name, namespace=_cfg.ns) # ✓ CorrectUpdate both locations:
projects/llm_d/orchestration/test_llmd.py:27projects/fournos_launcher/orchestration/test_llmd.py:27🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/llm_d/toolbox/capture_isvc_state/main.py` around lines 14 - 18, The function run(...) makes namespace a keyword-only parameter, so callers must pass it by name; update all calls to capture_isvc_state that currently pass namespace positionally (e.g., capture_isvc_state(_cfg.name, _cfg.ns)) to use a keyword argument instead (capture_isvc_state(_cfg.name, namespace=_cfg.ns)); search for calls to capture_isvc_state (and any direct calls to run(...)) in the tests and update them accordingly.
🧹 Nitpick comments (8)
projects/core/library/ci.py (1)
110-131: Consider refactoring to reduce code duplication.The
safe_ci_functiondecorator shares nearly identical implementation withsafe_ci_command(lines 87-108), differing only in success-path behavior. This duplication increases maintenance burden.♻️ Proposed refactor to extract common logic
+def _safe_wrapper_base(command_func, exit_on_success=True): + """ + Base wrapper for CI commands with consistent error handling. + + Args: + command_func: Function to execute safely + exit_on_success: Whether to call sys.exit on successful execution + """ + def wrapper(*args, **kwargs): + try: + result = command_func(*args, **kwargs) + if exit_on_success: + sys.exit(result) + return result + except Exception as e: + handle_ci_exception(e) + sys.exit(1) + + # Preserve original function metadata + wrapper.__name__ = command_func.__name__ + wrapper.__doc__ = command_func.__doc__ + wrapper.__module__ = command_func.__module__ + wrapper.__qualname__ = command_func.__qualname__ + + return wrapper + def safe_ci_command(command_func): """ - Decorator/wrapper for CI commands to provide consistent error handling. + Decorator for CI commands that exits the process with the command's return code. + + Use this for standalone CI scripts that should exit immediately. Args: command_func: Function to execute safely """ - def wrapper(*args, **kwargs): - try: - exit_code = command_func(*args, **kwargs) - sys.exit(exit_code) - except Exception as e: - handle_ci_exception(e) - sys.exit(1) - - # Preserve original function metadata - wrapper.__name__ = command_func.__name__ - wrapper.__doc__ = command_func.__doc__ - wrapper.__module__ = command_func.__module__ - wrapper.__qualname__ = command_func.__qualname__ - - return wrapper + return _safe_wrapper_base(command_func, exit_on_success=True) def safe_ci_function(command_func): """ - Decorator/wrapper for CI commands to provide consistent error handling. - This version does NOT exit on success. + Decorator for CI functions that returns the result without exiting. + + Use this for Click-based entrypoints or functions called by other code. Args: command_func: Function to execute safely """ - def wrapper(*args, **kwargs): - try: - return command_func(*args, **kwargs) - except Exception as e: - handle_ci_exception(e) - sys.exit(1) - - # Preserve original function metadata - wrapper.__name__ = command_func.__name__ - wrapper.__doc__ = command_func.__doc__ - wrapper.__module__ = command_func.__module__ - wrapper.__qualname__ = command_func.__qualname__ - - return wrapper + return _safe_wrapper_base(command_func, exit_on_success=False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/library/ci.py` around lines 110 - 131, The two decorators safe_ci_function and safe_ci_command duplicate nearly identical logic; refactor by extracting a factory like _make_safe_ci_decorator(exit_on_success: bool) that builds the inner wrapper, uses try/except to call command_func and handle_ci_exception(e), and conditionally calls sys.exit(0) on success or sys.exit(1) on failure according to the exit_on_success flag; then reimplement safe_ci_function = _make_safe_ci_decorator(exit_on_success=False) and safe_ci_command = _make_safe_ci_decorator(exit_on_success=True), preserving function metadata (wrapper.__name__, __doc__, __module__, __qualname__) and original behavior of handle_ci_exception and sys.exit calls.docs/orchestration/vaults.md (2)
112-126: Consider adding initialization note to validation example.The validation example at lines 112-126 assumes
vault.init()has already been called. Adding a brief comment or moving this after the "Initialization" section (line 233) would improve clarity for readers following top-to-bottom.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/orchestration/vaults.md` around lines 112 - 126, The validation example uses vault_manager (from get_vault_manager) but omits calling vault.init(); update the example to either add a short comment above it noting that vault.init() must be called first or move the entire snippet to follow the existing Initialization section; specifically reference get_vault_manager and vault_manager.validate_vault / validate_project_vaults / validate_all_vaults so readers know to run vault.init() before those validation calls.
84-91: Add language specifiers to fenced code blocks.Several code blocks are missing language specifiers, which affects syntax highlighting and linting compliance.
♻️ Proposed fix for path resolution examples
-``` +```text {env_var_value}/{filename}...
-+text
/secrets/ci/kubeconfigApply similar fixes to code blocks at lines 158, 270, 276, 282, and 288.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/orchestration/vaults.md` around lines 84 - 91, The fenced code blocks that show path examples such as "{env_var_value}/{filename}" and "/secrets/ci/kubeconfig" (and the other similar path/example blocks) lack language specifiers; update each triple-backtick fence to include a language hint (use "text") — e.g. change ``` to ```text for the shown path example blocks and the other similar code fences (the ones with path/example content) so they get proper syntax highlighting and satisfy linting.projects/core/library/vault.py (2)
84-86: Consider catching specific exceptions for vault loading.Catching bare
Exceptionis flagged by static analysis. For file loading, consider catching specific exceptions likeyaml.YAMLError,FileNotFoundError, andPermissionErrorfor clearer error handling.♻️ Optional: More specific exception handling
- except Exception as e: + except (yaml.YAMLError, OSError, KeyError) as e: logger.error(f"Failed to load vault definition from {vault_file}: {e}")🤖 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 84 - 86, Replace the broad "except Exception as e" in the vault loading code with specific exception handlers so failures are classified and logged properly: catch yaml.YAMLError for YAML parsing issues, FileNotFoundError for missing vault_file, PermissionError for access problems (and optionally OSError for other IO errors), and log each with the vault_file and error details; keep a small final generic except to re-raise or log unexpected exceptions. Target the try/except block around the vault loading function (the code that reads/parses vault_file in projects/core/library/vault.py).
68-68: Use explicit| Nonetype hints for optional parameters.Static analysis flags implicit
Optionalon lines 68 and 360.♻️ Proposed fix
- def __init__(self, vault_definitions_dir: Path = None): + def __init__(self, vault_definitions_dir: Path | None = None): ... -def init(vaults: List[str] = None): +def init(vaults: list[str] | None = None):Also applies to: 360-360
🤖 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 68, Update the optional parameter type hints to explicitly include None: change the signature of Vault.__init__ (the method shown as def __init__(self, vault_definitions_dir: Path = None)) to use the new union syntax (vault_definitions_dir: Path | None = None) and likewise update the other optional parameter at the other occurrence on line 360 to use the explicit `| None` annotation; ensure imports/types remain correct and mypy/flake8 will treat these as explicit Optionals.projects/core/dsl/runtime.py (1)
27-34: Use explicit| Nonetype hints for optional parameters.The function signature uses implicit
Optionaltypes (e.g.,task_args: dict = None), which is prohibited by PEP 484 and flagged by static analysis. Use explicit union types for clarity.♻️ Proposed fix
- def __init__(self, task_name: str, task_description: str, original_exception: Exception, task_args: dict = None, task_location: str = None, artifact_dir: str = None, task_context: dict = None): + def __init__(self, task_name: str, task_description: str, original_exception: Exception, task_args: dict | None = None, task_location: str | None = None, artifact_dir: str | None = None, task_context: dict | None = None):🤖 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 27 - 34, Update the __init__ signature to use explicit union type hints for the optional parameters instead of implicit Optional defaulting to None: change task_args: dict = None to task_args: dict | None = None, task_location: str = None to task_location: str | None = None, artifact_dir: str = None to artifact_dir: str | None = None, and task_context: dict = None to task_context: dict | None = None in the __init__ method so static analyzers accept the explicit None-able types while leaving required params (task_name, task_description, original_exception) unchanged.projects/fournos_launcher/toolbox/submit_and_wait/main.py (2)
28-28: Use explicit| Nonetype hint.Static analysis flags implicit
Optional. Useenv: dict | None = Nonefor PEP 484 compliance.♻️ Proposed fix
- env: dict = None, + env: dict | None = None,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py` at line 28, The parameter type for env currently uses an implicit Optional (env: dict = None); update its annotation to an explicit union by changing it to env: dict | None = None (e.g., in the function signature where env is declared) so static analysis recognizes the Optional type per PEP 484.
90-90: Remove extraneousfprefix from string literal.The string has no placeholders, so the
fprefix is unnecessary.♻️ Proposed fix
- return f"Inputs validated" + return "Inputs validated"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py` at line 90, Remove the unnecessary f-string prefix from the return statement that currently reads return f"Inputs validated" in main.py; update that return to be a plain string literal (i.e., drop the leading "f") so the function returns a normal string without formatting overhead.
🤖 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/dsl/shell.py`:
- Line 81: Several logger.info calls use unnecessary f-string prefixes (Ruff
F541); remove the leading f from these placeholder-free messages. Locate the
logger.info calls that log literal messages like "| <stdout not logged>", "|
<stderr not logged>", "No shell available" (the calls around the reported lines)
and change them from f"... " to plain string literals "..." so there is no
f-prefix on strings with no interpolation.
In `@projects/core/dsl/template.py`:
- Around line 46-64: The template auto-detection and _get_task_context() are
looking for a local variable named 'args' but the runtime passes
'readonly_args'; update the detection logic in the template.py functions (the
loop that assigns context_var and decides caller_file/templates_dir) and in
_get_task_context() to check for both 'args' and 'readonly_args' (e.g., if
'args' in local_vars or 'readonly_args' in local_vars, use that value) so the
task frame is correctly recognized and templates_dir/caller_file are set from
the task frame.
---
Duplicate comments:
In `@projects/core/dsl/toolbox.py`:
- Around line 107-109: The code currently calls clean_args(e.task_context) which
will throw if e.task_context is None because clean_args expects a mapping;
change to pass a safe default (e.g. clean_args(e.task_context or {})) or
explicitly check if e.task_context is None and yield a placeholder like "~~
<no context available>" before dumping; also remove the unnecessary f-string on
the "~~ CONTEXT:" line (use a plain string) so the yield reads consistently and
avoid the AttributeError when TaskExecutionError.__init__ leaves task_context as
None.
In `@projects/llm_d/toolbox/capture_isvc_state/main.py`:
- Around line 14-18: The function run(...) makes namespace a keyword-only
parameter, so callers must pass it by name; update all calls to
capture_isvc_state that currently pass namespace positionally (e.g.,
capture_isvc_state(_cfg.name, _cfg.ns)) to use a keyword argument instead
(capture_isvc_state(_cfg.name, namespace=_cfg.ns)); search for calls to
capture_isvc_state (and any direct calls to run(...)) in the tests and update
them accordingly.
---
Nitpick comments:
In `@docs/orchestration/vaults.md`:
- Around line 112-126: The validation example uses vault_manager (from
get_vault_manager) but omits calling vault.init(); update the example to either
add a short comment above it noting that vault.init() must be called first or
move the entire snippet to follow the existing Initialization section;
specifically reference get_vault_manager and vault_manager.validate_vault /
validate_project_vaults / validate_all_vaults so readers know to run
vault.init() before those validation calls.
- Around line 84-91: The fenced code blocks that show path examples such as
"{env_var_value}/{filename}" and "/secrets/ci/kubeconfig" (and the other similar
path/example blocks) lack language specifiers; update each triple-backtick fence
to include a language hint (use "text") — e.g. change ``` to ```text for the
shown path example blocks and the other similar code fences (the ones with
path/example content) so they get proper syntax highlighting and satisfy
linting.
In `@projects/core/dsl/runtime.py`:
- Around line 27-34: Update the __init__ signature to use explicit union type
hints for the optional parameters instead of implicit Optional defaulting to
None: change task_args: dict = None to task_args: dict | None = None,
task_location: str = None to task_location: str | None = None, artifact_dir: str
= None to artifact_dir: str | None = None, and task_context: dict = None to
task_context: dict | None = None in the __init__ method so static analyzers
accept the explicit None-able types while leaving required params (task_name,
task_description, original_exception) unchanged.
In `@projects/core/library/ci.py`:
- Around line 110-131: The two decorators safe_ci_function and safe_ci_command
duplicate nearly identical logic; refactor by extracting a factory like
_make_safe_ci_decorator(exit_on_success: bool) that builds the inner wrapper,
uses try/except to call command_func and handle_ci_exception(e), and
conditionally calls sys.exit(0) on success or sys.exit(1) on failure according
to the exit_on_success flag; then reimplement safe_ci_function =
_make_safe_ci_decorator(exit_on_success=False) and safe_ci_command =
_make_safe_ci_decorator(exit_on_success=True), preserving function metadata
(wrapper.__name__, __doc__, __module__, __qualname__) and original behavior of
handle_ci_exception and sys.exit calls.
In `@projects/core/library/vault.py`:
- Around line 84-86: Replace the broad "except Exception as e" in the vault
loading code with specific exception handlers so failures are classified and
logged properly: catch yaml.YAMLError for YAML parsing issues, FileNotFoundError
for missing vault_file, PermissionError for access problems (and optionally
OSError for other IO errors), and log each with the vault_file and error
details; keep a small final generic except to re-raise or log unexpected
exceptions. Target the try/except block around the vault loading function (the
code that reads/parses vault_file in projects/core/library/vault.py).
- Line 68: Update the optional parameter type hints to explicitly include None:
change the signature of Vault.__init__ (the method shown as def __init__(self,
vault_definitions_dir: Path = None)) to use the new union syntax
(vault_definitions_dir: Path | None = None) and likewise update the other
optional parameter at the other occurrence on line 360 to use the explicit `|
None` annotation; ensure imports/types remain correct and mypy/flake8 will treat
these as explicit Optionals.
In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py`:
- Line 28: The parameter type for env currently uses an implicit Optional (env:
dict = None); update its annotation to an explicit union by changing it to env:
dict | None = None (e.g., in the function signature where env is declared) so
static analysis recognizes the Optional type per PEP 484.
- Line 90: Remove the unnecessary f-string prefix from the return statement that
currently reads return f"Inputs validated" in main.py; update that return to be
a plain string literal (i.e., drop the leading "f") so the function returns a
normal string without formatting overhead.
🪄 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: cbe31401-7edb-45bb-82fa-2cc4054c9f5d
📒 Files selected for processing (17)
docs/orchestration/vaults.mdprojects/core/dsl/__init__.pyprojects/core/dsl/context.pyprojects/core/dsl/runner.pyprojects/core/dsl/runtime.pyprojects/core/dsl/shell.pyprojects/core/dsl/template.pyprojects/core/dsl/toolbox.pyprojects/core/library/ci.pyprojects/core/library/vault.pyprojects/fournos_launcher/orchestration/ci.pyprojects/fournos_launcher/orchestration/config.yamlprojects/fournos_launcher/orchestration/submit.pyprojects/fournos_launcher/toolbox/submit_and_wait/main.pyprojects/llm_d/orchestration/ci.pyprojects/llm_d/toolbox/capture_isvc_state/main.pyprojects/skeleton/orchestration/ci.py
✅ Files skipped from review due to trivial changes (3)
- projects/fournos_launcher/orchestration/ci.py
- projects/core/dsl/init.py
- projects/fournos_launcher/orchestration/config.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- projects/skeleton/orchestration/ci.py
- projects/core/dsl/runner.py
- projects/llm_d/orchestration/ci.py
- projects/core/dsl/context.py
|
• Link to the test results. • No reports index generated... Test configuration: |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
projects/fournos_launcher/orchestration/submit.py (2)
22-28:⚠️ Potential issue | 🟡 MinorAdd validation for
Nonekubeconfig path.If
get_vault_content_pathreturnsNone(vault not found or content not defined),str(kubeconfig_path)will produce the literal string"None", which sets an invalidKUBECONFIGpath. This would cause downstreamoccommands to fail with confusing errors.🛡️ Proposed fix
def prepare_env(): kubeconfig_path = vault.get_vault_content_path( config.project.get_config("fournos.kubeconfig.vault.name"), config.project.get_config("fournos.kubeconfig.vault.key") ) + if kubeconfig_path is None: + raise ValueError("Failed to retrieve kubeconfig path from vault") + os.environ["KUBECONFIG"] = str(kubeconfig_path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/fournos_launcher/orchestration/submit.py` around lines 22 - 28, In prepare_env, validate the result of vault.get_vault_content_path (kubeconfig_path) before setting os.environ["KUBECONFIG"]; if kubeconfig_path is None, raise a clear exception or log an error and exit (e.g., raise ValueError("kubeconfig not found in vault") or call sys.exit(1)) instead of setting the literal "None"; update the prepare_env function to check `if kubeconfig_path is None:` and handle the failure path, otherwise set os.environ["KUBECONFIG"] = str(kubeconfig_path).
31-34:⚠️ Potential issue | 🟡 MinorGuard against
Nonevalues from config lookups.If
overridesorextra_overrideskeys returnNonefrom the config, calling.update(None)will raise aTypeError. Whileoverridesis set byfilter_out_used_overrides(),extra_overridesmight not be defined.🛡️ Proposed fix
def submit_job(): overrides = {} - overrides.update(config.project.get_config("overrides")) - overrides.update(config.project.get_config("extra_overrides")) + overrides.update(config.project.get_config("overrides") or {}) + overrides.update(config.project.get_config("extra_overrides") or {})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/fournos_launcher/orchestration/submit.py` around lines 31 - 34, The submit_job function calls overrides.update(...) with values from config.project.get_config("overrides") and get_config("extra_overrides") which may be None; change the logic so you coerce or guard those lookups to an empty dict before calling update (e.g., assign tmp = config.project.get_config("overrides") or {} and similarly for "extra_overrides", or only call overrides.update(...) if the returned value is a dict), ensuring overrides is always a dict and avoiding TypeError in submit_job.
🧹 Nitpick comments (3)
projects/llm_d/orchestration/test_llmd.py (1)
25-27: Consider removing unused variablename.The variable
nameassigned on line 25 is never used. While the comment indicates it demonstrates an alternative config query approach, leaving unused variables in the code can be confusing. Consider either removing it or using it in place of_cfg.namefor consistency.🧹 Suggested cleanup
- name = config.project.get_config("tests.llmd.flavors") - - capture_isvc_state(_cfg.name, namespace=_cfg.ns) + capture_isvc_state(_cfg.name, namespace=_cfg.ns)Or, if you want to keep the demonstration:
name = config.project.get_config("tests.llmd.flavors") - capture_isvc_state(_cfg.name, namespace=_cfg.ns) + capture_isvc_state(name, namespace=_cfg.ns) # using the alternative config query🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/llm_d/orchestration/test_llmd.py` around lines 25 - 27, The local variable name is assigned from config.project.get_config("tests.llmd.flavors") but never used; either remove the unused assignment or use it consistently (e.g., replace _cfg.name with name when calling capture_isvc_state) to eliminate the dead variable and clarify intent—update the call site capture_isvc_state(_cfg.name, namespace=_cfg.ns) and related references accordingly or drop the name assignment entirely.projects/fournos_launcher/toolbox/submit_and_wait/main.py (2)
90-90: Remove extraneousfprefix from string literal.This f-string has no placeholders, so the
fprefix is unnecessary.♻️ Proposed fix
- return f"Inputs validated" + return "Inputs validated"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py` at line 90, The return statement currently uses an unnecessary f-string: change the return from using an f-prefixed string to a plain string literal (remove the leading "f" on the "return f\"Inputs validated\"" statement) so it becomes a normal string return; locate the return in the function that contains "return f\"Inputs validated\"" and replace it with "return \"Inputs validated\"" (no other logic changes required).
28-28: Use explicitOptionaltype hint forenvparameter.PEP 484 prohibits implicit
Optional. When a parameter has a default ofNone, the type hint should explicitly indicate it can beNone.♻️ Proposed fix
+from typing import Optional + def run( cluster_name: str, project: str, *, args: list = None, variables_overrides: dict = None, job_name: str = "", namespace: str = "fournos-jobs", owner: str = "", display_name: str = "", pipeline_name: str = "", - env: dict = None, + env: Optional[dict] = None, ):Note: The same issue applies to
argsandvariables_overridesparameters on lines 21-22.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py` at line 28, The parameters that default to None must use explicit Optional types: replace env: dict = None with env: Optional[dict] = None, and do the same for args and variables_overrides (e.g., args: Optional[list] = None, variables_overrides: Optional[dict] = None) in the function signature where they are declared; also add from typing import Optional at the top of the module so the names resolve.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@projects/fournos_launcher/orchestration/submit.py`:
- Around line 22-28: In prepare_env, validate the result of
vault.get_vault_content_path (kubeconfig_path) before setting
os.environ["KUBECONFIG"]; if kubeconfig_path is None, raise a clear exception or
log an error and exit (e.g., raise ValueError("kubeconfig not found in vault")
or call sys.exit(1)) instead of setting the literal "None"; update the
prepare_env function to check `if kubeconfig_path is None:` and handle the
failure path, otherwise set os.environ["KUBECONFIG"] = str(kubeconfig_path).
- Around line 31-34: The submit_job function calls overrides.update(...) with
values from config.project.get_config("overrides") and
get_config("extra_overrides") which may be None; change the logic so you coerce
or guard those lookups to an empty dict before calling update (e.g., assign tmp
= config.project.get_config("overrides") or {} and similarly for
"extra_overrides", or only call overrides.update(...) if the returned value is a
dict), ensuring overrides is always a dict and avoiding TypeError in submit_job.
---
Nitpick comments:
In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py`:
- Line 90: The return statement currently uses an unnecessary f-string: change
the return from using an f-prefixed string to a plain string literal (remove the
leading "f" on the "return f\"Inputs validated\"" statement) so it becomes a
normal string return; locate the return in the function that contains "return
f\"Inputs validated\"" and replace it with "return \"Inputs validated\"" (no
other logic changes required).
- Line 28: The parameters that default to None must use explicit Optional types:
replace env: dict = None with env: Optional[dict] = None, and do the same for
args and variables_overrides (e.g., args: Optional[list] = None,
variables_overrides: Optional[dict] = None) in the function signature where they
are declared; also add from typing import Optional at the top of the module so
the names resolve.
In `@projects/llm_d/orchestration/test_llmd.py`:
- Around line 25-27: The local variable name is assigned from
config.project.get_config("tests.llmd.flavors") but never used; either remove
the unused assignment or use it consistently (e.g., replace _cfg.name with name
when calling capture_isvc_state) to eliminate the dead variable and clarify
intent—update the call site capture_isvc_state(_cfg.name, namespace=_cfg.ns) and
related references accordingly or drop the name assignment entirely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d446b831-29a5-4dfa-a5ca-2b06bc2244de
📒 Files selected for processing (8)
projects/core/dsl/toolbox.pyprojects/fournos_launcher/orchestration/config.yamlprojects/fournos_launcher/orchestration/submit.pyprojects/fournos_launcher/orchestration/test_llmd.pyprojects/fournos_launcher/toolbox/submit_and_wait/main.pyprojects/fournos_launcher/toolbox/submit_and_wait/templates/job.yaml.j2projects/llm_d/orchestration/test_llmd.pyprojects/llm_d/toolbox/capture_isvc_state/main.py
💤 Files with no reviewable changes (1)
- projects/fournos_launcher/orchestration/test_llmd.py
✅ Files skipped from review due to trivial changes (1)
- projects/fournos_launcher/toolbox/submit_and_wait/templates/job.yaml.j2
🚧 Files skipped from review as they are similar to previous changes (1)
- projects/llm_d/toolbox/capture_isvc_state/main.py
Contributor
Author
|
/test fournos llm_d hello world |
|
• Link to the test results. • No reports index generated... Test configuration: |
|
@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. |
Contributor
Author
|
ok, looks good for today I'll fix the |
This was referenced Apr 13, 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
New Features
Bug Fixes
Documentation
Configuration