refactor: reduce cognitive complexity across 13 modules - #486
Conversation
Decompose 28 functions exceeding SonarQube's Cognitive Complexity threshold (S3776, max 15) using Extract Method pattern. Key changes: - update_check.rs: extract skip/fetch/notify helpers - config.rs: extract gitignore entry collectors - commands/status.rs: extract child validation helpers - commands/doctor.rs: extract per-check diagnostic functions - commands/skill.rs: extract suggest/install renderers - main.rs: extract subcommand handlers - linker.rs: extract path validators, fence detection, glob matching, clean per-type, symlink handling (6 issues) - mcp.rs: extract config resolution and write helpers - init.rs: extract scan helpers and wizard phases (2 issues) - skills/detect.rs: extract rule evaluators, metadata collectors (6 issues) - skills/suggest.rs: extract render sections - skills/update.rs: extract update phases - skills/install.rs: extract fetch/unpack archive helpers All 675 tests pass. No behavior changes.
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR refactors diagnostic, synchronization, initialization, skill, command, and update-check workflows into focused private helpers. Existing behavior remains unchanged except that additional ChangesWorkflow refactor
Estimated code review effort: 5 (Critical) | ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)❌ Error creating Unit Test PR.
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![semgrep-code-dallay[bot]](https://avatars.githubusercontent.com/u/47160086?s=60&v=4){kind=small}
| { | ||
| // Preserve unrelated top-level settings when overwriting for certain formatters | ||
| let existing = fs::read_to_string(&config_path).with_context(|| { | ||
| let existing = fs::read_to_string(config_path).with_context(|| { |
There was a problem hiding this comment.
Semgrep identified a blocking 🔴 issue in your code:
The application builds a file path from potentially untrusted data, which can lead to a path traversal vulnerability. An attacker can manipulate the path which the application uses to access files. If the application does not validate user input and sanitize file paths, sensitive files such as configuration or user data can be accessed, potentially creating or overwriting files. To prevent this vulnerability, validate and sanitize any input that is used to create references to file paths. Also, enforce strict file access controls. For example, choose privileges allowing public-facing applications to access only the required files.
Dataflow graph
flowchart LR
classDef invis fill:white, stroke: none
classDef default fill:#e7f5ff, color:#1c7fd6, stroke: none
subgraph File0["<b>src/mcp.rs</b>"]
direction LR
%% Source
subgraph Source
direction LR
v0["<a href=https://github.com/dallay/agentsync/blob/377ef4a9c6b81050beb4a551349b6b0d4101d8b4/src/mcp.rs#L1272 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 1272] config_path</a>"]
end
%% Intermediate
%% Sink
subgraph Sink
direction LR
v1["<a href=https://github.com/dallay/agentsync/blob/377ef4a9c6b81050beb4a551349b6b0d4101d8b4/src/mcp.rs#L1272 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 1272] config_path</a>"]
end
end
%% Class Assignment
Source:::invis
Sink:::invis
File0:::invis
%% Connections
Source --> Sink
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by tainted-path.
You can view more details about this finding in the Semgrep AppSec Platform.
| let existing = fs::read_to_string(&config_path).with_context(|| { | ||
| ) -> Result<(String, Option<String>)> { | ||
| if config_path.exists() && self.merge_strategy == McpMergeStrategy::Merge { | ||
| let existing = fs::read_to_string(config_path).with_context(|| { |
There was a problem hiding this comment.
Semgrep identified a blocking 🔴 issue in your code:
The application builds a file path from potentially untrusted data, which can lead to a path traversal vulnerability. An attacker can manipulate the path which the application uses to access files. If the application does not validate user input and sanitize file paths, sensitive files such as configuration or user data can be accessed, potentially creating or overwriting files. To prevent this vulnerability, validate and sanitize any input that is used to create references to file paths. Also, enforce strict file access controls. For example, choose privileges allowing public-facing applications to access only the required files.
Why this might be safe to ignore:
The matched read uses a config_path parameter in internal config-generation logic, not an Actix web handler or request-derived input as the rule is meant to detect. This looks like a taint-tracking mismatch rather than a user-controlled path traversal that an attacker can reach.
Dataflow graph
flowchart LR
classDef invis fill:white, stroke: none
classDef default fill:#e7f5ff, color:#1c7fd6, stroke: none
subgraph File0["<b>src/mcp.rs</b>"]
direction LR
%% Source
subgraph Source
direction LR
v0["<a href=https://github.com/dallay/agentsync/blob/377ef4a9c6b81050beb4a551349b6b0d4101d8b4/src/mcp.rs#L1248 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 1248] config_path</a>"]
end
%% Intermediate
%% Sink
subgraph Sink
direction LR
v1["<a href=https://github.com/dallay/agentsync/blob/377ef4a9c6b81050beb4a551349b6b0d4101d8b4/src/mcp.rs#L1248 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 1248] config_path</a>"]
end
end
%% Class Assignment
Source:::invis
Sink:::invis
File0:::invis
%% Connections
Source --> Sink
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by tainted-path.
You can view more details about this finding in the Semgrep AppSec Platform.
| if !src_path.exists() || !src_path.is_dir() { | ||
| return Ok((0, 0)); | ||
| } | ||
| for entry in fs::read_dir(src_path)? { |
There was a problem hiding this comment.
The application builds a file path from potentially untrusted data, which can lead to a path traversal vulnerability. An attacker can manipulate the path which the application uses to access files. If the application does not validate user input and sanitize file paths, sensitive files such as configuration or user data can be accessed, potentially creating or overwriting files. To prevent this vulnerability, validate and sanitize any input that is used to create references to file paths. Also, enforce strict file access controls. For example, choose privileges allowing public-facing applications to access only the required files.
⭐ Fixed in commit 23d1774 ⭐
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 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 `@src/commands/doctor.rs`:
- Around line 178-254: Clarify the issue-count contract between check_gitignore
and run_doctor so every detected .gitignore problem contributes correctly to the
doctor summary rather than being capped or undercounted. Preserve intentional
--no-changes behavior and add or update tests covering the reported summary
count. In check_gitignore, reuse the existing start_marker/end_marker results
instead of parsing those markers twice.
In `@src/commands/skill.rs`:
- Around line 815-938: Unify the install flow by having the Json arm use
resolve_install_mode_and_ids and the shared reporter-based install driver
instead of duplicating mode selection. Extract the
blocking_fetch_and_install_skill callback into a reusable function, and
consolidate the human line/live paths around one install driver that accepts a
reporter, preserving HumanLive’s finalize-before-return behavior.
In `@src/init.rs`:
- Around line 1681-1703: In copy_entries_to_dest, consolidate the identical
success println! and migrated increment from the directory and file branches
into one shared block after the conditional copy operation. Keep copy_dir_all
for directories and fs::copy for files, preserving the existing output and
migration count behavior.
In `@src/linker.rs`:
- Around line 1729-1736: Add focused tests around try_match_segment covering
multiple-** backtracking with a pattern like **/foo/**/bar, and a trailing **
that successfully matches zero segments. Verify both matching outcomes and
iterator/pattern advancement behavior without changing the implementation.
- Around line 1358-1374: Replace direct fs::remove_file calls in
clean_symlink_contents_target, clean_nested_glob_target, and
clean_module_map_target with the existing remove_symlink helper, preserving each
helper’s validation, dry-run, cache invalidation, counters, and output behavior.
In `@src/main.rs`:
- Around line 478-487: Replace the positional parameters of handle_apply with a
single ApplyArgs argument, remove the #[allow(clippy::too_many_arguments)]
attribute, and define or reuse ApplyArgs consistently with the flattened
Commands::Status argument pattern. Update the Commands::Apply handling call site
to construct ApplyArgs from its destructured fields, preserving all existing
values and behavior.
In `@src/skills/install.rs`:
- Around line 334-351: Update fetch_remote_data to return or propagate the
temporary download path instead of reading download.tmp into a Vec<u8>, while
preserving cleanup and error handling. Change the unpacking flow to open that
path with std::fs::File and pass the file to unpack_zip and unpack_tar_gz,
satisfying their Read + Seek and Read requirements respectively. Also enforce
the intended maximum response/download size while streaming untrusted URLs.
- Around line 251-286: Replace the empty-data sentinel in the archive-fetch flow
with an explicit FetchedSource variant: update fetch_local_data and
fetch_remote_data to return DirectoryCopied for copied directories and Archive {
.. } for archive bytes, then match on that result in the caller instead of
checking data.is_empty(). Preserve directory-copy completion while routing
zero-byte or truncated archives through normal format validation and rejection.
- Around line 394-434: Update zip_common_root to require root followed by a path
separator or an exact entry match, rather than using raw starts_with. Update
zip_entry_rel_path to remove subpath only when it matches a complete path
component, preserving entries such as “docs2” when filtering for “docs”; align
both checks with tar_common_root’s component-aware behavior.
- Around line 436-514: Update unpack_tar_gz to avoid collecting Archive::entries
into a Vec or reusing a consumed stream. Buffer the decompressed or input
archive bytes, create two fresh Archive instances, use the first pass to compute
tar_common_root, then use the second pass to filter entries and call unpack
while preserving the existing path-safety and subpath handling.
In `@src/skills/suggest.rs`:
- Around line 711-721: Resolve the unused format_installed_status helper by
choosing one consistent rendering path: either extend SuggestJsonRecommendation
with installed_version and update render_recommendations_section/render_human to
call format_installed_status, preserving version-aware labels, or remove
format_installed_status and its #[allow(dead_code)] suppression if versions
should not appear in human output.
In `@src/skills/update.rs`:
- Around line 28-30: Update the result handling in the surrounding update
function to destructure resolve_update_source(update_source).await? into the
local directory and TempDir guard in one binding. Retain a named unused guard
rather than `_`, and add a concise comment explaining that it must remain alive
so the temporary source directory is not deleted before later remote-update
reads complete.
- Around line 74-79: Remove the full registry read and contents logging from the
update registry check after the existing registry_path.exists() debug call. If
diagnostic logging is needed, use the existing registry entry handling later in
the update flow to log only the skill ID and version, never the registry body or
sensitive source field.
- Around line 164-202: Extract the repeated filesystem rollback sequence into a
shared helper near the update flow, taking skill_dir and backup_dir as inputs
and removing skill_dir before restoring backup_dir when present. Replace the
duplicated blocks in the parse_skill_manifest error branch and
update_registry_entry error branch with calls to this helper, while preserving
the registry-entry restoration in the latter branch.
In `@src/update_check.rs`:
- Around line 106-150: Update check_and_notify so a failure from
cache.save(&new_cache) does not return before the notification. Treat cache
persistence as best-effort, ignore or separately handle its result, and always
execute the existing eprintln! after a newer version has been validated.
- Around line 83-104: Update the reqwest Client builder in fetch_latest_version
to configure an identifying User-Agent, using the crate name and version, before
the existing timeout and request flow. Preserve the current optional
error-handling behavior and response deserialization.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7109c0f5-9e2b-4402-8318-c508133b79a4
📒 Files selected for processing (13)
src/commands/doctor.rssrc/commands/skill.rssrc/commands/status.rssrc/config.rssrc/init.rssrc/linker.rssrc/main.rssrc/mcp.rssrc/skills/detect.rssrc/skills/install.rssrc/skills/suggest.rssrc/skills/update.rssrc/update_check.rs
| for entry in fs::read_dir(&dest) | ||
| .with_context(|| format!("Failed to read destination directory: {}", dest.display()))? | ||
| { | ||
| let entry = | ||
| entry.with_context(|| format!("Failed to read entry in: {}", dest.display()))?; | ||
| if entry.path().is_symlink() { | ||
| if options.dry_run { | ||
| println!(" {} Would remove: {}", "→".cyan(), entry.path().display()); | ||
| } else { | ||
| self.revalidate_unlink_path(&entry.path())?; | ||
| fs::remove_file(entry.path())?; | ||
| self.invalidate_path_cache(); | ||
| println!(" {} Removed: {}", "✔".green(), entry.path().display()); | ||
| } | ||
| result.removed += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the remove_symlink helper and all symlink removal call sites.
set -euo pipefail
ast-grep run --pattern 'fn remove_symlink($$$) { $$$ }' --lang rust src
rg -n -C 3 'remove_symlink\s*\(|fs::remove_file\s*\(' --glob '*.rs' -g '!target/**'Repository: dallay/agentsync
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files around rust:"
git ls-files | rg '(^|/)src/.*\.rs$|Cargo\.toml$' | sed -n '1,120p'
echo
echo "Search linker and targeted symbols broadly:"
rg -n 'remove_symlink|clean_symlink_target|clean_symlink_contents_target|clean_nested_glob_target|clean_module_map_target|fs::remove_file|read_dir|is_symlink' --glob '*.rs' -g '!target/**' | sed -n '1,240p'Repository: dallay/agentsync
Length of output: 955
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Linker size and relevant line ranges:"
wc -l src/linker.rs
echo
sed -n '1,80p' src/linker.rs
echo
sed -n '1320,1485p' src/linker.rs
echo
echo "Rust crate/platform hints:"
cat Cargo.toml | sed -n '1,220p'Repository: dallay/agentsync
Length of output: 10621
🌐 Web query:
Rust std::fs::remove_file Windows directory symbolic link fails remove_dir instead
💡 Result:
On Windows, symbolic links are categorized as either file symlinks or directory symlinks, and the operating system enforces specific APIs for their removal based on this type [1][2][3]. If you attempt to use std::fs::remove_file to delete a symbolic link to a directory, or std::fs::remove_dir to delete a symbolic link to a file, the operation will fail because the API does not match the type of the link [1][2][4]. Specifically, Windows requires: - Directory symlinks: Must be removed using std::fs::remove_dir [2][3][4]. - File symlinks: Must be removed using std::fs::remove_file [2][4]. This behavior contrasts with Unix-like systems, where all symbolic links are treated as files and can be removed universally using std::fs::remove_file [2][4]. To handle this cross-platform or generically on Windows, you must determine the type of the symlink before attempting to remove it. You can check the file attributes of the path (using std::fs::symlink_metadata) to identify if it is a directory symlink [5][4]. Alternatively, libraries such as the symlink crate provide convenience wrappers that abstract these platform-specific requirements [2]. If you are writing manual logic, a common pattern is to attempt one removal method and fallback to the other if the operation fails with a specific error (e.g., ErrorKind::NotADirectory) [1].
Citations:
- 1: Fix Windows bootstrap panic on invalid symlink removal (issue #143045) rust-lang/rust#143052
- 2: https://gitlab.com/chris-morgan/symlink/-/blob/master/README.md
- 3: rm: Cannot remove a symlink pointing to a directory on Windows nushell/nushell#11461
- 4: https://doc.rust-lang.org/nightly/nightly-rustc/src/run_make_support/fs.rs.html
- 5: https://github.com/rust-lang/rust/blob/master/library/std/src/sys/fs/windows.rs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for helper definitions/imports and symlink removal:"
rg -n 'fn remove_symlink|symlink_contents_child|symlink target|symlink directory|remove_file|remove_dir|FileTypeExt|remove_dir_all|FileAttr|RemoveDir|RemoveFile' src/linker.rs src --glob '*.rs' | sed -n '1,260p'Repository: dallay/agentsync
Length of output: 3802
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "remove_symlink implementation:"
sed -n '1805,1825p' src/linker.rs
echo
echo "symlink-contents child validation implementation:"
sed -n '500,615p' src/commands/status.rsRepository: dallay/agentsync
Length of output: 4640
Use remove_symlink for all symlink cleanup helpers.
remove_symlink already distinguishes Windows directory symlinks from file symlinks, but clean_symlink_contents_target, clean_nested_glob_target, and clean_module_map_target still call fs::remove_file directly. They can fail on Windows when a symlink points to a directory, leaving stale managed symlinks behind. Use remove_symlink in those helpers as well.
🤖 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 `@src/linker.rs` around lines 1358 - 1374, Replace direct fs::remove_file calls
in clean_symlink_contents_target, clean_nested_glob_target, and
clean_module_map_target with the existing remove_symlink helper, preserving each
helper’s validation, dry-run, cache invalidation, counters, and output behavior.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @yacosta738. * #486 (comment) The following files were modified: * `src/commands/doctor.rs` * `src/commands/skill.rs` * `src/commands/status.rs` * `src/config.rs` * `src/init.rs` * `src/linker.rs` * `src/main.rs` * `src/mcp.rs` * `src/skills/detect.rs` * `src/skills/install.rs` * `src/skills/suggest.rs` * `src/skills/update.rs` * `src/update_check.rs`
- update_check.rs: add User-Agent header, notification before cache save - skills/install.rs: FetchedSource enum, component-aware path stripping, buffer-based tar extraction, 100MB download size limit - skills/update.rs: named TempDir guard, remove sensitive logging, extract rollback_skill_dir helper - skills/suggest.rs: remove dead format_installed_status - commands/skill.rs: unify install flow with shared install_skill_callback, NoopInstallReporter for Json arm - commands/doctor.rs: inline marker parsing in check_gitignore, mark gitignore_missing_section_is_issue as test-only - main.rs: ApplyArgs struct replaces positional parameters - linker.rs: use remove_symlink helper in clean targets, add glob tests - init.rs: consolidate copy_entries_to_dest success output
23d1774 to
963c065
Compare
Semgrep Findings — False PositivesAll 3 Semgrep
The Semgrep rule /fp Internal config paths derived from agentsync.toml, not user-controlled web input |
…tive-complexity # Conflicts: # pnpm-lock.yaml
Add 50 new unit tests covering extracted helper functions: - suggest.rs: render_detections/recommendations/selected/results sections - update_check.rs: should_skip_update_check, is_fresh edge cases - install.rs: zip/tar common_root, entry_rel_path, fetch_local_data - doctor_tests.rs: parse_markers (7 cases) - update.rs: rollback_skill_dir, create_backup, copy_dir_all Total tests: 725 (up from 675)
Add 19 more tests covering high-impact uncovered paths: - install.rs: unpack_zip (basic, root strip, subpath), unpack_tar_gz (same), find_best_skill_dir (4 cases), copy_dir_recursively, archive_path_is_unsafe - doctor.rs: check_source_directory (2 cases), extract_managed_entries (4 cases) Fix: move extract_managed_entries and copy_dir_all before test modules to satisfy clippy items-after-test-module lint.
|
Note Unit test generation is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/skills/update.rs (1)
161-164: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRoll back when the copy fails.
create_backupmoved the installed skill tobackup_dirbefore this function runs. Ifcopy_dir_allfails on line 164, the function returns immediately.skill_dirthen holds a partial copy, and the previous version stays inbackup_dir. The user loses the working skill.The manifest branch (line 171) and the registry branch (line 194) both call
rollback_skill_dir. Apply the same recovery here.🛡️ Proposed fix
- copy_dir_all(local_dir, skill_dir).map_err(SkillUpdateError::Io)?; + if let Err(e) = copy_dir_all(local_dir, skill_dir) { + rollback_skill_dir(skill_dir, backup_dir); + return Err(SkillUpdateError::Io(e)); + }🤖 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 `@src/skills/update.rs` around lines 161 - 164, Update the copy step in the skill update function to invoke rollback_skill_dir when copy_dir_all fails after removing the existing skill directory, restoring the backup before returning the error. Preserve the existing error propagation and keep the manifest and registry rollback behavior unchanged.
♻️ Duplicate comments (1)
src/skills/install.rs (1)
461-469: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize a trailing slash in
subpath.If the URL fragment is
#src/,subis"src/". Line 463 then compares against"src//", and line 465 compares against"src/". Neither matches an entry such assrc/SKILL.md. Every entry is filtered out, and the extraction directory stays empty. The install then fails without a clear cause.Trim the trailing separator before matching.
tar_entry_rel_pathusesPathsemantics, so the two extraction paths also disagree on this input.🐛 Proposed fix
if let Some(sub) = subpath { + let sub = sub.trim_end_matches('/'); // Strip subpath only at a component boundary if let Some(rest) = rel_path.strip_prefix(&format!("{sub}/")) {🤖 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 `@src/skills/install.rs` around lines 461 - 469, Normalize subpath before matching in the tar-entry filtering logic: trim its trailing path separator so a fragment such as “src/” is treated as “src”. Use the normalized value consistently for both the component-boundary prefix check and the exact-match check, matching the Path-based semantics used by tar_entry_rel_path and preserving existing behavior for non-trailing-slash subpaths.
🤖 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 `@openspec/changes/init-user-template/design.md`:
- Line 18: Fix the Markdown lint warnings in
openspec/changes/init-user-template/design.md at lines 18 and 68 by adding a
language to the code fence and a blank line before the fence; in
openspec/changes/init-user-template/exploration.md at lines 86, 92, and 97,
remove code-span padding and add blank lines after the headings; and in
openspec/changes/init-user-template/proposal.md at lines 11, 21, 30, and 33, add
blank lines after each heading.
In `@openspec/changes/init-user-template/state.yaml`:
- Around line 2-13: Regenerate the init-user-template status artifacts from the
final commit before archiving: update the unchecked entries in tasks.md for
Tasks 4.5 and 5.1–5.3, and refresh verify-report.md to reflect the final
verification results and actual test count. Keep state.yaml, tasks.md, and
verify-report.md consistent, including resolved warnings and the reported 725
tests.
In `@src/commands/doctor.rs`:
- Around line 675-678: Update test_check_source_directory_missing to create a
test-local temporary directory, derive a child path that is guaranteed not to
exist, and pass that missing child to check_source_directory. Preserve the
assertion that the function returns 1, and ensure the temporary directory
remains available for the test’s duration.
In `@src/skills/install.rs`:
- Around line 432-436: The common-root logic must reject archives containing
only a top-level file. In src/skills/install.rs lines 432-436, update the root
validation to require at least one entry beginning with “{root}/”; in lines
528-553, track whether any entry has more than one path component and return
None when none do.
- Around line 480-483: In the decompression flow around GzDecoder and the
decompressed buffer, enforce MAX_DECOMPRESSED_SIZE by wrapping the decoder with
Read::take before read_to_end, then reject the archive if the capped reader
reaches the limit. Preserve the existing SkillInstallError::Io mapping for read
failures and ensure archives at or beyond the configured limit are not accepted.
In `@src/update_check.rs`:
- Around line 293-364: Replace the process-wide environment mutation in
test_should_skip_when_no_update_check_env_set, test_should_skip_when_ci_env_set,
and test_should_skip_no_update_check_only_skips_on_1 with tests of a pure helper
that accepts the relevant environment and terminal conditions. Update
should_skip_update_check to delegate to that helper, preserving the existing
skip behavior without requiring tests to set or restore CI or
AGENTSYNC_NO_UPDATE_CHECK.
---
Outside diff comments:
In `@src/skills/update.rs`:
- Around line 161-164: Update the copy step in the skill update function to
invoke rollback_skill_dir when copy_dir_all fails after removing the existing
skill directory, restoring the backup before returning the error. Preserve the
existing error propagation and keep the manifest and registry rollback behavior
unchanged.
---
Duplicate comments:
In `@src/skills/install.rs`:
- Around line 461-469: Normalize subpath before matching in the tar-entry
filtering logic: trim its trailing path separator so a fragment such as “src/”
is treated as “src”. Use the normalized value consistently for both the
component-boundary prefix check and the exact-match check, matching the
Path-based semantics used by tar_entry_rel_path and preserving existing behavior
for non-trailing-slash subpaths.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b69ee8e6-1bb6-4a09-af67-89568c04478b
📒 Files selected for processing (21)
.codegraph/.gitignore.codex/AGENTS.md.codex/instructions.md.gitignoreopenspec/changes/init-user-template/design.mdopenspec/changes/init-user-template/exploration.mdopenspec/changes/init-user-template/proposal.mdopenspec/changes/init-user-template/specs/init-user-template/spec.mdopenspec/changes/init-user-template/state.yamlopenspec/changes/init-user-template/tasks.mdopenspec/changes/init-user-template/verify-report.mdsrc/commands/doctor.rssrc/commands/doctor_tests.rssrc/commands/skill.rssrc/init.rssrc/linker.rssrc/main.rssrc/skills/install.rssrc/skills/suggest.rssrc/skills/update.rssrc/update_check.rs
|
|
||
| ## Data Flow | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Markdown lint warnings across the OpenSpec documents.
openspec/changes/init-user-template/design.md#L18-L18: add a language to the code fence.openspec/changes/init-user-template/design.md#L68-L68: add a blank line before the code fence.openspec/changes/init-user-template/exploration.md#L86-L86: remove padding inside the code span.openspec/changes/init-user-template/exploration.md#L92-L92: add a blank line after the heading.openspec/changes/init-user-template/exploration.md#L97-L97: add a blank line after the heading.openspec/changes/init-user-template/proposal.md#L11-L11: add a blank line after the heading.openspec/changes/init-user-template/proposal.md#L21-L21: add a blank line after the heading.openspec/changes/init-user-template/proposal.md#L30-L30: add a blank line after the heading.openspec/changes/init-user-template/proposal.md#L33-L33: add a blank line after the heading.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 18-18: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 3 files
openspec/changes/init-user-template/design.md#L18-L18(this comment)openspec/changes/init-user-template/exploration.md#L86-L86openspec/changes/init-user-template/proposal.md#L11-L11
🤖 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 `@openspec/changes/init-user-template/design.md` at line 18, Fix the Markdown
lint warnings in openspec/changes/init-user-template/design.md at lines 18 and
68 by adding a language to the code fence and a blank line before the fence; in
openspec/changes/init-user-template/exploration.md at lines 86, 92, and 97,
remove code-span padding and add blank lines after the headings; and in
openspec/changes/init-user-template/proposal.md at lines 11, 21, 30, and 33, add
blank lines after each heading.
Source: Linters/SAST tools
| current_phase: verify-clean | ||
| completed: [explore, propose, spec, design, tasks, apply, verify, warning-fixes] | ||
| next: archive | ||
| updated: 2026-08-01 | ||
| verify_verdict: PASS | ||
| warnings_resolved: | ||
| - wizard+template integration test added (test_wizard_template_flow_end_to_end) | ||
| - documentation updated (cli.mdx, getting-started.mdx, configuration.mdx) | ||
| verification: | ||
| - cargo test --all-features: all pass (403+ tests) | ||
| - cargo clippy: clean | ||
| - pnpm docs:build: clean (14 pages) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the final verification metadata.
state.yaml reports resolved warnings and 403+ tests. However, openspec/changes/init-user-template/tasks.md still leaves Tasks 4.5 and 5.1–5.3 unchecked, while openspec/changes/init-user-template/verify-report.md still reports those items incomplete and records 662 tests. The PR context reports 725 tests.
Regenerate the status, task, and verification artifacts from the final commit before archiving.
🤖 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 `@openspec/changes/init-user-template/state.yaml` around lines 2 - 13,
Regenerate the init-user-template status artifacts from the final commit before
archiving: update the unchecked entries in tasks.md for Tasks 4.5 and 5.1–5.3,
and refresh verify-report.md to reflect the final verification results and
actual test count. Keep state.yaml, tasks.md, and verify-report.md consistent,
including resolved warnings and the reported 725 tests.
|
Request timed out after 900000ms (requestId=0ad466a4-660e-492e-9c64-39380ab39802) |
- doctor.rs: use tempdir child path instead of hardcoded path in test - install.rs: reject top-level-only archives in zip/tar common_root, add MAX_DECOMPRESSED_SIZE (500MB) with Read::take on gz decoder, normalize subpath trailing slash in zip and tar entry filtering - update_check.rs: extract pure should_skip() helper, replace unsafe env-mutating tests with deterministic pure-function tests - update.rs: rollback skill_dir on copy_dir_all failure Skipped: openspec markdown lint and state.yaml updates (internal artifacts, not shipped code)
|
1 participant
Summary
Resolves all 28 open SonarQube Cognitive Complexity issues (rule
rust:S3776) by decomposing oversized functions using the Extract Method pattern.Motivation
All 28 issues were CRITICAL severity on SonarCloud. Each function exceeded the maximum allowed cognitive complexity of 15.
Changes
src/update_check.rsshould_skip_update_check,fetch_latest_version,check_and_notifysrc/config.rscollect_agent_gitignore_entries,collect_target_gitignore_entriessrc/commands/status.rsvalidate_children_directory,collect_child_issuessrc/commands/doctor.rssrc/commands/skill.rssrc/main.rshandle_init,handle_apply,handle_cleansrc/linker.rssrc/mcp.rsresolve_config_content,write_or_report_configsrc/init.rssrc/skills/detect.rssrc/skills/suggest.rsformat_installed_statussrc/skills/update.rssrc/skills/install.rsVerification
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-features(675 tests, 0 failures)make verify-all(includes JS build, docs build)Notes