Docker Web improvements & better security#64
Merged
github-actions[bot] merged 10 commits intomainfrom Jan 24, 2026
Merged
Conversation
github-actions
bot
added
documentation
Contributor
|
Contributor
📝 WalkthroughWalkthroughAdds a FastAPI web server (auth, MFA, SSO), user/config JSON backends, upload endpoint, update checker, Docker/web deployment artifacts and docs, web-mode GUI adjustments (FilePicker/upload, session-first username), CLI early handling, tests and CI workflow updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as User/Browser
participant Server as FastAPI Server
participant Auth as AuthConfigManager
participant Users as UserManager
participant Store as users.json
Browser->>Server: POST /login (username,password)
Server->>Auth: verify_password(username,password)
Auth->>Users: verify_password(username,password)
Users->>Store: read users.json
alt password valid
Auth-->>Server: success
alt MFA required
Server->>Browser: Prompt for TOTP
Browser->>Server: POST /mfa/verify (code)
Server->>Auth: verify_totp(code)
Auth-->>Server: ok
end
Server->>Browser: Set session cookie, redirect /
else invalid
Server->>Browser: 401 Unauthorized
end
sequenceDiagram
participant Browser as User/Browser
participant Server as FastAPI Server
participant OAuth as OAuth Provider
participant Users as UserManager
participant Store as users.json
Browser->>Server: GET /login/entra
Server->>OAuth: Redirect to provider
OAuth->>Browser: Redirect back with code
Browser->>Server: GET /oauth_callback/entra?code=...
Server->>OAuth: Exchange code -> userinfo
alt allow_sso_registration
Server->>Users: create_user(oauth_user)
Users->>Store: write users.json
Server->>Browser: Set session, redirect /
else
Server->>Browser: Redirect /login?failed=1
end
sequenceDiagram
participant Browser as User/Browser
participant Server as FastAPI Server
participant FS as Temp Storage
participant Analyzer as Analysis flow
Browser->>Server: POST /upload (file)
Server->>FS: save to switchcraft_uploads/<tmp>
Server->>Analyzer: initiate analysis(tmp_path)
Analyzer-->>Server: results
Server-->>Browser: JSON (path/info)
Server->>FS: cleanup tmp file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Fix all issues with AI agents
In `@docs/docker_setup.md`:
- Around line 103-107: The nested list under the "Locked Out?" bullet is
mis-indented; adjust the indentation so the sublist items (the commands and
notes referencing `docker exec -it switchcraft sh`, deleting
`server/users.json`, and editing `server/users.json`) are properly nested under
the "Locked Out?" line (use a consistent indent—e.g., four spaces or a tab—for
those sublist lines) and ensure bullet markers remain consistent with the
surrounding list (asterisks). This will resolve MD007 by making "Locked Out?" a
parent bullet and the subsequent lines its nested list items.
In `@pyproject.toml`:
- Around line 29-31: Update the pyproject.toml dependency specs to avoid Python
3.14 breakage: in the test extra, replace or pin httpx to a release that
declares Python 3.14 support (or add a conditional/dev-only constraint to avoid
pulling httpx 0.28.1), and pin bcrypt to a known-compatible version (e.g.,
bcrypt <=4.0.1) or bump/lock passlib to a version that is compatible with bcrypt
(adjust the passlib and bcrypt entries in the [project.optional-dependencies]
test/dev groups accordingly); also consider adding a note or classifier check
for pyotp (or pin if necessary) so CI uses verified packages for Python 3.14.
Ensure you reference the httpx, passlib, bcrypt, and pyotp entries in
pyproject.toml when making these changes and verify in CI that tests run under
Python 3.14.
In `@scripts/prepare_web_dist.py`:
- Around line 34-38: The file imports the Python regex module as "re" but never
uses it, causing a flake8 F401 unused-import; remove the unused import statement
(the "import re" line) from scripts/prepare_web_dist.py so no code references
remain to the symbol "re" and the linter error is resolved.
In `@src/switchcraft/__init__.py`:
- Line 1: The package version constant __version__ in
src/switchcraft/__init__.py ("2026.1.6b1") is inconsistent with the version in
pyproject.toml ("2026.1.5b1"); pick the correct intended release and make both
sources identical by updating either __version__ to match pyproject.toml or
updating the version field in pyproject.toml to match __version__, then run your
packaging/version sanity check to confirm consistency.
In `@src/switchcraft/gui_modern/views/analyzer_view.py`:
- Around line 232-256: The _on_file_upload handler currently rebuilds a temp
path from e.file_name which causes collisions; instead parse the server upload
response (the returned {"uploaded": [...paths...]} array) from the event
(e.response / e.server_response / similar on the upload event) and use the
server-returned absolute saved path for start_analysis and cleanup_path in place
of the reconstructed Path; keep a fallback to the reconstructed path only if the
server response is missing, and still show the error snack when e.error is set.
Update both _on_file_picker_result (which triggers upload) and _on_file_upload
to consume and pass the server-returned path to self.start_analysis.
In `@src/switchcraft/server/app.py`:
- Line 72: The assets static directory is mounted twice using
app.mount("/assets", StaticFiles(directory=str(ASSETS_DIR)), name="assets");
remove the duplicate mount (the later occurrence) so only the initial mount call
remains, ensuring no override of the original route and retaining the first
app.mount with ASSETS_DIR and StaticFiles.
- Around line 243-245: The session cookie is being set without the secure flag;
update the resp.set_cookie calls (where token = serializer.dumps(...) and
resp.set_cookie("sc_session", ... ) appear) to pass secure=True when running in
production by reading a config flag (e.g., app.config or current_app.config key
like SESSION_COOKIE_SECURE or based on ENV/FLASK_ENV) so it remains configurable
for testing; apply the same change to the other occurrences of resp.set_cookie
in this module (the blocks around the other token creation lines referenced in
the comment).
- Around line 802-815: The middleware flet_auth_middleware is incorrectly
bypassing WebSocket upgrade requests with a no-op pass and then issuing a
RedirectResponse for unauthenticated websocket connections; change the WebSocket
handling so that when "websocket" is found in request.headers.get("upgrade",
"").lower() and no user is present the middleware returns await
call_next(request) (i.e., forward the upgrade instead of falling through to
RedirectResponse), keeping the existing whitelist and get_current_user logic
intact.
In `@src/switchcraft/server/user_manager.py`:
- Around line 146-156: The TOTP secrets are being stored in plaintext; update
set_totp_secret and get_totp_secret to encrypt/decrypt using the app's
SimpleCrypto: add or use a SimpleCrypto instance on the UserManager (e.g.,
self.crypto initialized in __init__), then in set_totp_secret call
self.crypto.encrypt(secret) and store the ciphertext in
data["users"][username]["totp_secret"]; in get_totp_secret read the stored
ciphertext and return self.crypto.decrypt(ciphertext) (handling missing values
and decryption errors by returning None). Ensure imports/reference to
SimpleCrypto from switchcraft.utils.crypto and that saved values remain strings.
- Around line 46-60: The default admin user created in _ensure_users_file
currently uses the insecure static password "admin"; instead generate a secure
random password (e.g., using secrets.token_urlsafe) when creating the initial
admin, store only its hash via _hash_password and save via _save_data, add a
flag on the user such as "must_change_password": True to enforce a password
reset on first login, and output or log the generated password exactly once
(using the server logger or stdout) so the operator can record it; update any
login flow to check "must_change_password" and require a new password before
clearing that flag.
In `@src/switchcraft/utils/config.py`:
- Around line 319-327: The get_secure_value and set_secure_value methods store
secrets in plaintext with only a "SECURE_" prefix; update them to emit a clear
warning via the module/class logger when set_secure_value is called (and
optionally when get_secure_value is used) to surface that secrets are stored
unencrypted, add a brief note in the class docstring documenting this limitation
and recommending a production secrets manager (e.g., HashiCorp Vault / AWS
Secrets Manager), and insert a TODO or configuration flag comment in
set_secure_value referencing integration with a real vault for production use so
maintainers know to replace this behavior.
In `@tests/test_server_auth.py`:
- Around line 1-7: The import block in tests/test_server_auth.py contains unused
symbols (pytest, TestClient, MagicMock, patch, AsyncMock, os, secrets, shutil,
Path); remove any imports that are not referenced elsewhere in the file (delete
unused names like MagicMock/AsyncMock/patch/os/secrets/shutil/Path if they
aren't used) so the file only imports the actually used symbols (e.g., pytest,
TestClient) and re-run lint to confirm no unused-import errors.
- Around line 107-164: In test_sso_registration_toggle, fix E712 comparisons:
replace the equality-to-boolean and equality-to-None checks with
identity/idiomatic assertions—change assert conf["allow_sso_registration"] ==
False to assert conf["allow_sso_registration"] is False, and change assert
user_manager.get_user("rejected@example.com") == None to assert
user_manager.get_user("rejected@example.com") is None (also update any other ==
False/== None in this test to use `is False`, `is None`, or `assert not
...`/`assert ...` as appropriate).
- Around line 12-39: The TestClient is instantiated at module import time,
causing the FastAPI lifespan to run before your fixture config overrides; fix by
removing the module-level client and creating TestClient(app) inside a fixture
that runs after setup_test_env (e.g., a new client fixture that yields
TestClient after auth_manager and user_manager config_dir/users_file are set and
_ensure_* called), change tests to accept that client fixture, and replace the
fixed "./temp_test_data" with a unique temp directory (use pytest's tmp_path or
generate a unique subdirectory) to avoid parallel-run collisions; reference
symbols: TestClient, app, setup_test_env, auth_manager, user_manager.
- Around line 165-197: test_feature_flags uses explicit True comparisons and
leaves global feature flags set; change assertions like assert conf["demo_mode"]
== True to truthy checks (e.g., assert conf["demo_mode"]) and similarly for
auth_disabled, and add cleanup at the end of test_feature_flags to reset both
flags via auth_manager.load_config()/client.post to their default values
(demo_mode=False, auth_disabled=False) so state doesn't leak to other tests;
locate the flag reads via auth_manager.load_config() and the setting calls to
client.post("/admin/settings", ...) when applying the changes.
🧹 Nitpick comments (16)
src/switchcraft/server/update_checker.py (2)
37-42: Consider adding a User-Agent header for GitHub API requests.GitHub API recommends including a User-Agent header. While the current implementation works, adding one improves compliance with API guidelines and helps avoid potential rate limiting issues.
Suggested improvement
response = await client.get( GITHUB_RELEASES_URL, - headers={"Accept": "application/vnd.github.v3+json"} + headers={ + "Accept": "application/vnd.github.v3+json", + "User-Agent": f"SwitchCraft/{__version__}" + } )
86-101: Use modern asyncio patterns to avoid deprecation warnings.
asyncio.get_event_loop()emits DeprecationWarning in Python 3.10+ when called from a context where no event loop is running. Useasyncio.get_running_loop()to explicitly detect if already in an async context, or rely onasyncio.run()for top-level synchronous entry points.Suggested improvement
def check_for_updates_sync() -> dict: """Synchronous wrapper for update check.""" import asyncio try: - loop = asyncio.get_event_loop() - if loop.is_running(): + try: + asyncio.get_running_loop() # If we're already in an async context, return a placeholder return { "has_update": False, "current_version": __version__, "error": "Cannot run sync check in async context" } - return loop.run_until_complete(check_for_updates()) - except RuntimeError: - # No event loop, create one - return asyncio.run(check_for_updates()) + except RuntimeError: + # No running event loop, safe to use asyncio.run + return asyncio.run(check_for_updates()) + except Exception as e: + return { + "has_update": False, + "current_version": __version__, + "error": str(e) + }src/switchcraft/utils/config.py (2)
291-301: Specify encoding when reading JSON file.The
open()call should specifyencoding="utf-8"to ensure consistent behavior across platforms, especially on Windows where the default encoding may differ.Suggested fix
def _load(self): try: import json if os.path.exists(self.file_path): - with open(self.file_path, "r") as f: + with open(self.file_path, "r", encoding="utf-8") as f: self._cache = json.load(f)
303-310: Specify encoding when writing JSON file.Same as the
_loadmethod, specifyencoding="utf-8"for consistent cross-platform behavior.Suggested fix
def _save(self): try: import json os.makedirs(os.path.dirname(self.file_path), exist_ok=True) - with open(self.file_path, "w") as f: + with open(self.file_path, "w", encoding="utf-8") as f: json.dump(self._cache, f, indent=4)src/switchcraft/main.py (2)
250-250: Remove unused imports or document if intentionally re-exported.Static analysis flags
JsonFileBackendandSessionStoreBackendas unused. If these are intentionally imported for re-export (as indicated in the AI summary), consider either:
- Adding them to an
__all__list to make the intent explicit- Using them in the code
- Removing if truly unused
Based on static analysis hints.
419-419: Page title hardcoded to "SwitchCraft Web".The title is now hardcoded to "SwitchCraft Web" for all modes. Consider conditionally setting the title based on
page.webto show "SwitchCraft" for desktop and "SwitchCraft Web" for web mode.Suggested improvement
- page.title = "SwitchCraft Web" + page.title = "SwitchCraft Web" if page.web else "SwitchCraft"src/switchcraft/server/user_manager.py (3)
3-3: Remove unused import.The
secretsmodule is imported but never used in this file.Suggested fix
import json import logging -import secrets from pathlib import PathBased on static analysis hints.
24-32: Fix return type hint for_hash_password.The function can return
Nonewhenpassword is None, but the return type hint specifiesstr. Update the type hint to reflect the actual behavior.Suggested fix
- def _hash_password(self, password: str) -> str: + def _hash_password(self, password: Optional[str]) -> Optional[str]: """Hash a password using bcrypt (Python 3.14 compatible).""" if password is None: return None
89-108: Fix inconsistent indentation increate_user.Lines 96-99 use 5-space indentation instead of the standard 4 spaces, causing static analysis errors.
Suggested fix
def create_user(self, username: str, password: str = None, role: str = "user", auto_hash: bool = True) -> bool: data = self._load_data() if username in data["users"]: return False pwd_hash = None if password: - if auto_hash: - pwd_hash = self._hash_password(password) - else: - pwd_hash = password # Already hashed or managed elsewhere + if auto_hash: + pwd_hash = self._hash_password(password) + else: + pwd_hash = password # Already hashed or managed elsewhereBased on static analysis hints.
src/switchcraft/gui_modern/views/settings_view.py (2)
1193-1206: Fix inconsistent indentation inexport_logsbranch.Lines 1194-1206 use inconsistent indentation (5 spaces instead of 4), causing static analysis errors.
Suggested fix
elif context == "export_logs": - if e.path: - try: - # e.path is the destination - # We need to copy from self._log_source_path (set in _export_logs) - src = getattr(self, "_log_source_path", None) - if src: - import shutil - shutil.copy2(src, e.path) - self._show_snack(f"Logs exported to {e.path}", "GREEN") - else: - self._show_snack("Log copy failed: Source lost", "RED") - except Exception as ex: - self._show_snack(f"Log export failed: {ex}", "RED") + if e.path: + try: + # e.path is the destination + # We need to copy from self._log_source_path (set in _export_logs) + src = getattr(self, "_log_source_path", None) + if src: + import shutil + shutil.copy2(src, e.path) + self._show_snack(f"Logs exported to {e.path}", "GREEN") + else: + self._show_snack("Log copy failed: Source lost", "RED") + except Exception as ex: + self._show_snack(f"Log export failed: {ex}", "RED")Based on static analysis hints.
1231-1242: Fix inconsistent indentation in log directory fallback.Lines 1232-1242 use inconsistent indentation (5 spaces instead of 4).
Suggested fix
else: - # Find in log dir - app_data = os.getenv('APPDATA') - if app_data: - log_dir = Path(app_data) / "FaserF" / "SwitchCraft" / "Logs" - else: - log_dir = Path.home() / ".switchcraft" / "logs" - - if log_dir.exists(): - log_files = sorted(log_dir.glob("SwitchCraft_Session_*.log"), key=os.path.getmtime, reverse=True) - if log_files: - source_path = log_files[0] + # Find in log dir + app_data = os.getenv('APPDATA') + if app_data: + log_dir = Path(app_data) / "FaserF" / "SwitchCraft" / "Logs" + else: + log_dir = Path.home() / ".switchcraft" / "logs" + + if log_dir.exists(): + log_files = sorted(log_dir.glob("SwitchCraft_Session_*.log"), key=os.path.getmtime, reverse=True) + if log_files: + source_path = log_files[0]Based on static analysis hints.
src/switchcraft/server/auth_config.py (3)
1-11: Remove unused import.Static analysis correctly identified that
osis imported but never used.Proposed fix
import json -import os import secrets from pathlib import Path
86-103: Default admin password is intentionally weak - ensure first-run warning is prominent.The default "admin" password is acceptable for initial setup since the
first_runflag triggers a security warning in the login UI. However, consider adding a log warning at startup when default credentials are active.Optional: Add startup warning log
def _create_default_config(self) -> Dict: # User requested default "admin" default_hash = self._hash_password("admin") + logger.warning("Creating default config with default admin password - change immediately!") config = {
58-65: Fix indentation to be a multiple of 4 spaces.The migration block has inconsistent indentation that triggers multiple flake8 errors. This affects readability and violates PEP 8.
Proposed fix
if "admin_password_hash" not in data and "admin_password" in data: - # Migrate plain to hash - try: - data["admin_password_hash"] = self._hash_password(data["admin_password"]) - del data["admin_password"] - self.save_config(data) - except Exception as e: - logger.error(f"Migration failed: {e}") + # Migrate plain to hash + try: + data["admin_password_hash"] = self._hash_password(data["admin_password"]) + del data["admin_password"] + self.save_config(data) + except Exception as e: + logger.error(f"Migration failed: {e}")src/switchcraft/server/app.py (2)
1-26: Remove unused imports.Multiple imports are flagged as unused by static analysis:
json,Optional,BackgroundTasks,JSONResponse,FletApp,SwitchCraftConfig.Proposed fix
import logging import os -import json from contextlib import asynccontextmanager from pathlib import Path -from typing import Optional -from fastapi import FastAPI, Request, Form, Depends, HTTPException, status, BackgroundTasks, UploadFile -from fastapi.responses import HTMLResponse, RedirectResponse, JSONResponse +from fastapi import FastAPI, Request, Form, Depends, HTTPException, status, UploadFile +from fastapi.responses import HTMLResponse, RedirectResponse from fastapi.staticfiles import StaticFiles from itsdangerous import URLSafeTimedSerializer import httpx import tempfile import shutil import flet as ft import pyotp -from flet.fastapi import FletApp from switchcraft.main import main as flet_main from switchcraft.server.auth_config import AuthConfigManager from switchcraft.server.user_manager import UserManager import switchcraft -from switchcraft.utils.config import SwitchCraftConfig from switchcraft.server.update_checker import check_for_updates
823-835: Upload filename sanitization could be bypassed.The sanitization only keeps alphanumeric characters and
-_., but doesn't prevent:
- Empty filenames after sanitization
- Files starting with
.(hidden files)- Very long filenames
Enhanced sanitization
`@app.post`("/upload") async def upload_endpoint(files: list[UploadFile]): saved_files = [] for file in files: safe_name = "".join(x for x in file.filename if x.isalnum() or x in "-_.") + # Prevent empty names, hidden files, and limit length + safe_name = safe_name.lstrip(".") + if not safe_name or len(safe_name) > 255: + safe_name = f"upload_{secrets.token_hex(8)}" path = UPLOAD_DIR / safe_name
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Around line 74-75: The sed-based replacement for the manifest "version" key
won't add the key if it doesn't exist; replace the sed line that targets
"version" with a JSON-safe update (e.g., use jq to set .version = env.VERSION or
a short Python one-liner) so the "version" field is inserted when missing and
updated when present (update the workflow step that currently runs sed on
src/switchcraft/assets/manifest.json to use jq/Python to set .version to
$VERSION).
In `@src/switchcraft/gui_modern/views/settings_view.py`:
- Around line 1150-1159: The current _import_settings handler calls
file_picker.pick_files but assumes the FilePickerResultEvent will supply a local
path (e.files[0].path), which is always None in web builds; modify
_import_settings to detect web/no-path cases and either show a user-facing
message disabling import on web or implement a web upload flow: after calling
file_picker.pick_files (or in the subsequent FilePickerResultEvent handler),
check if e.files and e.files[0].path is None (or use a runtime web flag) and
then either call an alternative routine that reads file bytes from the event
(for web) or present a dialog/notification that import is not supported in web;
keep references to _import_settings, file_picker.pick_files and
FilePickerResultEvent when implementing the branching so reviewers can locate
the change.
In `@src/switchcraft/server/app.py`:
- Around line 787-804: admin_reset deletes the users file but doesn't recreate
the default admin, leaving the system locked until restart; after removing
users_path (user_manager.users_file) call a user-manager helper to recreate and
persist the default admin immediately (e.g., user_manager.create_default_admin()
or user_manager.ensure_default_admin_exists()), or implement such a helper that
initializes the default admin and writes users.json, handling and logging any
errors before returning the redirect response from admin_reset.
- Around line 841-869: The upload_endpoint currently writes deterministic
clean_name files to UPLOAD_DIR which can be overwritten by concurrent uploads;
modify the logic after computing clean_name and before opening the file (where
path = UPLOAD_DIR / clean_name) to ensure a unique collision-safe path: if
path.exists(), append a short UUID hex (preserving extension) or create a
per-upload subdirectory named by uuid4 and use that path, repeating until the
candidate path is not present; ensure you use the final unique path when
opening/writing the file and when appending to saved_files (refer to
upload_endpoint, clean_name, path, UPLOAD_DIR, saved_files).
In `@src/switchcraft/server/user_manager.py`:
- Around line 51-60: Remove any logger.info calls that persist the
generated_password value; do not log the plaintext secret. Instead, keep the
stdout print statements that display generated_password and replace the
logger.info lines that contain the password with a single generic log (e.g.,
logger.info or logger.warning) such as "Initial admin password generated and
printed to stdout" so the secret is only sent to stdout. Update references
around generated_password and the logger.* calls in the block where the password
is created to ensure no plaintext credential is written to logs.
In `@tests/test_server_auth.py`:
- Around line 69-77: Remove the unused variable assignments to token (e.g., the
lines like token = resp.cookies["sc_session"]) in tests/test_server_auth.py;
keep the client.cookies.set("sc_session", resp.cookies["sc_session"]) and any
other cookie-setting logic, but delete the redundant token = ... assignments at
the occurrences flagged (the one after the admin login and the similar ones
later) so the tests no longer create an unused variable named token.
🧹 Nitpick comments (2)
scripts/prepare_web_dist.py (1)
34-59: Make CSS injection idempotent to avoid duplicate<style>blocks.
If this script runs multiple times, it will keep re-inserting the same CSS. Add a marker check to avoid duplication.♻️ Suggested fix (idempotent injection)
- custom_css = """ - <style> + custom_css = """ + <style> + /* switchcraft-loading-style */ `#loading` { background-color: `#1a1c1e` !important; display: flex; justify-content: center; align-items: center; flex-direction: column; } .lds-ring div { border-color: `#0066cc` transparent transparent transparent !important; } @@ - if "</head>" in content: + if "</head>" in content and "switchcraft-loading-style" not in content: content = content.replace("</head>", f"{custom_css}\n</head>")src/switchcraft/gui_modern/views/analyzer_view.py (1)
243-267: Harden upload-response parsing to avoid collisions if Flet’s field name differs.If
e.dataisn’t the actual server response for your Flet version, the fallback path reconstructs a deterministic filename and can reintroduce collision risk. Consider probing common response fields and adding a UUID suffix in the fallback.♻️ Suggested hardening
- analysis_path = None - if e.data: - try: - import json - resp = json.loads(e.data) + analysis_path = None + raw_resp = None + for attr in ("data", "response", "server_response"): + raw_resp = getattr(e, attr, None) or raw_resp + if raw_resp: + try: + import json + resp = json.loads(raw_resp) uploaded = resp.get("uploaded", []) if uploaded and len(uploaded) > 0: analysis_path = uploaded[0] logger.info(f"Using server-returned path: {analysis_path}") except Exception as ex: logger.warning(f"Failed to parse server upload response: {ex}") # Fallback to local reconstruction if server response missing/invalid if not analysis_path: # Reconstruct (Basic cleanup matching app.py logic) fname = e.file_name clean_name = "".join(x for x in fname if x.isalnum() or x in "-_.").lstrip(".") - analysis_path = str(Path(tempfile.gettempdir()) / "switchcraft_uploads" / clean_name) + import uuid + stem = Path(clean_name).stem + suffix = Path(clean_name).suffix + clean_name = f"{stem}_{uuid.uuid4().hex[:8]}{suffix}" + analysis_path = str(Path(tempfile.gettempdir()) / "switchcraft_uploads" / clean_name) logger.info(f"Falling back to reconstructed path: {analysis_path}")
Comment on lines
+91
to
+107
| def _create_default_config(self) -> Dict: | ||
| # User requested default "admin" | ||
| default_hash = self._hash_password("admin") | ||
|
|
||
| config = { | ||
| "admin_password_hash": default_hash, | ||
| "secret_key": secrets.token_hex(32), | ||
| "demo_mode": False, | ||
| "auth_disabled": False, | ||
| "allow_sso_registration": True, | ||
| "mfa_enabled": False, | ||
| "enforce_mfa": False, | ||
| "session_cookie_secure": False, | ||
| "totp_secret": "", # Base32 secret | ||
| "webauthn_credentials": [], # List of registered credentials | ||
| "first_run": True | ||
| } |
Contributor
There was a problem hiding this comment.
Default "admin" password is insecure and inconsistent with UserManager’s random initial admin.
_create_default_config() still hardcodes "admin". This weak default is used if users.json is missing (e.g., after reset), and it also drives the misleading first‑run warning. Consider generating a random password (and logging/printing it once) or delegating the initial admin credential to UserManager entirely.
🔐 Suggested direction (random default)
- # User requested default "admin"
- default_hash = self._hash_password("admin")
+ # Generate a secure random admin password for first run
+ default_password = secrets.token_urlsafe(16)
+ default_hash = self._hash_password(default_password)
+ logger.warning("Initial admin password generated (see stdout).")
+ print(f"\n!!! INITIAL ADMIN PASSWORD: {default_password} !!!\n")
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @.github/workflows/docs_preview.yml:
- Around line 54-68: The heredoc used to generate build_web/web_entry.py
currently preserves the leading spaces causing an IndentationError when the
generated Python file is executed; update the command that writes the file to
strip the leading indentation by changing the heredoc opener from << to <<- (or
by removing the extra indentation inside the heredoc), so the generated file
contains left-aligned Python code (affecting the block that creates web_entry.py
which imports os, sys, flet as ft, the WASM patch block, sys.path insertion,
switchcraft.IS_DEMO assignment, and the ft.run call).
In `@docs/docker_setup.md`:
- Around line 38-67: Add a short "Production Secret Management" note after the
SSO examples (near the "Single Sign-On (SSO) Setup" section) that warns against
passing secrets via command-line `docker run` and recommends secure
alternatives: using Docker Compose with .env files (secure file permissions),
Docker Secrets for Swarm, and external secret managers like HashiCorp Vault or
AWS Secrets Manager; reference the SSO examples that set SC_ENTRA_* and
SC_GITHUB_* environment variables so readers know which secrets to protect.
- Around line 78-80: Update the "Disable Auth" bullet under Server Settings to
include a prominent, bold security warning stating that enabling "Disable Auth"
(Auto-Login as Guest/Admin) disables all authentication and should only be used
in isolated, non-production environments; mention the risk (full public admin
access), recommend alternatives (use Demo Mode or enable proper auth), and
advise adding an explicit confirmation step or environment-only guard around the
Disable Auth option to prevent accidental use.
- Around line 30-31: Replace the casual parenthetical about the default password
with a prominent, emphatic warning block that calls out the default credentials
"**Username:** `admin`" and "**Password:** `admin`" as insecure for production;
explicitly state "DO NOT USE IN PRODUCTION" and instruct the operator to change
the admin password immediately, rotate any existing secrets, and store new
credentials in a secure secrets manager or environment variables before exposing
the service. Ensure the warning block appears directly below the credentials
lines so it's highly visible in the docs.
- Around line 104-107: Fix the "Locked Out?" sublist indentation to use
consistent 4-space indentation for the nested bullets under that heading and add
a safety warning telling users to stop the running container before modifying or
deleting server/users.json to avoid data corruption; update the two nested items
that currently read "docker exec -it switchcraft sh" and "Delete
server/users.json" (and the manual-edit note) to be indented uniformly and
prepend a short safety sentence referencing stopping the container (e.g., stop
the 'switchcraft' container) before running those steps.
🧹 Nitpick comments (5)
docs/docker_setup.md (4)
5-24: Add security warning in Quick Start section.The Quick Start section should immediately alert users to change the default credentials after first login. Consider adding a security notice after line 23 linking to the Authentication section or explicitly mentioning the default admin/admin credentials with a prominent warning.
📝 Suggested addition
Add after line 23:
> **⚠️ Security Notice:** The default login is `admin` / `admin`. Change this immediately after first login via the Admin Panel at `/admin`.
85-94: Add backup and secret key management guidance.The Persistence section should include guidance on:
- Backing up the data volume regularly (especially before updates)
- Verifying that auto-generated secret keys in
auth_config.jsonare sufficiently random- Secret key rotation procedures if needed
📝 Suggested additions
Add after line 94:
### Backup Recommendations ```bash # Backup the data volume docker run --rm -v switchcraft_data:/data -v $(pwd):/backup alpine tar czf /backup/switchcraft-backup-$(date +%Y%m%d).tar.gz /data # Restore from backup docker run --rm -v switchcraft_data:/data -v $(pwd):/backup alpine tar xzf /backup/switchcraft-backup-YYYYMMDD.tar.gz -C /Note: Secret keys in
auth_config.jsonare auto-generated on first run. Ensure backups are stored securely as they contain sensitive authentication data.</details> --- `98-98`: **Avoid time-specific future feature statements.** The phrase "for the next release" may become stale as releases progress. Consider rephrasing to remove the specific timeline reference. <details> <summary>📝 Suggested revision</summary> ```diff -* **Client Settings:** Currently, app preferences (Theme, etc.) are stored in the **Browser** (LocalStorage) to ensure fast load times. We are working on server-side roaming profiles for the next release. +* **Client Settings:** Currently, app preferences (Theme, etc.) are stored in the **Browser** (LocalStorage) to ensure fast load times. Server-side roaming profiles are planned for a future release.
1-107: Consider adding production deployment guidance.For users deploying SwitchCraft in production environments, consider adding sections covering:
- HTTPS/TLS Setup: Guidance on using reverse proxies (nginx, Traefik) for SSL termination
- Container Updates: Procedures for updating to new versions without data loss
- Resource Limits: Recommended CPU/memory limits for production
- Monitoring & Logging: How to access and manage application logs
- Network Security: Firewall configuration, network isolation best practices
These additions would make the guide more comprehensive for production deployments while maintaining its accessibility for development/testing scenarios.
.github/workflows/docs_preview.yml (1)
42-45: Consider extracting the complex one-liner to a helper script.The Python one-liner on line 45 is functional but difficult to read and maintain. For better maintainability, consider moving this logic to a dedicated script or at least formatting it across multiple lines.
♻️ Suggested improvement
- name: Pre-install Addons (Web) run: | mkdir -p src/switchcraft/addons - python -c "import zipfile, os, glob; from pathlib import Path; [zipfile.ZipFile(z).extractall(Path('src/switchcraft/addons') / Path(z).stem.replace('switchcraft_', '')) for z in glob.glob('src/switchcraft/assets/addons/*.zip')]" + python scripts/extract_addons.pyOr inline with better readability:
- python -c "import zipfile, os, glob; from pathlib import Path; [zipfile.ZipFile(z).extractall(Path('src/switchcraft/addons') / Path(z).stem.replace('switchcraft_', '')) for z in glob.glob('src/switchcraft/assets/addons/*.zip')]" + python -c " + import zipfile, glob + from pathlib import Path + for z in glob.glob('src/switchcraft/assets/addons/*.zip'): + zipfile.ZipFile(z).extractall( + Path('src/switchcraft/addons') / Path(z).stem.replace('switchcraft_', '') + ) + "
Comment on lines
+54
to
+68
| cat > build_web/web_entry.py <<EOF | ||
| import os | ||
| import sys | ||
| import flet as ft | ||
| # WASM compatibility patches | ||
| if sys.platform == "emscripten": | ||
| import pyodide_http | ||
| pyodide_http.patch_all() | ||
| sys.path.insert(0, os.getcwd()) | ||
| import switchcraft | ||
| switchcraft.IS_DEMO = True | ||
| import switchcraft.main | ||
| if __name__ == "__main__": | ||
| ft.run(switchcraft.main.main, assets_dir="assets") | ||
| EOF |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the actual docs_preview.yml file
fd "docs_preview.yml" --type fRepository: FaserF/SwitchCraft
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for workflow files in .github directory
find .github -name "*.yml" -o -name "*.yaml" 2>/dev/nullRepository: FaserF/SwitchCraft
Length of output: 737
🏁 Script executed:
#!/bin/bash
# Read the actual docs_preview.yml file around lines 54-68
cat -n .github/workflows/docs_preview.yml | sed -n '50,75p'Repository: FaserF/SwitchCraft
Length of output: 1248
🏁 Script executed:
#!/bin/bash
# Test the actual heredoc behavior with the indentation as shown in the workflow
# This simulates what happens in the GitHub Actions environment
cat > /tmp/test_workflow.sh <<'OUTER'
#!/bin/bash
# Simulate the exact workflow structure with indentation
bash -c '
cat > /tmp/web_entry.py <<EOF
import os
import sys
import flet as ft
# WASM compatibility patches
if sys.platform == "emscripten":
import pyodide_http
pyodide_http.patch_all()
sys.path.insert(0, os.getcwd())
import switchcraft
switchcraft.IS_DEMO = True
import switchcraft.main
if __name__ == "__main__":
ft.run(switchcraft.main.main, assets_dir="assets")
EOF
'
OUTER
bash /tmp/test_workflow.sh
# Now check the generated file for syntax errors
echo "=== Checking Python syntax ==="
python -m py_compile /tmp/web_entry.py 2>&1 || true
echo ""
echo "=== First 5 lines of generated file with visible spaces ==="
head -5 /tmp/web_entry.py | cat -ARepository: FaserF/SwitchCraft
Length of output: 479
Fix heredoc indentation in web_entry.py generation — will cause IndentationError at runtime.
The heredoc content has 10 leading spaces on each line. When the file is generated, these spaces become part of the Python file, causing an IndentationError on line 1. Use <<- instead of << to strip leading tabs, or remove the indentation from the heredoc content:
cat > build_web/web_entry.py <<-EOF
import os
import sys
import flet as ft
# WASM compatibility patches
if sys.platform == "emscripten":
import pyodide_http
pyodide_http.patch_all()
sys.path.insert(0, os.getcwd())
import switchcraft
switchcraft.IS_DEMO = True
import switchcraft.main
if __name__ == "__main__":
ft.run(switchcraft.main.main, assets_dir="assets")
EOF🤖 Prompt for AI Agents
In @.github/workflows/docs_preview.yml around lines 54 - 68, The heredoc used to
generate build_web/web_entry.py currently preserves the leading spaces causing
an IndentationError when the generated Python file is executed; update the
command that writes the file to strip the leading indentation by changing the
heredoc opener from << to <<- (or by removing the extra indentation inside the
heredoc), so the generated file contains left-aligned Python code (affecting the
block that creates web_entry.py which imports os, sys, flet as ft, the WASM
patch block, sys.path insertion, switchcraft.IS_DEMO assignment, and the ft.run
call).
Comment on lines
+30
to
+31
| - **Username:** `admin` | ||
| - **Password:** `admin` (You should change this immediately in the Admin Panel) |
Contributor
There was a problem hiding this comment.
Strengthen security warning for default credentials.
The note to change the default password should be more emphatic for production deployments. Consider using stronger language and making this a prominent warning block rather than a casual parenthetical note.
🔒 Suggested improvement
### Default Login
- **Username:** `admin`
-- **Password:** `admin` (You should change this immediately in the Admin Panel)
+- **Password:** `admin`
+
+> **🔴 SECURITY WARNING:** The default password `admin` is publicly known. You **must** change it immediately after first login via the Admin Panel (`/admin` → Security → Change Password). Failure to do so leaves your instance vulnerable to unauthorized access.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **Username:** `admin` | |
| - **Password:** `admin` (You should change this immediately in the Admin Panel) | |
| ### Default Login | |
| - **Username:** `admin` | |
| - **Password:** `admin` | |
| > **🔴 SECURITY WARNING:** The default password `admin` is publicly known. You **must** change it immediately after first login via the Admin Panel (`/admin` → Security → Change Password). Failure to do so leaves your instance vulnerable to unauthorized access. |
🤖 Prompt for AI Agents
In `@docs/docker_setup.md` around lines 30 - 31, Replace the casual parenthetical
about the default password with a prominent, emphatic warning block that calls
out the default credentials "**Username:** `admin`" and "**Password:** `admin`"
as insecure for production; explicitly state "DO NOT USE IN PRODUCTION" and
instruct the operator to change the admin password immediately, rotate any
existing secrets, and store new credentials in a secure secrets manager or
environment variables before exposing the service. Ensure the warning block
appears directly below the credentials lines so it's highly visible in the docs.
Comment on lines
+38
to
+67
| ### Single Sign-On (SSO) Setup | ||
| SwitchCraft supports OAuth2 login via **GitHub** and **Microsoft Entra ID (Office 365)**. | ||
| To enable SSO, you must provide the following environment variables when running the Docker container. | ||
|
|
||
| #### 1. Microsoft Entra ID (O365) | ||
| Register an App in Azure AD -> App Registrations. | ||
| * **Redirect URI:** `http://<your-domain>:8080/oauth_callback/entra` | ||
| * **API Permissions:** `User.Read` (Delegated) | ||
|
|
||
| ```bash | ||
| docker run -d ... \ | ||
| -e SC_ENTRA_CLIENT_ID="<your-client-id>" \ | ||
| -e SC_ENTRA_TENANT_ID="<your-tenant-id>" \ | ||
| -e SC_ENTRA_CLIENT_SECRET="<your-client-secret>" \ | ||
| -e SC_BASE_URL="http://myserver.com:8080" \ | ||
| switchcraft-web | ||
| ``` | ||
|
|
||
| *Users logging in via Entra for the first time will be **automatically created** as normal users.* | ||
|
|
||
| #### 2. GitHub | ||
| Register an OAuth App in GitHub Developer Settings. | ||
| * **Callback URL:** `http://<your-domain>:8080/oauth_callback/github` | ||
|
|
||
| ```bash | ||
| docker run -d ... \ | ||
| -e SC_GITHUB_CLIENT_ID="<your-client-id>" \ | ||
| -e SC_GITHUB_CLIENT_SECRET="<your-client-secret>" \ | ||
| switchcraft-web | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add guidance on secure secret management for production.
The examples show secrets passed as environment variables directly in docker run commands. While this works for testing, it's not secure for production as secrets may be exposed in shell history, process listings, and container inspect output.
Consider adding a note recommending:
- Using
.envfiles withdocker-compose(with appropriate file permissions) - Docker secrets for swarm deployments
- Secret management systems (Vault, AWS Secrets Manager, etc.) for production
📝 Suggested addition
Add after line 67:
> **🔒 Production Secret Management:**
> For production deployments, avoid passing secrets via command-line arguments.
> Use:
> - Docker Compose with `.env` files (set permissions to `600`)
> - Docker Secrets for Swarm mode
> - External secret management systems (HashiCorp Vault, AWS Secrets Manager, etc.)🤖 Prompt for AI Agents
In `@docs/docker_setup.md` around lines 38 - 67, Add a short "Production Secret
Management" note after the SSO examples (near the "Single Sign-On (SSO) Setup"
section) that warns against passing secrets via command-line `docker run` and
recommends secure alternatives: using Docker Compose with .env files (secure
file permissions), Docker Secrets for Swarm, and external secret managers like
HashiCorp Vault or AWS Secrets Manager; reference the SSO examples that set
SC_ENTRA_* and SC_GITHUB_* environment variables so readers know which secrets
to protect.
Comment on lines
+78
to
+80
| 2. **Server Settings**: | ||
| * **Demo Mode:** Switch app to Read-Only mode. | ||
| * **Disable Auth:** Allow public access without login (Auto-Login as Guest/Admin). |
Contributor
There was a problem hiding this comment.
Add security warning for "Disable Auth" feature.
The "Disable Auth" feature that allows auto-login as Admin is extremely dangerous and should have a prominent security warning. This effectively disables all security protections and should only be used in completely isolated, non-production environments.
⚠️ Suggested addition
2. **Server Settings**:
* **Demo Mode:** Switch app to Read-Only mode.
- * **Disable Auth:** Allow public access without login (Auto-Login as Guest/Admin).
+ * **Disable Auth:** Allow public access without login (Auto-Login as Guest/Admin).
+ > **⚠️ WARNING:** Disabling authentication removes all access controls. Only use this in isolated development/testing environments. Never enable this on internet-accessible instances.🤖 Prompt for AI Agents
In `@docs/docker_setup.md` around lines 78 - 80, Update the "Disable Auth" bullet
under Server Settings to include a prominent, bold security warning stating that
enabling "Disable Auth" (Auto-Login as Guest/Admin) disables all authentication
and should only be used in isolated, non-production environments; mention the
risk (full public admin access), recommend alternatives (use Demo Mode or enable
proper auth), and advise adding an explicit confirmation step or
environment-only guard around the Disable Auth option to prevent accidental use.
Comment on lines
+104
to
+107
| * **Locked Out?** | ||
| * `docker exec -it switchcraft sh` | ||
| * Delete `server/users.json` to reset the database (recreating default admin). | ||
| * Or manually edit `server/users.json` if you know JSON syntax. |
Contributor
There was a problem hiding this comment.
Fix list indentation and add safety warning.
The nested list items under "Locked Out?" have inconsistent indentation. Additionally, the instruction to delete users.json should include a safety note about stopping the container first to prevent data corruption.
✍️ Proposed fix
* **Locked Out?**
- * `docker exec -it switchcraft sh`
- * Delete `server/users.json` to reset the database (recreating default admin).
- * Or manually edit `server/users.json` if you know JSON syntax.
+ * `docker exec -it switchcraft sh`
+ * Stop the container (`docker stop switchcraft`) then delete `server/users.json` to reset the database (recreating default admin).
+ * Or manually edit `server/users.json` if you know JSON syntax (while container is stopped).Note: Changed indentation to 4 spaces for nested items and added safety guidance about stopping the container before modifying the database.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
105-105: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
105-105: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
106-106: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
106-106: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
107-107: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
107-107: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🤖 Prompt for AI Agents
In `@docs/docker_setup.md` around lines 104 - 107, Fix the "Locked Out?" sublist
indentation to use consistent 4-space indentation for the nested bullets under
that heading and add a safety warning telling users to stop the running
container before modifying or deleting server/users.json to avoid data
corruption; update the two nested items that currently read "docker exec -it
switchcraft sh" and "Delete server/users.json" (and the manual-edit note) to be
indented uniformly and prepend a short safety sentence referencing stopping the
container (e.g., stop the 'switchcraft' container) before running those steps.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Around line 75-76: The release workflow step uses python -c which reads
os.environ['VERSION'] but VERSION isn't exported here, causing a KeyError; fix
by exporting the VERSION environment variable (or passing it as an argument)
before running the python one-liner so the python process can access it; update
the step that runs the python -c line (the command referencing
'src/switchcraft/assets/manifest.json' and os.environ['VERSION']) to either
export VERSION into the environment or change the invocation to pass VERSION as
an argument and read from sys.argv in the python snippet.
In `@src/switchcraft/gui_modern/views/analyzer_view.py`:
- Around line 248-292: The fallback path reconstruction is incorrectly nested
inside the except block so it only runs on JSON parse errors; pull the fallback
block (the uuid/tempfile/clean_name reconstruction that sets analysis_path and
logs "Falling back...") out of the try/except and run it whenever analysis_path
is falsy by adding an "if not analysis_path:" guard after the try/except that
performs the uuid.uuid4-based filename cleanup (using e.file_name, clean_name,
tempfile.gettempdir(), Path) and sets analysis_path before calling
Path(analysis_path).exists() and start_analysis().
In `@src/switchcraft/gui_modern/views/settings_view.py`:
- Around line 1123-1283: The FilePicker callback handlers call SwitchCraftConfig
methods without restoring the backend; ensure the config backend is
re-initialized by invoking SwitchCraftConfig._ensure_backend() (or the
appropriate public ensure method if available) at the start of the user-facing
functions that trigger or handle file IO—specifically in _export_settings,
_import_settings, _on_file_picker_result (before any
SwitchCraftConfig.export_preferences()/import_preferences() usage), and
_export_logs—so the active backend is guaranteed before reading or writing
preferences or logs.
In `@src/switchcraft/main.py`:
- Around line 75-78: Adjust the indentation in the "--factory-reset" branch so
the local import line for SwitchCraftConfig matches the surrounding block
indentation (use 12 spaces instead of 13) to fix the E114 Flake8 error; locate
the if "--factory-reset" conditional and the try: block and re-indent the line
that reads "from switchcraft.utils.config import SwitchCraftConfig" to align
with the subsequent lines in that block.
In `@src/switchcraft/server/app.py`:
- Around line 559-561: The variable entra_tenant is assigned but never used;
either remove the unused assignment or surface it where SSO config is
assembled/displayed: update the code that constructs the SSO settings (the same
place that uses entra_client and github_client) to include entra_tenant in the
returned/configured SSO data or, if not needed, delete the entra_tenant
assignment to eliminate the unused variable. Ensure you reference the existing
variables entra_client, entra_tenant, and github_client so the change is
consistent with current SSO configuration handling.
- Around line 272-282: The current default_pw_warning string in app.py
incorrectly states the default password is "admin"; update the message in the
default_pw_warning assignment and the duplicate warning used in the POST /login
handler to reflect that a random password is generated by
UserManager._ensure_users_file() and printed to stdout (so users should check
the server logs or the initial startup output or use the Admin Panel to reset
credentials) — remove any hardcoded username/password example and instead
instruct users to check the startup log or visit the Admin Panel to set/change
the password; modify the messages where default_pw_warning is defined and where
the POST /login handler constructs its warning to use the new wording.
- Around line 715-719: The delete_user_route allows an admin to delete their own
account; add a guard that compares the requested username to the current
authenticated admin (the dependency parameter u passed into delete_user_route)
and return an error (e.g., HTMLResponse(..., 400)) if they match to prevent
self-deletion; additionally, ensure you do not remove the last remaining admin
by checking user_manager for an admin count or list (e.g.,
user_manager.get_admin_count() or user_manager.list_admins()) and refuse
deletion if it would leave zero admins—if those helpers don't exist, add a
method to user_manager to compute admin count before calling
user_manager.delete_user(username).
- Around line 538-545: The users_html construction inserts unescaped user data
(e.g., u['username'], u['role']) into HTML, enabling XSS; update the rendering
to HTML-escape all user-supplied values before concatenation (use a standard
helper like html.escape or flask.escape when building users_html and when
setting the hidden form value and cell text) so usernames, roles and
active_status are safe; also apply the same escaping to the admin page header
variable (the {user} insertion) to prevent script injection in the header.
♻️ Duplicate comments (1)
.github/workflows/docs_preview.yml (1)
54-68: Verify heredoc output is left-aligned to avoid Python indentation errors.
If the heredoc preserves leading spaces,web_entry.pycan fail withIndentationError. Please confirm the generated file has no unintended leading whitespace. If it does, switch to<<-EOFwith tab-indented content or left-align the heredoc body.#!/bin/bash # Run after the step executes (or locally after generating build_web/web_entry.py) # Expect: no leading spaces before 'import os' sed -n '1,5p' build_web/web_entry.py | cat -A python -m py_compile build_web/web_entry.py
🧹 Nitpick comments (10)
.github/workflows/ci-orchestrator.yml (1)
130-132: Advisory:auto-mergeis skipped whentest-e2eis skipped.The
auto-mergejob requirestest-e2e, which can be skipped in "Economy Mode" (line 104). In GitHub Actions, when a required job is skipped, dependent jobs are also skipped by default. This means PRs that don't trigger e2e tests will never be auto-merged.If this is unintentional, consider adding an
ifcondition to handle skipped jobs:if: | github.event_name == 'pull_request' && (needs.test-e2e.result == 'success' || needs.test-e2e.result == 'skipped')tests/test_settings_language.py (1)
1-4: Prefer the shared CI detection helper for the skip condition.
Usingis_ci_environment()keeps CI detection consistent across tests (CI/GITHUB_ACTIONS/GITHUB_RUN_ID) and avoids missing other CI providers.♻️ Proposed update
import unittest from unittest.mock import patch import sys import os +from conftest import is_ci_environment @@ - `@unittest.skipIf`(os.environ.get('GITHUB_ACTIONS') == 'true', "Hangs in CI due to timing/headless issues") + `@unittest.skipIf`(is_ci_environment(), "Hangs in CI due to timing/headless issues") def test_language_switch_functionality(self):Also applies to: 71-71
tests/test_ui_interactions.py (1)
114-115: Consider removing redundantflet.FilePickerpatches.
global_test_mocksalready patchesflet.FilePicker, so the per‑test patches add nesting without changing behavior.♻️ Example cleanup (apply similarly to other blocks)
with patch("switchcraft.utils.config.SwitchCraftConfig.get_value"), \ patch("switchcraft.utils.config.SwitchCraftConfig.set_user_preference"), \ patch("threading.Thread") as mock_thread, \ patch("switchcraft.gui_modern.views.settings_view.AuthService"), \ - patch("switchcraft.gui_modern.views.settings_view.SyncService"), \ - patch("flet.FilePicker"): + patch("switchcraft.gui_modern.views.settings_view.SyncService"):Also applies to: 244-245, 325-326, 373-374, 428-429
tests/test_server_auth.py (1)
16-23: ScopeFLET_PLATFORMto the fixture to avoid cross-test leakage.
Setting the env var at import time affects the entire test run; usingmonkeypatchconfines it to these tests and restores afterward.♻️ Proposed update
-# Needed for FletApp mount to work without GUI -os.environ["FLET_PLATFORM"] = "web" - -@pytest.fixture(autouse=True) -def setup_test_env(tmp_path): +@pytest.fixture(autouse=True) +def setup_test_env(tmp_path, monkeypatch): """Setup a temporary config directory for the duration of tests.""" + # Needed for FletApp mount to work without GUI + monkeypatch.setenv("FLET_PLATFORM", "web") test_dir = tmp_path / "temp_test_data" test_dir.mkdir(parents=True)src/switchcraft/server/user_manager.py (1)
85-87: Consider adding error handling to_save_data.If writing fails (e.g., disk full, permissions), the operation will raise an unhandled exception. For consistency with
_load_data, consider wrapping in try/except with logging.♻️ Optional improvement
def _save_data(self, data: Dict): - with open(self.users_file, "w") as f: - json.dump(data, f, indent=4) + try: + with open(self.users_file, "w") as f: + json.dump(data, f, indent=4) + except Exception as e: + logger.error(f"Failed to save users: {e}") + raisesrc/switchcraft/server/app.py (5)
91-112: Fix inconsistent indentation.Static analysis flagged non-standard indentation (3 spaces instead of 4) in the MFA enforcement logic and admin_required function. While functionally correct, this should be normalized.
♻️ Fix indentation
# Check for enforced MFA (skip if already on setup page or logout) if not request.url.path.startswith("/mfa/setup") and not request.url.path.startswith("/logout"): conf = auth_manager.load_config() if conf.get("enforce_mfa"): - # If user is admin and using global MFA, skip? - # No, if enforce_mfa is on, everyone needs a PERSONAL secret. - if not user_manager.get_totp_secret(user): - logger.info(f"Redirecting user '{user}' to MFA setup (enforced)") - raise HTTPException(status_code=status.HTTP_307_TEMPORARY_REDIRECT, headers={"Location": "/mfa/setup"}) + # If user is admin and using global MFA, skip? + # No, if enforce_mfa is on, everyone needs a PERSONAL secret. + if not user_manager.get_totp_secret(user): + logger.info(f"Redirecting user '{user}' to MFA setup (enforced)") + raise HTTPException(status_code=status.HTTP_307_TEMPORARY_REDIRECT, headers={"Location": "/mfa/setup"}) return user def admin_required(request: Request): user_name = login_required(request) u_info = user_manager.get_user(user_name) if not u_info or u_info.get("role") != "admin": - # Allow fallback if using auth_disabled (which implies admin rights for simplicity) - conf = auth_manager.load_config() - if conf.get("auth_disabled"): return "admin" - - raise HTTPException(status_code=403, detail="Admin privileges required") + # Allow fallback if using auth_disabled (which implies admin rights for simplicity) + conf = auth_manager.load_config() + if conf.get("auth_disabled"): + return "admin" + raise HTTPException(status_code=403, detail="Admin privileges required") return user_name
859-864: Moveuuidimport to the top of the file.The
uuidmodule is imported twice inside the function (lines 859 and 863). Static analysis flagged this as import shadowing. Move the import to the module level for clarity and efficiency.♻️ Move import to top of file
Add to imports at the top of the file (around line 11):
import uuidThen remove the inline imports:
if not clean_name: - import uuid clean_name = f"upload_{uuid.uuid4().hex[:8]}.bin" # Append short UUID and ensure uniqueness - import uuid uid = uuid.uuid4().hex[:8]
458-459: Split multiple statements on one line.Per static analysis (E701), this should be on separate lines for readability.
♻️ Fix style
email = me.get("mail") or me.get("userPrincipalName") - if not email: return HTMLResponse("Could not identify user email", 400) + if not email: + return HTMLResponse("Could not identify user email", 400)
925-949: Consider making language detection more extensible.The current implementation only checks for German (
de) and defaults to English for everything else. If more languages are added to the i18n system, this detection logic will need updates.This is acceptable for the current two-language support, but consider refactoring to check against
i18n.supported_languagesif the language list grows.
337-406: Consider adding rate limiting and CSRF protection.The login endpoint lacks:
- Rate limiting: No protection against brute-force password attempts
- CSRF tokens: Forms submit via POST but don't validate CSRF tokens
These aren't critical for initial deployment but should be considered for production hardening.
Comment on lines
+75
to
+76
| # JSON-safe update for main asset manifest | ||
| python -c "import json, os; p='src/switchcraft/assets/manifest.json'; d=json.load(open(p)) if os.path.exists(p) else {}; d.update({'version': os.environ['VERSION'], 'description': 'SwitchCraft - Enterprise Application Management'}); json.dump(d, open(p, 'w'), indent=4)" |
Contributor
There was a problem hiding this comment.
Export VERSION before the Python update (current step will fail).
python -c reads os.environ['VERSION'], but VERSION isn’t exported in this step, so the script will raise a KeyError and break the release. Export it (or pass via argv) before invoking Python.
✅ Proposed fix
- VERSION="${{ steps.version.outputs.version }}"
+ VERSION="${{ steps.version.outputs.version }}"
+ export VERSION
echo "Updating version to $VERSION"
# JSON-safe update for main asset manifest
- python -c "import json, os; p='src/switchcraft/assets/manifest.json'; d=json.load(open(p)) if os.path.exists(p) else {}; d.update({'version': os.environ['VERSION'], 'description': 'SwitchCraft - Enterprise Application Management'}); json.dump(d, open(p, 'w'), indent=4)"
+ python -c "import json, os; p='src/switchcraft/assets/manifest.json'; d=json.load(open(p)) if os.path.exists(p) else {}; d.update({'version': os.environ['VERSION'], 'description': 'SwitchCraft - Enterprise Application Management'}); json.dump(d, open(p, 'w'), indent=4)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # JSON-safe update for main asset manifest | |
| python -c "import json, os; p='src/switchcraft/assets/manifest.json'; d=json.load(open(p)) if os.path.exists(p) else {}; d.update({'version': os.environ['VERSION'], 'description': 'SwitchCraft - Enterprise Application Management'}); json.dump(d, open(p, 'w'), indent=4)" | |
| VERSION="${{ steps.version.outputs.version }}" | |
| export VERSION | |
| echo "Updating version to $VERSION" | |
| # JSON-safe update for main asset manifest | |
| python -c "import json, os; p='src/switchcraft/assets/manifest.json'; d=json.load(open(p)) if os.path.exists(p) else {}; d.update({'version': os.environ['VERSION'], 'description': 'SwitchCraft - Enterprise Application Management'}); json.dump(d, open(p, 'w'), indent=4)" |
🤖 Prompt for AI Agents
In @.github/workflows/release.yml around lines 75 - 76, The release workflow
step uses python -c which reads os.environ['VERSION'] but VERSION isn't exported
here, causing a KeyError; fix by exporting the VERSION environment variable (or
passing it as an argument) before running the python one-liner so the python
process can access it; update the step that runs the python -c line (the command
referencing 'src/switchcraft/assets/manifest.json' and os.environ['VERSION']) to
either export VERSION into the environment or change the invocation to pass
VERSION as an argument and read from sys.argv in the python snippet.
Comment on lines
+248
to
+292
| # Try to parse server response for absolute path (prevents collisions) | ||
| analysis_path = None | ||
| if e.data: | ||
| try: | ||
| import json | ||
| resp = json.loads(e.data) | ||
| uploaded = resp.get("uploaded", []) | ||
|
|
||
| # Probe multiple possible response fields | ||
| if uploaded and isinstance(uploaded, list) and len(uploaded) > 0: | ||
| analysis_path = uploaded[0] | ||
| elif resp.get("path"): | ||
| analysis_path = resp.get("path") | ||
| elif resp.get("filepath"): | ||
| analysis_path = resp.get("filepath") | ||
| elif resp.get("file"): | ||
| analysis_path = resp.get("file") | ||
|
|
||
| if analysis_path: | ||
| logger.info(f"Using server-returned path: {analysis_path}") | ||
| except Exception as ex: | ||
| logger.warning(f"Failed to parse server upload response: {ex}") | ||
|
|
||
| # Fallback to local reconstruction if server response missing/invalid | ||
| # Reconstruct (Basic cleanup matching app.py logic) | ||
| import uuid | ||
| fname = e.file_name | ||
| clean_name = "".join(x for x in fname if x.isalnum() or x in "-_.").lstrip(".") | ||
| # Add short UUID to avoid local collisions if multiple uploads happen rapidly | ||
| if clean_name: | ||
| parts = clean_name.rsplit(".", 1) | ||
| if len(parts) > 1: | ||
| clean_name = f"{parts[0]}_{uuid.uuid4().hex[:8]}.{parts[1]}" | ||
| else: | ||
| clean_name = f"{clean_name}_{uuid.uuid4().hex[:8]}" | ||
| else: | ||
| clean_name = f"upload_{uuid.uuid4().hex[:8]}.bin" | ||
|
|
||
| analysis_path = str(Path(tempfile.gettempdir()) / "switchcraft_uploads" / clean_name) | ||
| logger.info(f"Falling back to reconstructed path (with UUID): {analysis_path}") | ||
|
|
||
| if Path(analysis_path).exists(): | ||
| self.start_analysis(analysis_path, cleanup_path=analysis_path) | ||
| else: | ||
| self._show_snack("Upload finished but file path could not be verified.", "RED") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/switchcraft/gui_modern/views/analyzer_view.py | sed -n '248,292p'Repository: FaserF/SwitchCraft
Length of output: 2512
🏁 Script executed:
python3 << 'EOF'
from pathlib import Path
try:
result = Path(None)
print(f"Path(None) succeeded: {result}")
except Exception as e:
print(f"Path(None) raised: {type(e).__name__}: {e}")
EOFRepository: FaserF/SwitchCraft
Length of output: 147
Fix fallback path reconstruction control flow.
The fallback reconstruction code (lines 272–287) is indented inside the except Exception block, so it only runs when JSON parsing fails. This misses the case where e.data contains valid JSON but lacks all expected fields (uploaded, path, filepath, file), leaving analysis_path as None. When line 289 calls Path(analysis_path).exists(), it raises TypeError: expected str, bytes or os.PathLike object, not NoneType.
Move the fallback block outside the try-except and add an if not analysis_path: guard to ensure it runs whenever the server response is missing or lacks expected fields.
Suggested fix
# Try to parse server response for absolute path (prevents collisions)
analysis_path = None
if e.data:
try:
import json
resp = json.loads(e.data)
uploaded = resp.get("uploaded", [])
@@ -266,7 +266,7 @@
if analysis_path:
logger.info(f"Using server-returned path: {analysis_path}")
except Exception as ex:
logger.warning(f"Failed to parse server upload response: {ex}")
- # Fallback to local reconstruction if server response missing/invalid
- # Reconstruct (Basic cleanup matching app.py logic)
- import uuid
+ # Fallback to local reconstruction if server response missing/invalid
+ if not analysis_path:
+ import uuid
fname = e.file_name
clean_name = "".join(x for x in fname if x.isalnum() or x in "-_.").lstrip(".")🤖 Prompt for AI Agents
In `@src/switchcraft/gui_modern/views/analyzer_view.py` around lines 248 - 292,
The fallback path reconstruction is incorrectly nested inside the except block
so it only runs on JSON parse errors; pull the fallback block (the
uuid/tempfile/clean_name reconstruction that sets analysis_path and logs
"Falling back...") out of the try/except and run it whenever analysis_path is
falsy by adding an "if not analysis_path:" guard after the try/except that
performs the uuid.uuid4-based filename cleanup (using e.file_name, clean_name,
tempfile.gettempdir(), Path) and sets analysis_path before calling
Path(analysis_path).exists() and start_analysis().
Comment on lines
+1123
to
+1283
| async def _export_settings(self, e): | ||
| """ | ||
| Open a file-save dialog and write current user preferences to a JSON file. | ||
|
|
||
| Opens a save-file picker prompting the user for a destination (default filename "settings.json"); if a path is selected, exports the application preferences to that file as pretty-printed JSON and shows a success notification. | ||
| Export settings via FilePicker (Desktop) or Download (Web). | ||
| """ | ||
| from switchcraft.gui_modern.utils.file_picker_helper import FilePickerHelper | ||
| try: | ||
| path = FilePickerHelper.save_file(dialog_title=i18n.get("btn_export_settings") or "Export Settings", file_name="settings.json", allowed_extensions=["json"]) | ||
| if path: | ||
| prefs = SwitchCraftConfig.export_preferences() | ||
| with open(path, "w") as f: | ||
| json.dump(prefs, f, indent=4) | ||
| self._show_snack(f"{i18n.get('export_success') or 'Exported to'} {path}") | ||
| except Exception as ex: | ||
| logger.error(f"Failed to export settings: {ex}") | ||
| self._show_snack(f"{i18n.get('export_failed') or 'Export Failed'}: {ex}", "RED") | ||
| from switchcraft import IS_WEB | ||
| import base64 | ||
|
|
||
| prefs = SwitchCraftConfig.export_preferences() | ||
| json_str = json.dumps(prefs, indent=4) | ||
|
|
||
| if IS_WEB: | ||
| # Web: Trigger download via Data URI | ||
| b64 = base64.b64encode(json_str.encode("utf-8")).decode("utf-8") | ||
| data_uri = f"data:application/json;base64,{b64}" | ||
| # Use 'download' attribute simulation if possible, but Flet launch_url behaves like opening link | ||
| # Browsers usually download data URIs with correct MIME type | ||
| self._launch_url(data_uri) | ||
| self._show_snack("Export started (Check downloads)", "GREEN") | ||
| else: | ||
| # Desktop: Use Save File Dialog | ||
| self._picker_context = "export_settings" | ||
| await self.file_picker.save_file( | ||
| dialog_title=i18n.get("btn_export_settings") or "Export Settings", | ||
| file_name="settings.json", | ||
| allowed_extensions=["json"] | ||
| ) | ||
|
|
||
| def _import_settings(self, e): | ||
| async def _import_settings(self, e): | ||
| """ | ||
| Import settings via FilePicker. | ||
| """ | ||
| Import application settings from a user-selected JSON file. | ||
| self._picker_context = "import_settings" | ||
| await self.file_picker.pick_files( | ||
| allow_multiple=False, | ||
| allowed_extensions=["json"], | ||
| dialog_title=i18n.get("btn_import_settings") or "Import Settings" | ||
| ) | ||
|
|
||
| Opens a file picker restricted to a single `.json` file, parses the selected file as JSON, and applies the data via SwitchCraftConfig.import_preferences. Displays a success snack on successful import; on failure shows an error snack and logs the exception. JSON decoding errors are reported as an invalid JSON file. | ||
| def _on_file_picker_result(self, e): | ||
| """Handle results from the shared FilePicker.""" | ||
| context = getattr(self, "_picker_context", None) | ||
| if not context: | ||
| return | ||
|
|
||
| if context == "export_settings": | ||
| if e.path: | ||
| try: | ||
| prefs = SwitchCraftConfig.export_preferences() | ||
| with open(e.path, "w") as f: | ||
| json.dump(prefs, f, indent=4) | ||
| self._show_snack(f"{i18n.get('export_success') or 'Exported to'} {e.path}") | ||
| except Exception as ex: | ||
| logger.error(f"Failed to export settings: {ex}") | ||
| self._show_snack(f"Export Failed: {ex}", "RED") | ||
|
|
||
| elif context == "import_settings": | ||
| if e.files and len(e.files) > 0: | ||
| fpath = e.files[0].path | ||
| content = getattr(e.files[0], "content", None) | ||
|
|
||
| Parameters: | ||
| e: UI event or trigger that invoked this handler (ignored). | ||
| """ | ||
| try: | ||
| from switchcraft.gui_modern.utils.file_picker_helper import FilePickerHelper | ||
| path = FilePickerHelper.pick_file(allowed_extensions=["json"], allow_multiple=False) | ||
| if path: | ||
| try: | ||
| with open(path, "r", encoding="utf-8") as f: | ||
| data = json.load(f) | ||
| if fpath: | ||
| # Desktop Flow | ||
| with open(fpath, "r", encoding="utf-8") as f: | ||
| data = json.load(f) | ||
| elif content: | ||
| # Web Flow (Simulated/Future-Proofing for Flet Web Byte Access) | ||
| # content is usually bytes in Flet Web | ||
| data = json.loads(content.decode("utf-8")) | ||
| else: | ||
| # Fallback for Web where path is None and content not provided directly | ||
| # Or restricted environment | ||
| from switchcraft import IS_WEB | ||
| if IS_WEB: | ||
| self._show_snack("Settings import not supported in web version yet (Sandbox restriction)", "ORANGE") | ||
| else: | ||
| self._show_snack("Import failed: No file path or content received", "RED") | ||
| return | ||
|
|
||
| SwitchCraftConfig.import_preferences(data) | ||
| self._show_snack(i18n.get("import_success") or "Settings Imported. Please Restart.", "GREEN") | ||
| except json.JSONDecodeError as ex: | ||
| logger.error(f"Invalid JSON in settings file: {ex}") | ||
| self._show_snack(f"{i18n.get('import_failed') or 'Import Failed'}: Invalid JSON file", "RED") | ||
| except Exception as ex: | ||
| logger.exception(f"Error importing settings: {ex}") | ||
| self._show_snack(f"{i18n.get('import_failed') or 'Import Failed'}: {ex}", "RED") | ||
| except Exception as ex: | ||
| logger.exception(f"Error in import settings handler: {ex}") | ||
| self._show_snack(f"{i18n.get('import_failed') or 'Import Failed'}: {ex}", "RED") | ||
| logger.error(f"Error importing settings: {ex}") | ||
| self._show_snack(f"Import Failed: {ex}", "RED") | ||
|
|
||
| elif context == "export_logs": | ||
| if e.path: | ||
| try: | ||
| # e.path is the destination | ||
| # We need to copy from self._log_source_path (set in _export_logs) | ||
| src = getattr(self, "_log_source_path", None) | ||
| if src: | ||
| import shutil | ||
| shutil.copy2(src, e.path) | ||
| self._show_snack(f"Logs exported to {e.path}", "GREEN") | ||
| else: | ||
| self._show_snack("Log copy failed: Source lost", "RED") | ||
| except Exception as ex: | ||
| self._show_snack(f"Log export failed: {ex}", "RED") | ||
|
|
||
| # Reset context | ||
| self._picker_context = None | ||
|
|
||
| def _export_logs(self, e): | ||
| async def _export_logs(self, e): | ||
| """Export logs to a file. Includes current session log and recent log files.""" | ||
| import datetime | ||
| import os | ||
| from pathlib import Path | ||
| from switchcraft import IS_WEB | ||
| import base64 | ||
|
|
||
| timestamp = datetime.datetime.now().strftime("%Y%m%d_%H%M%S") | ||
| filename = f"SwitchCraft_Debug_{timestamp}.log" | ||
|
|
||
| from switchcraft.gui_modern.utils.file_picker_helper import FilePickerHelper | ||
| path = FilePickerHelper.save_file(dialog_title=i18n.get("help_export_logs") or "Export Logs", file_name=filename, allowed_extensions=["log", "txt"]) | ||
| # Locate the best log file to export | ||
| from switchcraft.utils.logging_handler import get_session_handler | ||
| handler = get_session_handler() | ||
| source_path = None | ||
|
|
||
| if not path: | ||
| return # User cancelled | ||
| if handler.current_log_path and handler.current_log_path.exists(): | ||
| if handler.file_handler: | ||
| handler.file_handler.flush() | ||
| source_path = handler.current_log_path | ||
| else: | ||
| # Find in log dir | ||
| app_data = os.getenv('APPDATA') | ||
| if app_data: | ||
| log_dir = Path(app_data) / "FaserF" / "SwitchCraft" / "Logs" | ||
| else: | ||
| log_dir = Path.home() / ".switchcraft" / "logs" | ||
|
|
||
| try: | ||
| from switchcraft.utils.logging_handler import get_session_handler | ||
| handler = get_session_handler() | ||
| if log_dir.exists(): | ||
| log_files = sorted(log_dir.glob("SwitchCraft_Session_*.log"), key=os.path.getmtime, reverse=True) | ||
| if log_files: | ||
| source_path = log_files[0] | ||
|
|
||
| # Try to export current session log first | ||
| exported = False | ||
| if handler.current_log_path and handler.current_log_path.exists(): | ||
| try: | ||
| if handler.file_handler: | ||
| handler.file_handler.flush() | ||
| import shutil | ||
| shutil.copy2(handler.current_log_path, path) | ||
| exported = True | ||
| logger.info(f"Exported current session log to {path}") | ||
| except Exception as ex: | ||
| logger.warning(f"Failed to export current session log: {ex}") | ||
|
|
||
| # If no current log, try to find and export recent log files | ||
| if not exported: | ||
| # Find log directory | ||
| app_data = os.getenv('APPDATA') | ||
| if app_data: | ||
| log_dir = Path(app_data) / "FaserF" / "SwitchCraft" / "Logs" | ||
| else: | ||
| log_dir = Path.home() / ".switchcraft" / "logs" | ||
|
|
||
| if log_dir.exists(): | ||
| # Find all session log files | ||
| log_files = sorted(log_dir.glob("SwitchCraft_Session_*.log"), key=os.path.getmtime, reverse=True) | ||
|
|
||
| if log_files: | ||
| # Export the most recent log file | ||
| try: | ||
| import shutil | ||
| shutil.copy2(log_files[0], path) | ||
| exported = True | ||
| logger.info(f"Exported recent log file to {path}") | ||
| except Exception as ex: | ||
| logger.error(f"Failed to export log file: {ex}") | ||
| self._show_snack(f"{i18n.get('logs_export_failed') or 'Log export failed'}: {ex}", "RED") | ||
| return | ||
| else: | ||
| logger.warning("No log files found to export") | ||
| self._show_snack(i18n.get("logs_no_logs_found") or "No log files found to export.", "ORANGE") | ||
| return | ||
| else: | ||
| logger.warning(f"Log directory does not exist: {log_dir}") | ||
| self._show_snack(i18n.get("logs_no_logs_found") or "No log files found to export.", "ORANGE") | ||
| return | ||
| if not source_path: | ||
| self._show_snack("No log file found to export.", "ORANGE") | ||
| return | ||
|
|
||
| # Prepare for export | ||
| if IS_WEB: | ||
| # Web: Read file and Download | ||
| try: | ||
| with open(source_path, "rb") as f: | ||
| content = f.read() | ||
| b64 = base64.b64encode(content).decode("utf-8") | ||
| data_uri = f"data:text/plain;base64,{b64}" | ||
| self._launch_url(data_uri) | ||
| self._show_snack("Log export started.", "GREEN") | ||
| except Exception as ex: | ||
| self._show_snack(f"Web Export Failed: {ex}", "RED") | ||
| else: | ||
| # Desktop: Save Dialog | ||
| self._log_source_path = source_path | ||
| self._picker_context = "export_logs" | ||
| await self.file_picker.save_file( | ||
| dialog_title=i18n.get("help_export_logs") or "Export Logs", | ||
| file_name=filename, | ||
| allowed_extensions=["log", "txt"] | ||
| ) |
Contributor
There was a problem hiding this comment.
Restore config backend before FilePicker-driven import/export.
These handlers call SwitchCraftConfig during callback execution; without _ensure_backend() the active backend can be lost and settings may read/write the wrong store (especially in web mode).
🛠️ Suggested fix
async def _export_settings(self, e):
"""
Export settings via FilePicker (Desktop) or Download (Web).
"""
+ self._ensure_backend()
from switchcraft import IS_WEB
import base64
@@
async def _import_settings(self, e):
"""
Import settings via FilePicker.
"""
+ self._ensure_backend()
self._picker_context = "import_settings"
await self.file_picker.pick_files(
@@
def _on_file_picker_result(self, e):
"""Handle results from the shared FilePicker."""
+ self._ensure_backend()
context = getattr(self, "_picker_context", None)
if not context:
return🤖 Prompt for AI Agents
In `@src/switchcraft/gui_modern/views/settings_view.py` around lines 1123 - 1283,
The FilePicker callback handlers call SwitchCraftConfig methods without
restoring the backend; ensure the config backend is re-initialized by invoking
SwitchCraftConfig._ensure_backend() (or the appropriate public ensure method if
available) at the start of the user-facing functions that trigger or handle file
IO—specifically in _export_settings, _import_settings, _on_file_picker_result
(before any SwitchCraftConfig.export_preferences()/import_preferences() usage),
and _export_logs—so the active backend is guaranteed before reading or writing
preferences or logs.
Comment on lines
+75
to
+78
| if "--factory-reset" in sys.argv: | ||
| try: | ||
| # Local import only for this command | ||
| from switchcraft.utils.config import SwitchCraftConfig |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and check its existence
git ls-files | grep -E "main\.py"Repository: FaserF/SwitchCraft
Length of output: 113
🏁 Script executed:
# Read the relevant section of src/switchcraft/main.py
if [ -f src/switchcraft/main.py ]; then
echo "=== Lines 70-85 of src/switchcraft/main.py ==="
cat -n src/switchcraft/main.py | sed -n '70,85p'
else
echo "File not found"
fiRepository: FaserF/SwitchCraft
Length of output: 905
🏁 Script executed:
# Check if there are any flake8 configuration files
find . -maxdepth 2 -type f \( -name ".flake8" -o -name "setup.cfg" -o -name "tox.ini" -o -name "pyproject.toml" \) | head -20Repository: FaserF/SwitchCraft
Length of output: 88
🏁 Script executed:
# Check flake8 configuration
cat .flake8Repository: FaserF/SwitchCraft
Length of output: 204
🏁 Script executed:
# Also check pyproject.toml for flake8 settings
cat pyproject.toml | head -50Repository: FaserF/SwitchCraft
Length of output: 1142
Fix indentation in the factory-reset block to satisfy Flake8.
Line 77 has 13 spaces (over-indented); should be 12 spaces to match line 78 and comply with E114.
🧹 Proposed fix
if "--factory-reset" in sys.argv:
try:
- # Local import only for this command
+ # Local import only for this command
from switchcraft.utils.config import SwitchCraftConfig📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if "--factory-reset" in sys.argv: | |
| try: | |
| # Local import only for this command | |
| from switchcraft.utils.config import SwitchCraftConfig | |
| if "--factory-reset" in sys.argv: | |
| try: | |
| # Local import only for this command | |
| from switchcraft.utils.config import SwitchCraftConfig |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 77-77: indentation is not a multiple of 4 (comment)
(E114)
[error] 77-77: over-indented (comment)
(E117)
🤖 Prompt for AI Agents
In `@src/switchcraft/main.py` around lines 75 - 78, Adjust the indentation in the
"--factory-reset" branch so the local import line for SwitchCraftConfig matches
the surrounding block indentation (use 12 spaces instead of 13) to fix the E114
Flake8 error; locate the if "--factory-reset" conditional and the try: block and
re-indent the line that reads "from switchcraft.utils.config import
SwitchCraftConfig" to align with the subsequent lines in that block.
Comment on lines
+272
to
+282
| # Default password warning (show if first_run is True) | ||
| default_pw_warning = "" | ||
| if conf.get("first_run", False): | ||
| default_pw_warning = ''' | ||
| <div class="warning"> | ||
| <strong>⚠️ Security Notice:</strong><br> | ||
| Default credentials are active:<br> | ||
| Username: <code>admin</code> | Password: <code>admin</code><br><br> | ||
| <strong>Please change the password immediately</strong> after logging in via the <a href="/admin">Admin Panel</a>. | ||
| </div> | ||
| ''' |
Contributor
There was a problem hiding this comment.
Default password warning is outdated and misleading.
The warning claims the default password is admin, but UserManager._ensure_users_file() now generates a random password printed to stdout. Users following this guidance will fail to log in.
🐛 Update the warning message
# Default password warning (show if first_run is True)
default_pw_warning = ""
if conf.get("first_run", False):
default_pw_warning = '''
<div class="warning">
<strong>⚠️ Security Notice:</strong><br>
- Default credentials are active:<br>
- Username: <code>admin</code> | Password: <code>admin</code><br><br>
- <strong>Please change the password immediately</strong> after logging in via the <a href="/admin">Admin Panel</a>.
+ First-run credentials are active.<br>
+ Username: <code>admin</code><br>
+ Password: <em>Check server logs/stdout for the generated password</em><br><br>
+ <strong>Please change the password immediately</strong> after logging in via the <a href="/admin">Admin Panel</a>.
</div>
'''Also apply the same fix to the duplicate warning in the POST /login handler (lines 388-398).
🤖 Prompt for AI Agents
In `@src/switchcraft/server/app.py` around lines 272 - 282, The current
default_pw_warning string in app.py incorrectly states the default password is
"admin"; update the message in the default_pw_warning assignment and the
duplicate warning used in the POST /login handler to reflect that a random
password is generated by UserManager._ensure_users_file() and printed to stdout
(so users should check the server logs or the initial startup output or use the
Admin Panel to reset credentials) — remove any hardcoded username/password
example and instead instruct users to check the startup log or visit the Admin
Panel to set/change the password; modify the messages where default_pw_warning
is defined and where the POST /login handler constructs its warning to use the
new wording.
Comment on lines
+538
to
+545
| # Render Users Table | ||
| users_html = "<table style='width:100%; text-align:left; border-collapse:collapse;'><tr><th>Username</th><th>Role</th><th>Active</th><th>Actions</th></tr>" | ||
| for u in users: | ||
| active_status = "✅" if u.get('is_active') else "❌" | ||
| users_html += f"<tr><td>{u['username']}</td><td>{u['role']}</td><td>{active_status}</td>" | ||
| # Prevent deleting self or critical admin if only one? | ||
| users_html += f"<td><form method='post' action='/admin/users/delete' onsubmit='return confirm(\"Delete?\");' style='display:inline;'><input type='hidden' name='username' value='{u['username']}'><button style='padding:2px 8px; font-size:12px; background:#cc0000; margin:0;'>Del</button></form></td></tr>" | ||
| users_html += "</table>" |
Contributor
There was a problem hiding this comment.
XSS vulnerability: User data inserted into HTML without escaping.
Usernames are interpolated directly into HTML without escaping. If a malicious username contains <script> tags or event handlers (e.g., from a crafted SSO account), it could execute arbitrary JavaScript in admin sessions.
🔒 Escape user data in HTML output
+from html import escape
+
# Render Users Table
users_html = "<table style='width:100%; text-align:left; border-collapse:collapse;'><tr><th>Username</th><th>Role</th><th>Active</th><th>Actions</th></tr>"
for u in users:
active_status = "✅" if u.get('is_active') else "❌"
- users_html += f"<tr><td>{u['username']}</td><td>{u['role']}</td><td>{active_status}</td>"
+ safe_username = escape(u['username'])
+ safe_role = escape(u['role'])
+ users_html += f"<tr><td>{safe_username}</td><td>{safe_role}</td><td>{active_status}</td>"
# Prevent deleting self or critical admin if only one?
- users_html += f"<td><form method='post' action='/admin/users/delete' onsubmit='return confirm(\"Delete?\");' style='display:inline;'><input type='hidden' name='username' value='{u['username']}'><button style='padding:2px 8px; font-size:12px; background:`#cc0000`; margin:0;'>Del</button></form></td></tr>"
+ users_html += f"<td><form method='post' action='/admin/users/delete' onsubmit='return confirm(\"Delete?\");' style='display:inline;'><input type='hidden' name='username' value='{safe_username}'><button style='padding:2px 8px; font-size:12px; background:`#cc0000`; margin:0;'>Del</button></form></td></tr>"
users_html += "</table>"Also apply escaping to the {user} variable in the admin page header at line 613.
🤖 Prompt for AI Agents
In `@src/switchcraft/server/app.py` around lines 538 - 545, The users_html
construction inserts unescaped user data (e.g., u['username'], u['role']) into
HTML, enabling XSS; update the rendering to HTML-escape all user-supplied values
before concatenation (use a standard helper like html.escape or flask.escape
when building users_html and when setting the hidden form value and cell text)
so usernames, roles and active_status are safe; also apply the same escaping to
the admin page header variable (the {user} insertion) to prevent script
injection in the header.
Comment on lines
+559
to
+561
| entra_client = os.environ.get("SC_ENTRA_CLIENT_ID", "") | ||
| entra_tenant = os.environ.get("SC_ENTRA_TENANT_ID", "") | ||
| github_client = os.environ.get("SC_GITHUB_CLIENT_ID", "") |
Contributor
There was a problem hiding this comment.
Unused variable entra_tenant.
Static analysis correctly identified that entra_tenant is assigned but never used. Either display it in the SSO configuration section or remove it.
♻️ Remove or use the variable
# SSO Configuration section
entra_client = os.environ.get("SC_ENTRA_CLIENT_ID", "")
- entra_tenant = os.environ.get("SC_ENTRA_TENANT_ID", "")
github_client = os.environ.get("SC_GITHUB_CLIENT_ID", "")Or display it in the admin UI if useful for debugging SSO configuration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| entra_client = os.environ.get("SC_ENTRA_CLIENT_ID", "") | |
| entra_tenant = os.environ.get("SC_ENTRA_TENANT_ID", "") | |
| github_client = os.environ.get("SC_GITHUB_CLIENT_ID", "") | |
| entra_client = os.environ.get("SC_ENTRA_CLIENT_ID", "") | |
| github_client = os.environ.get("SC_GITHUB_CLIENT_ID", "") |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 560-560: local variable 'entra_tenant' is assigned to but never used
(F841)
🤖 Prompt for AI Agents
In `@src/switchcraft/server/app.py` around lines 559 - 561, The variable
entra_tenant is assigned but never used; either remove the unused assignment or
surface it where SSO config is assembled/displayed: update the code that
constructs the SSO settings (the same place that uses entra_client and
github_client) to include entra_tenant in the returned/configured SSO data or,
if not needed, delete the entra_tenant assignment to eliminate the unused
variable. Ensure you reference the existing variables entra_client,
entra_tenant, and github_client so the change is consistent with current SSO
configuration handling.
Comment on lines
+715
to
+719
| @app.post("/admin/users/delete") | ||
| async def delete_user_route(username: str = Form(...), u: str = Depends(admin_required)): | ||
| if username == "admin": return HTMLResponse("Cannot delete root admin", 400) | ||
| user_manager.delete_user(username) | ||
| return RedirectResponse("/admin", status_code=303) |
Contributor
There was a problem hiding this comment.
Admin can delete their own account.
The check prevents deletion of the "admin" user, but doesn't prevent an admin from deleting themselves (if logged in as a different admin account). This could leave the system without any admin users.
♻️ Prevent self-deletion
`@app.post`("/admin/users/delete")
async def delete_user_route(username: str = Form(...), u: str = Depends(admin_required)):
- if username == "admin": return HTMLResponse("Cannot delete root admin", 400)
+ if username == "admin":
+ return HTMLResponse("Cannot delete root admin", 400)
+ if username == u:
+ return HTMLResponse("Cannot delete your own account", 400)
user_manager.delete_user(username)
return RedirectResponse("/admin", status_code=303)🧰 Tools
🪛 Flake8 (7.3.0)
[error] 717-717: multiple statements on one line (colon)
(E701)
🤖 Prompt for AI Agents
In `@src/switchcraft/server/app.py` around lines 715 - 719, The delete_user_route
allows an admin to delete their own account; add a guard that compares the
requested username to the current authenticated admin (the dependency parameter
u passed into delete_user_route) and return an error (e.g., HTMLResponse(...,
400)) if they match to prevent self-deletion; additionally, ensure you do not
remove the last remaining admin by checking user_manager for an admin count or
list (e.g., user_manager.get_admin_count() or user_manager.list_admins()) and
refuse deletion if it would leave zero admins—if those helpers don't exist, add
a method to user_manager to compute admin count before calling
user_manager.delete_user(username).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/conftest.py`:
- Around line 269-287: Import the threading module and capture the original
Thread class into a variable before replacing it so NameErrors are avoided: add
an import threading and assign original_thread = threading.Thread prior to
defining MockThread (which overrides start) and calling
monkeypatch.setattr(threading, "Thread", MockThread); ensure MockThread.start
still references original_thread.start(self) for non-switchcraft callers.
In `@tests/test_ui_interactions.py`:
- Around line 3-4: The file imports is_ci_environment from conftest but never
uses it, causing a Flake8 F401; remove the unused import (delete
is_ci_environment from the import list in tests/test_ui_interactions.py) or, if
the CI-skip behavior is required, re-enable the skip logic that references
is_ci_environment so the symbol is used; ensure the import list only contains
used names and run lint to confirm F401 is resolved.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Around line 29-31: The dependency list in pyproject.toml lacks explicit
compatibility guarantees for httpx and webauthn; update pyproject.toml to either
pin httpx and webauthn to known compatible releases or add upper/lower bounds
that reflect tested versions (e.g., set explicit version specifiers for "httpx"
and "webauthn" in the dependencies array), and add/ensure a CI job that runs the
test matrix on Python 3.14 (check the project’s requires-python and the
dependency entries like "urllib3>=2.6.3,<3" for consistency) so any
incompatibility is caught early.
In `@tests/conftest.py`:
- Around line 269-272: The UpdateChecker mock block in tests/conftest.py is
over-indented causing flake8 E111/E117; adjust the indentation of the line
setting monkeypatch.setattr(UpdateChecker, "check_for_updates", MagicMock(...))
so it aligns with the surrounding block (use the same indentation level as the
comment and the import — one tab plus eight spaces instead of one tab plus nine
spaces) to fix the lint error; keep the existing use of UpdateChecker and its
check_for_updates replacement and ensure the conditional referencing
"test_updater_logic.py" remains unchanged.
🧹 Nitpick comments (2)
tests/test_critical_ui_flows.py (1)
241-247: Keep AuthService patch active while rebuilding the cloud sync section.Calling
_build_cloud_sync_section()outside the patch can re-read real auth state and make the test environment-dependent. Consider moving it inside the patch block.♻️ Suggested tweak
- with patch('switchcraft.gui_modern.views.settings_view.AuthService.is_authenticated', return_value=False): - view = ModernSettingsView(mock_page) - mock_page.add(view) - - # Get the login button from cloud sync section - cloud_sync = view._build_cloud_sync_section() + with patch('switchcraft.gui_modern.views.settings_view.AuthService.is_authenticated', return_value=False): + view = ModernSettingsView(mock_page) + mock_page.add(view) + # Get the login button from cloud sync section + cloud_sync = view._build_cloud_sync_section()tests/test_ui_interactions.py (1)
289-289: Prefer conditional skip/xfail over unconditional skip.The commented skip (Line 289) and the unconditional skip (Line 393) both reduce clarity/coverage. Consider
skipifwith a concrete condition (e.g., CI env) orxfaillinked to an issue.Also applies to: 393-405
Comment on lines
+29
to
31
| "urllib3>=2.6.3,<3", | ||
| "packaging" | ||
| ] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && git ls-files | grep -E "(pyproject|setup\.py|\.github|tox\.ini)" | head -20Repository: FaserF/SwitchCraft
Length of output: 130
🏁 Script executed:
cat -n pyproject.toml | head -100Repository: FaserF/SwitchCraft
Length of output: 2661
🏁 Script executed:
fd -t f -e yml -e yaml | grep -E "(github|workflow|action|ci|tox)" | head -20Repository: FaserF/SwitchCraft
Length of output: 44
🏁 Script executed:
cat -n .github/workflows/*.yml 2>/dev/null | head -150Repository: FaserF/SwitchCraft
Length of output: 4883
🌐 Web query:
urllib3 2.6.3 Python 3.12 3.14 compatibility support
💡 Result:
- urllib3 2.6.3 supports Python 3.12: the PyPI metadata explicitly lists
Python :: 3.12and requires Python >= 3.9. [1] - urllib3 2.6.3 also declares support for Python 3.14: the same PyPI classifiers include
Python :: 3.14. [1] - Packaging note: urllib3 2.6.3 ships as a universal
py3-none-anywheel (pure Python), so there’s no CPython-version-specific wheel compatibility concern. [1] - Release context: 2.6.3 (2026-01-07) is primarily a security/bugfix release; nothing in the changelog indicates dropping or limiting 3.12/3.14 support. [2]
Sources: PyPI project metadata for urllib3 2.6.3 [1], urllib3 changelog [2].
🌐 Web query:
flet 0.80.4 Python 3.12 3.14 compatibility support
💡 Result:
- Flet 0.80.4 (library/CLI) supports Python 3.12 and Python 3.14: the
fletpackage on PyPI declares Requires: Python >= 3.10, and the wheel is py3-none-any, so it can be installed on Python 3.12 and 3.14. [1] - Docs match that baseline: Flet “requires Python 3.10 or later.” [2]
- Packaging note (
flet build): packaged apps run on an embedded Python 3.12.x runtime (documented as Python 3.12.7 runtime on all platforms), regardless of whether you develop on 3.12 or 3.14. [3] - Python 3.14 status: Python 3.14.0 was released Oct 7, 2025 and is now superseded by 3.14.2. [4]
Sources: [1] [2] [3] [4]
🌐 Web query:
fastapi httpx uvicorn webauthn Python 3.12 3.14 compatibility
💡 Result:
Compatibility snapshot (Python 3.12 vs 3.14)
FastAPI
- Python requirement:
>=3.9on PyPI. - Declared support/classifiers: includes Python 3.12, 3.13, 3.14. [1]
- 3.14 note: there’s a known warning/compat-layer concern around Pydantic v1 compatibility on Python 3.14 discussed in FastAPI’s tracker (even if you’re on Pydantic v2). [2]
Uvicorn
- Python 3.12: explicitly supported (release notes). [3]
- Python 3.14: explicitly supported starting Uvicorn 0.38.0 (Oct 18, 2025). [3]
HTTPX
- Python requirement:
>=3.8. [4] - Declared support/classifiers on PyPI: up to Python 3.12 (no 3.13/3.14 classifier shown). [4]
- Practically it may work on 3.13/3.14, but it’s not “declared supported” in the PyPI metadata.
WebAuthn (pip install webauthn)
- The PyPI name
webauthncorresponds to Duo Labs’ “py_webauthn” project (“Pythonic WebAuthn”). - Python requirement: listed as Python 3.9+ on the GitHub README / PyPI page. [5]
Practical recommendation
- Python 3.12: should be fine across FastAPI + Uvicorn + HTTPX +
webauthn, based on stated requirements/support. [1][3][4][5] - Python 3.14: strongest “official” story is FastAPI + Uvicorn (both declare/support 3.14), while HTTPX and
webauthndon’t clearly declare 3.14 in PyPI metadata (though they may still run). [1][3][4][5]
Sources: [1] [2] [3] [4] [5]
Python 3.12+ compatibility confirmed for most dependencies; minor gaps for httpx and webauthn on Python 3.14.
All primary dependencies satisfy the requires-python >= 3.12 constraint. Most also declare explicit support for Python 3.14:
- ✓ urllib3 2.6.3: Python >=3.9, explicitly supports 3.12 and 3.14
- ✓ flet 0.80.4 suite: Python >=3.10, supports 3.12 and 3.14
- ✓ fastapi: Python >=3.9, explicitly supports 3.12, 3.13, and 3.14 (note: minor Pydantic compatibility warning exists for 3.14)
- ✓ uvicorn: supports 3.12 and 3.14 (since v0.38.0, Oct 2025)
- ✓ pyotp: already marked verified for Python 3.14 (line 65)
- ⚠ httpx >=0.28.1: supports up to Python 3.12 on PyPI; no explicit 3.14 classifier but may work
- ⚠ webauthn: Python >=3.9; no explicit 3.14 declaration but may still function
Other dependencies (jinja2, itsdangerous, passlib, bcrypt, python-multipart, packaging) have no known compatibility issues. The CI workflow already tests builds on Python 3.14 without reported failures. Consider explicitly testing or pinning versions for httpx and webauthn if 3.14 support becomes a formal requirement.
🤖 Prompt for AI Agents
In `@pyproject.toml` around lines 29 - 31, The dependency list in pyproject.toml
lacks explicit compatibility guarantees for httpx and webauthn; update
pyproject.toml to either pin httpx and webauthn to known compatible releases or
add upper/lower bounds that reflect tested versions (e.g., set explicit version
specifiers for "httpx" and "webauthn" in the dependencies array), and add/ensure
a CI job that runs the test matrix on Python 3.14 (check the project’s
requires-python and the dependency entries like "urllib3>=2.6.3,<3" for
consistency) so any incompatibility is caught early.
Comment on lines
+269
to
+272
| # Mock UpdateChecker to prevent real update checks | ||
| from switchcraft.utils.app_updater import UpdateChecker | ||
| if "test_updater_logic.py" not in test_path: | ||
| monkeypatch.setattr(UpdateChecker, "check_for_updates", MagicMock(return_value=(False, "1.0.0", {}))) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/conftest.py | sed -n '265,275p' | cat -ARepository: FaserF/SwitchCraft
Length of output: 663
Fix over‑indentation in the UpdateChecker mock.
Line 272 has an extra space (1 tab + 9 spaces instead of 1 tab + 8 spaces), which violates flake8 E111/E117 and will fail lint in CI.
🛠️ Fix indentation
- if "test_updater_logic.py" not in test_path:
- monkeypatch.setattr(UpdateChecker, "check_for_updates", MagicMock(return_value=(False, "1.0.0", {})))
+ if "test_updater_logic.py" not in test_path:
+ monkeypatch.setattr(UpdateChecker, "check_for_updates", MagicMock(return_value=(False, "1.0.0", {})))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Mock UpdateChecker to prevent real update checks | |
| from switchcraft.utils.app_updater import UpdateChecker | |
| if "test_updater_logic.py" not in test_path: | |
| monkeypatch.setattr(UpdateChecker, "check_for_updates", MagicMock(return_value=(False, "1.0.0", {}))) | |
| # Mock UpdateChecker to prevent real update checks | |
| from switchcraft.utils.app_updater import UpdateChecker | |
| if "test_updater_logic.py" not in test_path: | |
| monkeypatch.setattr(UpdateChecker, "check_for_updates", MagicMock(return_value=(False, "1.0.0", {}))) |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 272-272: indentation is not a multiple of 4
(E111)
[error] 272-272: over-indented
(E117)
🤖 Prompt for AI Agents
In `@tests/conftest.py` around lines 269 - 272, The UpdateChecker mock block in
tests/conftest.py is over-indented causing flake8 E111/E117; adjust the
indentation of the line setting monkeypatch.setattr(UpdateChecker,
"check_for_updates", MagicMock(...)) so it aligns with the surrounding block
(use the same indentation level as the comment and the import — one tab plus
eight spaces instead of one tab plus nine spaces) to fix the lint error; keep
the existing use of UpdateChecker and its check_for_updates replacement and
ensure the conditional referencing "test_updater_logic.py" remains unchanged.
Contributor
|
🚫 Auto-Merge Skipped Auto-merge was not executed because review comments were found in the code. CodeRabbit has left 38 inline comment(s) and 1 PR comment(s) that need to be addressed.\n\nThere are 38 inline review comment(s) in the code that need attention (38 from CodeRabbit).\n\nPlease review and address the feedback before merging.\n\nOnce all review comments are resolved and CI workflows pass, auto-merge will proceed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.