feat: Add Debug Mode for Hook and Tool Troubleshooting#5
Conversation
tomaz-lc
force-pushed
the
feat/debug-mode-payload-logging
branch
3 times, most recently
from
f188393 to
a2003e8
Compare
![limacharlie-event-notifications[bot]](https://avatars.githubusercontent.com/u/2992821?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review Summary
I reviewed this PR for security vulnerabilities, bugs/logic errors, performance issues, and missing tests.
Summary
This is a well-implemented debug mode feature with good security considerations. The code uses secure file creation patterns (atomic permissions, symlink prevention) and has comprehensive test coverage.
Security Analysis
Good security practices observed:
- Debug mode is opt-in (disabled by default)
- Directory permissions set to 0o700 (owner only)
- File permissions set to 0o600 atomically at creation using
create_new+mode() - Uses
create_new(O_EXCLsemantics) to prevent symlink attacks - Unique timestamped filenames prevent predictable paths
- Proper TOCTOU race handling in cleanup
Found one issue:
- Minor TOCTOU race in
get_debug_log_pathin loader.rs (see inline comment)
Test Coverage
The PR includes comprehensive tests for:
- Payload classification across provider formats
- Debug config defaults and backwards compatibility
- Directory/file permissions on Unix
- Symlink attack prevention
- Cleanup functionality
This review was generated automatically by Claude Code.
| // Create directory with restrictive permissions if it doesn't exist | ||
| if !debug_dir.exists() { | ||
| fs::create_dir_all(&debug_dir) | ||
| .with_context(|| format!("Unable to create debug directory: {}", debug_dir.display()))?; | ||
|
|
||
| // Set restrictive permissions on Unix (owner only) | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::PermissionsExt; | ||
| let perms = fs::Permissions::from_mode(0o700); | ||
| fs::set_permissions(&debug_dir, perms).with_context(|| { | ||
| format!( | ||
| "Unable to set permissions on debug directory: {}", | ||
| debug_dir.display() | ||
| ) | ||
| })?; | ||
| } | ||
| } |
There was a problem hiding this comment.
Low Priority: TOCTOU race condition in directory permission setting.
Unlike the implementation in src/logging.rs (lines 145-168), this function has a minor race: if the directory already exists (created by another process or previous run) with insecure permissions, this code path will not fix them because the set_permissions call is inside the if !debug_dir.exists() check.
The src/logging.rs implementation handles this correctly by always calling set_permissions after directory creation (see lines 161-168).
Consider moving the #[cfg(unix)] permission-setting block outside the if !debug_dir.exists() block so that pre-existing directories with incorrect permissions will also have their permissions corrected.
There was a problem hiding this comment.
This has been addressed by the latest change (force push) - f778fcf.
- add `debug` config field (opt-in, defaults to false)
- add `viberails debug` command to enable/disable debug mode
- add `viberails debug-clean` command to remove accumulated debug logs
- log full payloads and hook processing details at debug level
- use secure debug directory (~/.local/share/viberails/debug/) with 0o700 permissions
- generate unique log filenames with timestamp and random suffix (debug-{ts}-{random}.log)
- set 0o600 permissions on debug log files for owner-only access
- add payload structure logging to understand provider formats
- fix UTF-8 safe truncation for payload logging (avoid panic on multi-byte chars)
- fix notification success log to only appear on actual success
- add comprehensive tests for payload classification and debug features
debug mode is useful for:
- troubleshooting hooks that aren't triggering or behaving unexpectedly
- understanding payload formats from different providers (Claude Code, Cursor, Codex, etc.)
- verifying cloud API communication and response handling
- checking tool_use vs prompt classification decisions
- testing new provider integrations
- diagnosing timing or connectivity issues
debug logging has zero overhead when disabled (log level filtering).
note: debug logs accumulate over time (one file per hook invocation).
use `viberails debug-clean` to remove old logs and free disk space.
usage:
viberails debug # enable debug mode
viberails debug --disable # disable debug mode
viberails debug-clean # remove all debug logs
ls ~/.local/share/viberails/debug/ # view debug log files
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
tomaz-lc
force-pushed
the
feat/debug-mode-payload-logging
branch
from
a2003e8 to
f778fcf
Compare
2 participants
Description
This PR adds a debug mode that logs detailed information about hook processing, payloads, and cloud communication to help with local development and troubleshooting.
Background / Context
When developing locally or troubleshooting hook issues, it's difficult to understand what's happening under the hood. Common scenarios include:
Changes
New Commands
viberails debug- Enable debug modeviberails debug --disable- Disable debug modeviberails debug-clean- Remove accumulated debug log filesFeatures
debugboolean in user config (opt-in, defaults to false)~/.local/share/viberails/debug/with restrictive permissions (0o700 directory, 0o600 files)debug-{timestamp}-{random}.log) to preserve historydebug-cleanremoves accumulated logs to free disk spaceBug Fixes
Security Considerations
0o700permissions (owner only)0o600permissions (owner read/write only)Race Condition & Attack Prevention
create_new+mode(0o600)on Unix to set permissions atomically at file creation, preventing TOCTOU race where file briefly has insecure default permissionsO_EXCLsemantics (create_new) which fails if the path already exists, preventing symlink attacks where an attacker could create a symlink to overwrite sensitive filesDirBuilder::mode()on Unix to create directory with secure permissions atomicallyUsage
Testing