fix(linux): ensure secretstorage is bundled in Linux binary (ACS-310) #1211
Conversation
Linux binary installations were missing the `secretstorage` package, causing OAuth token storage via Freedesktop.org Secret Service to fail silently. The build script cache was created before secretstorage was added to requirements.txt (Jan 16, 2026), so the package was not being validated during bundling. Changes: - Add platform-aware critical packages validation in download-python.cjs - Add platform-aware critical packages validation in python-env-manager.ts - Add Linux secretstorage warning in dependency_validator.py - Add comprehensive tests for Linux secretstorage validation The fix follows the same pattern as the Windows pywin32 fix (ACS-306): - Platform-specific packages are only validated on their target platform - Linux: secretstorage (OAuth token storage via keyring) - Windows: pywintypes (MCP library dependency) - macOS: No platform-specific packages The Linux validation emits a warning (not blocking error) since the app gracefully falls back to .env file storage when secretstorage is unavailable. Refs: ACS-310
The tests test_windows_skips_secretstorage_validation and test_macos_skips_secretstorage_validation were failing on Windows CI because they didn't mock the pywintypes import. When running on actual Windows in CI, the pywin32 validation runs first and exits because pywin32 isn't installed in the test environment. The fix mocks pywintypes to succeed so we can properly test that secretstorage validation is skipped on non-Linux platforms.
Changes made based on CodeRabbit AI review: Backend (dependency_validator.py): - Rename _exit_with_secretstorage_warning to _warn_missing_secretstorage to accurately reflect that it doesn't exit (it only emits a warning) - Add sys.stderr.flush() to ensure warning is immediately visible Frontend (download-python.cjs): - Fix critical __init__.py validation logic - packages are now only considered valid if directory+__init__.py exists OR single-file module exists Frontend (python-env-manager.ts): - Use platform abstraction (isWindows(), isLinux()) instead of process.platform - Fix critical packages validation to match download-python.cjs logic Tests (test_dependency_validator.py): - Update function name to _warn_missing_secretstorage - Add is_windows patches to Linux tests to prevent pywin32 validation from running on Windows CI Refs: ACS-310
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds Linux secretstorage validation (non-blocking warning) and refactors Windows pywin32 checks; centralizes platform-critical package checks in frontend scripts and broadens package-existence detection; expands tests to cover Linux warning path, Windows behavior, and lazy-import ordering. Changes
Sequence Diagram(s)(omitted — changes are focused on validation and messaging, not a new multi-component sequential flow) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
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. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
Summary of ChangesHello @StillKnotKnown, 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 addresses a critical build system issue that caused Linux binaries to be shipped without the necessary Highlights
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
|
![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 effectively addresses the issue of the secretstorage dependency not being bundled in Linux binaries by adding platform-aware validation at both build and runtime. The changes are well-implemented and follow existing patterns for platform-specific dependencies. The test suite has also been thoroughly updated to cover the new logic for Linux, as well as to improve the overall structure of the dependency validation tests.
I have a few points of feedback:
- A critical concern regarding a potential regression in
pywin32validation on Windows, which seems to have been unintentionally altered. - A suggestion to improve the robustness of the new warning message for missing
secretstorageon Linux. - A note on code duplication in the build script that could be refactored for better maintainability.
Overall, this is a solid fix, and once the critical issue is addressed, it will be a great improvement.
| # pywin32 is required on all Python versions on Windows (ACS-306) | ||
| # The MCP library unconditionally imports win32api on Windows | ||
| if sys.platform == "win32": | ||
| if is_windows() and sys.version_info >= (3, 12): |
There was a problem hiding this comment.
This change appears to introduce a regression related to pywin32 validation on Windows. The previous implementation (from ACS-306) validated this dependency on all Windows Python versions, as it was deemed critical for the MCP library. This new logic restricts the check to only Python 3.12+, which could lead to runtime errors for users on older Python versions on Windows.
The removed tests (test_windows_python_311_validates_pywin32, test_windows_python_310_validates_pywin32) confirm this was the intended behavior before. If pywin32 is still required for all versions, this condition should be reverted.
| if is_windows() and sys.version_info >= (3, 12): | |
| if is_windows(): |
| def _warn_missing_secretstorage() -> None: | ||
| """Emit warning message for missing secretstorage. | ||
| Note: This is a warning, not a hard error - the app will fall back to .env | ||
| file storage for OAuth tokens. We warn users to ensure they understand the | ||
| security implications. | ||
| """ | ||
| # Use sys.prefix to detect the virtual environment path | ||
| venv_activate = Path(sys.prefix) / "bin" / "activate" | ||
|
|
||
| sys.stderr.write( | ||
| "Warning: Linux dependency 'secretstorage' is not installed.\n" | ||
| "\n" | ||
| "Auto Claude can use secretstorage for secure OAuth token storage via\n" | ||
| "the system keyring (gnome-keyring, kwallet, etc.). Without it, tokens\n" | ||
| "will be stored in plaintext in your .env file.\n" | ||
| "\n" | ||
| "To enable keyring integration:\n" | ||
| "1. Activate your virtual environment:\n" | ||
| f" source {venv_activate}\n" | ||
| "\n" | ||
| "2. Install secretstorage:\n" | ||
| " pip install 'secretstorage>=3.3.3'\n" | ||
| "\n" | ||
| " Or reinstall all dependencies:\n" | ||
| " pip install -r requirements.txt\n" | ||
| "\n" | ||
| "Note: The app will continue to work, but OAuth tokens will be stored\n" | ||
| "in your .env file instead of the system keyring.\n" | ||
| "\n" | ||
| f"Current Python: {sys.executable}\n" | ||
| ) | ||
| sys.stderr.flush() | ||
| # Continue execution - this is a warning, not a blocking error |
There was a problem hiding this comment.
The warning message for missing secretstorage unconditionally includes instructions to source a virtual environment activation script. However, the script at Path(sys.prefix) / "bin" / "activate" may not exist, for example if the user is running with a system Python installation. This could lead to confusing instructions.
To improve this, you could check if venv_activate.exists() before including the activation step in the message, similar to how _exit_with_pywin32_error handles it. This makes the instructions more robust and user-friendly.
def _warn_missing_secretstorage() -> None:
"""Emit warning message for missing secretstorage.
Note: This is a warning, not a hard error - the app will fall back to .env
file storage for OAuth tokens. We warn users to ensure they understand the
security implications.
"""
# Use sys.prefix to detect the virtual environment path
venv_activate = Path(sys.prefix) / "bin" / "activate"
activation_step = ""
install_step_number = "1."
if venv_activate.exists():
activation_step = (
"1. Activate your virtual environment:\n"
f" source {venv_activate}\n\n"
)
install_step_number = "2."
sys.stderr.write(
"Warning: Linux dependency 'secretstorage' is not installed.\n"
"\n"
"Auto Claude can use secretstorage for secure OAuth token storage via\n"
"the system keyring (gnome-keyring, kwallet, etc.). Without it, tokens\n"
"will be stored in plaintext in your .env file.\n"
"\n"
"To enable keyring integration:\n"
f"{activation_step}"
f"{install_step_number} Install secretstorage:\n"
" pip install 'secretstorage>=3.3.3'\n"
"\n"
" Or reinstall all dependencies:\n"
" pip install -r requirements.txt\n"
"\n"
"Note: The app will continue to work, but OAuth tokens will be stored\n"
"in your .env file instead of the system keyring.\n"
"\n"
f"Current Python: {sys.executable}\n"
)
sys.stderr.flush()
# Continue execution - this is a warning, not a blocking error| // secretstorage is platform-critical for Linux (ACS-310) - required for OAuth token storage | ||
| const platformCriticalPackages = { | ||
| 'win32': ['pywintypes'], // Check for 'pywintypes' instead of 'pywin32' (pywin32 installs top-level modules) | ||
| 'linux': ['secretstorage'] // Linux OAuth token storage via Freedesktop.org Secret Service | ||
| }; | ||
| const criticalPackages = ['claude_agent_sdk', 'dotenv', 'pydantic_core'] | ||
| .concat(info.nodePlatform === 'win32' ? ['pywintypes'] : []); | ||
| .concat(platformCriticalPackages[info.nodePlatform] || []); | ||
| const missingPackages = criticalPackages.filter(pkg => { | ||
| const pkgPath = path.join(sitePackagesDir, pkg); | ||
| const initFile = path.join(pkgPath, '__init__.py'); | ||
| const initPath = path.join(pkgPath, '__init__.py'); | ||
| // For single-file modules (like pywintypes.py), check for the file directly | ||
| const moduleFile = path.join(sitePackagesDir, pkg + '.py'); | ||
| // Package is valid if directory+__init__.py exists OR single-file module exists | ||
| return !fs.existsSync(initFile) && !fs.existsSync(moduleFile); | ||
| return !(fs.existsSync(pkgPath) && fs.existsSync(initPath)) && !fs.existsSync(moduleFile); | ||
| }); |
There was a problem hiding this comment.
This block of logic for determining and validating critical packages is duplicated later in the file (around line 825). This duplication makes the code harder to maintain, as any changes need to be applied in two places (as was done in this PR).
To improve maintainability, consider refactoring this logic into a reusable helper function. This function could take sitePackagesDir and nodePlatform as arguments and return the list of missing packages.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/backend/core/dependency_validator.py`:
- Around line 97-121: The warning message unconditionally suggests sourcing
sys.prefix/bin/activate which is often invalid for system Python; update the
logic around venv_activate in dependency_validator.py to only include the
"source {venv_activate}" line when an actual virtualenv activation script exists
or when running inside a venv (e.g., check
Path(sys.prefix)/"bin"/"activate".exists() or compare sys.prefix !=
sys.base_prefix / check VIRTUAL_ENV), otherwise omit that activation instruction
and keep the rest of the message (use the existing venv_activate, sys.prefix,
and sys.stderr.write symbols to find and modify the block).
In `@apps/frontend/src/main/python-env-manager.ts`:
- Around line 131-152: The frontend currently treats secretstorage as a critical
package by listing it under platformCriticalPackages.linux and including it in
criticalPackages (used by the hasBundledPackages check), which forces venv
creation on Linux; remove 'secretstorage' from platformCriticalPackages.linux
and instead add it to an optional/auxiliary packages list checked separately
(e.g., optionalPackages or nonCriticalPackages) and update the missing-check
logic so hasBundledPackages only considers criticalPackages, while the new
optional check logs a warning (using the existing logger) if secretstorage is
absent but does not cause hasBundledPackages() to return false; adjust
references to platformCriticalPackages, criticalPackages, and the
missingPackages/filter logic accordingly.
In `@tests/test_dependency_validator.py`:
- Around line 267-292: The test test_windows_skips_secretstorage_validation
currently patches is_linux instead of forcing the Windows branch; update the
test to patch core.dependency_validator.is_windows (return_value=True) so the
Windows path is exercised, keep the existing patches for builtins.__import__ and
core.dependency_validator._warn_missing_secretstorage and then call
validate_platform_dependencies() and assert mock_warning.assert_not_called() to
ensure secretstorage validation is skipped on Windows.
- Fix venv activation warning to only suggest sourcing activate script when it actually exists (not for system Python) - Move secretstorage from critical to optional packages in frontend runtime check to avoid forcing venv creation on Linux (build script still validates it as critical) - Update test_windows_skips_secretstorage_validation to properly exercise Windows path by mocking is_windows - Add test for activation instruction omission when script doesn't exist Refs: ACS-310
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
StillKnotKnown
added
bug
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
🔴 Blocked - 3 critical/high/medium issue(s) must be fixed.
Blocked by 1 critical issues
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Critical: Regression: pywin32 validation restricted to Python 3.12+ instead of all Windows versions (apps/backend/core/dependency_validator.py:23)
Findings Summary
- Critical: 1 issue(s)
- High: 1 issue(s)
- Medium: 1 issue(s)
- Low: 2 issue(s)
Generated by Auto Claude PR Review
Findings (5 selected of 5 total)
🔴 [ff18efe7483a] [CRITICAL] Regression: pywin32 validation restricted to Python 3.12+ instead of all Windows versions
📁 apps/backend/core/dependency_validator.py:23
The requirements.txt (line 23) explicitly states 'Required on all Python versions on Windows (ACS-306) - MCP library unconditionally imports win32api'. However, the validator at line 23 only checks if is_windows() and sys.version_info >= (3, 12). This means Windows users on Python 3.10 or 3.11 will NOT receive the helpful error message when pywin32 is missing - they'll get a cryptic import error instead. This directly contradicts the ACS-306 fix intention.
Suggested fix:
Remove the Python version check. Change line 23 from `if is_windows() and sys.version_info >= (3, 12):` to simply `if is_windows():` to validate pywin32 on all Windows Python versions as per ACS-306.
🟠 [7d95d9c9a1fe] [HIGH] Tests explicitly test for the regression behavior
📁 tests/test_dependency_validator.py:99
The test test_windows_python_311_skips_validation (line 99-116) and test_windows_python_310_skips_validation (line 190-206) explicitly verify that Python 3.10/3.11 on Windows skip pywin32 validation. This contradicts the ACS-306 requirement documented in requirements.txt. The tests enforce the incorrect behavior and must be updated alongside the fix.
Suggested fix:
After fixing the validator, update the tests: (1) Remove `test_windows_python_311_skips_validation` and `test_windows_python_310_skips_validation`, (2) Add tests verifying validation RUNS on Python 3.10/3.11 on Windows.
🟡 [6a399c5282a1] [MEDIUM] Duplicated platformCriticalPackages object defined twice in same file
📁 apps/frontend/scripts/download-python.cjs:714
The platformCriticalPackages constant is defined identically at line 714 (cache validation) and line 826 (post-install verification). If the package list changes, both locations must be updated, creating risk of inconsistency. This violates DRY principle.
Suggested fix:
Extract `platformCriticalPackages` to a single constant at module scope (near STRIP_PATTERNS around line 30), then reference it in both validation locations.
🔵 [1efc5e3002f4] [LOW] Inconsistent package classification between build script and runtime manager
📁 apps/frontend/src/main/python-env-manager.ts:131
download-python.cjs treats secretstorage as a 'critical' package (build fails if missing), but python-env-manager.ts treats it as 'optional' (only logs warning). The comment in download-python.cjs says 'Same list exists in python-env-manager.ts - keep them in sync' but they intentionally differ. This inconsistency is confusing and the comment is misleading.
Suggested fix:
Update the comment in download-python.cjs (lines 710, 822) to clarify the intentional difference: 'Note: python-env-manager.ts treats secretstorage as optional (falls back to .env) while this script validates it during build.'
🔵 [fc06737aa2a9] [LOW] Step numbering inconsistency in warning message when venv doesn't exist
📁 apps/backend/core/dependency_validator.py:115
When the venv activate script exists, the message shows steps '1. Activate... 2. Install secretstorage'. But when activate doesn't exist, the message still says '2. Install secretstorage' without a step 1, which is confusing. The pywin32 error message handles this correctly by removing step numbers entirely.
Suggested fix:
Follow the pattern in `_exit_with_pywin32_error()`. When activation_prefix is empty, change '2. Install secretstorage:' to just 'Install secretstorage:' (remove the number).
This review was generated by Auto Claude.
… issues) CRITICAL: Remove Python 3.12+ version check from Windows pywin32 validation - pywin32 is required on ALL Python versions on Windows per ACS-306 - MCP library unconditionally imports win32api, not just on Python 3.12+ - Changed from `if is_windows() and sys.version_info >= (3, 12):` to `if is_windows():` HIGH: Update tests to validate pywin32 on Python 3.10 and 3.11 on Windows - Replaced `test_windows_python_311_skips_validation` with `test_windows_python_311_validates_pywin32` - Replaced `test_windows_python_310_skips_validation` with `test_windows_python_310_validates_pywin32` - Both tests now verify that validation RUNS on these Python versions MEDIUM: Extract duplicated platformCriticalPackages to single constant - Created PLATFORM_CRITICAL_PACKAGES constant at module scope in download-python.cjs - Both validation locations now reference the same constant - Added clarifying comment about intentional difference from python-env-manager.ts LOW: Fix step numbering inconsistency in warning message - When venv activate script doesn't exist, "Install secretstorage:" is shown (no step number) - When activate script exists, "1. Activate... 2. Install..." is shown - Matches the pattern used in _exit_with_pywin32_error() LOW: Update comments to clarify build vs runtime package classification - download-python.cjs treats secretstorage as critical (must be bundled) - python-env-manager.ts treats secretstorage as optional (logs warning, doesn't block) - Comments now explain this intentional difference Refs: ACS-310, ACS-306
Fix Windows CI test failure by properly mocking platform detection functions instead of just sys.platform. The test_validate_platform_dependencies_does_not_import_graphiti test now patches core.dependency_validator.is_windows/is_linux to avoid pywin32 import issues on Windows CI. Refs: ACS-310, ACS-253
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_dependency_validator.py`:
- Around line 389-402: The assertion logic in
test_warning_message_omits_activation_when_no_script is flawed: replace the
faulty assertion that uses "or" with a check that no line contains the
activation hint; specifically, capture the stderr from calling
_warn_missing_secretstorage() and assert that neither the phrase "Activate your
virtual environment" nor any line containing the substring "source" exists in
message (e.g., ensure all(line.find("source") == -1 for line in
message.splitlines()) ), while still asserting "Install secretstorage" is
present; update the assertions around message, "Activate your virtual
environment", and "source" accordingly.
Replace the faulty 'or' assertion with a proper check using all() to verify
that no line contains the 'source' substring when activation script doesn't exist.
The previous logic 'assert "source" not in message or "source" not in message.split("\n")'
was logically incorrect and could produce false positives.
Addressed CodeRabbit review comment.
Refs: ACS-310
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_dependency_validator.py`:
- Around line 264-282: The test
test_linux_with_secretstorage_installed_continues should assert that the warning
helper is NOT invoked; update the test to patch
core.dependency_validator._warn_missing_secretstorage (e.g., with patch or mock)
and after calling validate_platform_dependencies() assert that the patched
_warn_missing_secretstorage was not called, ensuring the absence of
secretstorage does not trigger a warning in this scenario.
…nstalled Update test_linux_with_secretstorage_installed_continues to patch and assert that _warn_missing_secretstorage is NOT called when secretstorage is installed. This ensures the test properly verifies that no warning is emitted in the success case, matching the pattern used in other tests in this class. Addressed CodeRabbit refactor suggestion. Refs: ACS-310
🤖 Auto Claude PR Review🟠 Follow-up Review: Needs Revision
Resolution Status
VerdictAll 5 previous findings have been verified as RESOLVED. The author addressed all issues systematically: (1) CRITICAL pywin32 regression fixed by removing Python version check, (2) HIGH test regression fixed by renaming tests and correcting assertions, (3) MEDIUM duplicate constant consolidated into single PLATFORM_CRITICAL_PACKAGES, (4) LOW package classification inconsistency documented as intentional design, (5) LOW step numbering fixed with conditional logic. No new issues found in code review. However, CI status shows 3 checks still pending - cannot approve for merge until CI completes. Once CI passes, this will be READY_TO_MERGE. Review ProcessAgents invoked: resolution-verifier, new-code-reviewer This is an AI-generated follow-up review using parallel specialist analysis with finding validation. This review identified blockers that must be resolved before merge. Generated by Auto Claude. |
2 participants
Base Branch
developbranch (required for all feature/fix PRs)Description
Fixes a build system caching issue where Linux binaries were shipping without the
secretstoragepackage, causing OAuth token storage via Freedesktop.org Secret Service (gnome-keyring/kwallet) to fail silently.The Python runtime cache was created before
secretstoragewas added to requirements.txt (ACS-293, January 16, 2026), so the build script's critical packages validation was not checking for this Linux-specific dependency. This caused the app to fall back to .env file storage instead of the system keyring.This fix adds platform-aware critical packages validation to both the build script (
download-python.cjs) and runtime manager (python-env-manager.ts), following the same pattern used for Windowspywin32validation (ACS-306).Related Issue
Refs: ACS-310 #1206 #1204
Type of Change
Area
Commit Message Format
Follow conventional commits:
<type>: <subject>Types: feat, fix, docs, style, refactor, test
Example:
feat: add user authentication systemChecklist
developbranchPlatform Testing Checklist
secretstorageon Linux platformpywintypesvalidation not affectedplatform/module (isLinux(),isWindows())CI/Testing Requirements:
Screenshots
N/A - No UI changes
Feature Toggle
N/A - Bug fix
Breaking Changes
Breaking: No
Details: This fix only affects the build and bundling process. Runtime behavior is unchanged - if
secretstorageis missing, the app still falls back to .env file storage (now with a warning).Summary by CodeRabbit
Bug Fixes
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.