Add system info and health API

[heavy-d]

new system endpoint that returns

• file paths
• os
• cuda version
• availability of core features

[claude]

Claude finished @heavy-d's task —— View job

---

PR Review: Add system info and health API

Review Checklist:

• Read and understand repository setup from CLAUDE.md
• Analyze changed files structure and organization
• Review src/nodetool/api/model.py changes
• Review src/nodetool/api/server.py modifications
• Review new src/nodetool/api/system/__init__.py
• Review new src/nodetool/api/system/health.py
• Review new src/nodetool/api/system/info.py
• Review src/nodetool/common/system_stats.py changes
• Check for security implications
• Assess performance considerations
• Evaluate error handling and edge cases
• Check for test coverage needs
• Provide comprehensive feedback and recommendations

---

🔍 Review Summary

This PR adds comprehensive system information and health monitoring endpoints. The implementation is well-structured with good separation of concerns, proper error handling, and thoughtful security considerations. However, there are several areas that would benefit from improvements.

✅ Positive Aspects

1. Good Architecture & Organization

• Clean separation into info.py and health.py modules
• Well-structured Pydantic models with proper field descriptions
• Consistent API patterns following existing conventions
• Proper router registration in server.py:102

2. Security Considerations

• System endpoints appropriately restricted to non-production environments
• Path expansion handled carefully with proper error catching
• No sensitive information directly exposed in responses

3. Error Handling

• Extensive try/catch blocks with proper fallback values
• Safe import patterns using try/except for optional dependencies
• Graceful degradation when services/paths are unavailable

🚨 Issues Found

1. Critical: Path Information Leakage src/nodetool/api/system/info.py:102-114

The get_paths_info() function returns expanded filesystem paths including potentially sensitive information:

return {
    "settings_path": settings_path,  # Could expose username/home directory structure
    "secrets_path": secrets_path,    # Critical - this exposes secret file location
    ...
}

Security Risk: This could enable path traversal attacks or reveal internal system structure to unauthorized users.

Recommendation: Either remove this endpoint entirely or sanitize paths by showing only relative/generic references.

2. Missing Input Validation src/nodetool/api/system/__init__.py:96

The health endpoint doesn't validate that run_health_checks() returns expected structure:

result: Dict[str, Any] = run_health_checks()
checks_list: List[Dict[str, Any]] = result.get("checks", []) or []

Risk: Could cause runtime errors if the health check function returns unexpected data.

Fix: Add proper validation or use TypedDict for the return type.

3. Performance Concerns

• CPU Sampling: psutil.cpu_percent(interval=1) in get_system_stats() blocks for 1 second, making the /stats endpoint very slow
• Cache TTL Too Short: 3-second cache on system info is too aggressive and defeats the purpose
• NVML Resource Management: Missing proper cleanup in exception cases

4. Inconsistent Data Return src/nodetool/api/system/info.py:113

The paths info returns a mixed field that wasn't in the original PathsInfo model:

"electron_main_log_file": electron_main_log_file  # This field is not in PathsInfo model

5. Error Response Inconsistency

Health checks use different status formats ("ok"/"warn"/"error") vs typical HTTP status patterns, which may confuse API consumers.

📝 Recommendations

High Priority

1. Remove or sanitize path information from system info endpoint
2. Add proper input validation for health check responses
3. Fix missing field in PathsInfo model (line 113)
4. Improve performance of stats endpoint by reducing CPU sampling interval or making it configurable

Medium Priority

5. Add comprehensive tests for the new endpoints
6. Increase cache TTL to 30+ seconds for system info
7. Add API documentation with examples
8. Implement proper resource cleanup for NVML operations

Low Priority

9. Consistent error response format across all endpoints
10. Add configuration options for health check frequency/depth

🧪 Missing Test Coverage

The PR adds significant new functionality but no corresponding tests. Based on existing patterns in tests/common/test_system_stats.py, tests should be added for:

• System info endpoint responses
• Health check functionality
• Error cases and fallback behavior
• Cache behavior validation
• Edge cases (missing permissions, invalid paths, etc.)

📋 Code Quality Notes

• Good: Extensive logging and error handling
• Good: Proper use of type hints and Pydantic models
• Good: Following existing code patterns and conventions
• Needs Improvement: Some functions are quite long and could benefit from decomposition
• Needs Improvement: Missing docstrings on some public functions

🎯 Overall Assessment

This is a solid foundation for system monitoring functionality, but requires security fixes before merging. The code quality is good and follows repository conventions well. With the recommended changes, this would be a valuable addition to the API.

Recommendation: 🚫 Request changes - address security and validation issues before approval.

---

To fully mitigate the risk of path traversal and ensure that only paths within the allowed directories can be opened, we should:

• Ensure that all entries in safe_roots are resolved to absolute paths before comparison.
• When checking if the requested path is within a safe root, use the resolved absolute paths for both.
• Optionally, handle the case where is_relative_to is not available (Python <3.9), but since the code uses it, we assume Python 3.9+.
• Make these changes in the /open_in_explorer endpoint, specifically in the block where safe_roots are used and compared.

No new imports are needed, but we should add a line to resolve each root directory before the comparison loop.

---

To fix the problem, the code should ensure that all manipulations of the user-provided path are performed before the safety check. Specifically, after expanding environment variables and resolving the path, and after replacing the path with its parent directory if it is a file, the code should then check if the final path is within one of the safe root directories using is_relative_to. This ensures that no matter what the user provides, the path that is ultimately opened is always within an allowed directory. Only after passing this check should the path be used.

Steps to fix:

• Move the "is file" check and possible parent directory replacement before the safety check.
• Only perform the is_relative_to check on the final path that will be opened.
• No additional imports are needed, as all required modules are already imported.

Edit only the relevant region in src/nodetool/api/model.py (lines 393–401).

---

[claude]

Claude finished @heavy-d's task —— View job

---

PR Review: Add system info and health API

Review Checklist:

• Read and understand repository setup from CLAUDE.md
• Analyze changed files structure and organization
• Review src/nodetool/api/model.py changes
• Review src/nodetool/api/server.py modifications
• Review new src/nodetool/api/system/__init__.py
• Review new src/nodetool/api/system/health.py
• Review new src/nodetool/api/system/info.py
• Review src/nodetool/common/system_stats.py changes
• Check for security implications
• Assess performance considerations
• Evaluate error handling and edge cases
• Check for test coverage needs
• Provide comprehensive feedback and recommendations

---

🔍 Review Summary

This PR adds comprehensive system information and health monitoring endpoints. The implementation shows good architectural design with proper separation of concerns, but has several critical security and performance issues that need to be addressed.

✅ Positive Aspects

1. Good Architecture & Organization

• Clean separation into dedicated system module with separate info.py and health.py files
• Well-structured Pydantic models with proper field descriptions and validation
• Consistent API patterns following existing FastAPI conventions
• Proper router registration restricted to non-production environments (server.py:102)

2. Security Measures

• System endpoints appropriately restricted to non-production environments only
• Comprehensive error handling with safe fallback values
• Safe import patterns using try/except for optional dependencies
• Good NVML resource cleanup in system_stats.py:60-64

3. Robust Error Handling

• Extensive try/catch blocks throughout with graceful degradation
• Health check validation with fallback response (__init__.py:131-141)
• Safe path existence checking in health checks

🚨 Critical Issues Found

1. 🔴 CRITICAL: Path Information Security Risk

Location: src/nodetool/api/system/info.py:102-114

The get_paths_info() function exposes sensitive filesystem paths that could enable security attacks:

return {
    "settings_path": settings_path,    # Exposes user directory structure  
    "secrets_path": secrets_path,      # 🚨 CRITICAL: Exposes secret file locations
    "data_dir": data_dir,             # Could reveal system layout
    # ... other potentially sensitive paths
}

Security Impact:

• Path Traversal Risk: Attackers could use this information to construct path traversal attacks
• Information Disclosure: Reveals internal system structure and sensitive file locations
• Attack Surface Expansion: Provides reconnaissance data for further attacks

GitHub Security Alerts: This issue is already flagged by GitHub Advanced Security as "Uncontrolled data used in path expression" on lines 380 and 383 in model.py.

Recommendation: Either remove this endpoint entirely or sanitize paths to show only relative/generic references without revealing actual filesystem locations.

2. 🔴 Model-Response Mismatch

Location: src/nodetool/api/system/info.py:113 vs src/nodetool/api/system/__init__.py:38

The PathsInfo Pydantic model defines electron_main_log_file but the response includes this field without it being declared:

# In PathsInfo model - MISSING this field
class PathsInfo(BaseModel):
    # ... other fields
    # electron_main_log_file NOT DEFINED HERE

# But returned in get_paths_info():
"electron_main_log_file": electron_main_log_file  # This will cause validation errors

This will cause Pydantic validation failures at runtime.

3. 🔴 Performance Issues

Previous Issue Addressed: Good fix in system_stats.py:36 changing from interval=1 to interval=0 to avoid blocking CPU measurement.

Remaining Issue: Cache TTL of 3 seconds (__init__.py:66) is too aggressive for system info that rarely changes. This defeats caching benefits and wastes resources.

🟡 Medium Priority Issues

4. Input Validation Gaps

Location: src/nodetool/api/system/__init__.py:98-106

While the health endpoint has good validation, there are still edge cases not handled:

• No validation that status values are in expected set
• Missing validation for numeric fields in summary

5. Error Response Inconsistency

Health checks use custom status format ("ok"/"warn"/"error") which differs from standard HTTP status patterns, potentially confusing API consumers.

6. Resource Management

The health check's _is_writable() function creates test files but could fail to clean them up in edge cases.

🟢 Minor Issues

7. Code Quality

• Some functions are quite long and could benefit from decomposition
• Missing docstrings on some public functions
• Inconsistent exception handling patterns

📝 Recommendations

🔴 Must Fix Before Merge:

1. Remove or heavily sanitize path information from system info endpoint
2. Fix the missing electron_main_log_file field in PathsInfo model
3. Address GitHub Security Alerts for path expression vulnerabilities

🟡 Should Fix:

4. Increase cache TTL to 30-60 seconds for system info
5. Add comprehensive tests for new endpoints (following patterns in test_system_stats.py)
6. Improve input validation robustness
7. Add API documentation with examples

🟢 Nice to Have:

8. Consistent error response formats across endpoints
9. Better resource cleanup in edge cases
10. Decompose long functions for better maintainability

🧪 Missing Test Coverage

The PR adds significant functionality without corresponding tests. Based on existing patterns in tests/common/test_system_stats.py, tests should cover:

• System info endpoint responses and caching
• Health check functionality and validation
• Error cases and fallback behavior
• Edge cases (missing permissions, invalid paths, etc.)

🎯 Overall Assessment

This PR provides valuable system monitoring functionality with good architectural design, but has critical security vulnerabilities that must be addressed before merging. The path information exposure is a significant security risk that could enable attacks.

The code quality is generally good and follows repository conventions well. With the recommended security fixes and improvements, this would be a solid addition to the API.

Recommendation: 🚫 Request Changes - Must address critical security issues and model validation problems before approval.

---

[claude]

Claude finished @heavy-d's task —— View job

---

PR Review: Add system info and health API

Review Checklist:

• Read and understand repository setup from CLAUDE.md
• Analyze changed files structure and organization
• Review src/nodetool/api/model.py changes
• Review src/nodetool/api/server.py modifications
• Review new src/nodetool/api/system/__init__.py
• Review new src/nodetool/api/system/health.py
• Review new src/nodetool/api/system/info.py
• Review src/nodetool/common/system_stats.py changes
• Check for security implications
• Assess performance considerations
• Evaluate error handling and edge cases
• Check for test coverage needs
• Provide comprehensive feedback and recommendations

---

🔍 Review Summary

This PR adds comprehensive system information and health monitoring endpoints to the API. The implementation shows good architectural design with proper separation of concerns, but has critical security issues that must be addressed before merging.

✅ Positive Aspects

1. Excellent Architecture & Organization

• Clean separation into dedicated system module with separate info.py and health.py files
• Well-structured Pydantic models with descriptive field names and proper validation
• Consistent API patterns following existing FastAPI conventions
• Proper router registration restricted to non-production environments (server.py:102)

2. Strong Security Awareness

• System endpoints appropriately restricted to non-production environments only
• Comprehensive error handling with safe fallback values throughout
• Safe import patterns using try/except for optional dependencies
• Good NVML resource cleanup in system_stats.py:60-64

3. Robust Error Handling

• Extensive try/catch blocks throughout with graceful degradation
• Health check validation with comprehensive fallback response (__init__.py:131-141)
• Safe path existence checking and permission testing in health checks

4. Performance Improvements

• Fixed: Previous CPU sampling issue resolved in system_stats.py:36 (changed from interval=1 to interval=0)
• Proper NVML resource management with cleanup

🚨 Critical Issues Found

1. 🔴 SECURITY: Path Information Exposure Risk

Location: src/nodetool/api/system/info.py:83 and huggingface_cache_dir return

While the PR author made good progress addressing path security by using template paths for most system directories, two critical vulnerabilities remain:

# Line 83 - STILL EXPOSES REAL PATHS
huggingface_cache_dir = str(Path(HF_HUB_CACHE).resolve())

# Line 93 - STILL EXPOSES REAL PATHS  
ollama_models_dir = str(ollama_path) if ollama_path else ""

Security Impact:

• Path Traversal Risk: Attackers could use this information for path traversal attacks
• Information Disclosure: Reveals actual filesystem structure and user directories
• Attack Surface: Provides reconnaissance data for further attacks
• GitHub Security Alert: This directly corresponds to the flagged vulnerabilities in the automated security scan

Recommendation:

• Return template paths for these as well (e.g., ~/.cache/huggingface instead of resolved paths)
• Or remove these fields entirely from the API response

2. 🔴 MODEL VALIDATION: Missing Field Definition

Location: src/nodetool/api/system/__init__.py:38 vs info.py:128

The PathsInfo model is missing the electron_main_log_file field but it's returned in the data:

# PathsInfo model missing this field:
class PathsInfo(BaseModel):
    # ... other fields
    # electron_main_log_file: str  # <-- MISSING

# But returned in get_paths_info():
return {
    # ... other fields  
    "electron_main_log_file": electron_main_log_file,  # <-- Will cause validation error
}

Impact: This will cause Pydantic validation failures at runtime.

Fix: Add electron_main_log_file: str to the PathsInfo model.

🟡 Medium Priority Issues

3. Performance: Cache TTL Too Aggressive

Location: src/nodetool/api/system/__init__.py:66

_TTL_SECONDS = 3.0  # Too short for system info that rarely changes

System information (OS, versions, paths) changes very infrequently. A 3-second cache defeats the purpose and wastes resources.

Recommendation: Increase to 30-60 seconds or make configurable.

4. Input Validation Enhancements

Location: src/nodetool/api/system/__init__.py:108-116

While the health endpoint has good validation, there are edge cases:

• Status values should be validated against expected set ["ok", "warn", "error"]
• Numeric summary fields could benefit from range validation

5. Resource Management Edge Cases

Location: src/nodetool/api/system/health.py:27-39

The _is_writable() function creates test files but could fail to clean them up in some edge cases, though the current implementation with unlink(missing_ok=True) handles most scenarios well.

🟢 Minor Issues

6. Code Quality

• Some functions are quite long (e.g., get_paths_info()) and could benefit from decomposition
• Missing docstrings on some public functions, though private functions are well documented
• Inconsistent exception handling patterns in some places

7. API Consistency

Health checks use custom status format ("ok"/"warn"/"error") which differs from standard HTTP status patterns, though this is reasonable for this use case.

📝 Specific Recommendations

🔴 Must Fix Before Merge:

1. Replace resolved paths with template paths for huggingface_cache_dir and ollama_models_dir in info.py:83,93
2. Add missing electron_main_log_file field to PathsInfo model in __init__.py:38
3. Address GitHub Security Alerts for path expression vulnerabilities

🟡 Should Fix:

4. Increase cache TTL to 30-60 seconds for system info
5. Add comprehensive tests for new endpoints (following patterns in test_system_stats.py)
6. Improve input validation robustness for health checks
7. Add API documentation with examples

🟢 Nice to Have:

8. Decompose long functions for better maintainability
9. Add docstrings to public functions
10. Consider making cache TTL configurable

🧪 Missing Test Coverage

The PR adds significant functionality without corresponding tests. Based on existing patterns in tests/common/test_system_stats.py, tests should cover:

• System info endpoint responses and caching behavior
• Health check functionality and validation logic
• Error cases and fallback behavior
• Edge cases (missing permissions, invalid paths, etc.)
• Model validation (especially the missing field issue)

🎯 Overall Assessment

This PR provides valuable system monitoring functionality with excellent architectural design and follows repository conventions well. The security-conscious approach (template paths for most directories, non-production restriction) shows good awareness.

However, critical security vulnerabilities remain that must be addressed before merging. The path information exposure via resolved HuggingFace and Ollama directories creates the same security risks that were addressed for other paths.

The code quality is generally good with comprehensive error handling and proper resource management.

Recommendation: 🚫 Request Changes - Must address critical security issues and model validation problems before approval.

---

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

To fix the problem, we should avoid logging the original path, which may contain sensitive information. If debug logging is necessary, only the masked (template) path should be logged, as it is specifically designed to avoid exposing sensitive details. The change should be made in the _path_to_template function in src/nodetool/api/system/health.py, specifically on line 54. The print statement should be modified to only log the masked path, not the original. No new imports or methods are required.

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.