[core] Improve the toolbox & dsl#29
Merged
kpouget merged 3 commits intoopenshift-psap:mainfrom Apr 16, 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 |
📝 WalkthroughWalkthroughThis change refactors the DSL task management system from relying on global mutable state to using a centralized Changes
Sequence DiagramsequenceDiagram
participant TaskDec as Task Decorator
participant ScriptMgr as ScriptManager
participant Runtime as Runtime Executor
participant Shell as Shell Executor
participant Logger as DSL Logger
TaskDec->>ScriptMgr: register_task(task_info, source_file)
ScriptMgr->>ScriptMgr: Store task in _task_registry
ScriptMgr->>ScriptMgr: Create TaskResult for task
TaskDec->>TaskDec: Assign result container to wrapper.status
Runtime->>ScriptMgr: get_tasks_from_file(rel_filename)
ScriptMgr-->>Runtime: Return list of tasks for file
Runtime->>Runtime: Execute normal phase tasks
Runtime->>Shell: Execute each task
Shell->>Shell: Run subprocess with capture_output=True
Shell->>Logger: Log output if enabled
Shell-->>Runtime: Return execution result
Runtime->>ScriptMgr: get_task_result(task_name)
ScriptMgr-->>Runtime: Return TaskResult
Runtime->>ScriptMgr: _set_result(result)
Runtime->>Runtime: Execute always phase tasks
Runtime->>Shell: Execute each always task
Shell-->>Runtime: Return execution result
Runtime->>ScriptMgr: _set_result(result)
Runtime->>ScriptMgr: clear_tasks(file_path)
ScriptMgr->>ScriptMgr: Clear tasks and results for file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
projects/core/dsl/task.py (1)
180-184:⚠️ Potential issue | 🔴 CriticalRegister tasks with the same file key that runtime uses.
task()stores tasks underos.path.relpath(definition_filename), butexecute_tasks()looks them up withPath(filename).relative_to(env.FORGE_HOME)inprojects/core/dsl/runtime.pyLines 69-73. When the process CWD is notFORGE_HOME, those keys diverge and Line 105 inprojects/core/dsl/runtime.pyraisesNo tasks found for file ....📍 Suggested normalization
- # Make filename relative to current working directory - try: - rel_definition_filename = os.path.relpath(definition_filename) - except ValueError: - rel_definition_filename = definition_filename + # Make filename relative to FORGE_HOME to match runtime lookup + try: + rel_definition_filename = str(Path(definition_filename).relative_to(env.FORGE_HOME)) + except ValueError: + rel_definition_filename = definition_filenameAlso add
from pathlib import Pathnear the imports.Also applies to: 223-224
🤖 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 180 - 184, task registration uses os.path.relpath(definition_filename) which doesn't match execute_tasks() lookup (Path(filename).relative_to(env.FORGE_HOME)), causing missing-task errors; change task() to normalize the key using Path(definition_filename).relative_to(env.FORGE_HOME) (wrapped in try/except to fall back to Path(definition_filename).resolve().relative_to(Path.cwd()) or the original string if relative_to fails) and convert to str before storing, and make the same change for the second occurrence around lines 223-224; also add "from pathlib import Path" to the imports so Path is available.projects/core/dsl/shell.py (1)
27-34:⚠️ Potential issue | 🟠 MajorDon't force every command through in-memory capture.
Hardcoding
capture_output=Truemeans every command now buffers stdout/stderr in RAM, including thestdout_destcase where the payload is written to disk afterward. That removes the previous escape hatch for large or streaming commands and can turn artifact captures into a memory spike.♻️ Suggested fix
def run( command: str, check: bool = True, + capture_output: bool = True, shell: bool = True, stdout_dest: Optional[Union[str, Path]] = None, log_stdout: bool = True, log_stderr: bool = True, ) -> CommandResult: @@ result = subprocess.run( command, shell=shell, check=False, # We handle check ourselves - capture_output=True, + capture_output=capture_output, text=True ) + + stdout = result.stdout or "" + stderr = result.stderr or "" cmd_result = CommandResult( - stdout=result.stdout or "", - stderr=result.stderr or "", + stdout=stdout, + stderr=stderr, returncode=result.returncode, command=command ) @@ - if result.stdout and result.stdout.strip(): + if stdout.strip(): if stdout_dest: logger.info(f"| <stdout saved into {stdout_dest}>") elif log_stdout: - logger.info(f"| <stdout> {result.stdout.strip()}") + logger.info(f"| <stdout> {stdout.strip()}") else: logger.info("| <stdout logging skipped>") - if result.stderr and result.stderr.strip(): + if stderr.strip(): if log_stderr: - logger.info(f"| <stderr> {result.stderr.strip()}") + logger.info(f"| <stderr> {stderr.strip()}") else: logger.info("| <stderr logging skipped>") - if not (result.stdout.strip() or result.stderr.strip()): + if not (stdout.strip() or stderr.strip()): logger.info("| <no output>")If
stdout_destis the main large-output path, an even better follow-up is to stream directly to the file instead of capturing first.Also applies to: 53-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/dsl/shell.py` around lines 27 - 34, The run function currently forces capture_output=True which buffers all stdout/stderr in memory; change it to decide how to wire subprocess stdout/stderr based on the parameters: if stdout_dest is provided, open that file and pass the file handle as stdout to subprocess (and do not set capture_output), otherwise if log_stdout or log_stderr is true use subprocess.PIPE to capture those streams; ensure stderr is similarly routed (PIPE or file/None) rather than always capturing both. Update the logic in run (and the corresponding block around lines 53-90) to set stdout/stderr args dynamically and close any opened file handles; consider recommending a streaming path (directly writing to stdout_dest) as a follow-up for very large outputs.
🤖 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/runtime.py`:
- Around line 100-114: The first execution loop should skip tasks marked with
always_execute and only run non-always tasks: in the loop that pops from
file_tasks (using get_script_manager().get_tasks_from_file and variable
current_task_info), if current_task_info.always_execute is True then defer it
(e.g., append to a separate always_tasks list) instead of executing it; call
_execute_single_task(current_task_info, args, shared_context) only for
non-always tasks. On failure, the recovery loop must iterate over that
always_tasks list (not all remaining file_tasks) and call
_execute_single_task(task_info, args, shared_context) for each, ensuring the
loop variable is task_info (not the undefined name `task`) so no NameError
occurs.
In `@projects/core/dsl/script_manager.py`:
- Around line 41-43: The _task_results dict currently keys only by task_name
causing collisions across scripts; change the keying to a file-scoped composite
key (e.g. f"{source_file}:{task_name}" or a tuple (source_file, task_name))
wherever TaskResult entries are created, accessed, or deleted: update the
registration logic that populates self._task_results (refer to _task_registry
and the method that registers tasks), update lookups that read task
results/status (wherever _task_results[task_name] is used), and update
clear_tasks(source_file) to remove only entries whose composite key matches the
given source_file; ensure TaskResult creation, updates, and any
status.return_value references use the new composite key consistently to avoid
cross-file replacement.
---
Outside diff comments:
In `@projects/core/dsl/shell.py`:
- Around line 27-34: The run function currently forces capture_output=True which
buffers all stdout/stderr in memory; change it to decide how to wire subprocess
stdout/stderr based on the parameters: if stdout_dest is provided, open that
file and pass the file handle as stdout to subprocess (and do not set
capture_output), otherwise if log_stdout or log_stderr is true use
subprocess.PIPE to capture those streams; ensure stderr is similarly routed
(PIPE or file/None) rather than always capturing both. Update the logic in run
(and the corresponding block around lines 53-90) to set stdout/stderr args
dynamically and close any opened file handles; consider recommending a streaming
path (directly writing to stdout_dest) as a follow-up for very large outputs.
In `@projects/core/dsl/task.py`:
- Around line 180-184: task registration uses
os.path.relpath(definition_filename) which doesn't match execute_tasks() lookup
(Path(filename).relative_to(env.FORGE_HOME)), causing missing-task errors;
change task() to normalize the key using
Path(definition_filename).relative_to(env.FORGE_HOME) (wrapped in try/except to
fall back to Path(definition_filename).resolve().relative_to(Path.cwd()) or the
original string if relative_to fails) and convert to str before storing, and
make the same change for the second occurrence around lines 223-224; also add
"from pathlib import Path" to the imports so Path is available.
🪄 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: 5bf2d60a-9767-484a-b5a3-0cfbca2ebe84
📒 Files selected for processing (8)
projects/core/dsl/__init__.pyprojects/core/dsl/log.pyprojects/core/dsl/runner.pyprojects/core/dsl/runtime.pyprojects/core/dsl/script_manager.pyprojects/core/dsl/shell.pyprojects/core/dsl/task.pyprojects/core/dsl/toolbox.py
Comment on lines
+100
to
+114
| # Execute tasks only from the calling file | ||
| script_manager = get_script_manager() | ||
| file_tasks = list(script_manager.get_tasks_from_file(rel_filename)) | ||
|
|
||
| if not file_tasks: | ||
| logger.error(f"No tasks found for file: {rel_filename}") | ||
| log_completion_banner(function_args, status="NO_TASKS") | ||
| raise RuntimeError(f"No tasks found for file: {rel_filename}") | ||
|
|
||
| execution_error = None | ||
|
|
||
| # Execute normal tasks | ||
| try: | ||
| for task_info in normal_tasks: | ||
| _execute_single_task(task_info, args, shared_context) | ||
| while file_tasks: | ||
| current_task_info = file_tasks.pop(0) | ||
| _execute_single_task(current_task_info, args, shared_context) |
There was a problem hiding this comment.
@always handling is broken in both execution phases.
The first loop never checks always_execute, so @always tasks run like normal tasks. After a failure, the second loop then runs every remaining task as if it were @always, and Line 139 calls _execute_single_task(task_info, ...) with an undefined name. A normal task failure will either execute non-@always tasks unexpectedly or crash with NameError.
🛠️ Suggested structure
- file_tasks = list(script_manager.get_tasks_from_file(rel_filename))
-
- if not file_tasks:
+ registered_tasks = list(script_manager.get_tasks_from_file(rel_filename))
+
+ if not registered_tasks:
logger.error(f"No tasks found for file: {rel_filename}")
log_completion_banner(function_args, status="NO_TASKS")
raise RuntimeError(f"No tasks found for file: {rel_filename}")
+
+ file_tasks = [task for task in registered_tasks if not task.get('always_execute')]
+ always_tasks = [task for task in registered_tasks if task.get('always_execute')]
@@
- if file_tasks:
- logging.warning("Executing the `@always` tasks ...")
- while file_tasks:
+ if always_tasks:
+ logger.warning("Executing the `@always` tasks ...")
+ while always_tasks:
try:
- current_task_info = file_tasks.pop(0)
- _execute_single_task(task_info, args, shared_context)
+ current_task_info = always_tasks.pop(0)
+ _execute_single_task(current_task_info, args, shared_context)Also applies to: 133-140
🤖 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 100 - 114, The first execution
loop should skip tasks marked with always_execute and only run non-always tasks:
in the loop that pops from file_tasks (using
get_script_manager().get_tasks_from_file and variable current_task_info), if
current_task_info.always_execute is True then defer it (e.g., append to a
separate always_tasks list) instead of executing it; call
_execute_single_task(current_task_info, args, shared_context) only for
non-always tasks. On failure, the recovery loop must iterate over that
always_tasks list (not all remaining file_tasks) and call
_execute_single_task(task_info, args, shared_context) for each, ensuring the
loop variable is task_info (not the undefined name `task`) so no NameError
occurs.
Comment on lines
+41
to
+43
| self._task_registry: Dict[str, List[dict]] = {} | ||
| # Task results organized by task name | ||
| self._task_results: Dict[str, TaskResult] = {} |
There was a problem hiding this comment.
Task results need a file-scoped key, not just task_name.
_task_results[task_name] collides as soon as two scripts define the same task function name. The later registration replaces the earlier TaskResult, so some_task.status.return_value can point at the wrong execution, and clear_tasks(source_file) can delete another file's status object.
Also applies to: 45-60, 81-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/core/dsl/script_manager.py` around lines 41 - 43, The _task_results
dict currently keys only by task_name causing collisions across scripts; change
the keying to a file-scoped composite key (e.g. f"{source_file}:{task_name}" or
a tuple (source_file, task_name)) wherever TaskResult entries are created,
accessed, or deleted: update the registration logic that populates
self._task_results (refer to _task_registry and the method that registers
tasks), update lookups that read task results/status (wherever
_task_results[task_name] is used), and update clear_tasks(source_file) to remove
only entries whose composite key matches the given source_file; ensure
TaskResult creation, updates, and any status.return_value references use the new
composite key consistently to avoid cross-file replacement.
Contributor
Author
|
merging this before working on |
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
Release Notes
Bug Fixes
Refactor