feat: boost windows & fix SSL in desktop#1282
Conversation
rayrayraykk
temporarily deployed
to
maintainer-approved
GitHub Actions
Inactive
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and debuggability of the desktop application across Windows and macOS. It tackles critical issues such as subprocess communication deadlocks on Windows, improves the reliability of SSL certificate discovery, and introduces more flexible logging configurations. These changes collectively aim to provide a more stable user experience and streamline troubleshooting for developers. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR updates desktop startup behavior and packaging scripts to improve Windows reliability and ensure SSL certificate environment variables are set correctly in packaged apps.
Changes:
- Adjust daemon log file handling to use non-rotating logging on Windows/Linux and rotating logs on macOS.
- Update
copaw desktopto start the server viapython -m copaw app, add SSL cert diagnostics, and drain subprocess output on Windows to avoid blocking. - Update Windows/macOS pack launchers to preserve PATH, set SSL cert env vars, and pass through a configurable log level.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/copaw/utils/logging.py | Switches daemon file logging to platform-specific handlers (FileHandler vs RotatingFileHandler). |
| src/copaw/cli/desktop_cmd.py | Improves Windows subprocess output handling and starts the app via the CLI app command; adds SSL_CERT_FILE diagnostics. |
| src/copaw/app/_app.py | Adds startup elapsed-time debug logging during FastAPI lifespan startup. |
| scripts/pack/build_win.ps1 | Updates generated .bat launchers to preserve PATH, improve certifi lookup, and pass --log-level. |
| scripts/pack/build_macos.sh | Updates the .app launcher to preserve PATH and pass --log-level. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| is_windows_or_linux = platform.system() in ("Windows", "Linux") | ||
| if is_windows_or_linux: | ||
| file_handler = logging.FileHandler( | ||
| log_path, | ||
| encoding="utf-8", | ||
| mode="a", | ||
| ) | ||
| else: | ||
| file_handler = logging.handlers.RotatingFileHandler( | ||
| log_path, | ||
| encoding="utf-8", | ||
| maxBytes=_COPAW_LOG_MAX_BYTES, | ||
| backupCount=_COPAW_LOG_BACKUP_COUNT, | ||
| ) |
There was a problem hiding this comment.
add_copaw_file_handler() now uses a plain FileHandler on Linux, which disables log rotation entirely and can let copaw.log grow without bound. If the file-locking issue is Windows-specific, consider limiting the non-rotating handler to Windows and keeping RotatingFileHandler for Linux (or implement rotation another way on Linux).
| if platform.system() == "Windows": | ||
| file_handler.setLevel(logging.INFO) | ||
|
|
There was a problem hiding this comment.
On Windows the file handler level is forced to INFO, which will drop DEBUG logs even when COPAW_LOG_LEVEL=debug (or --log-level debug) is requested. If this isn’t intentional, prefer leaving the handler at NOTSET (default) or aligning it with the copaw logger’s configured level so file output matches the selected verbosity.
| if platform.system() == "Windows": | |
| file_handler.setLevel(logging.INFO) |
| set /p CERT_FILE=<"%CERT_TMP%" | ||
| del "%CERT_TMP%" 2>nul |
There was a problem hiding this comment.
This set /p CERT_FILE=<"%CERT_TMP%" will print an error (and may behave unexpectedly) if the preceding Python command fails to create the temp file. Consider guarding with if exist "%CERT_TMP%" (...) (and/or clearing CERT_FILE) to avoid noisy startup failures when certifi isn’t importable.
| set /p CERT_FILE=<"%CERT_TMP%" | |
| del "%CERT_TMP%" 2>nul | |
| if exist "%CERT_TMP%" ( | |
| set /p CERT_FILE=<"%CERT_TMP%" | |
| del "%CERT_TMP%" 2>nul | |
| ) else ( | |
| set "CERT_FILE=" | |
| ) |
|
|
||
| startup_elapsed = time.time() - startup_start_time | ||
| logger.debug( | ||
| f"Application startup completed in {startup_elapsed:.3f} seconds", |
There was a problem hiding this comment.
This debug log uses an f-string, which formats the message even when debug logging is disabled. Prefer parameterized logging (e.g., logger.debug("... %.3f ...", startup_elapsed)) to avoid unnecessary work on non-debug runs.
| f"Application startup completed in {startup_elapsed:.3f} seconds", | |
| "Application startup completed in %.3f seconds", | |
| startup_elapsed, |
| echo Log Level: %COPAW_LOG_LEVEL% | ||
| echo SSL_CERT_FILE: %SSL_CERT_FILE% | ||
| echo REQUESTS_CA_BUNDLE: %REQUESTS_CA_BUNDLE% | ||
| echo CURL_CA_BUNDLE: %CURL_CA_BUNDLE% | ||
| echo. | ||
| if not exist "%USERPROFILE%\.copaw\config.json" ( | ||
| echo [Init] Creating config... | ||
| "%~dp0python.exe" -m copaw init --defaults --accept-security | ||
| "%~dp0python.exe" -u -m copaw init --defaults --accept-security | ||
| ) | ||
| echo [Launch] Starting CoPaw Desktop... | ||
| echo [Launch] Starting CoPaw Desktop with log-level=%COPAW_LOG_LEVEL%... | ||
| echo Press Ctrl+C to stop | ||
| echo. | ||
| "%~dp0python.exe" -m copaw desktop | ||
| "%~dp0python.exe" -u -m copaw desktop --log-level %COPAW_LOG_LEVEL% |
There was a problem hiding this comment.
In the debug launcher, COPAW_LOG_LEVEL is expanded unquoted in multiple places (echo Log Level: %COPAW_LOG_LEVEL%, the [Launch] message, and the python.exe invocation), so a value containing metacharacters like & or | will be parsed by cmd.exe and can inject additional commands. If an attacker can set COPAW_LOG_LEVEL in the environment, they can execute arbitrary commands whenever this debug launcher runs, both during the echo diagnostics and when starting Python. Quote the COPAW_LOG_LEVEL expansions in these lines (including the --log-level argument) so that they are treated as a single literal argument/value rather than command syntax.
| "%~dp0python.exe" -u -m copaw init --defaults --accept-security | ||
| ) | ||
| "%~dp0python.exe" -m copaw desktop | ||
| "%~dp0python.exe" -u -m copaw desktop --log-level %COPAW_LOG_LEVEL% |
There was a problem hiding this comment.
The value of COPAW_LOG_LEVEL is interpolated unquoted into the python.exe command line, so a crafted value containing characters like &, | or > will be interpreted by cmd.exe and can inject additional commands. An attacker who can influence the environment (e.g., set COPAW_LOG_LEVEL) could cause arbitrary commands to execute when the launcher runs instead of just passing a log-level argument to Python. Quote the log-level argument (for example, by passing it as a single quoted token to --log-level) so that cmd.exe treats the entire value as data rather than part of the command syntax.
| "%~dp0python.exe" -u -m copaw desktop --log-level %COPAW_LOG_LEVEL% | |
| "%~dp0python.exe" -u -m copaw desktop --log-level "%COPAW_LOG_LEVEL%" |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements, primarily focused on improving the build process and runtime behavior on Windows, as well as fixing SSL certificate handling for the desktop application. Key changes include updating the build scripts for both macOS and Windows to correctly set the PATH, handle log levels, and more robustly determine the SSL certificate path. The Windows build script now uses a temporary file to capture Python script output, avoiding potential hangs. In the Python code, the desktop command now uses threaded stream readers to prevent subprocess output buffers from blocking on Windows, and the logging utility has been updated to use FileHandler on Windows and Linux to prevent file locking issues. My review includes a couple of suggestions to improve code clarity and error handling for better maintainability.
Note: Security Review is unavailable for this PR.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Silently passing on all exceptions with except Exception: pass can hide bugs and make debugging difficult. It's better to at least log the exception to be aware of potential issues, especially during development. Consider logging the exception using the _log_desktop function.
| except Exception: | |
| pass | |
| except Exception as e: | |
| _log_desktop(f'[_stream_reader] Error reading from subprocess stream: {e!r}') |
| is_windows_or_linux = platform.system() in ("Windows", "Linux") | ||
| if is_windows_or_linux: | ||
| file_handler = logging.FileHandler( | ||
| log_path, | ||
| encoding="utf-8", | ||
| mode="a", | ||
| ) | ||
| else: | ||
| file_handler = logging.handlers.RotatingFileHandler( | ||
| log_path, | ||
| encoding="utf-8", | ||
| maxBytes=_COPAW_LOG_MAX_BYTES, | ||
| backupCount=_COPAW_LOG_BACKUP_COUNT, | ||
| ) | ||
|
|
||
| if platform.system() == "Windows": | ||
| file_handler.setLevel(logging.INFO) |
There was a problem hiding this comment.
The call to platform.system() is made twice. It's more efficient and readable to call it once and store the result in a variable for later use. This avoids redundant system calls and simplifies the logic.
| is_windows_or_linux = platform.system() in ("Windows", "Linux") | |
| if is_windows_or_linux: | |
| file_handler = logging.FileHandler( | |
| log_path, | |
| encoding="utf-8", | |
| mode="a", | |
| ) | |
| else: | |
| file_handler = logging.handlers.RotatingFileHandler( | |
| log_path, | |
| encoding="utf-8", | |
| maxBytes=_COPAW_LOG_MAX_BYTES, | |
| backupCount=_COPAW_LOG_BACKUP_COUNT, | |
| ) | |
| if platform.system() == "Windows": | |
| file_handler.setLevel(logging.INFO) | |
| system = platform.system() | |
| if system in ("Windows", "Linux"): | |
| file_handler = logging.FileHandler( | |
| log_path, | |
| encoding="utf-8", | |
| mode="a", | |
| ) | |
| else: | |
| file_handler = logging.handlers.RotatingFileHandler( | |
| log_path, | |
| encoding="utf-8", | |
| maxBytes=_COPAW_LOG_MAX_BYTES, | |
| backupCount=_COPAW_LOG_BACKUP_COUNT, | |
| ) | |
| if system == "Windows": | |
| file_handler.setLevel(logging.INFO) |
Description
[Describe what this PR does and why]
Related Issue: Fixes #(issue_number) or Relates to #(issue_number)
Security Considerations: [If applicable, e.g. channel auth, env/config handling]
Type of Change
Component(s) Affected
Checklist
pre-commit run --all-fileslocally and it passespytestor as relevant) and they passTesting
[How to test these changes]
Local Verification Evidence
Additional Notes
[Optional: any other context]