feat: add file permission checking to dynamo_check.py #2899
Conversation
keivenchang
requested review from
atchernych,
biswapanda,
hhzhang16,
hutm,
ishandhanani,
julienmancuso,
mohammedabdulwahhab and
nnshah1
as code owners
WalkthroughRefactors deploy/dynamo_check.py to replace fast_mode with a thorough_check flag, adds FilePermissionsInfo, propagates the flag through SystemInfo, CargoInfo, DynamoRuntimeInfo, DynamoFrameworkInfo, and DynamoInfo, adjusts cargo target discovery and size reporting, and updates CLI/help and example outputs accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI (dynamo_check)
participant Sys as SystemInfo
participant Perm as FilePermissionsInfo
participant Cg as CargoInfo
participant Run as DynamoRuntimeInfo
participant Fr as DynamoFrameworkInfo
User->>CLI: Invoke (optional --thorough-check)
CLI->>Sys: new SystemInfo(thorough_check)
Sys->>Perm: new FilePermissionsInfo(thorough_check)
Sys->>Cg: new CargoInfo(thorough_check)
Sys->>Run: new DynamoRuntimeInfo(workspace, thorough_check)
Sys->>Fr: new DynamoFrameworkInfo(workspace, thorough_check)
alt thorough_check = False
Perm->>Sys: Directory-level writability results
Cg->>Cg: Discover target path (metadata/env/default)
Cg-->>Sys: Basic status (no sizes)
Run-->>Sys: Lightweight runtime status
Fr-->>Sys: Lightweight framework status
else thorough_check = True
Perm->>Perm: Recursive per-file permissions audit
Cg->>Cg: Discover target path
Cg->>Cg: Compute target size and per-file details
Cg-->>Sys: Detailed cargo target report
Run->>Run: Deep runtime validations
Fr->>Fr: Deep framework validations
Run-->>Sys: Detailed runtime report
Fr-->>Sys: Detailed framework report
end
Sys-->>CLI: Composed system report (includes File Permissions)
CLI-->>User: Rendered output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
deploy/dynamo_check.py (3)
1512-1513: Verify parameter propagation through DynamoRuntimeInfoThe
thorough_checkparameter is added but never used withinDynamoRuntimeInfo. While this maintains API consistency for potential future use, it could be removed if not needed.If the parameter won't be used in this class, consider documenting why it's included for API consistency, or remove it:
- def __init__(self, workspace_dir: str, thorough_check: bool = False): - self.thorough_check = thorough_check + def __init__(self, workspace_dir: str, thorough_check: bool = False): + # thorough_check parameter maintained for API consistency but not used + self.thorough_check = thorough_check
1691-1692: Same unused parameter pattern in DynamoFrameworkInfoSimilar to
DynamoRuntimeInfo, thethorough_checkparameter is stored but never used.Consider the same approach - either document the parameter or remove if truly unnecessary:
- def __init__(self, workspace_dir: str, thorough_check: bool = False): - self.thorough_check = thorough_check + def __init__(self, workspace_dir: str, thorough_check: bool = False): + # thorough_check parameter maintained for API consistency but not used + self.thorough_check = thorough_check
775-821: Consider limiting directory traversal depth for performanceThe
_count_writable_filesmethod recursively walks the entire directory tree, which could be slow for very large directories. Consider adding a max depth limit or file count limit.Add depth limiting to prevent excessive traversal:
def _count_writable_files( self, directory: str, recursive: bool = False, exclude_files: Optional[List[str]] = None, + max_depth: int = 5, + max_files: int = 10000, ) -> Tuple[int, int, List[str]]: """Count total files and non-writable files in directory Returns: Tuple of (total_files, non_writable_files, non_writable_list) """ exclude_files = exclude_files or [] total_files = 0 non_writable_files = 0 non_writable_list = [] if recursive: # Walk through all files in the directory tree recursively for root, dirs, files in os.walk(directory): + # Check depth limit + depth = root[len(directory):].count(os.sep) + if depth >= max_depth: + dirs[:] = [] # Don't recurse deeper + continue + for file in files: + # Check file count limit + if total_files >= max_files: + return total_files, non_writable_files, non_writable_list + file_path = os.path.join(root, file) # Skip symbolic links if os.path.islink(file_path): continue
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
deploy/dynamo_check.py(16 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-15T02:01:01.238Z
Learnt from: keivenchang
PR: ai-dynamo/dynamo#2453
File: deploy/dynamo_check.py:739-741
Timestamp: 2025-08-15T02:01:01.238Z
Learning: In the dynamo project, missing cargo should be treated as a fatal error in dynamo_check.py because developers need cargo to build the Rust components. The tool is designed to validate the complete development environment, not just import functionality.
Applied to files:
deploy/dynamo_check.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test - vllm
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (13)
deploy/dynamo_check.py (13)
234-235: Verify consistency:thorough_checkis expected to be boolean but inconsistent handlingThe
thorough_checkparameter is declared asboolin the type hint, but throughout the file it's consistently passed as a boolean viaargs.thorough_checkwhich comes fromaction="store_true"in argparse. The implementation is correct.
597-608: Well-structured class with clear documentationThe
FilePermissionsInfoclass has excellent documentation explaining its purpose, the directories it checks, and the behavior differences between default and thorough modes. The structure follows the established pattern in the file.
625-746: Robust unified permission checking with good error handlingThe
_check_permissions_unifiedmethod provides a well-designed abstraction for permission checking that:
- Handles multiple candidate paths gracefully
- Provides clear error messages for different failure scenarios
- Supports both directory-only and recursive file checking modes
- Has comprehensive error handling
747-773: Security-aware permission checking logicThe
_is_effectively_writablemethod correctly handles three scenarios for determining writability:
- Direct write access
- Root user (can override permissions)
- File ownership (can chmod)
This is a more accurate approach than simple
os.accesschecks.
796-797: Good practice: Skip symbolic links during permission checksSkipping symbolic links prevents following potentially circular references and avoids permission issues with link targets outside the workspace.
836-865: Comprehensive cargo target path discoveryThe
_get_cargo_target_path_candidatesmethod uses multiple strategies to find the cargo target directory:
- Most accurate:
cargo metadatacommand- Environment variable:
CARGO_TARGET_DIR- Fallback: default location
~/.cargo/targetThis ensures the tool works in various development setups.
867-908: Proper reuse of existing workspace detection logicThe method correctly reuses
DynamoInfo.find_workspace()andDynamoInfo.is_dynamo_workspace()static methods rather than duplicating the logic, maintaining DRY principles.
939-964: Handle non-existent cargo target gracefullyThe cargo target permission check correctly shows a warning (not error) when the path doesn't exist, which is appropriate since the target directory might not exist in a fresh checkout before the first build.
999-1000: Consistent parameter propagationThe
thorough_checkparameter is correctly passed through the class hierarchy fromSystemInfo→CargoInfo→ directory size calculations.
1055-1061: Performance-aware size calculationDirectory size calculation is correctly gated by the
thorough_checkflag, avoiding expensiveduoperations in default mode.
1832-1833: Proper parameter propagation to child componentsThe
DynamoInfoclass correctly propagatesthorough_checkto bothDynamoRuntimeInfoandDynamoFrameworkInfo, maintaining consistency even though those classes don't currently use it.
2057-2060: Clear and consistent command-line interfaceThe
--thorough-checkflag is well-documented and follows common CLI patterns. The help text clearly explains what additional checks are performed.
910-937: Verify venv site-packages inclusion
Sandbox lackspython3-venv, so we couldn’t confirm ifsite.getsitepackages()returns the active virtual environment’s path. Activate a real venv locally and run:import site print(site.getsitepackages())to verify; if it doesn’t include the venv’s
site-packages, add its path viasys.prefixbefore permission checks.
keivenchang
force-pushed
the
keivenchang/dynamo_check.py-add-file-permission-checks
branch
from
c2e7d17 to
215ffa2
Compare
- Add FilePermissionsInfo class with unified permission checking function - Check writability of dynamo root, .git, RUSTUP_HOME, CARGO_HOME, cargo target, and site-packages - Support directory-only (default) and recursive file checking (--thorough-check) - Implement ownership-aware permission logic for files owned by user or root - Replace --fast flag with --thorough-check for detailed analysis Signed-off-by: Keiven Chang <[email protected]>
Signed-off-by: Keiven Chang <[email protected]>
Signed-off-by: Keiven Chang <[email protected]>
…nings - Add separate UserInfo node showing username, uid, gid - Display user info right before File Permissions section - Show ownership information when running as root (writable (owned by USER)) - Add warning symbol (⚠️ ) when running as root but directory not owned by root - Improve username detection with multiple fallback methods - Update label formatting to use colon separator Signed-off-by: Keiven Chang <[email protected]>
…d PYTHONPATH validation - Add UserInfo node showing username, uid, gid below OS section - Implement part_of_previous metadata for sub-category tree rendering with │ connector - Add PYTHONPATH validation with red highlighting for invalid paths - Update Framework section to only show existing frameworks - Fix metadata type annotation to support Any values - Remove unused variable to pass linting - Update example documentation for all new features Signed-off-by: Keiven Chang <[email protected]>
…erse mode - Replace direct escape sequences with Colors constants for better maintainability - Add disk space checking in thorough mode with warning for <10% free space - Add --terse mode showing only essential info (OS, User, GPU, Framework, Dynamo) and errors - Integrate disk space info inline with directory entries instead of separate section Signed-off-by: Keiven Chang <[email protected]>
keivenchang
force-pushed
the
keivenchang/dynamo_check.py-add-file-permission-checks
branch
from
215ffa2 to
eae4b6f
Compare
Signed-off-by: Keiven Chang <[email protected]>
keivenchang
deleted the
keivenchang/dynamo_check.py-add-file-permission-checks
branch
Signed-off-by: Keiven Chang <[email protected]> Signed-off-by: Kristen Kelleher <[email protected]>
Overview:
Add file permission checking functionality to the
dynamo_check.pydiagnostic tool. The new feature validates writability of critical development directories and files needed for Dynamo development workflows.Details:
--thoroughflag, which is slower)Where should the reviewer start?
deploy/dynamo_check.py - Focus on the FilePermissionsInfo class (lines ~592-950).