fix: run hooks when no_poll is True #686
Conversation
WalkthroughThe updates enhance task configuration metadata in the loader by including additional descriptive and proxy-related fields. In the runner base, the logic controlling the activation of hooks is adjusted to no longer depend on the Changes
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
secator/loader.py (1)
43-43: Consider handling cases where docstring is None.While most classes should have docstrings, it's safer to handle cases where
cls.__doc__might be None.- 'description': cls.__doc__, + 'description': cls.__doc__ or '',
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
secator/loader.py(1 hunks)secator/runners/_base.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
secator/loader.py (2)
secator/template.py (1)
TemplateLoader(11-55)secator/output_types/_base.py (1)
get_name(97-98)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: integration (3.11, ubuntu-latest)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
secator/loader.py (1)
40-51: LGTM! Enhanced task metadata provides useful configuration details.The addition of task description, proxy configuration flags, and command attributes enriches the task configuration metadata. The use of
getattr()with sensible defaults (True for proxy flags) is appropriate for handling optional class attributes.
| @@ -132,7 +132,7 @@ def __init__(self, config, inputs=[], results=[], run_opts={}, hooks={}, validat | |||
| self.enable_duplicate_check = self.run_opts.get('enable_duplicate_check', True) | |||
| self.enable_profiles = self.run_opts.get('enable_profiles', True) | |||
| self.enable_reports = self.run_opts.get('enable_reports', not self.sync) and not self.dry_run and not self.no_process and not self.no_poll # noqa: E501 | |||
| self.enable_hooks = self.run_opts.get('enable_hooks', True) and not self.dry_run and not self.no_process and not self.no_poll # noqa: E501 | |||
| self.enable_hooks = self.run_opts.get('enable_hooks', True) and not self.dry_run and not self.no_process # noqa: E501 | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the logical consistency with the no_process flag.
While this change correctly removes the direct dependency on no_poll for hook enablement, there may still be an indirect dependency. When no_poll=True, the yielder() method sets no_process=True (lines 703-706), which would still disable hooks due to the and not self.no_process condition here and additional checks in the run_hooks() method (lines 769-783).
Please run the following script to verify the hook execution behavior when no_poll=True:
🏁 Script executed:
#!/bin/bash
# Description: Verify hook execution logic when no_poll is True
# Search for all places where no_process affects hook execution
echo "=== Hook enabling logic ==="
rg -A 3 -B 3 "enable_hooks.*no_process"
echo -e "\n=== Hook execution checks ==="
rg -A 5 -B 2 "no_process.*hook"
echo -e "\n=== Where no_process gets set based on no_poll ==="
rg -A 3 -B 3 "no_process.*=.*True"Length of output: 6245
Ensure hooks aren’t disabled by no_process when no_poll=True
We verified that although the direct no_poll check was removed from enable_hooks, the yielder() method still sets self.no_process = True when no_poll=True, and both the enable_hooks flag (line 135) and the run_hooks() skip logic (lines 769–783) honor no_process, effectively preventing hooks from running.
Please update the no_poll branch in secator/runners/_base.py to stop setting no_process and only disable reports:
--- a/secator/runners/_base.py
@@ 703,8 +703,7 @@ class BaseRunner:
# Runner process options
self.no_poll = self.run_opts.get('no_poll', False)
self.no_process = not self.run_opts.get('process', True)
@@ -703,8 +703,7 @@ class BaseRunner:
elif isinstance(item, str):
# …
- if self.no_poll:
- self.enable_reports = False
- self.no_process = True
- return
+ if self.no_poll:
+ self.enable_reports = False
+ returnThis will allow hooks to run under no_poll=True as intended.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.enable_hooks = self.run_opts.get('enable_hooks', True) and not self.dry_run and not self.no_process # noqa: E501 | |
| # Runner process options | |
| self.no_poll = self.run_opts.get('no_poll', False) | |
| self.no_process = not self.run_opts.get('process', True) | |
| if self.no_poll: | |
| self.enable_reports = False | |
| return |
🤖 Prompt for AI Agents
In secator/runners/_base.py at line 135, the enable_hooks flag is set to False
when no_process is True, but no_process is currently set to True in the
yielder() method when no_poll=True, which unintentionally disables hooks. To fix
this, modify the no_poll branch in the yielder() method (around lines 703-706)
to stop setting self.no_process = True and instead only disable reports. This
change will ensure that hooks are not disabled when no_poll=True, allowing them
to run as intended while still disabling reports.
🤖 I have created a release *beep* *boop* --- ## [0.16.2](v0.16.1...v0.16.2) (2025-06-06) ### Bug Fixes * run hooks when no_poll is True ([#686](#686)) ([32e8dba](32e8dba)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Added a changelog entry for version 0.16.2, documenting a bug fix related to hook execution with the `no_poll` flag. - **Chores** - Updated the project version to 0.16.2. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
New Features
Bug Fixes