Fix network-ops tool wrapper bugs from test analysis#72
Merged
Conversation
…nalysis Address confirmed tool bugs from three test sessions (482 total calls, 62 errors). Fixes silent data loss in SMB listing, broken multi-group netexec queries, naive arg splitting in SharpView, and impacket script discovery failing on shell wrapper scripts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This pull request hardens several network-ops tool wrappers to better handle real-world CLI quirks observed during cross-session error analysis, improving robustness of SMB enumeration, NetExec group queries, SharpView argument parsing, and Impacket script discovery.
Changes:
- Preserve partial
smbclientrecursive listing output even when the process exits non-zero, and surface warnings. - Query NetExec
--groupsone group at a time and join outputs to support multiple requested groups. - Fix SharpView argument parsing for quoted/space-containing arguments using
shlex.split(). - Improve Impacket script-path discovery when PATH points to shell wrapper scripts.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| capabilities/network-ops/tools/smbclient.py | Reworks smb_list_files execution to preserve partial output and handle timeouts. |
| capabilities/network-ops/tools/sharpview.py | Uses shlex.split() to correctly parse quoted SharpView argument strings. |
| capabilities/network-ops/tools/netexec.py | Loops --groups queries per group to match NetExec CLI constraints; joins results. |
| capabilities/network-ops/tools/impacket.py | Adds wrapper detection and PATH-following logic for Impacket script discovery. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Always append warning banner in smb_list_files when exit is non-zero, even without stderr output - Log full exceptions server-side in netexec group queries instead of exposing raw exception text (may contain credentials) in tool output - Add binary file detection to _is_python_script() for robustness Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use posix=False in shlex.split for SharpView args to preserve backslashes in AD-style domain\user arguments - Fix docstring for _extract_real_path_from_wrapper to match return type Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Rename generator variable to avoid shadowing typing import in sharpview - Strip quotes from extracted script paths in impacket wrapper parser Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment on lines
+192
to
+195
| # netexec --groups only accepts one group at a time, so we loop | ||
| # and join results when multiple groups are requested. | ||
| results: list[str] = [] | ||
| for group in groups: |
Contributor
Author
There was a problem hiding this comment.
Refuted. The original code would have produced args=["--groups"] with no group value on empty input, which netexec rejects. Our change doesn't introduce new silent-failure behavior. Adding empty-list validation is not a pattern used elsewhere in this codebase's tool methods.
Comment on lines
+340
to
+343
| # netexec --groups only accepts one group at a time, so we loop | ||
| # and join results when multiple groups are requested. | ||
| results: list[str] = [] | ||
| for group in groups: |
Contributor
Author
There was a problem hiding this comment.
Refuted. Same reasoning as SMB variant above.
Comment on lines
+13
to
+14
| def _is_python_script(path: Path) -> bool: | ||
| """Check if a file is a real Python script (not a shell wrapper or binary).""" |
Contributor
Author
There was a problem hiding this comment.
Refuted. The docstring already says "not a shell wrapper or binary" which accurately describes the exclusion logic. It's a private helper with a single caller — further rewording adds no value.
mkultraWasHere
added a commit
that referenced
this pull request
Jul 7, 2026
Version bump for tool wrapper bug fixes merged in #72. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
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.
Fixes confirmed tool bugs surfaced by cross-session error analysis of three network-ops agent runs (482 tool calls, 62 errors).
Fixed
smb_list_filesno longer silently discards valid directory listings when smbclient exits non-zero due to inaccessible subdirectories — partial output is now preserved with warnings appendednetexec_smb_enum_group_membersandnetexec_ldap_enum_group_membersno longer break when passed multiple groups — netexec--groupsonly accepts one value, so groups are now queried individually and results joinedSharpView._executeno longer breaks arguments containing spaces (e.g.-Domain "my domain.local") — replacedstr.split()withshlex.split()_get_impacket_script_path()now detects shell wrappers and follows them to the actual script directoryNotes
session_analysis_cross_session.mdis intentionally not included in this PR