Refactor kit model loading and enhance workflow handling#50
Merged
Conversation
- Introduced a new function `load_installed_kit_model` to handle loading of installed kits with improved source precedence. - Updated various command scripts to replace `load_kit_model` with `load_installed_kit_model` for consistency and to support new functionality. - Added `_public_workflow_ids_from_resource` to extract public workflow IDs from resources, enhancing workflow management. - Modified `_kit_model_to_info` to include workflows from both public components and resources. - Updated tests to validate new behavior, including handling of public components and workflows in various scenarios. - Enhanced error handling and reporting for kit model loading and workflow extraction. Signed-off-by: ainetx <viator@via-net.org>
📝 WalkthroughWalkthroughIntroduces ChangesInstalled Kit Model, Multi-Kit Normalize, and Deprecated Workflow Exposure
Sequence Diagram(s)sequenceDiagram
participant CLI as cfs kit normalize
participant cmd_kit_normalize
participant load_canonical_models
participant load_installed_kit_model
participant _merge_core_authoritative_model
participant kit_models_to_toml_data
CLI->>cmd_kit_normalize: --kit slug1 --kit slug2
cmd_kit_normalize->>load_canonical_models: kit source path
load_canonical_models-->>cmd_kit_normalize: [KitModel, KitModel, ...]
cmd_kit_normalize->>cmd_kit_normalize: filter by --kit slugs, validate unknowns
cmd_kit_normalize->>cmd_kit_normalize: block subset write without --output/--dry-run/--stdout
loop for each selected model
cmd_kit_normalize->>load_installed_kit_model: kit_source, core_kit, kit_slug
load_installed_kit_model->>_merge_core_authoritative_model: core model + source model
_merge_core_authoritative_model-->>load_installed_kit_model: merged KitModel
load_installed_kit_model-->>cmd_kit_normalize: merged KitModel
end
cmd_kit_normalize->>kit_models_to_toml_data: [merged KitModel, ...]
kit_models_to_toml_data-->>cmd_kit_normalize: canonical TOML dict
cmd_kit_normalize-->>CLI: JSON result {kits, kits_normalized, kit?}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
skills/studio/scripts/studio/utils/kit_model.py (1)
1437-1499: ⚡ Quick winExtract resource serialization to clear the static-analysis failure.
kit_models_to_toml_datais now mostly a per-resource field mapper; moving that block into a_resource_to_toml_item(resource)helper should reduce the reported cognitive complexity without changing output.🤖 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/utils/kit_model.py` around lines 1437 - 1499, The kit_models_to_toml_data function has high cognitive complexity due to the large block of conditional field mappings for each resource. Extract the resource serialization logic (the inner loop that processes each resource and all the conditional checks for resource attributes like public, description, aliases, etc.) into a new helper function named _resource_to_toml_item that takes a resource parameter and returns the constructed dictionary item. Then replace the extracted code block with a call to this new helper function while keeping the outer loop structure in kit_models_to_toml_data intact. This reduces nesting and complexity without changing the output behavior.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 `@architecture/features/kit-management.md`:
- Around line 190-194: The documentation for the kit normalize CLI flow is
missing the `--stdout` option that is actually supported by the implementation.
Add `--stdout` as an alternative output option to the trigger command line
(where it shows `[--output <path>]`) and also include it in step 1 where the
parse-args description mentions `[--output <path>]`. The `--stdout` option
should be documented as a mutually exclusive alternative to `--output` for
directing normalization results to standard output instead of a file.
In `@skills/studio/scripts/studio/commands/adapter_info.py`:
- Around line 451-454: The boolean parsing for public using
bool(binding.get("public", False)) incorrectly treats non-empty strings like
"false" as truthy, causing non-public resources to be incorrectly included in
the workflows set. Replace this with strict boolean parsing that properly
handles string representations (e.g., checking if the value equals "true"
case-insensitively or is already a boolean True). Additionally, normalize the
kind casing by converting it to lowercase before checking against the set
{"skill", "workflow"} to avoid case-sensitive misses when comparing the kind
value.
In `@skills/studio/scripts/studio/utils/kit_model.py`:
- Line 1404: The artifact_bindings assignment at line 1404 uses a simple `or`
operator which causes it to discard all bindings from either core_resource or
source_resource instead of merging them. Replace this simple fallback logic with
code that properly merges artifact_bindings from both core_resource and
source_resource, combining bindings per artifact kind so that disjoint artifact
kinds from both sources are preserved in the final merged result.
---
Nitpick comments:
In `@skills/studio/scripts/studio/utils/kit_model.py`:
- Around line 1437-1499: The kit_models_to_toml_data function has high cognitive
complexity due to the large block of conditional field mappings for each
resource. Extract the resource serialization logic (the inner loop that
processes each resource and all the conditional checks for resource attributes
like public, description, aliases, etc.) into a new helper function named
_resource_to_toml_item that takes a resource parameter and returns the
constructed dictionary item. Then replace the extracted code block with a call
to this new helper function while keeping the outer loop structure in
kit_models_to_toml_data intact. This reduces nesting and complexity without
changing the output behavior.
🪄 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: 3201a70b-be7c-4174-b22c-a4695a07704e
📒 Files selected for processing (10)
architecture/features/kit-management.mdskills/studio/scripts/studio/commands/adapter_info.pyskills/studio/scripts/studio/commands/agents.pyskills/studio/scripts/studio/commands/kit.pyskills/studio/scripts/studio/commands/resolve_vars.pyskills/studio/scripts/studio/utils/kit_model.pytests/test_adapter_info.pytests/test_agents_coverage.pytests/test_kit.pytests/test_kit_manifest_validate.py
Comment on lines
+190
to
194
| **Trigger**: User runs `cfs kit normalize <path> [--from manifest|layout|core] [--kit <slug>|--kit all] [--output <path>] [--dry-run]` | ||
|
|
||
| **Steps**: | ||
| 1. [x] - `p1` - Parse CLI arguments (path, --from, --output, --dry-run) - `inst-normalize-parse-args` | ||
| 1. [x] - `p1` - Parse CLI arguments (path, --from, repeated/comma-separated `--kit`, --output, --dry-run) - `inst-normalize-parse-args` | ||
| 2. [x] - `p1` - Validate source directory or installed resource binding source exists - `inst-normalize-validate-source` |
There was a problem hiding this comment.
Normalize CLI flow docs omit the supported --stdout option.
The trigger and parse-args step no longer document --stdout, but the command implementation supports it. This creates user-facing inconsistency.
Suggested doc fix
-**Trigger**: User runs `cfs kit normalize <path> [--from manifest|layout|core] [--kit <slug>|--kit all] [--output <path>] [--dry-run]`
+**Trigger**: User runs `cfs kit normalize <path> [--from manifest|layout|core] [--kit <slug>|--kit all] [--output <path>] [--dry-run] [--stdout]`
-1. [x] - `p1` - Parse CLI arguments (path, --from, repeated/comma-separated `--kit`, --output, --dry-run) - `inst-normalize-parse-args`
+1. [x] - `p1` - Parse CLI arguments (path, --from, repeated/comma-separated `--kit`, --output, --dry-run, --stdout) - `inst-normalize-parse-args`🤖 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 `@architecture/features/kit-management.md` around lines 190 - 194, The
documentation for the kit normalize CLI flow is missing the `--stdout` option
that is actually supported by the implementation. Add `--stdout` as an
alternative output option to the trigger command line (where it shows `[--output
<path>]`) and also include it in step 1 where the parse-args description
mentions `[--output <path>]`. The `--stdout` option should be documented as a
mutually exclusive alternative to `--output` for directing normalization results
to standard output instead of a file.
Comment on lines
+451
to
+454
| kind = str(binding.get("kind", "") or "") | ||
| public = bool(binding.get("public", False)) | ||
| if public and kind in {"skill", "workflow"}: | ||
| workflows.add(str(resource_id)) |
There was a problem hiding this comment.
Use strict boolean parsing for public in legacy fallback workflow derivation.
The current coercion treats non-empty strings (for example "false") as truthy, which can incorrectly include non-public resource IDs in deprecated workflows output. Also normalize kind casing to avoid case-sensitive misses.
Proposed patch
- kind = str(binding.get("kind", "") or "")
- public = bool(binding.get("public", False))
- if public and kind in {"skill", "workflow"}:
+ kind = str(binding.get("kind", "") or "").strip().lower()
+ public_value = binding.get("public", False)
+ public = public_value if isinstance(public_value, bool) else False
+ if public and kind in {"skill", "workflow"}:
workflows.add(str(resource_id))🤖 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/adapter_info.py` around lines 451 -
454, The boolean parsing for public using bool(binding.get("public", False))
incorrectly treats non-empty strings like "false" as truthy, causing non-public
resources to be incorrectly included in the workflows set. Replace this with
strict boolean parsing that properly handles string representations (e.g.,
checking if the value equals "true" case-insensitively or is already a boolean
True). Additionally, normalize the kind casing by converting it to lowercase
before checking against the set {"skill", "workflow"} to avoid case-sensitive
misses when comparing the kind value.
|
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.
load_installed_kit_modelto handle loading of installed kits with improved source precedence.load_kit_modelwithload_installed_kit_modelfor consistency and to support new functionality._public_workflow_ids_from_resourceto extract public workflow IDs from resources, enhancing workflow management._kit_model_to_infoto include workflows from both public components and resources.Summary by CodeRabbit
Release Notes
New Features
--kitselector option tocfs kit normalizecommand for choosing specific kits from multi-kit manifests; use--kit allto normalize all kits.Bug Fixes
cfs infooutput to properly expose deprecated workflows and skill resources in kit details.