feature improvments & small fixes #12
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a modular addon system with runtime delegation, history persistence/UI, queue-based batch analysis, advanced addon implementations (analyzers, Intune, winget, AI), CI/release changes to produce a Windows addon artifact, multiple GUI refactors, tests updates, and a few new dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as GUI/App
participant AddonSvc as AddonService
participant FS as Filesystem
participant AddonMod as Addon Module
participant HistorySvc as HistoryService
participant Analyzer as AnalyzerView
App->>AddonSvc: register_addons()
AddonSvc->>FS: resolve addon dir & check package presence
alt addon present
AddonSvc->>AddonMod: import advanced module (ai/intune/analyzers)
AddonMod-->>AddonSvc: module object
AddonSvc-->>App: expose advanced implementations
else addon absent
AddonSvc-->>App: return None (use local stubs)
end
App->>HistorySvc: instantiate()
Analyzer->>Analyzer: enqueue dropped files
loop per queued file
Analyzer->>AddonMod: call UniversalAnalyzer.analyze(...) or stub
AddonMod-->>Analyzer: analysis result
Analyzer->>HistorySvc: add_entry(metadata)
HistorySvc-->>Analyzer: persisted confirmation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/switchcraft_advanced/utils/winget.py (1)
9-9: Remove or generalize the hardcoded developer path.This constant contains a hardcoded personal path (
c:/Users/fseitz/GitHub/...) that won't work for other users. Sincelocal_repois now deprecated and set toNone, this constant appears to be dead code. Consider removing it entirely.-WINGET_PKGS_PATH = Path("c:/Users/fseitz/GitHub/winget-pkgs/manifests") # Local optimization +# Local winget-pkgs repository path (deprecated - now using CLI only) +# WINGET_PKGS_PATH = Nonetests/test_phase4.py (1)
9-9: Patch decorators target incorrect module namespace; imports won't be properly mocked.Lines 30 and 50 patch
switchcraft_advanced.services.intune_service, but line 9 imports fromswitchcraft.services.intune_service. Patches must target objects in the namespace where they are looked up, meaning these decorators should patchswitchcraft.services.intune_service.requests.getandswitchcraft.services.intune_service.subprocess.runto affect the imported module.tests/test_phase2.py (1)
10-11: Import paths need to be updated for addon architecture.The CI pipeline is failing with
ModuleNotFoundError: No module named 'switchcraft.services.ai_service'. Based on the PR's addon architecture changes,SwitchCraftAIhas been moved to the addon package. The imports should be updated to use the new addon paths or the test should conditionally skip when addons are unavailable.-from switchcraft.services.ai_service import SwitchCraftAI -from switchcraft.analyzers.universal import UniversalAnalyzer +try: + from switchcraft_advanced.services.ai_service import SwitchCraftAI + from switchcraft_advanced.analyzers.universal import UniversalAnalyzer + HAS_ADDON = True +except ImportError: + HAS_ADDON = FalseThen add
@unittest.skipUnless(HAS_ADDON, "Addon not installed")to the test class or relevant methods.src/switchcraft/gui/app.py (1)
638-640:AIViewis used but not imported.
AIViewis referenced insetup_helper_tabbut the import was removed/commented out (lines 26-27). This will raiseNameErrorwhen the AI addon is loaded.Add a dynamic import for AIView when the addon is available:
def setup_helper_tab(self): + try: + from switchcraft_advanced.gui.views.ai_view import AIView + except ImportError: + logger.error("AIView not available - addon not installed") + return self.ai_view = AIView(self.tab_helper, self.ai_service) self.ai_view.pack(fill="both", expand=True)
🧹 Nitpick comments (15)
src/switchcraft_debug/console.py (1)
26-27: Uselogging.exceptionto capture the stack trace.
logging.exceptionautomatically includes the traceback, which aids debugging console teardown failures.except Exception as e: - logger.error(f"Error closing console: {e}") + logger.exception("Error closing console")src/switchcraft_ai/service.py (1)
82-92: Error messages should be internationalized.The hardcoded English error strings are inconsistent with the rest of the application, which uses
i18n.get()for user-facing messages. Consider adding translation keys for these error messages.- if self.provider == "openai": - if self.client: - return self._ask_openai(query) - else: - return "Error: OpenAI Provider configured but not initialized. Please check your API Key in Settings." - - elif self.provider == "gemini": - if self.client: - return self._ask_gemini(query) - else: - return "Error: Gemini Provider configured but not initialized. Please check your API Key in Settings." + if self.provider == "openai": + if self.client: + return self._ask_openai(query) + else: + return i18n.get("ai_provider_not_initialized", provider="OpenAI") + + elif self.provider == "gemini": + if self.client: + return self._ask_gemini(query) + else: + return i18n.get("ai_provider_not_initialized", provider="Gemini")Then add the corresponding translation keys to
de.jsonanden.json.src/switchcraft/services/addon_service.py (2)
13-18: Annotate class-level mutable attribute withClassVar.The
ADDONSdictionary is a class-level mutable attribute and should be annotated withtyping.ClassVarfor clarity and to satisfy type checkers.+from typing import Optional, List, ClassVar, Dict + class AddonService: # Addon Definitions - ADDONS = { + ADDONS: ClassVar[Dict[str, str]] = { "advanced": "switchcraft_advanced", "winget": "switchcraft_winget", "ai": "switchcraft_ai", "debug": "switchcraft_debug" }
123-129: Uselogging.exceptionto preserve the traceback.When catching exceptions for logging purposes,
logging.exceptionautomatically includes the traceback, which aids debugging.try: if addon_path.exists(): shutil.rmtree(addon_path) return True except Exception as e: - logger.error(f"Uninstall failed: {e}") + logger.exception(f"Uninstall failed for {addon_id}") return Falsesrc/switchcraft/services/history_service.py (1)
13-20: Consider cross-platform path handling.The current implementation relies on the
APPDATAenvironment variable, which is Windows-specific. On Linux/macOS systems, this will always fall back tohistory.jsonin the current working directory, which may not be the desired behavior.Consider using a cross-platform approach:
def _get_history_path(self): - app_data = os.getenv('APPDATA') - if not app_data: - return Path("history.json") # Fallback - - dir_path = Path(app_data) / "FaserF" / "SwitchCraft" + if os.name == 'nt': # Windows + app_data = os.getenv('APPDATA') + if app_data: + dir_path = Path(app_data) / "FaserF" / "SwitchCraft" + else: + dir_path = Path.home() / ".switchcraft" + else: # Linux/macOS + dir_path = Path.home() / ".switchcraft" + dir_path.mkdir(parents=True, exist_ok=True) return dir_path / "history.json"tests/test_winget.py (1)
6-8: Patch paths should target the module under test.The patches target
shutil.whichandsubprocess.runglobally, but they should patch where they're imported in the module under test to ensure the mocks take effect correctly.- @patch('shutil.which', return_value="C:\\winget.exe") - @patch('subprocess.run') + @patch('switchcraft_advanced.utils.winget.shutil.which', return_value="C:\\winget.exe") + @patch('switchcraft_advanced.utils.winget.subprocess.run') def test_search_by_name_cli_found(self, mock_run, mock_which):Apply the same fix to
test_search_packages_found(lines 26-28) andtest_search_no_cli(line 45).src/switchcraft/gui/views/history_view.py (2)
46-50: Catch specific exception for datetime parsing.Using a bare
exceptcatches all exceptions includingKeyboardInterruptandSystemExit. For datetime parsing, catchValueErrorspecifically.try: dt = datetime.fromisoformat(ts_str) date_display = dt.strftime("%Y-%m-%d %H:%M") - except: + except ValueError: date_display = ts_str
74-76: Consider adding confirmation before clearing history.Clearing all history is a destructive operation. A confirmation dialog would prevent accidental data loss.
def clear_history(self): + from tkinter import messagebox + if not messagebox.askyesno("Confirm", "Clear all history?"): + return self.history_service.clear() self.load_history()src/switchcraft/gui/views/analyzer_view.py (1)
240-240: Uselogger.exceptionto capture stack trace.When logging exceptions,
logger.exceptionautomatically includes the traceback, which aids debugging.- except Exception as e: logger.error(f"Failed to save history: {e}") + except Exception as e: + logger.exception(f"Failed to save history: {e}")src/switchcraft/gui/app.py (1)
187-198: Accessing private_tab_dictis fragile.
CTkTabview._tab_dictis a private implementation detail that may change between customtkinter versions.Consider using the public API or try/except approach:
def _toggle_winget_tab(self, enabled): """Show or hide the Winget tab based on settings.""" if enabled: - if "Winget Store" not in self.tabview._tab_dict: + try: + self.tabview.tab("Winget Store") # Check if exists + except ValueError: self.tab_winget = self.tabview.add("Winget Store") self.setup_winget_tab() else: - if "Winget Store" in self.tabview._tab_dict: - self.tabview.delete("Winget Store") + try: + self.tabview.delete("Winget Store") + except ValueError: + passsrc/switchcraft_advanced/analyzers/universal.py (3)
129-134: Dead code and bare except.
safe_all_argsis assigned but never used, and the bareexcept: passsilently swallows all errors.- # 0. Try "All at once" strategy first (Phase 2 Req) - # Many CLIs basically dump help if you send ANY invalid or help arg. - # Sending multiple might trigger help immediately or error-help. - # We skip params that trigger installation (/silent, /s) for safety here. - safe_all_args = ["/?", "--help", "-h", "/h"] - try: - # Just try a few common ones concatenated? No, usually executables take one mode. - # But we can try to RUN once with a safe help argument and see if it dumps EVERYTHING. - pass - except: pass + # "All at once" strategy is implemented below (lines 140-175)
86-104: Silent exception handling hides potential issues.Several
try-except-passblocks silently swallow exceptions. Consider logging at debug level for troubleshooting.try: with py7zr.SevenZipFile(file_path, mode='r') as z: for filename in z.getnames(): if filename.lower().endswith('.msi'): return f"MSI Wrapper (contains {filename})" - except Exception: - pass + except Exception as e: + logger.debug(f"py7zr inspection failed: {e}")
409-409: Rename unused loop variable.The
dirsvariable is not used within the loop body.- for root, dirs, files in os.walk(temp_dir): + for root, _dirs, files in os.walk(temp_dir):src/switchcraft_advanced/services/intune_service.py (2)
19-19: Use explicitOptionaltype hint.The parameter should use
Optional[str]instead ofstr = Nonefor PEP 484 compliance.Apply this diff:
- def __init__(self, tools_dir: str = None): + def __init__(self, tools_dir: Optional[str] = None):Add the import at the top:
import logging import os import subprocess import requests +from typing import Optional from pathlib import Path
50-50: Use explicitOptionaltype hint.Same issue as line 19—use
Optional[str]for thecatalog_folderparameter.Apply this diff:
- def create_intunewin(self, source_folder: str, setup_file: str, output_folder: str, catalog_folder: str = None, quiet: bool = True) -> str: + def create_intunewin(self, source_folder: str, setup_file: str, output_folder: str, catalog_folder: Optional[str] = None, quiet: bool = True) -> str:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.github/workflows/ci.yml(1 hunks).github/workflows/release.yml(2 hunks).gitignore(1 hunks)src/switchcraft/analyzers/universal.py(1 hunks)src/switchcraft/assets/lang/de.json(1 hunks)src/switchcraft/assets/lang/en.json(1 hunks)src/switchcraft/gui/app.py(7 hunks)src/switchcraft/gui/views/analyzer_view.py(4 hunks)src/switchcraft/gui/views/history_view.py(1 hunks)src/switchcraft/gui/views/settings_view.py(4 hunks)src/switchcraft/gui/views/winget_view.py(2 hunks)src/switchcraft/services/addon_service.py(1 hunks)src/switchcraft/services/history_service.py(1 hunks)src/switchcraft/services/intune_service.py(1 hunks)src/switchcraft_advanced/analyzers/universal.py(1 hunks)src/switchcraft_advanced/services/intune_service.py(1 hunks)src/switchcraft_advanced/utils/winget.py(6 hunks)src/switchcraft_ai/service.py(1 hunks)src/switchcraft_debug/console.py(1 hunks)switchcraft.spec(1 hunks)tests/test_phase2.py(1 hunks)tests/test_phase4.py(2 hunks)tests/test_phase7.py(1 hunks)tests/test_winget.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
src/switchcraft/services/addon_service.py (2)
src/switchcraft/utils/config.py (2)
SwitchCraftConfig(8-139)get_value(22-59)src/switchcraft/utils/i18n.py (1)
get(110-137)
src/switchcraft/gui/views/history_view.py (2)
src/switchcraft/services/history_service.py (2)
get_history(22-32)clear(52-53)src/switchcraft/gui/app.py (1)
start_analysis_tab(665-666)
src/switchcraft/gui/views/analyzer_view.py (1)
src/switchcraft/services/history_service.py (1)
add_entry(34-50)
tests/test_winget.py (1)
src/switchcraft_advanced/utils/winget.py (2)
search_by_name(19-28)search_packages(102-182)
src/switchcraft/analyzers/universal.py (2)
src/switchcraft/services/addon_service.py (1)
AddonService(11-129)src/switchcraft_advanced/analyzers/universal.py (6)
UniversalAnalyzer(13-547)check_wrapper(77-109)brute_force_help(111-234)detect_silent_disabled(480-539)extract_and_analyze_nested(310-478)cleanup_temp_dir(541-547)
src/switchcraft/services/intune_service.py (1)
src/switchcraft_advanced/services/intune_service.py (5)
IntuneService(11-362)authenticate(124-141)upload_win32_app(183-334)create_intunewin(50-120)assign_to_group(336-362)
src/switchcraft/gui/views/settings_view.py (1)
src/switchcraft/utils/config.py (3)
SwitchCraftConfig(8-139)set_user_preference(96-120)set_secret(133-139)
src/switchcraft_advanced/analyzers/universal.py (3)
src/switchcraft/analyzers/universal.py (6)
UniversalAnalyzer(12-32)check_wrapper(19-20)brute_force_help(22-23)extract_and_analyze_nested(28-29)detect_silent_disabled(25-26)cleanup_temp_dir(31-32)src/switchcraft/analyzers/exe.py (1)
ExeAnalyzer(10-729)src/switchcraft/analyzers/msi.py (1)
MsiAnalyzer(9-74)
src/switchcraft/gui/views/winget_view.py (1)
src/switchcraft_advanced/utils/winget.py (1)
search_packages(102-182)
🪛 GitHub Actions: CI
tests/test_phase2.py
[error] 10-10: ModuleNotFoundError: No module named 'switchcraft.services.ai_service'. 3 errors during collection while running 'pytest'.
tests/test_phase7.py
[error] 5-5: AttributeError: type object 'AddonService' has no attribute 'import_advanced_module'. Did you mean: 'import_addon_module'? 3 errors during collection while running 'pytest'.
tests/test_phase4.py
[error] 9-9: AttributeError: type object 'AddonService' has no attribute 'import_advanced_module'. Did you mean: 'import_addon_module'? 3 errors during collection while running 'pytest'.
src/switchcraft/services/intune_service.py
[error] 7-7: AttributeError: type object 'AddonService' has no attribute 'import_advanced_module'. Did you mean: 'import_addon_module'?
🪛 Ruff (0.14.8)
src/switchcraft/services/addon_service.py
13-18: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
42-42: Multiple statements on one line (colon)
(E701)
64-64: Multiple statements on one line (colon)
(E701)
69-69: Multiple statements on one line (colon)
(E701)
91-91: Multiple statements on one line (colon)
(E701)
95-95: Multiple statements on one line (colon)
(E701)
112-112: Multiple statements on one line (colon)
(E701)
126-126: Consider moving this statement to an else block
(TRY300)
127-127: Do not catch blind exception: Exception
(BLE001)
128-128: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/switchcraft/gui/views/history_view.py
49-49: Do not use bare except
(E722)
63-63: Do not perform function call item.get in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
src/switchcraft_debug/console.py
26-26: Do not catch blind exception: Exception
(BLE001)
27-27: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/switchcraft/gui/views/analyzer_view.py
96-96: Multiple statements on one line (colon)
(E701)
102-102: Multiple statements on one line (colon)
(E701)
240-240: Do not catch blind exception: Exception
(BLE001)
240-240: Multiple statements on one line (colon)
(E701)
240-240: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
254-254: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
tests/test_winget.py
8-8: Unused method argument: mock_which
(ARG002)
28-28: Unused method argument: mock_which
(ARG002)
46-46: Unused method argument: mock_which
(ARG002)
src/switchcraft/services/history_service.py
30-30: Do not catch blind exception: Exception
(BLE001)
31-31: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
59-59: Do not catch blind exception: Exception
(BLE001)
60-60: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/switchcraft_advanced/services/intune_service.py
19-19: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
37-37: Probable use of requests call without timeout
(S113)
45-45: Consider moving this statement to an else block
(TRY300)
46-46: Do not catch blind exception: Exception
(BLE001)
47-47: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
50-50: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
57-57: Avoid specifying long messages outside the exception class
(TRY003)
100-100: subprocess call: check for execution of untrusted input
(S603)
113-113: Abstract raise to an inner function
(TRY301)
113-113: Avoid specifying long messages outside the exception class
(TRY003)
116-116: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
117-117: Use raise without specifying exception name
Remove exception name
(TRY201)
118-118: try-except block with duplicate exception Exception
(B025)
119-119: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
120-120: Use raise without specifying exception name
Remove exception name
(TRY201)
134-134: Probable use of requests call without timeout
(S113)
138-138: Consider moving this statement to an else block
(TRY300)
139-139: Do not catch blind exception: Exception
(BLE001)
140-140: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
141-141: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
141-141: Avoid specifying long messages outside the exception class
(TRY003)
178-178: Consider moving this statement to an else block
(TRY300)
179-179: Do not catch blind exception: Exception
(BLE001)
180-180: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
197-197: Avoid specifying long messages outside the exception class
(TRY003)
210-210: Abstract raise to an inner function
(TRY301)
210-210: Avoid specifying long messages outside the exception class
(TRY003)
213-213: Using xml to parse untrusted data is known to be vulnerable to XML attacks; use defusedxml equivalents
(S314)
234-234: Abstract raise to an inner function
(TRY301)
234-234: Avoid specifying long messages outside the exception class
(TRY003)
241-241: Do not catch blind exception: Exception
(BLE001)
242-242: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
242-242: Avoid specifying long messages outside the exception class
(TRY003)
259-259: Probable use of requests call without timeout
(S113)
264-264: Multiple statements on one line (colon)
(E701)
269-269: Probable use of requests call without timeout
(S113)
286-286: Probable use of requests call without timeout
(S113)
292-292: Multiple statements on one line (colon)
(E701)
304-304: Probable use of requests call without timeout
(S113)
308-308: Multiple statements on one line (colon)
(E701)
315-315: Probable use of requests call without timeout
(S113)
324-324: Probable use of requests call without timeout
(S113)
327-327: Multiple statements on one line (colon)
(E701)
329-329: Consider moving this statement to an else block
(TRY300)
332-332: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
334-334: Use raise without specifying exception name
Remove exception name
(TRY201)
356-356: Probable use of requests call without timeout
(S113)
359-359: Consider moving this statement to an else block
(TRY300)
361-361: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
362-362: Use raise without specifying exception name
Remove exception name
(TRY201)
src/switchcraft/analyzers/universal.py
19-19: Unused method argument: path
(ARG002)
22-22: Unused method argument: path
(ARG002)
25-25: Unused method argument: path
(ARG002)
25-25: Unused method argument: output
(ARG002)
28-28: Unused method argument: path
(ARG002)
src/switchcraft/gui/app.py
32-32: Redefinition of unused HistoryService from line 31
Remove definition: HistoryService
(F811)
91-91: Do not catch blind exception: Exception
(BLE001)
104-104: Do not catch blind exception: Exception
(BLE001)
src/switchcraft/services/intune_service.py
34-34: Unused method argument: args
(ARG002)
34-34: Unused method argument: kwargs
(ARG002)
src/switchcraft_advanced/utils/winget.py
16-16: Unused method argument: product_code
(ARG002)
30-30: Unused method argument: product_name
(ARG002)
35-35: Multiple statements on one line (colon)
(E701)
133-133: Abstract raise to an inner function
(TRY301)
133-133: Avoid specifying long messages outside the exception class
(TRY003)
165-165: Multiple statements on one line (colon)
(E701)
src/switchcraft_advanced/analyzers/universal.py
73-73: Consider moving this statement to an else block
(TRY300)
74-74: Do not catch blind exception: Exception
(BLE001)
90-91: try-except-pass detected, consider logging the exception
(S110)
90-90: Do not catch blind exception: Exception
(BLE001)
103-104: try-except-pass detected, consider logging the exception
(S110)
103-103: Do not catch blind exception: Exception
(BLE001)
106-106: Do not catch blind exception: Exception
(BLE001)
129-129: Local variable safe_all_args is assigned to but never used
Remove assignment to unused variable safe_all_args
(F841)
134-134: Do not use bare except
(E722)
134-134: try-except-pass detected, consider logging the exception
(S110)
134-134: Multiple statements on one line (colon)
(E701)
152-152: subprocess call: check for execution of untrusted input
(S603)
153-153: Consider [str(file_path), *cmd_args] instead of concatenation
Replace with [str(file_path), *cmd_args]
(RUF005)
174-174: Do not catch blind exception: Exception
(BLE001)
186-186: subprocess call: check for execution of untrusted input
(S603)
187-187: Consider [str(file_path), *cmd_args] instead of concatenation
Replace with [str(file_path), *cmd_args]
(RUF005)
222-222: Do not catch blind exception: Exception
(BLE001)
362-362: subprocess call: check for execution of untrusted input
(S603)
388-388: subprocess call: check for execution of untrusted input
(S603)
409-409: Loop control variable dirs not used within loop body
Rename unused dirs to _dirs
(B007)
438-438: Do not catch blind exception: Exception
(BLE001)
475-475: Do not catch blind exception: Exception
(BLE001)
536-537: try-except-pass detected, consider logging the exception
(S110)
536-536: Do not catch blind exception: Exception
(BLE001)
546-546: Do not catch blind exception: Exception
(BLE001)
src/switchcraft/gui/views/winget_view.py
89-89: Do not catch blind exception: Exception
(BLE001)
97-97: Multiple statements on one line (colon)
(E701)
🔇 Additional comments (14)
src/switchcraft/assets/lang/de.json (1)
49-51: LGTM!The new translation keys are properly added and the German translations are grammatically correct. The keys align with the English counterparts mentioned in the AI summary.
.github/workflows/ci.yml (1)
6-21: LGTM! CI optimizations improve resource usage.The addition of
paths-ignorefilters and concurrency controls are well-implemented best practices. These changes will:
- Skip CI runs for documentation-only changes
- Cancel redundant in-progress runs when new commits are pushed
.gitignore (1)
57-57: LGTM! Consistent with existing test artifact patterns.The addition of
phase2_err.outaligns with other test output files already being ignored (e.g.,smoke_out.txt,test_results.txt).src/switchcraft/assets/lang/en.json (1)
50-51: LGTM! New translation keys are well-placed.The new localization entries for update checking follow the existing naming conventions and are logically placed with related update messages.
tests/test_phase4.py (1)
30-30: Patch targets updated for addon architecture.The patch decorators now target
switchcraft_advanced.services.intune_serviceinstead of the base module, aligning with the addon-based refactoring described in the PR.Also applies to: 50-50
src/switchcraft/gui/views/winget_view.py (2)
84-94: Good addition of error handling for search failures.The try-except block properly captures errors from
search_packages()(which can raiseRuntimeErrorper the relevant code snippet) and displays them to the user. This improves the user experience when Winget CLI issues occur.Note: The static analysis hint about catching blind
Exceptionis a false positive here—catching all exceptions in UI code prevents crashes and allows graceful error display to users.
96-103: Error display logic is well-implemented.The error parameter addition and corresponding UI display provide clear feedback to users when search operations fail.
tests/test_phase2.py (1)
54-55: Patch target correctly updated for addon path.The patch target now points to
switchcraft_advanced.analyzers.universal.subprocess.run, which aligns with the addon-based architecture. This is correct assuming the import issue above is resolved..github/workflows/release.yml (1)
167-171: Addon packaging step looks correct for Windows.The step correctly gates on
windows-latestand uses PowerShell'sCompress-Archiveto package the addon.tests/test_winget.py (1)
45-49: Good coverage for CLI-unavailable scenario.The test correctly verifies fallback behavior when winget CLI is not available—returning
Nonefromsearch_by_nameand an empty list fromsearch_packages.src/switchcraft/gui/views/settings_view.py (1)
531-551: Validation logic is correct.The save flow properly validates that a non-local provider requires an API key before persisting settings. The conditional secret storage is appropriate.
src/switchcraft_advanced/analyzers/universal.py (1)
13-47: Comprehensive brute force implementation.The
UniversalAnalyzerprovides thorough wrapper detection and parameter discovery. The approach of trying multiple help arguments and analyzing output patterns is well-structured.src/switchcraft/analyzers/universal.py (2)
1-10: LGTM! Clean addon loading pattern.The dynamic import mechanism correctly delegates to the addon when available and provides a graceful fallback.
12-32: LGTM! Stub implementation maintains API compatibility.The stub correctly preserves the method signatures for API compatibility when the addon is unavailable. The static analysis warnings about unused parameters are expected and can be safely ignored—this is a valid stub pattern.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/switchcraft_advanced/services/intune_service.py (1)
180-329: Verify XML parsing security.The upload flow correctly includes timeouts on all HTTP requests. However, line 210 uses
xml.etree.ElementTree.parse()to parse the .intunewin file, which is external input.As flagged by static analysis and past reviews, consider using
defusedxmlto prevent XML bomb and entity expansion attacks:+from defusedxml import ElementTree as DefusedET with z.open(det_xml) as f: - tree = ET.parse(f) + tree = DefusedET.parse(f)Add
defusedxmlto your project dependencies if not already present.
🧹 Nitpick comments (8)
src/switchcraft_advanced/utils/winget.py (1)
102-183: Consider simplifying the exception handling pattern.The current pattern catches
RuntimeErroronly to re-raise it immediately (lines 179-180). This can be simplified by excludingRuntimeErrorfrom the exception handler.Apply this diff:
return results - except RuntimeError as e: - raise e except Exception as e: + if isinstance(e, RuntimeError): + raise logger.error(f"Winget Search Error: {e}") return []Alternatively, use a more specific exception type in the except clause to exclude RuntimeError:
- except RuntimeError as e: - raise e - except Exception as e: + except (OSError, ValueError, TypeError) as e: logger.error(f"Winget Search Error: {e}") return []src/switchcraft_debug/console.py (1)
29-45: Consider usinglogger.exceptionfor better debugging.The cleanup logic is sound, but using
logger.exceptioninstead oflogger.errorwould provide stack trace information that could be valuable for debugging console teardown issues.Apply this diff:
except Exception as e: - logger.error(f"Error closing console: {e}") + logger.exception(f"Error closing console: {e}")src/switchcraft/gui/views/analyzer_view.py (3)
80-99: Consider splitting the one-liner for better readability.The drop handling logic correctly parses TkinterDnD's file list format. However, line 96 combines a conditional and return on one line.
As per static analysis hints, apply this diff to improve readability:
- if not files: return + if not files: + return
101-108: Consider formatting consistency for one-line conditionals.The queue processing logic is sound. However, lines 102 and 105 use one-line if/return statements.
As per static analysis hints, consider this formatting improvement:
def _process_queue(self): - if self._is_analyzing: return + if self._is_analyzing: + return - if not self.queue: - return # Done + if not self.queue: + return # Done next_file = self.queue.pop(0) self._start_analysis(next_file)
232-240: Uselogging.exceptionfor better diagnostics.The history integration is well-guarded with
hasattrand defensive exception handling. However, usinglogger.errorloses the stack trace.As per static analysis hints, apply this diff:
if hasattr(self.app, 'history_service'): try: self.app.history_service.add_entry({ "filename": path.name, "filepath": str(path), "product": info.product_name or "Unknown", "type": info.installer_type }) - except Exception as e: logger.error(f"Failed to save history: {e}") + except Exception: + logger.exception("Failed to save history")src/switchcraft/gui/app.py (2)
45-65: Consider clarifying CLI addon installation flow.The
--install-addonsflag is handled, but the implementation comments note it's not truly headless—it schedules a GUI prompt rather than performing a silent install.If silent/headless installation is desired, consider implementing
AddonService.install_addon_silently()or similar. If the current approach (GUI prompt with CLI flag) is intentional, simplify the comments to make this clear:# CLI flag triggers addon check/prompt on startup if "--install-addons" in sys.argv: self.after(2000, self._check_addon_status)
637-648: Uselogging.exceptionfor better diagnostics.The dynamic AI view loading is well-structured. However, using
logger.errorin the exception handler loses the stack trace.As per static analysis hints, apply this diff:
except Exception as e: - logger.error(f"Failed to setup AI Helper tab: {e}") + logger.exception("Failed to setup AI Helper tab")src/switchcraft_advanced/services/intune_service.py (1)
140-178: Consider using a proper JWT library for robustness.The manual JWT decoding works, but using a library like
PyJWTwould be more maintainable and handle edge cases better.As noted in past reviews:
import jwt def verify_graph_permissions(self, token): """Verifies Graph API permissions from JWT token.""" try: # Decode without signature verification (already validated by Microsoft) payload = jwt.decode(token, options={"verify_signature": False}) roles = payload.get("roles", []) # ... rest of logic except jwt.DecodeError: return False, "Invalid token format"If you keep the manual approach, the current implementation is functional for this internal tool context.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/ci.yml(2 hunks).github/workflows/release.yml(2 hunks).gitignore(1 hunks)README.md(1 hunks)src/switchcraft/analyzers/universal.py(1 hunks)src/switchcraft/gui/app.py(8 hunks)src/switchcraft/gui/views/analyzer_view.py(5 hunks)src/switchcraft/gui/views/settings_view.py(5 hunks)src/switchcraft/services/history_service.py(1 hunks)src/switchcraft/services/intune_service.py(1 hunks)src/switchcraft_advanced/services/intune_service.py(1 hunks)src/switchcraft_advanced/utils/winget.py(7 hunks)src/switchcraft_debug/console.py(1 hunks)tests/test_addons.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/switchcraft/analyzers/universal.py (1)
src/switchcraft/services/addon_service.py (1)
import_addon_module(61-76)
src/switchcraft/gui/views/analyzer_view.py (2)
src/switchcraft/services/history_service.py (1)
add_entry(34-50)src/switchcraft/services/notification_service.py (2)
NotificationService(15-74)send_notification(24-50)
src/switchcraft/gui/app.py (4)
src/switchcraft/services/addon_service.py (4)
AddonService(11-129)is_addon_installed(39-50)register_addons(53-58)import_addon_module(61-76)src/switchcraft/gui/views/intune_view.py (1)
IntuneView(12-285)src/switchcraft/gui/views/history_view.py (1)
HistoryView(6-76)src/switchcraft/utils/i18n.py (1)
get(110-137)
src/switchcraft/services/intune_service.py (1)
src/switchcraft/services/addon_service.py (2)
AddonService(11-129)import_addon_module(61-76)
tests/test_addons.py (1)
src/switchcraft/services/addon_service.py (4)
is_addon_installed(39-50)import_addon_module(61-76)install_addon(79-106)uninstall_addon(109-129)
src/switchcraft/gui/views/settings_view.py (4)
src/switchcraft/utils/i18n.py (1)
get(110-137)src/switchcraft/utils/config.py (3)
SwitchCraftConfig(8-139)set_user_preference(96-120)set_secret(133-139)src/switchcraft/services/addon_service.py (1)
is_addon_installed(39-50)src/switchcraft_debug/console.py (2)
DebugConsole(7-45)toggle(13-45)
src/switchcraft_advanced/services/intune_service.py (1)
src/switchcraft/services/intune_service.py (7)
IntuneService(12-38)is_tool_available(19-20)download_tool(22-23)create_intunewin(31-32)authenticate(25-26)upload_win32_app(28-29)assign_to_group(34-35)
🪛 LanguageTool
README.md
[grammar] ~60-~60: Ensure spelling is correct
Context: ...Enable the debug console on startup (if addon is installed). ## 🧩 Addon System (Mod...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~62-~62: Ensure spelling is correct
Context: ...startup (if addon is installed). ## 🧩 Addon System (Modular Features) SwitchCraft ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~64-~64: Ensure spelling is correct
Context: ...es) SwitchCraft now supports a modular addon system to keep the core application lig...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~71-~71: Ensure spelling is correct
Context: ...ng console for troubleshooting. Manage addons via Settings > Addon Manager or CLI...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~71-~71: Ensure spelling is correct
Context: ...ooting. Manage addons via Settings > Addon Manager or CLI: ```bash # Install all...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 Ruff (0.14.8)
src/switchcraft/analyzers/universal.py
19-19: Unused method argument: path
(ARG002)
22-22: Unused method argument: path
(ARG002)
25-25: Unused method argument: path
(ARG002)
25-25: Unused method argument: output
(ARG002)
28-28: Unused method argument: path
(ARG002)
src/switchcraft/gui/views/analyzer_view.py
96-96: Multiple statements on one line (colon)
(E701)
102-102: Multiple statements on one line (colon)
(E701)
240-240: Do not catch blind exception: Exception
(BLE001)
240-240: Multiple statements on one line (colon)
(E701)
240-240: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
254-254: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/switchcraft/gui/app.py
89-89: Do not catch blind exception: Exception
(BLE001)
102-102: Do not catch blind exception: Exception
(BLE001)
646-646: Do not catch blind exception: Exception
(BLE001)
647-647: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/switchcraft_advanced/utils/winget.py
16-16: Unused method argument: product_code
(ARG002)
30-30: Unused method argument: product_name
(ARG002)
35-35: Multiple statements on one line (colon)
(E701)
132-132: Abstract raise to an inner function
(TRY301)
164-164: Multiple statements on one line (colon)
(E701)
180-180: Use raise without specifying exception name
Remove exception name
(TRY201)
src/switchcraft/services/intune_service.py
16-16: Unused method argument: tools_dir
(ARG002)
37-37: Unused method argument: args
(ARG002)
37-37: Unused method argument: kwargs
(ARG002)
src/switchcraft_advanced/services/intune_service.py
19-19: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
45-45: Consider moving this statement to an else block
(TRY300)
47-47: Redundant exception object included in logging.exception call
(TRY401)
50-50: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
57-57: Avoid specifying long messages outside the exception class
(TRY003)
100-100: subprocess call: check for execution of untrusted input
(S603)
113-113: Abstract raise to an inner function
(TRY301)
113-113: Avoid specifying long messages outside the exception class
(TRY003)
116-116: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
117-117: Use raise without specifying exception name
Remove exception name
(TRY201)
135-135: Consider moving this statement to an else block
(TRY300)
137-137: Redundant exception object included in logging.exception call
(TRY401)
138-138: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
138-138: Avoid specifying long messages outside the exception class
(TRY003)
175-175: Consider moving this statement to an else block
(TRY300)
176-176: Do not catch blind exception: Exception
(BLE001)
177-177: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
194-194: Avoid specifying long messages outside the exception class
(TRY003)
207-207: Abstract raise to an inner function
(TRY301)
207-207: Avoid specifying long messages outside the exception class
(TRY003)
210-210: Using xml to parse untrusted data is known to be vulnerable to XML attacks; use defusedxml equivalents
(S314)
231-231: Abstract raise to an inner function
(TRY301)
231-231: Avoid specifying long messages outside the exception class
(TRY003)
238-238: Do not catch blind exception: Exception
(BLE001)
239-239: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
239-239: Avoid specifying long messages outside the exception class
(TRY003)
261-261: Multiple statements on one line (colon)
(E701)
287-287: Multiple statements on one line (colon)
(E701)
303-303: Multiple statements on one line (colon)
(E701)
322-322: Multiple statements on one line (colon)
(E701)
324-324: Consider moving this statement to an else block
(TRY300)
327-327: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
329-329: Use raise without specifying exception name
Remove exception name
(TRY201)
354-354: Consider moving this statement to an else block
(TRY300)
356-356: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
357-357: Use raise without specifying exception name
Remove exception name
(TRY201)
src/switchcraft_debug/console.py
41-41: Do not catch blind exception: Exception
(BLE001)
42-42: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (38)
.gitignore (1)
57-60: LGTM!The new ignore rules for addon and test outputs are appropriate and consistent with existing patterns in the file.
.github/workflows/ci.yml (3)
6-17: LGTM!The paths-ignore rules appropriately skip CI runs for documentation-only changes, reducing resource usage while maintaining coverage for code changes.
19-21: LGTM!The concurrency configuration prevents duplicate CI runs for the same branch, improving resource efficiency.
49-49: LGTM!The PYTHONPATH augmentation correctly includes all addon source directories, enabling tests to import modules from the expanded codebase layout. The use of
$(pwd)is appropriate for bash.README.md (1)
57-82: LGTM!The documentation clearly explains the new addon system, CLI flags, and recently added features. The structure is user-friendly and provides appropriate examples.
.github/workflows/release.yml (2)
167-171: LGTM!The addon package creation is correctly gated to Windows-only builds using the if condition. The PowerShell command appropriately compresses the addon directory.
217-224: LGTM!The addon upload step is properly gated to Windows-only with
if: matrix.os == 'windows-latest', addressing the concern from previous reviews about cross-platform build failures.src/switchcraft/analyzers/universal.py (2)
1-11: LGTM!The addon-driven loading pattern is well-implemented. The fallback to a local stub when the addon is unavailable ensures graceful degradation.
12-32: Stub implementation maintains API compatibility.The unused method arguments flagged by static analysis are intentional to maintain interface compatibility with the addon-provided implementation. The stub provides appropriate fallback behavior when the addon is not installed.
src/switchcraft_advanced/utils/winget.py (2)
9-31: LGTM!The removal of local repository support simplifies the implementation and focuses on CLI-based Winget integration, which is the more reliable and maintainable approach.
33-100: LGTM!The enhanced
_search_cliimplementation includes proper input validation, explicit interactivity flags, robust error handling, and improved parsing logic. The Windows-specific startupinfo handling is appropriate.src/switchcraft_debug/console.py (1)
7-28: LGTM!The state tracking and idempotency checks prevent resource leaks and misuse. The enable logic properly stores original streams and checks the
AllocConsolereturn value. Line buffering is appropriate for console output.tests/test_addons.py (1)
36-68: LGTM!The test methods appropriately cover addon detection, module import, installation, and uninstallation scenarios. The tests verify both positive and negative cases.
src/switchcraft/gui/views/analyzer_view.py (4)
35-36: LGTM!The queue and in-flight state initialization properly supports the new batch processing feature.
115-120: LGTM!The idempotency check prevents concurrent analyses and ensures sequential processing of the queue.
246-252: LGTM!Correctly resets
_is_analyzingtoFalsein the exception handler, ensuring the queue isn't permanently blocked after a failure.
259-259: LGTM! Batch processing flow is correct.The state management and queue progression are properly implemented:
- Line 259 resets
_is_analyzingat the start of result rendering.- Lines 432-436 schedule the next queue item AFTER displaying current results.
This correctly addresses the concern from previous reviews about skipping result display during batch processing.
Also applies to: 432-436
src/switchcraft/gui/app.py (5)
81-106: LGTM!The addon loading is properly defensive:
AddonService.register_addons()is called early to set up paths.- Optional addons (AI, Winget) fail gracefully with informational logging.
- Broad exception handling is appropriate here since addon failures shouldn't crash the app.
185-197: LGTM!The dynamic tab management correctly checks for tab existence before adding or removing the Winget tab.
653-659: LGTM!Correctly wires the
on_winget_togglecallback to enable dynamic Winget tab management from settings.
673-676: LGTM!The history tab setup correctly initializes
HistoryViewwith the history service.
354-354: LGTM!Consistent use of i18n for the update check dialog title.
src/switchcraft/services/intune_service.py (2)
6-10: LGTM!The addon import correctly uses
import_addon_module("advanced", "services.intune_service")and conditionally loads the real implementation.
12-38: LGTM!The stub implementation provides a consistent API surface:
- All methods that require addon functionality raise
NotImplementedError.get_device_groupsreturns an empty list as a safe fallback.- Indentation and behavior are now consistent across all stub methods.
src/switchcraft/services/history_service.py (4)
9-20: LGTM!The history path resolution correctly uses
APPDATAwith a fallback, and creates necessary directories safely.
22-32: LGTM!The history loading correctly handles missing files and uses
logging.exceptionfor diagnostics, as recommended by past reviews.
34-50: LGTM!The entry addition correctly timestamps items and limits history size to prevent unbounded growth.
55-60: LGTM!The save method correctly uses
logging.exceptionfor diagnostics, as recommended by past reviews.src/switchcraft/gui/views/settings_view.py (6)
17-21: LGTM!The
on_winget_togglecallback parameter correctly enables dynamic Winget tab management.
53-57: LGTM!The Winget toggle correctly saves the preference and safely invokes the callback when present.
546-588: LGTM!The addon manager UI provides a clear interface for managing addons, with appropriate "Recommended" labels and install/uninstall actions.
589-604: LGTM!The install and uninstall methods correctly delegate to
AddonServiceand appropriately prompt for restart to ensure addons are properly loaded.
476-487: LGTM!The AI configuration enhancements improve user experience:
- "Get API Key" button provides direct links to provider key pages.
- Validation ensures non-local providers have API keys before saving.
- Secret storage is correctly conditional on provider type.
Also applies to: 516-544
605-628: LGTM!The debug console setup gracefully handles the optional debug addon:
- Shows informative message when addon is not installed.
- Dynamically imports and invokes the console toggle when available.
- Provides error feedback if the module is unexpectedly missing.
src/switchcraft_advanced/services/intune_service.py (4)
31-48: LGTM!The tool download correctly includes a timeout (line 37) and uses
logging.exception(line 47), addressing past review recommendations.
50-117: LGTM!The
create_intunewinmethod correctly handles subprocess execution with proper error logging. The duplicate exception handler mentioned in past reviews is not present in the current code.
121-138: LGTM!Authentication correctly includes a timeout (line 131) and uses
logging.exception(line 137), addressing past review recommendations.
331-357: LGTM!The group assignment correctly includes a timeout (line 351), addressing past review recommendations.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/switchcraft_advanced/utils/winget.py (1)
231-237: Remove dead code:_construct_github_urlis never called.The
_construct_github_urlmethod (lines 231-237) referencesself.local_repo, which is set toNoneat line 14. Although the try-except block prevents runtime errors, this method is unused throughout the codebase and should be removed.src/switchcraft/gui/views/analyzer_view.py (1)
17-17: Critical: This import causes the CI pipeline failure.The
from switchcraft.utils.winget import WingetHelperimport fails because the module doesn't exist. This is the same issue noted inapp.py. TheWingetHelperclass exists insrc/switchcraft_advanced/utils/winget.py(the addon), not in the core package.You need to either:
- Create a stub/proxy module at
switchcraft/utils/winget.pythat delegates to the addon- Make this import conditional/lazy based on addon availability
🧹 Nitpick comments (9)
src/switchcraft_advanced/utils/winget.py (3)
16-17: Document deprecated methods or raise NotImplementedError.The
search_by_product_codemethod is now a no-op that silently returnsNone. If this method is intentionally deprecated, consider either:
- Adding a docstring or comment explaining the deprecation.
- Raising
NotImplementedErrorto make it explicit that this functionality is not supported.This will prevent confusion for callers who expect functionality.
Apply this diff to make the deprecation explicit:
def search_by_product_code(self, product_code: str) -> Optional[str]: + """Deprecated: Product code search is no longer supported.""" return NoneOr raise an error:
def search_by_product_code(self, product_code: str) -> Optional[str]: - return None + raise NotImplementedError("Product code search is no longer supported")
63-66: Clarify error handling for non-zero exit codes.The code logs a warning for non-zero exit codes but then continues to parse the output with
pass. This behavior is inconsistent withsearch_packages(lines 129-132), which raisesRuntimeErrorfor non-zero/non-1 exit codes.If the intent is to gracefully handle exit code 1 (no results) while continuing to parse, consider making this explicit:
if proc.returncode != 0: - logger.warning(f"Winget CLI failed code {proc.returncode}") - # Code 1 usually means no results, but we should verify stdout - pass + if proc.returncode == 1: + # Exit code 1 typically means no results; attempt to parse anyway + logger.debug(f"Winget CLI returned code 1 (possibly no results)") + else: + logger.warning(f"Winget CLI failed with unexpected code {proc.returncode}") + return NoneThis makes the error handling strategy clear and consistent across methods.
164-164: Minor style: Multiple statements on one line.Static analysis flags line 164 for having multiple statements on one line (E701). Consider splitting for better readability:
- if line.startswith("---") or "package found" in line: continue + if line.startswith("---") or "package found" in line: + continuepyproject.toml (1)
32-34: Consider pinning dependency versions for production stability.The new dependencies (
defusedxml,PyJWT) are appropriate for the security and JWT functionality introduced in this PR. However, all dependencies lack version constraints, which could lead to unexpected breaking changes when newer versions are released.Consider specifying minimum versions or compatible ranges:
- "defusedxml", - "PyJWT" + "defusedxml>=0.7.0", + "PyJWT>=2.0.0"src/switchcraft/gui/app.py (1)
171-182: Accessing private_tab_dictattribute is fragile.The
_tab_dictattribute is an internal implementation detail ofCTkTabview. This could break silently ifcustomtkinterchanges its internal structure in future versions.Consider wrapping this in a try-except or checking if the public API provides an alternative method for tab existence checking.
src/switchcraft/gui/views/analyzer_view.py (1)
102-110: Potential thread safety concern with_is_analyzingflag.The
_is_analyzingflag is accessed and modified from both the main thread (Lines 103, 118, 122) and the analyzer thread (Line 254). While Python's GIL provides some protection for simple boolean assignments, the check-then-act pattern at Lines 102-104 and 117-120 could theoretically have race conditions.For this GUI application with user-initiated actions, this is likely fine in practice, but consider using
threading.Lockif issues arise with rapid file drops.Also applies to: 117-128
src/switchcraft_advanced/services/intune_service.py (1)
117-119: Use bareraiseinstead ofraise eto preserve traceback.Using
raise einstead ofraisecan affect traceback in certain edge cases. Apply the same fix to similar patterns at lines 315 and 343.except Exception as e: - logger.error(f"Error running IntuneWinAppUtil: {e}") - raise e + logger.exception("Error running IntuneWinAppUtil") + raisesrc/switchcraft_ai/service.py (2)
138-148: Guard against empty response from Gemini.
response.textcould potentially beNoneor empty in edge cases (e.g., safety filters blocking content). Consider adding a fallback.response = self.client.generate_content(full_prompt) - return response.text + return response.text or "I couldn't generate a response."
150-165:generate_install_scripttool ignores requiredsilent_argsparameter.The tool schema (lines 38-43) declares
silent_argsas required, but_execute_toolnever uses it. Either use the argument in the mock response or update the schema to make it optional.if name == "generate_install_script": - # Just mock relevant action or return instructions for now, - # fully wiring this needs callbacks to the UI or passing the App instance. - # For now return success text - return {"status": "success", "info": "Script generation logic simulated. Tell user it's ready (Mock)."} + silent_args = args.get("silent_args", "") + return {"status": "success", "info": f"Script generation simulated with args: {silent_args} (Mock)."}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
phase4_err.outis excluded by!**/*.out
📒 Files selected for processing (10)
.gitignore(1 hunks)pyproject.toml(1 hunks)src/switchcraft/gui/app.py(8 hunks)src/switchcraft/gui/views/analyzer_view.py(5 hunks)src/switchcraft/services/ai_service.py(1 hunks)src/switchcraft_advanced/services/intune_service.py(1 hunks)src/switchcraft_advanced/utils/winget.py(7 hunks)src/switchcraft_ai/service.py(1 hunks)src/switchcraft_debug/console.py(1 hunks)tests/test_phase2.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .gitignore
- src/switchcraft_debug/console.py
- tests/test_phase2.py
🧰 Additional context used
🧬 Code graph analysis (4)
src/switchcraft/services/ai_service.py (2)
src/switchcraft/services/addon_service.py (2)
AddonService(11-129)import_addon_module(61-76)src/switchcraft_ai/service.py (3)
SwitchCraftAI(10-215)update_context(74-77)ask(79-93)
src/switchcraft/gui/app.py (9)
src/switchcraft/services/addon_service.py (3)
register_addons(53-58)import_addon_module(61-76)is_addon_installed(39-50)src/switchcraft/services/intune_service.py (1)
IntuneService(12-38)src/switchcraft/gui/views/winget_view.py (1)
WingetView(16-306)src/switchcraft/gui/views/history_view.py (1)
HistoryView(6-76)src/switchcraft/services/history_service.py (1)
HistoryService(9-60)src/switchcraft_ai/service.py (1)
SwitchCraftAI(10-215)src/switchcraft_advanced/utils/winget.py (1)
WingetHelper(12-237)src/switchcraft/utils/i18n.py (1)
get(110-137)src/switchcraft_ai/gui/view.py (1)
AIView(8-78)
src/switchcraft/gui/views/analyzer_view.py (2)
src/switchcraft/services/history_service.py (1)
add_entry(34-50)src/switchcraft/services/notification_service.py (2)
NotificationService(15-74)send_notification(24-50)
src/switchcraft_ai/service.py (3)
src/switchcraft/utils/config.py (3)
SwitchCraftConfig(8-139)get_value(22-59)get_secret(123-130)src/switchcraft/services/ai_service.py (3)
SwitchCraftAI(12-24)update_context(20-21)ask(23-24)src/switchcraft/utils/i18n.py (1)
get(110-137)
🪛 GitHub Actions: CI
src/switchcraft/gui/app.py
[error] 1-1: Failed to import GUI module due to missing switchcraft.utils.winget (ModuleNotFoundError).
src/switchcraft/gui/views/analyzer_view.py
[error] 17-17: ModuleNotFoundError: No module named 'switchcraft.utils.winget' imported by WingetHelper used in GUI initialization.
🪛 Ruff (0.14.8)
src/switchcraft/services/ai_service.py
23-23: Unused method argument: query
(ARG002)
src/switchcraft/gui/app.py
75-75: Do not catch blind exception: Exception
(BLE001)
88-88: Do not catch blind exception: Exception
(BLE001)
src/switchcraft_advanced/utils/winget.py
16-16: Unused method argument: product_code
(ARG002)
30-30: Unused method argument: product_name
(ARG002)
35-35: Multiple statements on one line (colon)
(E701)
132-132: Abstract raise to an inner function
(TRY301)
164-164: Multiple statements on one line (colon)
(E701)
src/switchcraft/gui/views/analyzer_view.py
257-257: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/switchcraft_advanced/services/intune_service.py
21-21: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
47-47: Consider moving this statement to an else block
(TRY300)
49-49: Redundant exception object included in logging.exception call
(TRY401)
52-52: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
102-102: subprocess call: check for execution of untrusted input
(S603)
115-115: Abstract raise to an inner function
(TRY301)
115-115: Avoid specifying long messages outside the exception class
(TRY003)
118-118: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
119-119: Use raise without specifying exception name
Remove exception name
(TRY201)
137-137: Consider moving this statement to an else block
(TRY300)
139-139: Redundant exception object included in logging.exception call
(TRY401)
140-140: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
140-140: Avoid specifying long messages outside the exception class
(TRY003)
162-162: Consider moving this statement to an else block
(TRY300)
163-163: Do not catch blind exception: Exception
(BLE001)
180-180: Avoid specifying long messages outside the exception class
(TRY003)
193-193: Abstract raise to an inner function
(TRY301)
193-193: Avoid specifying long messages outside the exception class
(TRY003)
217-217: Abstract raise to an inner function
(TRY301)
217-217: Avoid specifying long messages outside the exception class
(TRY003)
224-224: Do not catch blind exception: Exception
(BLE001)
225-225: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
225-225: Avoid specifying long messages outside the exception class
(TRY003)
247-247: Multiple statements on one line (colon)
(E701)
273-273: Multiple statements on one line (colon)
(E701)
289-289: Multiple statements on one line (colon)
(E701)
308-308: Multiple statements on one line (colon)
(E701)
310-310: Consider moving this statement to an else block
(TRY300)
313-313: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
315-315: Use raise without specifying exception name
Remove exception name
(TRY201)
340-340: Consider moving this statement to an else block
(TRY300)
342-342: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
343-343: Use raise without specifying exception name
Remove exception name
(TRY201)
src/switchcraft_ai/service.py
59-59: Multiple statements on one line (colon)
(E701)
67-67: Multiple statements on one line (colon)
(E701)
71-71: Do not catch blind exception: Exception
(BLE001)
72-72: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
135-135: Do not catch blind exception: Exception
(BLE001)
146-146: Consider moving this statement to an else block
(TRY300)
147-147: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (18)
src/switchcraft_advanced/utils/winget.py (2)
180-181: LGTM: RuntimeError is now properly re-raised.The previous concern about
RuntimeErrorbeing caught and swallowed has been addressed. The exception is now re-raised at lines 180-181, ensuring it propagates to the UI as intended.
185-229: LGTM: Robust package details retrieval.The
get_package_detailsmethod is well-implemented with:
- Consistent CLI flags (
--disable-interactivity,--accept-source-agreements)- Proper Windows startup info handling
- Robust key-value parsing with support for multi-line values
- Appropriate error handling and logging
src/switchcraft/gui/app.py (5)
67-92: Addon loading with broad exception handling is appropriate here.The try-except blocks catching
Exceptionfor addon loading (Lines 75, 88) are reasonable for this use case. Addons are optional components that may fail for various reasons (missing files, import errors, version mismatches), and the logging provides adequate visibility. The application correctly falls back to functioning without the addon.
153-169: Addon status check implementation looks correct.The one-time prompt logic using
AddonPromptShownconfig flag prevents user annoyance, and theis_addon_installed("advanced")call now correctly passes the requiredaddon_idparameter.
622-644: Dynamic addon loading for AI view and settings callback wiring are well-implemented.The
setup_helper_tabproperly handles the case where the AI addon module might be missing, with appropriate logging. TheSettingsViewcallback wiring enables dynamic tab toggling.
659-661: History tab setup is straightforward and correct.
29-29: Review comment focuses on incorrect location for the missing module issue.Line 29 imports the
WingetViewclass, not theswitchcraft.utils.wingetmodule. WingetView properly comments out that import with "Moved to Addon" and receives the helper via dependency injection. The actual missing module imports are insrc/switchcraft/main.py(line 14) andsrc/switchcraft/gui/views/analyzer_view.py(line 17), whereswitchcraft.utils.wingetis directly imported without guards. These files, not app.py line 29, require fixes.Likely an incorrect or invalid review comment.
src/switchcraft/gui/views/analyzer_view.py (4)
35-36: Queue-based batch processing implementation is well-designed.The queue and
_is_analyzingstate management enable proper batch file processing. The regex pattern for parsing TkinterDnD data correctly handles both paths with spaces (enclosed in curly braces) and regular paths.Minor note: The
import reinside_on_drop(Line 85) could be moved to the top of the file for consistency, but this is a negligible concern.Also applies to: 80-100
234-243: History integration is correctly implemented with proper error handling.The defensive
hasattrcheck and exception handling ensure that history saving failures don't crash the analysis workflow. The entry structure (filename, filepath, product, type) aligns with theHistoryService.add_entrymethod expectations.
261-262: Batch processing now correctly displays results before proceeding.The implementation addresses the previous review concern - results are now fully displayed before scheduling the next queue item with
self.after(1500, self._process_queue). The 1.5-second delay provides users time to see the current analysis results.Also applies to: 435-439
249-259: Error handling correctly resets state and notifies user.The exception handler properly:
- Resets
_is_analyzing = Falseto allow subsequent analyses- Uses
logger.exceptionto capture full traceback- Sends user notification about the failure
The
SystemExitcatch (Lines 256-259) is defensive against misbehaving analyzer code.src/switchcraft_advanced/services/intune_service.py (3)
142-164: JWT decoding without verification is acceptable here.The token is obtained directly from Microsoft's OAuth endpoint, so signature verification has already been performed by the identity provider. The
verify_signature=Falseoption is appropriate for simply inspecting claims from a trusted source. The comment at line 145 documents this decision.
166-310: Upload workflow implementation is comprehensive and addresses previous review concerns.The multi-step upload process correctly:
- Uses
defusedxmlfor secure XML parsing (Line 196)- Includes appropriate timeouts (60s for API calls, 300s for blob upload)
- Provides progress callbacks for UI feedback
- Handles the complex Graph API flow for Win32 LOB app uploads
123-140: Authentication and group assignment methods are correctly implemented.Both methods include the required timeouts (30s and 60s respectively) as requested in previous reviews. The exception handling provides adequate error information.
Also applies to: 317-343
src/switchcraft/services/ai_service.py (2)
12-24: Stub implementation correctly maintains interface compatibility.The stub class provides the same interface (
update_context,ask) as the real implementation, allowing the application to function without the addon. The unusedqueryparameter at Line 23 is intentional for interface compatibility - the static analysis hint (ARG002) can be safely ignored here.The warning log and user-friendly return message appropriately guide users toward installing the addon.
6-10: The module-level addon loading pattern is safe; no import order issues exist.The delegation to the addon's
SwitchCraftAIwhen available provides seamless integration. The module-level import is protected becauseAddonService.import_addon_module()internally callsregister_addons()before importing the addon module, ensuringsys.pathis configured regardless of whenai_service.pyis first imported.src/switchcraft_ai/service.py (2)
79-93: Clean fallback logic.The routing to provider-specific handlers with graceful fallback to local regex is well-structured.
167-215: Solid local fallback implementation.The rule-based logic with i18n support provides reasonable answers when AI providers are unavailable. The language detection heuristic is simple but effective for the use case.
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 (3)
src/switchcraft/services/intune_service.py (3)
68-68: Fix inconsistent indentation.Line 68 has an extra leading space before
try:that breaks the standard indentation pattern.🔎 Apply this diff to fix the indentation:
if setup_path.is_absolute(): # If absolute, verify it starts with source_folder - try: + try: rel_path = setup_path.relative_to(source_folder)
88-89: Fix inconsistent indentation.Line 89 has an extra leading space before
cmd.extendthat breaks the standard indentation pattern.🔎 Apply this diff to fix the indentation:
if catalog_folder: - cmd.extend(["-a", str(Path(catalog_folder).resolve())]) + cmd.extend(["-a", str(Path(catalog_folder).resolve())])
200-202: Fix inconsistent indentation.Line 201 has an extra leading space before the comment that breaks the standard indentation pattern.
🔎 Apply this diff to fix the indentation:
if enc_node is None: - # Fallback search - enc_node = root.find(".//EncryptionInfo") + # Fallback search + enc_node = root.find(".//EncryptionInfo")
🧹 Nitpick comments (15)
src/switchcraft/utils/winget.py (5)
1-5: Remove unused imports and avoid re-importingjsoninside methods.
yamlandPathare imported but never used. Additionally,jsonis imported at the module level but then re-imported insidesearch_packages(line 41) andget_package_details(line 91), which Ruff flags as F811.🔎 Apply this diff to clean up imports:
import logging import json -import yaml -from pathlib import Path from typing import Optional, List, DictAnd remove the local imports at lines 41 and 91:
- import json
61-62: Style: Avoid multiple statements on one line.Ruff flags E701. Split the early return for better readability.
🔎 Suggested fix:
output = proc.stdout.strip() - if not output: return [] + if not output: + return []
84-86: Uselogging.exceptionto capture stack trace on errors.When catching exceptions,
logging.exceptionautomatically includes the stack trace, which aids debugging. Ruff flags TRY400.🔎 Suggested fix:
except Exception as e: - logger.error(f"Winget PS Search Error: {e}") + logger.exception(f"Winget PS Search Error: {e}") return []
124-126: Uselogging.exceptionfor better error diagnostics.Same as noted for
search_packages- Ruff TRY400.🔎 Suggested fix:
except Exception as e: - logger.error(f"Winget PS Details Error: {e}") + logger.exception(f"Winget PS Details Error: {e}") return {}
9-12: Consider adding a platform check for Windows.This helper relies on PowerShell and
Find-WinGetPackage, which are Windows-specific. If this code could be invoked on non-Windows platforms, consider adding a platform check to provide a clear error message rather than a cryptic subprocess failure.import platform class WingetHelper: def __init__(self): if platform.system() != "Windows": logger.warning("WingetHelper is only supported on Windows") self.local_repo = Nonesrc/switchcraft_ai/service.py (1)
156-166: Consider minor exception handling refinement.The current exception handling works correctly, but you could improve consistency slightly by moving the return statement into an else block (as suggested by static analysis). However, the current pattern matches
_ask_openai, so consistency within the file is maintained.Optional: Slightly more explicit structure
def _ask_gemini(self, user_query: str) -> str: try: # Simple content generation for Gemini # Context injection context_str = f"Context: {json.dumps(self.context)}" if self.context else "" full_prompt = f"{context_str}\n\nUser: {user_query}" response = self.client.generate_content(full_prompt) return response.text - except Exception as e: + except Exception: + logger.exception("Gemini Error") - return f"Gemini Error: {e}" + return "Gemini Error: Unable to generate response"src/switchcraft/services/intune_service.py (5)
138-140: Improve exception handling with proper exception chaining.The exception handling can be improved by using
raise ... from eto preserve the original exception context, which helps with debugging.🔎 Apply this diff to improve exception handling:
except Exception as e: - logger.exception(f"Authentication failed: {e}") - raise RuntimeError(f"Authentication failed: {e}") + logger.exception("Authentication failed") + raise RuntimeError("Authentication failed") from e
117-119: Preferlogging.exceptionand bareraisein exception handlers.For better debugging, use
logging.exceptionto automatically include the traceback, and use bareraiseto preserve the full exception context.🔎 Apply this diff to improve exception handling:
except Exception as e: - logger.error(f"Error running IntuneWinAppUtil: {e}") - raise e + logger.exception("Error running IntuneWinAppUtil") + raise
223-224: Improve exception handling with proper exception chaining.Use
raise ... from eto preserve the original exception context for better debugging.🔎 Apply this diff:
except Exception as e: - raise RuntimeError(f"Failed to parse .intunewin: {e}") + raise RuntimeError(f"Failed to parse .intunewin: {e}") from eNote: This also fixes the inconsistent indentation on line 224.
311-314: Preferlogging.exceptionand bareraisein exception handlers.For better debugging, use
logging.exceptionto automatically include the traceback, and use bareraiseto preserve the full exception context.🔎 Apply this diff:
except Exception as e: - logger.error(f"Upload process failed: {e}") + logger.exception("Upload process failed") # Try to cleanup? - raise e + raise
340-342: Preferlogging.exceptionand bareraisein exception handlers.For better debugging, use
logging.exceptionto automatically include the traceback, and use bareraiseto preserve the full exception context.🔎 Apply this diff:
except Exception as e: - logger.error(f"Failed to assign group: {e}") - raise e + logger.exception("Failed to assign group") + raisesrc/switchcraft_advanced/services/intune_service.py (1)
117-119: Improve exception handling patterns throughout the file.Multiple exception handlers can be improved by following Python best practices:
- Use
logging.exceptioninstead oflogging.errorin exception handlers for automatic traceback inclusion- Use bare
raiseinstead ofraise eto preserve full exception context- Use
raise ... from efor proper exception chainingAffected locations:
- Lines 117-119 (create_intunewin)
- Lines 138-140 (authenticate)
- Lines 223-224 (upload_win32_app parsing)
- Lines 311-314 (upload_win32_app upload)
- Lines 340-342 (assign_to_group)
🔎 Example fixes:
For lines 117-119:
except Exception as e: - logger.error(f"Error running IntuneWinAppUtil: {e}") - raise e + logger.exception("Error running IntuneWinAppUtil") + raiseFor lines 138-140:
except Exception as e: - logger.exception(f"Authentication failed: {e}") - raise RuntimeError(f"Authentication failed: {e}") + logger.exception("Authentication failed") + raise RuntimeError("Authentication failed") from eApply similar patterns to the other locations.
Also applies to: 138-140, 223-224, 311-314, 340-342
src/switchcraft/gui/views/settings_view.py (3)
37-40: Consider applying i18n consistently to all tab labels.Only the General tab uses
i18n.get()with a fallback. The "Updates" and "Help" tabs are hardcoded English strings, which could limit internationalization support.🔎 Apply this diff to add i18n support for all tabs:
- self.tab_general = self.tabview.add(i18n.get("settings_hdr_update") or "General") # "General" / "Update Settings" - self.tab_updates = self.tabview.add("Updates") # Kept simple or could be merged + self.tab_general = self.tabview.add(i18n.get("settings_tab_general") or "General") + self.tab_updates = self.tabview.add(i18n.get("settings_tab_updates") or "Updates") self.tab_deploy = self.tabview.add(i18n.get("deployment_title") or "Deployment") - self.tab_help = self.tabview.add("Help") + self.tab_help = self.tabview.add(i18n.get("settings_tab_help") or "Help")
265-311: Consider breaking down this complex method.The
toggle_signmethod spans 46 lines and handles multiple responsibilities: toggling state, detecting certificates, prompting the user, and saving configuration. This high cyclomatic complexity makes it harder to test and maintain.Consider extracting helper methods like:
_auto_select_certificate()for certificate detection and selection logic (lines 273-308)_prompt_for_certificate(certs)for the multi-certificate selection dialog (lines 286-303)_set_certificate(thumbprint, subject)to centralize certificate configuration (repeated in lines 284-285, 302-303)This would improve testability and readability.
326-337: Clarify certificate entry editability behavior.The comments (lines 332-336) indicate uncertainty about whether the certificate path should be editable after browsing. Currently, the entry is set to
disabledafter selection, which prevents manual corrections and is inconsistent with typical file browser UX.Consider keeping the entry editable after browsing to allow manual path corrections:
🔎 Apply this diff to make the entry editable:
def browse_cert(self): file_path = ctk.filedialog.askopenfilename(filetypes=[("PFX Certificate", "*.pfx")]) if file_path: self.cert_path_entry.configure(state="normal") self.cert_path_entry.delete(0, "end") self.cert_path_entry.insert(0, file_path) - self.cert_path_entry.configure(state="disabled") # Keep it read-only to avoid manual typos? Or allow edit? - # Actually, standard behavior is usually editable. - # But here we used disabled for Store-Cert. + # Keep editable to allow manual corrections SwitchCraftConfig.set_user_preference("CodeSigningCertPath", file_path) SwitchCraftConfig.set_user_preference("CodeSigningCertThumbprint", "")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/switchcraft/assets/lang/de.json(2 hunks)src/switchcraft/assets/lang/en.json(2 hunks)src/switchcraft/gui/views/settings_view.py(3 hunks)src/switchcraft/services/intune_service.py(12 hunks)src/switchcraft/utils/winget.py(2 hunks)src/switchcraft_advanced/services/intune_service.py(1 hunks)src/switchcraft_ai/service.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/switchcraft/assets/lang/en.json
- src/switchcraft/assets/lang/de.json
🧰 Additional context used
🧬 Code graph analysis (5)
src/switchcraft/services/intune_service.py (1)
src/switchcraft_advanced/services/intune_service.py (1)
verify_graph_permissions(142-164)
src/switchcraft_ai/service.py (3)
src/switchcraft/utils/config.py (3)
SwitchCraftConfig(8-139)get_value(22-59)get_secret(123-130)src/switchcraft/services/ai_service.py (3)
SwitchCraftAI(12-24)update_context(20-21)ask(23-24)src/switchcraft/utils/i18n.py (1)
get(110-137)
src/switchcraft_advanced/services/intune_service.py (1)
src/switchcraft/utils/i18n.py (1)
get(110-137)
src/switchcraft/utils/winget.py (2)
src/switchcraft/utils/i18n.py (1)
get(110-137)src/switchcraft/gui/app.py (1)
run(45-51)
src/switchcraft/gui/views/settings_view.py (3)
src/switchcraft/utils/i18n.py (1)
get(110-137)src/switchcraft/utils/config.py (2)
get_value(22-59)set_user_preference(96-120)src/switchcraft/gui/app.py (1)
run(45-51)
🪛 Ruff (0.14.8)
src/switchcraft/services/intune_service.py
49-49: Redundant exception object included in logging.exception call
(TRY401)
137-137: Consider moving this statement to an else block
(TRY300)
139-139: Redundant exception object included in logging.exception call
(TRY401)
140-140: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
140-140: Avoid specifying long messages outside the exception class
(TRY003)
246-246: Multiple statements on one line (colon)
(E701)
src/switchcraft_ai/service.py
57-57: Consider [self.messages[0], *self.messages[-10:]] instead of concatenation
Replace with [self.messages[0], *self.messages[-10:]]
(RUF005)
84-84: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
164-164: Consider moving this statement to an else block
(TRY300)
165-165: Do not catch blind exception: Exception
(BLE001)
src/switchcraft_advanced/services/intune_service.py
21-21: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
47-47: Consider moving this statement to an else block
(TRY300)
49-49: Redundant exception object included in logging.exception call
(TRY401)
52-52: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
102-102: subprocess call: check for execution of untrusted input
(S603)
115-115: Abstract raise to an inner function
(TRY301)
115-115: Avoid specifying long messages outside the exception class
(TRY003)
118-118: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
119-119: Use raise without specifying exception name
Remove exception name
(TRY201)
137-137: Consider moving this statement to an else block
(TRY300)
139-139: Redundant exception object included in logging.exception call
(TRY401)
140-140: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
140-140: Avoid specifying long messages outside the exception class
(TRY003)
162-162: Consider moving this statement to an else block
(TRY300)
163-163: Do not catch blind exception: Exception
(BLE001)
179-179: Avoid specifying long messages outside the exception class
(TRY003)
192-192: Abstract raise to an inner function
(TRY301)
192-192: Avoid specifying long messages outside the exception class
(TRY003)
216-216: Abstract raise to an inner function
(TRY301)
216-216: Avoid specifying long messages outside the exception class
(TRY003)
223-223: Do not catch blind exception: Exception
(BLE001)
224-224: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
224-224: Avoid specifying long messages outside the exception class
(TRY003)
246-246: Multiple statements on one line (colon)
(E701)
272-272: Multiple statements on one line (colon)
(E701)
288-288: Multiple statements on one line (colon)
(E701)
307-307: Multiple statements on one line (colon)
(E701)
309-309: Consider moving this statement to an else block
(TRY300)
312-312: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
314-314: Use raise without specifying exception name
Remove exception name
(TRY201)
339-339: Consider moving this statement to an else block
(TRY300)
341-341: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
342-342: Use raise without specifying exception name
Remove exception name
(TRY201)
src/switchcraft/utils/winget.py
41-41: Redefinition of unused json from line 2
Remove definition: json
(F811)
62-62: Multiple statements on one line (colon)
(E701)
82-82: Consider moving this statement to an else block
(TRY300)
84-84: Do not catch blind exception: Exception
(BLE001)
85-85: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
91-91: Redefinition of unused json from line 2
Remove definition: json
(F811)
104-104: subprocess call: check for execution of untrusted input
(S603)
107-107: Multiple statements on one line (colon)
(E701)
124-124: Do not catch blind exception: Exception
(BLE001)
125-125: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/switchcraft/gui/views/settings_view.py
255-255: subprocess call: check for execution of untrusted input
(S603)
258-258: Multiple statements on one line (colon)
(E701)
259-259: Multiple statements on one line (colon)
(E701)
260-260: Consider moving this statement to an else block
(TRY300)
261-261: Do not catch blind exception: Exception
(BLE001)
262-262: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
391-391: Unused method argument: key
(ARG002)
🔇 Additional comments (9)
src/switchcraft/utils/winget.py (1)
14-33: LGTM!The method correctly delegates to
search_packagesand constructs a valid winget.run URL from the package ID. The fallback to returningNonewhen no results are found is appropriate.src/switchcraft_ai/service.py (6)
17-50: LGTM! Well-structured initialization.The
__init__method properly initializes all necessary state and tool definitions. The tool schemas are well-defined with clear descriptions and parameters.
59-82: Excellent improvements addressing past review comments.The provider initialization properly addresses previous concerns:
- Uses
"gemini-1.5-flash"instead of the unsupported"gemini-pro"model- Logs warnings when API keys are missing for both providers
- Provides sensible model defaults
These changes ensure users understand when AI features are unavailable due to missing credentials.
92-106: LGTM! Clean provider delegation pattern.The routing logic clearly separates concerns and provides a sensible fallback to the local rule-based system when AI providers are unavailable.
123-127: Great improvement! JSONDecodeError handling added.This properly addresses the past review comment about handling malformed JSON from the model. The try-except block with a fallback to an empty dict ensures the tool execution continues gracefully even with invalid arguments.
152-154: Proper exception logging implemented.Using
logger.exception()preserves the stack trace, which addresses the past review comment about blind exception catching. This will make debugging much easier.
185-233: LGTM! Comprehensive rule-based fallback with i18n support.The
_ask_smart_regexmethod provides an excellent fallback system with:
- Automatic language detection (German/English)
- Context-aware responses using installer analysis data
- Proper i18n integration for all responses
- Coverage of common user queries (SmartScreen, Winget, Intune, PSExec)
This ensures users get helpful responses even without AI provider credentials.
src/switchcraft/gui/views/settings_view.py (2)
17-21: LGTM! Callback parameter wired correctly.The
on_winget_togglecallback parameter is properly added and stored, addressing the previous review comment about enabling dynamic Winget tab visibility control.
61-72: LGTM! Winget toggle implementation is correct.The toggle correctly saves the preference and invokes the callback when provided, enabling dynamic tab visibility control.
| setup_path = Path(setup_file) | ||
| if setup_path.is_absolute(): | ||
| # If absolute, verify it starts with source_folder | ||
| try: |
There was a problem hiding this comment.
Fix inconsistent indentation at multiple locations.
Several lines have extra leading spaces that break the standard indentation pattern:
- Line 68:
try:statement - Line 89:
cmd.extendcall - Line 201: Fallback search comment
🔎 Apply these diffs to fix the indentation:
if setup_path.is_absolute():
# If absolute, verify it starts with source_folder
- try:
+ try:
rel_path = setup_path.relative_to(source_folder) if catalog_folder:
- cmd.extend(["-a", str(Path(catalog_folder).resolve())])
+ cmd.extend(["-a", str(Path(catalog_folder).resolve())]) if enc_node is None:
- # Fallback search
- enc_node = root.find(".//EncryptionInfo")
+ # Fallback search
+ enc_node = root.find(".//EncryptionInfo")Also applies to: 88-89, 200-202
🤖 Prompt for AI Agents
In src/switchcraft_advanced/services/intune_service.py around lines 68, 88-89,
and 200-202, several lines have extra leading spaces that break the file's
indentation: adjust the indentation so the `try:` at line 68 aligns with its
surrounding block level, remove the extra spaces before the `cmd.extend` call on
lines 88-89 so it lines up with other statements in the same block, and fix the
fallback search comment and its surrounding lines (200-202) to the same
indentation level as nearby comments/logic; ensure consistency with the module's
existing indent style (e.g., 4 spaces per indent level) and run a quick
lint/format pass to verify no other indentation errors remain.
| # Implement sliding window for history to prevent unbounded growth | ||
| if len(self.messages) > 20: | ||
| # Keep system prompt (index 0) and last 10 messages | ||
| self.messages = [self.messages[0]] + self.messages[-10:] |
There was a problem hiding this comment.
Critical: Sliding window logic is in the wrong location.
The sliding window implementation to prevent unbounded message history growth is placed in _init_client, which is called during __init__ when self.messages is always empty. This code will never execute because len(self.messages) will be 0 at initialization time.
The sliding window should be moved to _ask_openai (before line 111) where it will actually trim the message history during runtime as messages accumulate across multiple calls.
🔎 Move the sliding window to `_ask_openai`
Remove from _init_client:
def _init_client(self):
try:
- # Implement sliding window for history to prevent unbounded growth
- if len(self.messages) > 20:
- # Keep system prompt (index 0) and last 10 messages
- self.messages = [self.messages[0]] + self.messages[-10:]
-
if self.provider == "openai":Add to _ask_openai before appending the user query:
def _ask_openai(self, user_query: str) -> str:
# ... (Existing OpenAI Logic, compacted) ...
try:
+ # Implement sliding window for history to prevent unbounded growth
+ if len(self.messages) > 20:
+ # Keep system prompt (index 0) and last 10 messages
+ self.messages = [self.messages[0]] + self.messages[-10:]
+
self.messages.append({"role": "user", "content": user_query})Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.8)
57-57: Consider [self.messages[0], *self.messages[-10:]] instead of concatenation
Replace with [self.messages[0], *self.messages[-10:]]
(RUF005)
🤖 Prompt for AI Agents
In src/switchcraft_ai/service.py around lines 54-57, the sliding window that
trims self.messages is currently in _init_client where self.messages is empty
and never trimmed; move that logic into _ask_openai (before line 111) so it
executes at runtime. Specifically, remove the len(self.messages) > 20 check and
crop from _init_client, and insert equivalent code at the start of _ask_openai
(before appending the incoming user query) to keep the system prompt and the
last 10 messages when history exceeds 20 entries.
| self.ai_key = ctk.CTkEntry(row2, show="*", width=200) | ||
| # Load key logic (omitted for brevity, handled by secure store) | ||
| # self.ai_key.insert(0, ...) | ||
| self.ai_key.pack(side="right") | ||
|
|
||
| ctk.CTkLabel(frame_about, text="SwitchCraft", font=ctk.CTkFont(size=24, weight="bold")).pack() | ||
| ctk.CTkLabel(frame_about, text=f"{i18n.get('about_version')}: {__version__}").pack() | ||
| ctk.CTkLabel(frame_about, text=f"{i18n.get('about_dev')}: FaserF").pack() | ||
| # Save Button specifically for AI? Or global? | ||
| # For now rely on manual edits or global save if we add one. |
There was a problem hiding this comment.
Incomplete API key implementation.
The API key entry field is displayed but lacks both load and save logic. Users can type a key but it won't be persisted, leading to confusion.
🔎 Apply this diff to wire up API key loading and saving:
self.ai_key = ctk.CTkEntry(row2, show="*", width=200)
- # Load key logic (omitted for brevity, handled by secure store)
- # self.ai_key.insert(0, ...)
+ # Load from secure storage
+ api_key = SwitchCraftConfig.get_secret("AIApiKey")
+ if api_key:
+ self.ai_key.insert(0, api_key)
self.ai_key.pack(side="right")
- # Save Button specifically for AI? Or global?
- # For now rely on manual edits or global save if we add one.
+ # Save button for API key
+ def save_api_key():
+ key_value = self.ai_key.get()
+ if key_value:
+ SwitchCraftConfig.set_secret("AIApiKey", key_value)
+ messagebox.showinfo(i18n.get("settings_saved"), i18n.get("settings_saved"))
+
+ ctk.CTkButton(frame, text=i18n.get("btn_save") or "Save", command=save_api_key).pack(pady=5)
ctk.CTkLabel(frame, text=i18n.get("privacy_note_local"), font=ctk.CTkFont(size=10), text_color="gray").pack(pady=5)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/switchcraft/assets/lang/de.json (1)
20-20: Duplicate key:settings_savedappears twice.Line 20 has
"settings_saved": "Einstellungen gespeichert."and line 221 has"settings_saved": "Einstellungen erfolgreich gespeichert.". JSON doesn't support duplicate keys - the second definition will override the first. Remove one to avoid confusion.🔎 Remove the duplicate at line 20:
"settings_lang": "Sprache", - "settings_saved": "Einstellungen gespeichert.", "settings_debug": "Debug-Protokollierung",Also applies to: 221-221
♻️ Duplicate comments (8)
src/switchcraft/utils/winget.py (3)
43-48: Command injection vulnerability remains unaddressed.The
queryparameter is directly interpolated into the PowerShell command string without sanitization, allowing command injection via malicious input liketest'; Remove-Item C:\\ -Recurse; '.This was flagged in a previous review. The recommended fix is to escape single quotes for PowerShell single-quoted strings:
+ def _escape_powershell_string(self, value: str) -> str: + """Escape single quotes for PowerShell single-quoted strings.""" + return value.replace("'", "''") + def search_packages(self, query: str) -> List[Dict[str, str]]: ... - ps_script = f"Find-WinGetPackage -Query '{query}' -Source winget | ..." + safe_query = self._escape_powershell_string(query) + ps_script = f"Find-WinGetPackage -Query '{safe_query}' -Source winget | ..."
94-96: Same command injection vulnerability withpackage_id.Apply the same escaping pattern here as recommended for
search_packages.
105-110: Missing return code check before parsing JSON.Unlike
search_packages, this method doesn't verifyproc.returncodebefore parsing output. Add a returncode check for consistency withsearch_packages.src/switchcraft/gui/views/settings_view.py (5)
256-263: Code style issues in certificate detection logic.Previous review flagged: multiple statements on one line (258-259), missing else block (260), and use of
logger.errorinstead oflogger.exception(262). These issues remain unaddressed.
391-393: Stub always returns "stable", ignoring thekeyparameter.This was flagged in a previous review. The method should use
SwitchCraftConfig.get_value(key, default)to read actual user preferences. Per the relevant code snippet fromsrc/switchcraft/utils/config.py,get_valuerespects policy precedence order.
395-397: Update channel selection is not persisted.Previous review flagged that
change_update_channelis a stub. User selections won't be saved. UseSwitchCraftConfig.set_user_preference("UpdateChannel", value.lower())to persist the choice.
376-378: Stub implementation without TODO marker or user feedback.The
_verify_save_intunemethod is wired to the UI but has no implementation. Users clicking the verify button will see no response. Add a TODO comment and temporary user feedback.
189-195: API key entry lacks load and save logic.Previous review flagged that users can type a key but it won't be persisted. The entry field should load from and save to secure storage.
🧹 Nitpick comments (4)
src/switchcraft/gui/views/settings_view.py (1)
98-100: Multiple statements on one line reduce readability.Static analysis flagged E701 violations. Split these into separate lines for better readability.
🔎 Suggested fix:
- if "dev" in v_low: current_channel = "dev" - elif "beta" in v_low: current_channel = "beta" - else: current_channel = "stable" + if "dev" in v_low: + current_channel = "dev" + elif "beta" in v_low: + current_channel = "beta" + else: + current_channel = "stable"src/switchcraft/services/intune_service.py (3)
176-177: Remove redundant imports.
zipfileandbase64are already imported at the top of the file (lines 8-9). These local imports shadow the module-level imports unnecessarily.🔎 Remove redundant imports:
def upload_win32_app(self, token, intunewin_path, app_info, progress_callback=None): """ Uploads a .intunewin package to Intune. app_info: dict with keys: displayName, description, publisher, installCommandLine, uninstallCommandLine """ - import zipfile - import base64 - headers = {"Authorization": f"Bearer {token}", "Content-Type": "application/json"}
142-144: Useraise ... from efor proper exception chaining.Static analysis (B904) recommends using exception chaining to preserve the original traceback.
🔎 Suggested fix:
except Exception as e: logger.exception(f"Authentication failed: {e}") - raise RuntimeError(f"Authentication failed: {e}") + raise RuntimeError(f"Authentication failed: {e}") from e
251-251: Multiple statements on one line.Split for better readability.
🔎 Suggested fix:
- if progress_callback: progress_callback(0.2, "App entity created.") + if progress_callback: + progress_callback(0.2, "App entity created.")Same pattern applies to lines 277 and 293.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(1 hunks)src/switchcraft/assets/lang/de.json(2 hunks)src/switchcraft/assets/lang/en.json(2 hunks)src/switchcraft/gui/views/settings_view.py(2 hunks)src/switchcraft/services/intune_service.py(12 hunks)src/switchcraft/utils/winget.py(3 hunks)tests/test_addons.py(1 hunks)tests/test_phase7.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_addons.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_phase7.py (2)
src/switchcraft/utils/winget.py (2)
WingetHelper(9-127)search_by_name(14-33)src/switchcraft_advanced/utils/winget.py (2)
WingetHelper(12-237)search_by_name(19-28)
src/switchcraft/services/intune_service.py (1)
src/switchcraft_advanced/services/intune_service.py (2)
verify_graph_permissions(142-164)upload_win32_app(166-314)
src/switchcraft/gui/views/settings_view.py (2)
src/switchcraft/utils/i18n.py (1)
get(110-137)src/switchcraft/utils/config.py (2)
get_value(22-59)set_user_preference(96-120)
🪛 GitHub Actions: CI
tests/test_phase7.py
[error] 22-22: TestIntuneService.test_switchcraft_winget_fix assertion failed: expected URL 'https://github.com/microsoft/winget-pkgs/tree/master/manifests/s/FaserF/SwitchCraft' but got 'https://winget.run/pkg/FaserF/SwitchCraft'.
🪛 LanguageTool
README.md
[grammar] ~60-~60: Ensure spelling is correct
Context: ...Enable the debug console on startup (if addon is installed). ## 🧩 Addon System (Mod...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~62-~62: Ensure spelling is correct
Context: ...startup (if addon is installed). ## 🧩 Addon System (Modular Features) SwitchCraft ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~64-~64: Ensure spelling is correct
Context: ...es) SwitchCraft now supports a modular addon system to keep the core application lig...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~71-~71: Ensure spelling is correct
Context: ...ng console for troubleshooting. Manage addons via Settings > Addon Manager or CLI...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~71-~71: Ensure spelling is correct
Context: ...ooting. Manage addons via Settings > Addon Manager or CLI: ```bash # Install all...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 Ruff (0.14.8)
src/switchcraft/services/intune_service.py
53-53: Redundant exception object included in logging.exception call
(TRY401)
141-141: Consider moving this statement to an else block
(TRY300)
143-143: Redundant exception object included in logging.exception call
(TRY401)
144-144: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
176-176: Redefinition of unused zipfile from line 8
Remove definition: zipfile
(F811)
177-177: Redefinition of unused base64 from line 9
Remove definition: base64
(F811)
251-251: Multiple statements on one line (colon)
(E701)
src/switchcraft/gui/views/settings_view.py
98-98: Multiple statements on one line (colon)
(E701)
99-99: Multiple statements on one line (colon)
(E701)
100-100: Multiple statements on one line (colon)
(E701)
255-255: subprocess call: check for execution of untrusted input
(S603)
258-258: Multiple statements on one line (colon)
(E701)
259-259: Multiple statements on one line (colon)
(E701)
260-260: Consider moving this statement to an else block
(TRY300)
261-261: Do not catch blind exception: Exception
(BLE001)
262-262: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
391-391: Unused method argument: key
(ARG002)
🔇 Additional comments (5)
README.md (1)
57-82: Documentation updates look good.The new sections for Global CLI Flags, Addon System, and Recently Added Features are clear and well-organized. They accurately reflect the modular addon architecture introduced in this PR.
Note: Static analysis flagged "addon" as a spelling issue, but this is a valid and widely-used technical term in software development.
src/switchcraft/services/intune_service.py (2)
12-13: Good security improvements with DefusedXML and PyJWT.Using
defusedxmlfor XML parsing protects against XML entity expansion attacks. The JWT decoding withverify_signature=Falseis appropriate here since Microsoft Graph already validated the token.
43-43: Good: Timeouts added to network requests.Adding
timeout=30to network calls prevents the application from hanging indefinitely on network issues. This is a reliability improvement.Also applies to: 137-137
src/switchcraft/assets/lang/de.json (1)
50-52: Translation additions align with UI refactor.The new keys (
no_update_found,update_check_title,settings_hdr_update,settings_hdr_ai,btn_*,lbl_*,settings_saved) are consistent with the Settings view modularization. German translations appear accurate.Also applies to: 211-221
src/switchcraft/assets/lang/en.json (1)
188-198: Translation additions look good.New keys align with the UI refactor and are consistent with the German translations. The key naming follows the established convention (
settings_hdr_*,btn_*,lbl_*).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
tests/test_phase7.py (1)
54-54: Remove unusedkwargsvariable.The
kwargsvariable is unpacked but never used in the test. Since only the command arguments are verified, you can simplify the unpacking.🔎 Apply this diff to remove the unused variable:
- args, kwargs = mock_popen.call_args + args, _ = mock_popen.call_args cmd = args[0]src/switchcraft/services/intune_service.py (5)
7-7: Good security improvements, but remove unused import.Adding
defusedxmlandjwtare excellent security practices. However, the standardxml.etree.ElementTreeimport at Line 7 is now unused sinceDefusedETreplaces it throughout the code.🔎 Remove the unused import:
-import xml.etree.ElementTree as ETAlso applies to: 12-13
53-53: Remove redundant exception object from log message.
logger.exception()automatically includes exception information, so explicitly adding{e}in the message is redundant.🔎 Simplify the log message:
- logger.exception(f"Failed to download IntuneWinAppUtil: {e}") + logger.exception("Failed to download IntuneWinAppUtil")
56-56: Add type hint for optional parameter.The
progress_callbackparameter should have a type hint indicating it's optional.🔎 Add proper type hint:
- def create_intunewin(self, source_folder: str, setup_file: str, output_folder: str, catalog_folder: str = None, quiet: bool = True, progress_callback=None) -> str: + def create_intunewin(self, source_folder: str, setup_file: str, output_folder: str, catalog_folder: str = None, quiet: bool = True, progress_callback=None) -> str:At the top of the file, add:
from typing import Optional, CallableThen update the signature:
- def create_intunewin(self, source_folder: str, setup_file: str, output_folder: str, catalog_folder: str = None, quiet: bool = True, progress_callback=None) -> str: + def create_intunewin(self, source_folder: str, setup_file: str, output_folder: str, catalog_folder: str = None, quiet: bool = True, progress_callback: Optional[Callable[[str], None]] = None) -> str:
160-160: Remove redundant exception object from log message.Same as earlier -
logger.exception()automatically includes exception information.🔎 Simplify the log message:
- logger.exception(f"Authentication failed: {e}") + logger.exception("Authentication failed")
193-194: Remove redundant local imports.
zipfileandbase64are already imported at the module level (lines 8-9). These local re-imports are unnecessary.🔎 Remove the redundant imports:
def upload_win32_app(self, token, intunewin_path, app_info, progress_callback=None): """ Uploads a .intunewin package to Intune. app_info: dict with keys: displayName, description, publisher, installCommandLine, uninstallCommandLine """ - import zipfile - import base64 - headers = {"Authorization": f"Bearer {token}", "Content-Type": "application/json"}src/switchcraft/gui/views/intune_view.py (1)
200-200: Remove unused variable assignment.The variable
outis assigned but never used since the streaming callback provides all output in real-time.🔎 Remove the unused assignment:
# Pass callback for streaming - out = self.intune_service.create_intunewin( + self.intune_service.create_intunewin( source_folder=s_source, setup_file=s_setup, output_folder=s_output, quiet=b_quiet, progress_callback=_log_callback )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/switchcraft/gui/views/intune_view.py(2 hunks)src/switchcraft/services/intune_service.py(13 hunks)tests/test_phase7.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_phase7.py (1)
src/switchcraft/utils/winget.py (2)
WingetHelper(9-127)search_by_name(14-33)
src/switchcraft/services/intune_service.py (1)
src/switchcraft_advanced/services/intune_service.py (3)
create_intunewin(52-119)verify_graph_permissions(142-164)upload_win32_app(166-314)
src/switchcraft/gui/views/intune_view.py (2)
src/switchcraft/utils/i18n.py (1)
get(110-137)src/switchcraft/services/intune_service.py (1)
create_intunewin(56-140)
🪛 GitHub Actions: CI
src/switchcraft/services/intune_service.py
[error] 106-140: PermissionError: [Errno 13] Permission denied: '/tmp/tmp_y1kpqpi/IntuneWinAppUtil.exe'. Error occurred when attempting to run IntuneWinAppUtil.exe via subprocess in create_intunewin. Ensure the binary is executable in the CI environment or adjust file permissions.
🪛 Ruff (0.14.8)
tests/test_phase7.py
54-54: Unpacked variable kwargs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
src/switchcraft/services/intune_service.py
53-53: Redundant exception object included in logging.exception call
(TRY401)
56-56: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
106-106: subprocess call: check for execution of untrusted input
(S603)
136-136: Abstract raise to an inner function
(TRY301)
136-136: Avoid specifying long messages outside the exception class
(TRY003)
158-158: Consider moving this statement to an else block
(TRY300)
160-160: Redundant exception object included in logging.exception call
(TRY401)
161-161: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
161-161: Avoid specifying long messages outside the exception class
(TRY003)
193-193: Redefinition of unused zipfile from line 8
Remove definition: zipfile
(F811)
194-194: Redefinition of unused base64 from line 9
Remove definition: base64
(F811)
268-268: Multiple statements on one line (colon)
(E701)
src/switchcraft/gui/views/intune_view.py
200-200: Local variable out is assigned to but never used
Remove assignment to unused variable out
(F841)
🔇 Additional comments (13)
tests/test_phase7.py (2)
11-25: Previous critical issue resolved - test now matches implementation.The test correctly verifies that
WingetHelper.search_by_namereturns the winget.run URL format (https://winget.run/pkg/FaserF/SwitchCraft) as implemented in the refactored code. The mock setup properly patchessearch_packagesto provide the expected package ID structure, and the assertions align with the URL construction logic.
27-28: Previous major issue resolved - patch target now correct.The patch decorator now correctly targets
switchcraft.services.intune_service.subprocess.Popen, matching the module under test. This ensures the subprocess call is properly mocked where it's looked up byIntuneService.src/switchcraft/services/intune_service.py (7)
43-43: Good addition of timeout parameter.Adding
timeout=30prevents indefinite hangs during tool download.
106-133: Excellent streaming implementation for real-time feedback.The change from
subprocess.runtosubprocess.Popenwith line-by-line streaming is well-implemented and provides real-time feedback to the UI via the progress callback.
154-154: Good addition of timeout parameter.Adding
timeout=30prevents indefinite hangs during authentication.
163-186: Improved JWT verification using PyJWT library.Replacing manual JWT parsing with the PyJWT library is a solid improvement. The
verify_signature=Falseoption is appropriate here since Graph API performs signature verification.
217-217: Excellent security improvement using defusedxml.Replacing unsafe XML parsing with
DefusedET.parseprotects against XML bomb and external entity attacks.
263-263: Good reliability improvements with timeout parameters.Adding timeout parameters to all HTTP requests prevents indefinite hangs and improves application resilience. The 60-second timeout for API calls and 300 seconds for file uploads are reasonable values.
Also applies to: 271-271, 288-288, 306-306, 317-317, 326-326
358-358: Good addition of timeout parameter.Consistent with other HTTP calls in the file.
src/switchcraft/gui/views/intune_view.py (4)
137-146: Good UX improvement with collapsible log area.The collapsible terminal output provides optional detail without cluttering the interface. The default hidden state is user-friendly.
148-154: Clean toggle implementation.The visibility check using
winfo_ismapped()and button label updates are implemented correctly.
192-206: Well-implemented streaming log callback.The callback implementation is thread-safe using
self.after(0, ...)and provides good UX with auto-scrolling. The streaming approach means the returned output string is no longer needed.
183-213: Thoughtful UX for button state management.The button text updates provide helpful feedback during execution without forcing the log area to be visible. The conditional checks ensure the UI behaves appropriately whether the log is shown or hidden.
Description
Please include a summary of the change and which issue is fixed.
Fixes # (issue)
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist:
Summary by CodeRabbit
New Features
Improvements
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.