Conversation
📝 WalkthroughWalkthroughRemoves all legacy ChangesCopilot Legacy Removal, Dynamic .gitignore, and Kit Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…s for generated agent resources Signed-off-by: ainetx <viator@via-net.org>
…anced resource metadata Signed-off-by: ainetx <viator@via-net.org>
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
skills/studio/scripts/studio/commands/agents.py (2)
3795-3834: ⚖️ Poor tradeoffCognitive complexity exceeds threshold; consider decomposition.
SonarCloud flags complexity of 25 (allowed 15). The function performs multiple distinct tasks that could be extracted:
- Collecting install marker paths
- Collecting static skill output paths from config
- Running processors and aggregating their results
Extracting these into focused helpers would improve testability and maintainability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/studio/scripts/studio/commands/agents.py` around lines 3795 - 3834, The function list_managed_agent_output_paths has exceeded the cognitive complexity threshold of 15 with a complexity of 25. Refactor this function by extracting its three main concerns into separate helper functions: create one helper to collect install marker paths from _INSTALL_MARKERS, create another to collect static skill output paths from the config (the logic handling agent_cfg, skills_cfg, outputs, and rel_path extraction), and create a third helper to execute all the processors (_process_workflows, _process_skills, _process_kit_workflow_skills, _process_subagents, and _process_kit_public_agents_and_rules) and aggregate their managed paths. Then simplify list_managed_agent_output_paths to call these three helpers in sequence and return the sorted result, significantly reducing its cognitive complexity.Source: Linters/SAST tools
3758-3792: ⚖️ Poor tradeoffConsider extracting helpers to reduce cognitive complexity.
SonarCloud flags cognitive complexity of 31 (allowed 15). The nested loops for key iteration, value iteration, and path normalization can be simplified by extracting a small
_normalize_path_candidatehelper.♻️ Sketch of possible refactor
def _normalize_path_candidate(candidate: Any, project_root: Path) -> Optional[str]: """Normalize a single path candidate to project-relative string.""" if not isinstance(candidate, str) or not candidate.strip(): return None candidate_path = Path(candidate) if candidate_path.is_absolute(): return _safe_relpath(candidate_path, project_root) return candidate.replace("\\", "/").strip("/") or None def _collect_managed_result_paths( project_root: Path, section: Dict[str, Any], ) -> Set[str]: """Collect normalized project-relative output paths from a generator result.""" paths: Set[str] = set() # Collect from status keys for key in ("created", "updated", "unchanged", "deleted"): for value in section.get(key, []) or []: raw = value[-1] if isinstance(value, tuple) and value else value if normalized := _normalize_path_candidate(raw, project_root): paths.add(normalized) # Collect from outputs for output in section.get("outputs", []) or []: if isinstance(output, dict): if normalized := _normalize_path_candidate(output.get("path"), project_root): paths.add(normalized) return paths🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/studio/scripts/studio/commands/agents.py` around lines 3758 - 3792, The _collect_managed_result_paths function has high cognitive complexity due to nested loops and duplicated path normalization logic. Extract the path normalization logic that currently appears twice (once in the key iteration loop and once in the outputs loop) into a separate helper function _normalize_path_candidate that takes a candidate value and project_root parameter and returns Optional[str]. This helper should handle the string type checking, stripping, absolute path resolution, and relative path normalization. Then simplify both the key loop and outputs loop by calling this new helper instead of repeating the normalization code inline.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/studio/scripts/studio/commands/agents.py`:
- Around line 3768-3780: The code accesses value[-1] without checking if the
tuple is empty, which will raise an IndexError if an empty tuple is passed as a
value. Add a guard condition in the isinstance(value, tuple) check to ensure the
tuple is not empty before attempting to access its last element with value[-1].
Update the condition to verify both that value is a tuple and that it contains
at least one element before proceeding to access value[-1].
---
Nitpick comments:
In `@skills/studio/scripts/studio/commands/agents.py`:
- Around line 3795-3834: The function list_managed_agent_output_paths has
exceeded the cognitive complexity threshold of 15 with a complexity of 25.
Refactor this function by extracting its three main concerns into separate
helper functions: create one helper to collect install marker paths from
_INSTALL_MARKERS, create another to collect static skill output paths from the
config (the logic handling agent_cfg, skills_cfg, outputs, and rel_path
extraction), and create a third helper to execute all the processors
(_process_workflows, _process_skills, _process_kit_workflow_skills,
_process_subagents, and _process_kit_public_agents_and_rules) and aggregate
their managed paths. Then simplify list_managed_agent_output_paths to call these
three helpers in sequence and return the sorted result, significantly reducing
its cognitive complexity.
- Around line 3758-3792: The _collect_managed_result_paths function has high
cognitive complexity due to nested loops and duplicated path normalization
logic. Extract the path normalization logic that currently appears twice (once
in the key iteration loop and once in the outputs loop) into a separate helper
function _normalize_path_candidate that takes a candidate value and project_root
parameter and returns Optional[str]. This helper should handle the string type
checking, stripping, absolute path resolution, and relative path normalization.
Then simplify both the key loop and outputs loop by calling this new helper
instead of repeating the normalization code inline.
🪄 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: c0187146-dd2f-49dc-bd10-fe4c4910d538
📒 Files selected for processing (10)
.bootstrap/config/core.tomlskills/studio/scripts/studio/commands/agents.pyskills/studio/scripts/studio/commands/init.pyskills/studio/scripts/studio/commands/kit.pytests/test_agents_coverage.pytests/test_cli_integration.pytests/test_init_update_footprint.pytests/test_kit.pytests/test_kit_manifest_install.pytests/test_update.py
💤 Files with no reviewable changes (1)
- tests/test_cli_integration.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/test_init_update_footprint.py
- tests/test_kit_manifest_install.py
- tests/test_update.py
- tests/test_agents_coverage.py
- skills/studio/scripts/studio/commands/init.py
- tests/test_kit.py
- skills/studio/scripts/studio/commands/kit.py
Comment on lines
+3768
to
+3780
| for value in values: | ||
| if isinstance(value, tuple): | ||
| candidates = [value[-1]] | ||
| else: | ||
| candidates = [value] | ||
| for candidate in candidates: | ||
| if not isinstance(candidate, str) or not candidate.strip(): | ||
| continue | ||
| candidate_path = Path(candidate) | ||
| if candidate_path.is_absolute(): | ||
| paths.add(_safe_relpath(candidate_path, project_root)) | ||
| else: | ||
| paths.add(candidate.replace("\\", "/").strip("/")) |
There was a problem hiding this comment.
Guard against empty tuples to prevent IndexError.
If an empty tuple is passed as a value, value[-1] at line 3770 will raise an IndexError. While generators likely don't produce empty tuples, adding a guard improves robustness.
🛡️ Proposed fix
for value in values:
- if isinstance(value, tuple):
+ if isinstance(value, tuple) and value:
candidates = [value[-1]]
else:
candidates = [value]📝 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.
Suggested change
| for value in values: | |
| if isinstance(value, tuple): | |
| candidates = [value[-1]] | |
| else: | |
| candidates = [value] | |
| for candidate in candidates: | |
| if not isinstance(candidate, str) or not candidate.strip(): | |
| continue | |
| candidate_path = Path(candidate) | |
| if candidate_path.is_absolute(): | |
| paths.add(_safe_relpath(candidate_path, project_root)) | |
| else: | |
| paths.add(candidate.replace("\\", "/").strip("/")) | |
| for value in values: | |
| if isinstance(value, tuple) and value: | |
| candidates = [value[-1]] | |
| else: | |
| candidates = [value] | |
| for candidate in candidates: | |
| if not isinstance(candidate, str) or not candidate.strip(): | |
| continue | |
| candidate_path = Path(candidate) | |
| if candidate_path.is_absolute(): | |
| paths.add(_safe_relpath(candidate_path, project_root)) | |
| else: | |
| paths.add(candidate.replace("\\", "/").strip("/")) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/studio/scripts/studio/commands/agents.py` around lines 3768 - 3780,
The code accesses value[-1] without checking if the tuple is empty, which will
raise an IndexError if an empty tuple is passed as a value. Add a guard
condition in the isinstance(value, tuple) check to ensure the tuple is not empty
before attempting to access its last element with value[-1]. Update the
condition to verify both that value is a tuple and that it contains at least one
element before proceeding to access value[-1].
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.
Replace wildcard patterns in the gitignore with explicit file paths for better clarity and maintenance. This change enhances the management of generated agent resources.
Summary by CodeRabbit
Refactor
Bug Fixes