Conversation
- Updated assertions in `test_generate_manifest_agents.py` to reflect the expected existence of TOML files for OpenAI agents. - Enhanced test descriptions to clarify the behavior of OpenAI manifest agents regarding TOML file generation and content. - Modified `test_schema_translation.py` to correct the expected values for the `skip` and `skip_reason` fields in the translated schema. - Added tests in `test_spec_coverage.py` to handle cases where mixed known and unknown systems are specified, ensuring proper failure responses. - Introduced a new test in `test_workspace.py` to validate rejection of unreachable local paths when adding workspaces. Signed-off-by: ainetx <viator@via-net.org>
…ved validation Signed-off-by: ainetx <viator@via-net.org>
Signed-off-by: ainetx <viator@via-net.org>
…rations - Implemented `test_cli_fs_invariants_e2e.py` to cover filesystem invariants and stale-output cleanup in the CLI. - Added tests for kit installation, updates, and resource path validation. - Created `test_cli_human_mode_e2e.py` to validate human-mode CLI interactions for agent generation and kit flows. - Included tests for agent generation with user prompts and kit updates with mixed decisions. Signed-off-by: ainetx <viator@via-net.org>
…cy project bootstrapping and cache creation Signed-off-by: ainetx <viator@via-net.org>
… workspace diagnostics Signed-off-by: ainetx <viator@via-net.org>
- Updated the CLI command routing in `cli.py` to enhance command parsing and execution flow. - Simplified context management by removing unnecessary variables in `agents.py`. - Improved error handling for workspace source paths in `workspace_add.py`, allowing unreachable paths to be added with appropriate status updates. - Enhanced JSON output formatting for various commands, ensuring consistent user feedback. - Adjusted tests to reflect changes in command behavior, including accepting unreachable paths and updating expected return codes. - Added new comments for better code clarity and maintainability. Signed-off-by: ainetx <viator@via-net.org>
…d project configuration details Signed-off-by: ainetx <viator@via-net.org>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughProject-rule documentation and CLI contracts were revised, managed root-file rewrite behavior was narrowed to explicit repair flows, agent and kit generation logic was expanded, new kit fixtures were added, and unit and e2e tests were broadened across related Studio commands. ChangesProject rules, runtime, and CLI coverage
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/fixtures/kits/example-mixed/manifest.toml (1)
1-13:⚠️ Potential issue | 🟠 MajorManifest is incomplete and resource IDs do not match test expectations.
The test at lines 1347-1356 in
tests/test_cli_example_kits_e2e.pyvalidates that the installed core kit registers resources with IDsdiscovery,review,reviewer, andplanner(each with subagents), but the manifest only defines:
[[skills]]with idlegacy-discovery(test expectsdiscovery)[[workflows]]with idlegacy-release(test expectsreview)- No
[[agents]]sections (test expectsreviewerandplannerwith subagents)The fixture must define canonical resource names and agent configurations with subagents to pass the test.
📋 Expected manifest structure
[manifest] version = "2.0" [[agents]] id = "reviewer" description = "Reviewer agent" prompt_file = "agents/reviewer.md" [[agents.subagents]] id = "reviewer-helper" source = "reviewer-helper" [[agents]] id = "planner" description = "Planner agent" prompt_file = "agents/planner.md" [[agents.subagents]] id = "planner-helper" source = "planner-helper" [[skills]] id = "discovery" description = "Discovery skill" prompt_file = "skills/discovery/SKILL.md" [[skills]] id = "review" description = "Review skill" prompt_file = "skills/review/SKILL.md"🤖 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 `@tests/fixtures/kits/example-mixed/manifest.toml` around lines 1 - 13, The manifest file contains incorrect resource IDs that do not match test expectations in test_cli_example_kits_e2e.py. Update the skills section by changing the "legacy-discovery" id to "discovery" and the workflows section by changing the "legacy-release" id to "review" with its prompt_file pointing to "skills/review/SKILL.md". Additionally, add two new agents sections before the skills section: one with id "reviewer" referencing "agents/reviewer.md" with a subagent "reviewer-helper", and another with id "planner" referencing "agents/planner.md" with a subagent "planner-helper". Each agent must have its subagents properly defined under [[agents.subagents]] with id and source fields.
🧹 Nitpick comments (10)
tests/fixtures/kits/example-mixed/artifacts/ADR/example.md (1)
1-1: 💤 Low valueConsider adding a top-level heading to conform to markdown best practices.
The file starts with a placeholder token rather than a heading. While this doesn't break functionality, markdown linting (MD041) recommends that the first line be a top-level heading (e.g.,
# Example Mixed ADR Example).🤖 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 `@tests/fixtures/kits/example-mixed/artifacts/ADR/example.md` at line 1, The file currently begins with a placeholder token `@cpt-example:cpt-example-mixed-adr-example:p1` instead of a markdown heading, which violates markdown linting rule MD041. Replace this placeholder token with a top-level heading (using # syntax) that describes the document content, such as `# Example Mixed ADR Example`, to conform to markdown best practices and enable proper linting compliance.Source: Linters/SAST tools
tests/fixtures/kits/example-mixed/artifacts/ADR/template.md (1)
1-1: 💤 Low valueConsider adding a top-level heading to conform to markdown best practices.
The file starts with a template token rather than a heading. Markdown linting (MD041) recommends the first line be a top-level heading, though this does not affect test functionality.
🤖 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 `@tests/fixtures/kits/example-mixed/artifacts/ADR/template.md` at line 1, The template.md file starts with a template token (`@cpt-template`:cpt-example-mixed-adr-template:p1) instead of a top-level markdown heading, which violates markdown linting standards (MD041). Add a heading at the very beginning of the file using the markdown heading syntax (# followed by the heading text) to conform to best practices. This heading should appear before or replace the template token line depending on the intended structure of the ADR template.Source: Linters/SAST tools
tests/test_kit.py (1)
2954-2954: ⚡ Quick winRename this test to match the new failure semantics.
The method name still says
nonblocking_warn, but assertions now enforceFAILwithrc == 2. Renaming will avoid misleading future regressions/debugging.🤖 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 `@tests/test_kit.py` at line 2954, The test method name `test_check_updates_remote_failure_is_nonblocking_warn` is misleading because the actual test assertions validate a blocking FAIL status with rc == 2, not a nonblocking warning. Rename this test method to accurately reflect the failure semantics it is testing (the method should indicate it expects a blocking failure/FAIL state with return code 2 rather than a nonblocking warning).tests/test_workspace.py (1)
2433-2444: ⚡ Quick winAssert the persisted source payload, not only call count.
Line 2443 currently checks only that
add_sourcewas called once. Add an assertion on the argument passed toadd_sourceso this test also protects the persistence contract (not just emitted JSON).Suggested test tightening
assert data["status"] == "ADDED" assert data["source"]["path"] == "../missing-docs" ws_cfg.add_source.assert_called_once() + added_source = ws_cfg.add_source.call_args.args[0] + assert added_source.path == "../missing-docs" ws_cfg.save.assert_called_once()🤖 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 `@tests/test_workspace.py` around lines 2433 - 2444, The test currently only verifies that ws_cfg.add_source was called once using assert_called_once(), but does not validate the actual argument payload passed to this method. To fix this, replace the assert_called_once() call with assert_called_once_with() and pass the expected source payload argument, or alternatively add a separate assertion that accesses the call arguments of ws_cfg.add_source to verify the persisted source object matches the expected contract (verifying properties like the path "../missing-docs" were correctly passed to the persistence layer).skills/studio/scripts/studio/commands/agents.py (4)
5539-5566: ⚡ Quick winUse a shared
.gitignorefilename constant.SonarCloud flags the repeated
".gitignore"literal. Defining a module-level constant and reusing it here and adjacent helpers is a small cleanup that clears the check.Suggested direction
+GITIGNORE_FILENAME = ".gitignore" + def _write_expected_gitignore_block( project_root: Path, expected_block: str, *, dry_run: bool, ) -> str: @@ - gitignore_path = project_root / ".gitignore" + gitignore_path = project_root / GITIGNORE_FILENAME🤖 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 5539 - 5566, The code repeatedly uses the string literal ".gitignore" which SonarCloud flags as code duplication. Define a module-level constant for the gitignore filename (e.g., GITIGNORE_FILENAME = ".gitignore") at the top of the file, then replace all occurrences of the ".gitignore" string literal in the function starting with gitignore_path and in any adjacent helper functions with this constant reference.Source: Linters/SAST tools
5461-5495: ⚡ Quick winExtract partial-reason category builders.
SonarCloud flags
_collect_partial_reasonsabove the configured complexity limit. Moving error, preserved-output, and skipped-component collection into small helpers will keep the JSON shape unchanged and clear the failing check.🤖 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 5461 - 5495, The `_collect_partial_reasons` function exceeds the configured complexity limit. Extract the logic for building error categories, preserved outputs collection, and skipped components collection into separate helper functions. Create three new helper functions to handle: collecting error messages and appending to the categories list, gathering preserved outputs from all section types, and building the skipped components list. Update the main `_collect_partial_reasons` function to call these helpers instead of containing the inline logic, maintaining the same JSON output structure in the final partial_entry dictionary.Source: Linters/SAST tools
4015-4069: ⚡ Quick winReduce
_scan_owned_generated_outputscomplexity to unblock SonarCloud.The nested provider/root/file/ownership branches exceed the configured cognitive-complexity threshold. Extracting “read file safely” and “classify owned output” helpers should keep behavior unchanged while satisfying the check.
🤖 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 4015 - 4069, The _scan_owned_generated_outputs function has excessive nested loops and conditionals that exceed SonarCloud's cognitive complexity threshold. Extract two helper functions: one to safely read file content (encapsulating the try/except OSError and UnicodeDecodeError logic), and another to classify output ownership and kind (encapsulating the if/else block containing _is_studio_managed_toml_output, _extract_studio_follow_target, _extract_studio_endpoint_target, and _is_pure_studio_generated checks). Update the main function to call these helpers while preserving the exact same behavior and logic.Source: Linters/SAST tools
4073-4136: ⚡ Quick winSplit
collect_managed_outputsinto smaller collection phases.SonarCloud flags this helper over the allowed cognitive complexity. Pull marker/configured outputs, dry-run generated outputs, and on-disk scan merging into separate helpers so the managed-output contract remains testable and the check passes.
🤖 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 4073 - 4136, The collect_managed_outputs function exceeds acceptable cognitive complexity and needs to be refactored into smaller, testable helpers. Extract the marker and configured outputs collection logic (the _INSTALL_MARKERS loop and agent skills outputs loop) into a separate helper function. Extract the dry-run generation logic (the sections processing with _process_workflows, _process_skills, _process_kit_workflow_skills, _process_subagents, and _process_kit_public_agents_and_rules calls) into another helper function. Extract the on-disk scanning logic (the _scan_owned_generated_outputs call) into a third helper. Then simplify collect_managed_outputs to call these three helpers in sequence, merge their results into the managed dictionary, and return the sorted list, maintaining the same public contract and testability.Source: Linters/SAST tools
skills/studio/scripts/studio/commands/kit.py (2)
3320-3358: ⚡ Quick winSplit kit-update partial reason collection.
SonarCloud flags this function above the configured complexity threshold. Pulling error, declined-file, prune, and action-category collection into helpers should keep the output contract unchanged and clear the check.
🤖 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/kit.py` around lines 3320 - 3358, The function _collect_kit_update_partial_reasons exceeds the complexity threshold. Extract the four main processing blocks into separate helper functions: one to collect errors from the result and update the entry and categories, one to collect declined files, one to collect prune_required items, and one to determine action categories (aborted, failed, partial). Call these helpers in sequence from _collect_kit_update_partial_reasons, passing the result and entry dictionaries and categories list as needed, ensuring the final entry structure and its categories list are constructed identically to the current implementation.Source: Linters/SAST tools
1313-1334: ⚡ Quick winReduce
_manifest_public_subagent_sourcescomplexity while preserving validation.SonarCloud flags this helper over the configured complexity threshold. Extracting subagent source normalization/validation into a small helper keeps the containment checks focused and clears the check.
🤖 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/kit.py` around lines 1313 - 1334, The _manifest_public_subagent_sources function exceeds the complexity threshold due to nested loops and multiple validation conditions. Extract the subagent source normalization and validation logic into a separate helper function that takes a subagent dictionary and returns the normalized source path if valid or None if it fails any checks. This helper should encapsulate the raw source extraction, PurePosixPath normalization, and validation checks for absolute paths and parent directory references. Then refactor _manifest_public_subagent_sources to call this helper function, leaving only the resource iteration, deduplication logic (seen set), and result accumulation in the main function.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 3850-3918: The `_cleanup_disabled_public_agent_outputs` function
currently only processes top-level public agent components but does not handle
nested subagents defined in `component.subagents`. Add logic after the existing
target loop (which handles top-level agent outputs) to iterate through any
nested subagents present in the component, and apply the same disabled target
cleanup logic to each nested subagent by checking if each nested subagent target
is disabled and deleting its generated output files accordingly.
- Around line 4966-4970: The status assignment on line 4966 uses only the
legacy_result status, which can mask failures that occurred in v2_agents or
skills generation. Instead of solely relying on legacy_result.get("status",
"PASS"), you need to check if v2_agents or skills contain errors or preserved
outputs, and if they do, set the combined status to reflect that failure state
rather than defaulting to PASS. Additionally, ensure that when such failures are
detected, the partial_reasons field is populated with the error details from
v2_agents and skills so that _build_result can report the true overall status
and include meaningful failure information.
In `@skills/studio/scripts/studio/commands/init.py`:
- Around line 370-373: The except ValueError block is creating bogus relative
`.gitignore` patterns for external kit paths by returning normalized.strip("/"),
which doesn't actually ignore external kits and can accidentally match unrelated
in-repo paths. Instead of stripping leading slashes from the normalized path
when the ValueError is caught (indicating the path is outside project_root),
return the absolute path by using resolved.as_posix() so that external kit paths
are properly represented as absolute paths in the gitignore file.
In `@skills/studio/scripts/studio/commands/kit.py`:
- Around line 3644-3674: The code currently does not account for "aborted"
update actions when determining if a command is incomplete or when showing the
"All kits are up to date" message. Add a call to _count_kit_update_actions to
count actions with "aborted" status (similar to how n_partial is counted), then
modify the command_incomplete logic to also check if there are aborted actions
(n_aborted > 0), and ensure the final message condition at the end also checks
that there are no aborted actions before showing "All kits are up to date".
- Around line 2086-2090: The code currently copies files from
extra_subagent_sources without validating that the source files actually exist
or are regular files. Before the shutil.copy2 call in the loop, add validation
to check that the source file exists using the kit_source path and that it is a
file (not a directory). If validation fails, handle the error by returning a
structured install failure or warning instead of allowing shutil.copy2 to raise
an unhandled exception that crashes the CLI. This ensures malformed kits with
missing or invalid subagent source paths are handled gracefully.
In `@skills/studio/scripts/studio/commands/spec_coverage.py`:
- Around line 89-101: The validation logic in the system_slugs check (lines
89-101) now accepts child system slugs as valid, but the scan entrypoint at line
110 only dispatches collection from top-level systems in the meta.systems
collection. Update the scanning dispatch logic to resolve and handle child
system slugs in addition to top-level systems, ensuring that when a user
provides a child slug via the --system selector, the actual file collection
properly scans the corresponding child codebase paths and returns meaningful
results instead of passing with zero files.
In `@skills/studio/scripts/studio/utils/kit_model.py`:
- Around line 1415-1457: The current merge logic uses truthy fallbacks (the "or"
pattern) for source-owned metadata fields like tools, skills, subagents, and
target_configs, which prevents source values from intentionally clearing or
resetting these fields. For fields that belong to the source resource (tools,
disallowed_tools, skills, subagents, target_configs), remove the fallback to
core_resource values and use only the source_resource values directly, even if
they are empty or None. Additionally, for the fields with conditional merge
logic (mode, isolation, role, target, provider), reconsider the logic to
distinguish between "not explicitly set" and "intentionally set to a default
value" so that explicit source settings are not overridden by core values.
In `@tests/test_cli_fs_invariants_e2e.py`:
- Around line 32-37: The _run_main function calls main() with JSON mode enabled
but does not restore the JSON mode state afterward, which can cause the global
state in studio.utils.ui to remain modified and affect subsequent tests. Save
the initial JSON mode setting before calling main(), then restore it after the
call completes (ideally using a try/finally block or context manager pattern) to
ensure test isolation and prevent test-order coupling.
In `@tests/test_cli_gitignore_e2e.py`:
- Around line 36-41: The _run_main helper function is leaving JSON mode in a
mutated state after the main(argv) call completes, causing order-dependent test
failures across test invocations. Modify _run_main to capture the JSON mode
state before calling main(argv), and then restore that original state after the
main() call completes and before returning. This ensures each test invocation of
_run_main does not affect the global JSON mode setting for subsequent tests.
In `@tests/test_cli_kit_utility_e2e.py`:
- Around line 43-53: The _run_main function calls main(argv) which can modify
global UI mode state (e.g., when --json flag is passed), but this global state
is not restored after the function returns, causing subsequent tests to see a
modified global state and become flaky. Capture the original UI mode state
before calling main() within the _run_main function, then restore it in a
finally block to ensure the global state is always reset after each test call
regardless of whether an exception occurs.
In `@tests/test_cli_validation_e2e.py`:
- Around line 54-68: The _run_main function saves and restores JSON mode state
but does not reset it to a clean state at the beginning of execution. This
causes test isolation issues where if a prior test leaves JSON mode enabled,
subsequent tests that do not use the --json flag will still run in JSON mode,
making output assertions flaky. After saving the current JSON mode state with
saved_json_mode = is_json_mode(), immediately call set_json_mode(False) before
the os.chdir(cwd) call to ensure each test execution starts with JSON mode
disabled, then restore the original state in the finally block.
---
Outside diff comments:
In `@tests/fixtures/kits/example-mixed/manifest.toml`:
- Around line 1-13: The manifest file contains incorrect resource IDs that do
not match test expectations in test_cli_example_kits_e2e.py. Update the skills
section by changing the "legacy-discovery" id to "discovery" and the workflows
section by changing the "legacy-release" id to "review" with its prompt_file
pointing to "skills/review/SKILL.md". Additionally, add two new agents sections
before the skills section: one with id "reviewer" referencing
"agents/reviewer.md" with a subagent "reviewer-helper", and another with id
"planner" referencing "agents/planner.md" with a subagent "planner-helper". Each
agent must have its subagents properly defined under [[agents.subagents]] with
id and source fields.
---
Nitpick comments:
In `@skills/studio/scripts/studio/commands/agents.py`:
- Around line 5539-5566: The code repeatedly uses the string literal
".gitignore" which SonarCloud flags as code duplication. Define a module-level
constant for the gitignore filename (e.g., GITIGNORE_FILENAME = ".gitignore") at
the top of the file, then replace all occurrences of the ".gitignore" string
literal in the function starting with gitignore_path and in any adjacent helper
functions with this constant reference.
- Around line 5461-5495: The `_collect_partial_reasons` function exceeds the
configured complexity limit. Extract the logic for building error categories,
preserved outputs collection, and skipped components collection into separate
helper functions. Create three new helper functions to handle: collecting error
messages and appending to the categories list, gathering preserved outputs from
all section types, and building the skipped components list. Update the main
`_collect_partial_reasons` function to call these helpers instead of containing
the inline logic, maintaining the same JSON output structure in the final
partial_entry dictionary.
- Around line 4015-4069: The _scan_owned_generated_outputs function has
excessive nested loops and conditionals that exceed SonarCloud's cognitive
complexity threshold. Extract two helper functions: one to safely read file
content (encapsulating the try/except OSError and UnicodeDecodeError logic), and
another to classify output ownership and kind (encapsulating the if/else block
containing _is_studio_managed_toml_output, _extract_studio_follow_target,
_extract_studio_endpoint_target, and _is_pure_studio_generated checks). Update
the main function to call these helpers while preserving the exact same behavior
and logic.
- Around line 4073-4136: The collect_managed_outputs function exceeds acceptable
cognitive complexity and needs to be refactored into smaller, testable helpers.
Extract the marker and configured outputs collection logic (the _INSTALL_MARKERS
loop and agent skills outputs loop) into a separate helper function. Extract the
dry-run generation logic (the sections processing with _process_workflows,
_process_skills, _process_kit_workflow_skills, _process_subagents, and
_process_kit_public_agents_and_rules calls) into another helper function.
Extract the on-disk scanning logic (the _scan_owned_generated_outputs call) into
a third helper. Then simplify collect_managed_outputs to call these three
helpers in sequence, merge their results into the managed dictionary, and return
the sorted list, maintaining the same public contract and testability.
In `@skills/studio/scripts/studio/commands/kit.py`:
- Around line 3320-3358: The function _collect_kit_update_partial_reasons
exceeds the complexity threshold. Extract the four main processing blocks into
separate helper functions: one to collect errors from the result and update the
entry and categories, one to collect declined files, one to collect
prune_required items, and one to determine action categories (aborted, failed,
partial). Call these helpers in sequence from
_collect_kit_update_partial_reasons, passing the result and entry dictionaries
and categories list as needed, ensuring the final entry structure and its
categories list are constructed identically to the current implementation.
- Around line 1313-1334: The _manifest_public_subagent_sources function exceeds
the complexity threshold due to nested loops and multiple validation conditions.
Extract the subagent source normalization and validation logic into a separate
helper function that takes a subagent dictionary and returns the normalized
source path if valid or None if it fails any checks. This helper should
encapsulate the raw source extraction, PurePosixPath normalization, and
validation checks for absolute paths and parent directory references. Then
refactor _manifest_public_subagent_sources to call this helper function, leaving
only the resource iteration, deduplication logic (seen set), and result
accumulation in the main function.
In `@tests/fixtures/kits/example-mixed/artifacts/ADR/example.md`:
- Line 1: The file currently begins with a placeholder token
`@cpt-example:cpt-example-mixed-adr-example:p1` instead of a markdown heading,
which violates markdown linting rule MD041. Replace this placeholder token with
a top-level heading (using # syntax) that describes the document content, such
as `# Example Mixed ADR Example`, to conform to markdown best practices and
enable proper linting compliance.
In `@tests/fixtures/kits/example-mixed/artifacts/ADR/template.md`:
- Line 1: The template.md file starts with a template token
(`@cpt-template`:cpt-example-mixed-adr-template:p1) instead of a top-level
markdown heading, which violates markdown linting standards (MD041). Add a
heading at the very beginning of the file using the markdown heading syntax (#
followed by the heading text) to conform to best practices. This heading should
appear before or replace the template token line depending on the intended
structure of the ADR template.
In `@tests/test_kit.py`:
- Line 2954: The test method name
`test_check_updates_remote_failure_is_nonblocking_warn` is misleading because
the actual test assertions validate a blocking FAIL status with rc == 2, not a
nonblocking warning. Rename this test method to accurately reflect the failure
semantics it is testing (the method should indicate it expects a blocking
failure/FAIL state with return code 2 rather than a nonblocking warning).
In `@tests/test_workspace.py`:
- Around line 2433-2444: The test currently only verifies that ws_cfg.add_source
was called once using assert_called_once(), but does not validate the actual
argument payload passed to this method. To fix this, replace the
assert_called_once() call with assert_called_once_with() and pass the expected
source payload argument, or alternatively add a separate assertion that accesses
the call arguments of ws_cfg.add_source to verify the persisted source object
matches the expected contract (verifying properties like the path
"../missing-docs" were correctly passed to the persistence layer).
🪄 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: 5937cb7c-39b1-45be-9b31-29d994bec454
📒 Files selected for processing (81)
README.mdarchitecture/DESIGN.mdarchitecture/features/agent-integration.mdarchitecture/features/core-infra.mdarchitecture/researches/contracts-doc-gaps-2026-06.mdarchitecture/specs/artifacts-registry.mdarchitecture/specs/cli.mdarchitecture/specs/kit/kit.mdarchitecture/specs/sysprompts.mdguides/AGENT-TOOLS.mdguides/CONFIGURATION.mdguides/USAGE-GUIDE.mdskills/studio/scripts/studio/cli.pyskills/studio/scripts/studio/commands/__init__.pyskills/studio/scripts/studio/commands/agents.pyskills/studio/scripts/studio/commands/init.pyskills/studio/scripts/studio/commands/kit.pyskills/studio/scripts/studio/commands/spec_coverage.pyskills/studio/scripts/studio/commands/workspace_info.pyskills/studio/scripts/studio/utils/context.pyskills/studio/scripts/studio/utils/kit_model.pyskills/studio/studio.clispecsrc/studio_proxy/cli.pytests/fixtures/kits/example-legacy/agents.tomltests/fixtures/kits/example-legacy/agents/planner.mdtests/fixtures/kits/example-legacy/agents/reviewer.mdtests/fixtures/kits/example-legacy/artifacts/FEATURE/example.mdtests/fixtures/kits/example-legacy/artifacts/FEATURE/template.mdtests/fixtures/kits/example-legacy/conf.tomltests/fixtures/kits/example-legacy/constraints.tomltests/fixtures/kits/example-legacy/core.tomltests/fixtures/kits/example-legacy/manifest.tomltests/fixtures/kits/example-legacy/skills/guide.mdtests/fixtures/kits/example-legacy/workflows/release.mdtests/fixtures/kits/example-mixed/.cf-studio-kit.tomltests/fixtures/kits/example-mixed/agents/planner-helper.mdtests/fixtures/kits/example-mixed/agents/planner.mdtests/fixtures/kits/example-mixed/agents/reviewer-helper.mdtests/fixtures/kits/example-mixed/agents/reviewer.mdtests/fixtures/kits/example-mixed/artifacts/ADR/example.mdtests/fixtures/kits/example-mixed/artifacts/ADR/template.mdtests/fixtures/kits/example-mixed/conf.tomltests/fixtures/kits/example-mixed/constraints.tomltests/fixtures/kits/example-mixed/core.tomltests/fixtures/kits/example-mixed/manifest.tomltests/fixtures/kits/example-mixed/skills/discovery/SKILL.mdtests/fixtures/kits/example-mixed/skills/review/SKILL.mdtests/fixtures/kits/example-v2/.cf-studio-kit.tomltests/fixtures/kits/example-v2/agents/planner-helper.mdtests/fixtures/kits/example-v2/agents/planner.mdtests/fixtures/kits/example-v2/agents/reviewer-helper.mdtests/fixtures/kits/example-v2/agents/reviewer.mdtests/fixtures/kits/example-v2/artifacts/FEATURE/example.mdtests/fixtures/kits/example-v2/artifacts/FEATURE/template.mdtests/fixtures/kits/example-v2/artifacts/PRD/example.mdtests/fixtures/kits/example-v2/artifacts/PRD/template.mdtests/fixtures/kits/example-v2/constraints.tomltests/fixtures/kits/example-v2/skills/discovery/SKILL.mdtests/fixtures/kits/example-v2/skills/review/SKILL.mdtests/test_agents_coverage.pytests/test_cli_agents_e2e.pytests/test_cli_artifact_tools_e2e.pytests/test_cli_contracts_e2e.pytests/test_cli_example_kits_e2e.pytests/test_cli_fs_invariants_e2e.pytests/test_cli_gitignore_e2e.pytests/test_cli_human_mode_e2e.pytests/test_cli_kit_utility_e2e.pytests/test_cli_map_public_e2e.pytests/test_cli_navigation_e2e.pytests/test_cli_router_e2e.pytests/test_cli_setup_e2e.pytests/test_cli_update_e2e.pytests/test_cli_validate_e2e.pytests/test_cli_validation_e2e.pytests/test_cli_workspace_diag_e2e.pytests/test_generate_manifest_agents.pytests/test_kit.pytests/test_schema_translation.pytests/test_spec_coverage.pytests/test_workspace.py
- Updated `_normalize_ignored_kit_path` to return the resolved path directly when the project path cannot be normalized. - Refactored `_manifest_public_subagent_sources` to improve readability and modularity by introducing `_normalize_manifest_public_subagent_source`. - Enhanced error handling in `install_kit_with_manifest` to check for missing subagent sources and ensure proper file existence before copying. - Improved the `_collect_kit_update_partial_reasons` function by breaking it down into smaller helper functions for better clarity and maintainability. - Modified the `cmd_kit_update` function to account for aborted updates in the output structure. - Updated the `spec_coverage` command to collect system files based on selected system slugs, including child nodes. - Simplified the merging logic in `_merge_core_authoritative_model` by directly using source fields where applicable. - Adjusted test cases to cover new functionality and ensure proper validation of agent and kit behaviors. - Added tests for handling missing subagent sources during kit installation and for verifying system filter functionality in spec coverage. Signed-off-by: ainetx <viator@via-net.org>
…handling Signed-off-by: ainetx <viator@via-net.org>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@tests/test_cli_fs_invariants_e2e.py`:
- Around line 95-110: The global JSON mode cleanup via set_json_mode(False) at
the end of the test only executes if all assertions pass before it, leaving the
global state dirty if an assertion fails. Refactor the test to guarantee cleanup
happens regardless of assertion outcomes by either wrapping the test logic in a
try/finally block with set_json_mode(False) in the finally clause, or by calling
self.addCleanup(set_json_mode, False) immediately after the initial
set_json_mode(True) call at the beginning of the test. This ensures the global
JSON mode is always reset to False even if an earlier assertion fails.
In `@tests/test_cli_validation_e2e.py`:
- Around line 76-91: The cleanup call to set_json_mode(False) at the end of the
test will not execute if any preceding assertion fails, which causes JSON mode
state to leak into subsequent tests and creates order-dependent test failures.
Wrap the entire test body from the initial set_json_mode(True) call through all
the assertions in a try-finally block to ensure that set_json_mode(False) is
always executed unconditionally, regardless of whether the test passes or fails.
In `@tests/test_init_update_footprint.py`:
- Around line 113-121: The test
test_normalize_ignored_kit_path_preserves_absolute_external_path uses a
POSIX-specific absolute path "/opt/external-kits/demo" which is not recognized
as absolute on Windows, causing the _normalize_ignored_kit_path function to
incorrectly treat it as relative and making the test fail for the wrong reason.
Replace the POSIX path with a Windows-compatible absolute path such as
"C:/external-kits/demo" or use platform-specific path handling
(PureWindowsPath/PurePosixPath) to ensure the test properly validates that
absolute external paths are preserved on all operating systems.
In `@tests/test_kit_manifest_install.py`:
- Around line 706-713: The issue is that install_kit_with_manifest copies
regular resources before validating that subagent sources exist, causing partial
installs even when installation fails. Refactor install_kit_with_manifest in
skills/studio/scripts/studio/commands/kit.py to perform all subagent source
existence checks before any file copying operations begin. Additionally, add a
no-write assertion in this test case to verify that the installed kit directory
remains unmodified when installation fails due to missing subagent helper files,
ensuring the failure path does not leave partial writes on the filesystem.
🪄 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: da199252-0d3a-4cba-86c2-5bd797893bb3
📒 Files selected for processing (18)
skills/studio/scripts/studio/commands/agents.pyskills/studio/scripts/studio/commands/init.pyskills/studio/scripts/studio/commands/kit.pyskills/studio/scripts/studio/commands/spec_coverage.pyskills/studio/scripts/studio/utils/kit_model.pytests/fixtures/kits/example-mixed/artifacts/ADR/example.mdtests/fixtures/kits/example-mixed/artifacts/ADR/template.mdtests/fixtures/kits/example-mixed/manifest.tomltests/test_agents_coverage.pytests/test_cli_fs_invariants_e2e.pytests/test_cli_gitignore_e2e.pytests/test_cli_kit_utility_e2e.pytests/test_cli_validation_e2e.pytests/test_init_update_footprint.pytests/test_kit.pytests/test_kit_manifest_install.pytests/test_spec_coverage.pytests/test_workspace.py
✅ Files skipped from review due to trivial changes (3)
- tests/fixtures/kits/example-mixed/artifacts/ADR/template.md
- tests/fixtures/kits/example-mixed/manifest.toml
- tests/fixtures/kits/example-mixed/artifacts/ADR/example.md
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/test_workspace.py
- skills/studio/scripts/studio/commands/init.py
- skills/studio/scripts/studio/commands/spec_coverage.py
- tests/test_cli_gitignore_e2e.py
- skills/studio/scripts/studio/utils/kit_model.py
- tests/test_cli_kit_utility_e2e.py
- skills/studio/scripts/studio/commands/kit.py
Comment on lines
+95
to
+110
| with TemporaryDirectory() as td: | ||
| set_json_mode(True) | ||
|
|
||
| def _fake_main(_argv): | ||
| set_json_mode(False) | ||
| print('{"status":"PASS"}') | ||
| return 0 | ||
|
|
||
| with patch(f"{__name__}.main", side_effect=_fake_main): | ||
| rc, out, _stderr = _run_main(["doctor"], cwd=Path(td)) | ||
|
|
||
| self.assertEqual(rc, 0) | ||
| self.assertEqual(out["status"], "PASS") | ||
| self.assertTrue(is_json_mode()) | ||
| set_json_mode(False) | ||
|
|
There was a problem hiding this comment.
Guard JSON-mode cleanup in the test with finally/addCleanup.
Line 109 only resets global JSON mode on the success path. If an assertion fails before that line, later tests can inherit stale global state.
Suggested change
def test_run_main_restores_json_mode(self):
with TemporaryDirectory() as td:
set_json_mode(True)
+ self.addCleanup(set_json_mode, False)
def _fake_main(_argv):
set_json_mode(False)
print('{"status":"PASS"}')
return 0
@@
self.assertEqual(rc, 0)
self.assertEqual(out["status"], "PASS")
self.assertTrue(is_json_mode())
- set_json_mode(False)🤖 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 `@tests/test_cli_fs_invariants_e2e.py` around lines 95 - 110, The global JSON
mode cleanup via set_json_mode(False) at the end of the test only executes if
all assertions pass before it, leaving the global state dirty if an assertion
fails. Refactor the test to guarantee cleanup happens regardless of assertion
outcomes by either wrapping the test logic in a try/finally block with
set_json_mode(False) in the finally clause, or by calling
self.addCleanup(set_json_mode, False) immediately after the initial
set_json_mode(True) call at the beginning of the test. This ensures the global
JSON mode is always reset to False even if an earlier assertion fails.
Comment on lines
+76
to
+91
| with TemporaryDirectory() as td: | ||
| set_json_mode(True) | ||
|
|
||
| def _fake_main(_argv): | ||
| from studio.utils.ui import is_json_mode, set_json_mode as _set_json_mode | ||
|
|
||
| print(json.dumps({"json_mode": is_json_mode()})) | ||
| _set_json_mode(True) | ||
| return 0 | ||
|
|
||
| with patch.object(cli, "main", side_effect=_fake_main): | ||
| rc, stdout, _stderr = _run_main(["--json", "validate"], cwd=Path(td)) | ||
|
|
||
| self.assertEqual(rc, 0) | ||
| self.assertFalse(json.loads(stdout)["json_mode"]) | ||
| set_json_mode(False) |
There was a problem hiding this comment.
Make JSON-mode cleanup unconditional in this test.
set_json_mode(False) runs only after assertions. If an assertion fails, JSON mode can leak into later tests and cause order-dependent failures.
Proposed fix
def test_run_main_starts_with_json_mode_disabled(self):
- from studio.utils.ui import set_json_mode
+ from studio.utils.ui import is_json_mode, set_json_mode
with TemporaryDirectory() as td:
- set_json_mode(True)
+ prior_json_mode = is_json_mode()
+ set_json_mode(True)
+ self.addCleanup(set_json_mode, prior_json_mode)
@@
self.assertEqual(rc, 0)
self.assertFalse(json.loads(stdout)["json_mode"])
- set_json_mode(False)🧰 Tools
🪛 ast-grep (0.43.0)
[info] 81-81: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"json_mode": is_json_mode()})
Note: Security best practice.
(use-jsonify)
🤖 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 `@tests/test_cli_validation_e2e.py` around lines 76 - 91, The cleanup call to
set_json_mode(False) at the end of the test will not execute if any preceding
assertion fails, which causes JSON mode state to leak into subsequent tests and
creates order-dependent test failures. Wrap the entire test body from the
initial set_json_mode(True) call through all the assertions in a try-finally
block to ensure that set_json_mode(False) is always executed unconditionally,
regardless of whether the test passes or fails.
Comment on lines
+113
to
+121
| def test_normalize_ignored_kit_path_preserves_absolute_external_path(): | ||
| project_root = Path("/tmp/project") | ||
| studio_root = project_root / ".bootstrap" | ||
| normalized = _normalize_ignored_kit_path( | ||
| project_root, | ||
| studio_root, | ||
| "/opt/external-kits/demo", | ||
| ) | ||
| assert normalized == "/opt/external-kits/demo" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify pathlib absolute-path semantics that affect this test.
python - <<'PY'
from pathlib import PureWindowsPath, PurePosixPath
print("PurePosixPath('/opt/external-kits/demo').is_absolute() ->",
PurePosixPath('/opt/external-kits/demo').is_absolute())
print("PureWindowsPath('/opt/external-kits/demo').is_absolute() ->",
PureWindowsPath('/opt/external-kits/demo').is_absolute())
print("PureWindowsPath('C:/external-kits/demo').is_absolute() ->",
PureWindowsPath('C:/external-kits/demo').is_absolute())
PYRepository: constructorfabric/studio
Length of output: 260
🏁 Script executed:
# Find the test file and examine the test function
cat -n tests/test_init_update_footprint.py | sed -n '100,140p'Repository: constructorfabric/studio
Length of output: 1595
🏁 Script executed:
# Find where _normalize_ignored_kit_path is defined
fd -t f "\.py$" | xargs rg -l "_normalize_ignored_kit_path" | head -5Repository: constructorfabric/studio
Length of output: 150
🏁 Script executed:
# Find and read the _normalize_ignored_kit_path function
rg -A 20 "def _normalize_ignored_kit_path" skills/studio/scripts/studio/commands/init.pyRepository: constructorfabric/studio
Length of output: 757
🏁 Script executed:
# Check imports at top of init.py to understand what Path is being used
head -30 skills/studio/scripts/studio/commands/init.pyRepository: constructorfabric/studio
Length of output: 1511
🏁 Script executed:
# Get the complete _normalize_ignored_kit_path function including the except clause
rg -A 30 "def _normalize_ignored_kit_path" skills/studio/scripts/studio/commands/init.pyRepository: constructorfabric/studio
Length of output: 1095
Make this absolute-path test OS-agnostic.
This assertion is POSIX-specific. On Windows, Path("/opt/external-kits/demo").is_absolute() returns False, so the function treats it as relative and the test fails for the wrong reason. Use a Windows-appropriate absolute path like "C:/external-kits/demo" (or use PureWindowsPath/PurePosixPath semantics) to ensure the test correctly validates that absolute external paths are preserved regardless of platform.
🧰 Tools
🪛 ast-grep (0.43.0)
[info] 113-113: Do not hardcode temporary file or directory names
Context: "/tmp/project"
Note: [CWE-377].
(hardcoded-tmp-file)
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 114-114: Make sure publicly writable directories are used safely here.
🪛 Ruff (0.15.17)
[error] 114-114: Probable insecure usage of temporary file or directory: "/tmp/project"
(S108)
🤖 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 `@tests/test_init_update_footprint.py` around lines 113 - 121, The test
test_normalize_ignored_kit_path_preserves_absolute_external_path uses a
POSIX-specific absolute path "/opt/external-kits/demo" which is not recognized
as absolute on Windows, causing the _normalize_ignored_kit_path function to
incorrectly treat it as relative and making the test fail for the wrong reason.
Replace the POSIX path with a Windows-compatible absolute path such as
"C:/external-kits/demo" or use platform-specific path handling
(PureWindowsPath/PurePosixPath) to ensure the test properly validates that
absolute external paths are preserved on all operating systems.
…nd improve error reporting feat: add sandbox_mode to agent TOML validation and processing feat: implement validation for manifest public subagent sources in kit installation feat: add workspace upgrade error handling in context management test: add tests for unknown system selector handling in spec coverage test: enhance workspace info tests for metadata errors and context upgrade issues Signed-off-by: ainetx <viator@via-net.org>
…mands Signed-off-by: ainetx <viator@via-net.org>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/kit.py`:
- Around line 2149-2159: The validation for public subagent sources using
_validate_manifest_public_subagent_sources is occurring too late in the function
execution flow, after the code has already branched on install_mode. Move the
subagent_source_error validation block and its error handling to execute before
any install_mode-based branching (before the code path that returns early for
register-mode). This ensures the validation runs consistently across all install
modes, preventing malformed subagents[].source values from passing validation in
register mode while failing in copy mode.
🪄 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: 2d95bcb3-c84c-4174-8132-ce66aeb12e29
📒 Files selected for processing (14)
architecture/specs/cli.mdskills/studio/scripts/studio/commands/agents.pyskills/studio/scripts/studio/commands/kit.pyskills/studio/scripts/studio/commands/spec_coverage.pyskills/studio/scripts/studio/commands/workspace_info.pyskills/studio/scripts/studio/utils/context.pyskills/studio/scripts/studio/utils/diff_engine.pyskills/studio/scripts/studio/utils/manifest.pytests/test_cli_example_kits_e2e.pytests/test_cli_validation_e2e.pytests/test_cli_workspace_diag_e2e.pytests/test_diff_engine.pytests/test_generate_manifest_agents.pytests/test_kit_manifest_install.py
🚧 Files skipped from review as they are similar to previous changes (8)
- skills/studio/scripts/studio/commands/spec_coverage.py
- tests/test_kit_manifest_install.py
- tests/test_cli_validation_e2e.py
- architecture/specs/cli.md
- skills/studio/scripts/studio/commands/workspace_info.py
- skills/studio/scripts/studio/utils/context.py
- tests/test_cli_workspace_diag_e2e.py
- skills/studio/scripts/studio/commands/agents.py
…llation Signed-off-by: ainetx <viator@via-net.org>
|
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.
Refactor tests for manifest generation and schema translation, improve validation in CLI utility and router tests, and add end-to-end tests for CLI filesystem invariants and human mode operations. Update error handling for workspace source paths and enhance documentation for project configuration details.
Summary by CodeRabbit
.cf-studio-kit.toml) with richer install/update and partial-outcome reporting.workspace-info..codex/.cf-installed)..gitignoreoutput for ignored vs host-visible paths.AGENTS.mdmanaged metadata is refreshed only during explicit setup/repair/update flows (not ordinary read-only commands).generate-agents,kit update, andkit check-updatesnow follow clearerPARTIAL/warning and failure contracts (including nonzero exit codes on failures).--cf-studio-root(legacy aliases supported);spec-coverage --systemvalidates selectors (unknown systems fail).