Conversation
- Use single Lock() instead of RLock() + Lock() pattern - Delete expired entries inside lock to prevent stale reads - Move configPath calculation inside lock for consistency Fixes race condition where concurrent cache Get() calls could: - Double-load config for same directory - Read entry after it was invalidated by another goroutine
- Remove defer close(sigChan) that caused goroutine to exit with code 128 on normal process completion (reading from closed channel returns zero value) - Add signal.Stop(sigChan) inside signal handler goroutine for proper cleanup - Add explicit cleanup() call before return for normal/exit path - Remove exitProcess(1) on error - let function return normally with cleanup Fixes goroutine leak and incorrect exit code (128) when subprocess exits without receiving a signal.
Replace loop-based backoff calculation with bit shifting: - O(1) instead of O(min(attempt, 60)) iterations - More idiomatic Go code using 1 << attempt - Explicit cap at 60 bits (2^60 > MaxSafeBackoffFactor) Note: Original issue overstated the problem - loop was already O(60) max due to early break, but bit shifting is cleaner.
Add validation limits as documented constants: - MaxProviderNameLength = 50 - MaxModelNameLength = 100 Use constants in both provider.go and setup.go for better maintainability and self-documenting code.
Remove TestSwitchCmd_ProviderNotFound and TestSwitchCmd_ClaudeNotFound which were empty stubs with only t.Skip() - no actual test implementation. These placeholder tests provided no value and the skip messages were generic without issue references.
Handle error from os.Pipe() in two test functions: - TestSwitchCmd_WithAPIKey_Success - TestSwitchCmd_WithoutAPIKey_Success Previously the error was ignored with _, which could lead to nil pointer assignment if Pipe() fails.
Add comment explaining that CHANGELOG.md is maintained manually following Keep a Changelog format, so auto-generation is disabled.
Replace fmt.Errorf with kairoerrors for consistent error handling: - NetworkError for network-related failures - FileSystemError for file operations - RuntimeError for external command execution Uses WrapError for errors with causes and NewError for status codes.
Replace fmt.Errorf with kairoerrors (typed errors) across the codebase for consistent error handling: - cmd/audit.go: ConfigError - cmd/audit_helpers.go: ConfigError - cmd/config_tx.go: ConfigError, FileSystemError - cmd/metrics.go: ConfigError - cmd/setup.go: ValidationError, ConfigError, CryptoError - internal/backup/backup.go: FileSystemError - internal/config/loader.go: ConfigError, FileSystemError - internal/recover/recover.go: CryptoError - internal/validate/provider.go: ValidationError - internal/wrapper/wrapper.go: ValidationError, FileSystemError
Add tests for: 1. Empty config files (TestLoadConfigEmptyFile) 2. Whitespace-only config files (TestLoadConfigWhitespaceOnly) 3. Comment-only config files (TestLoadConfigCommentOnly) 4. Corrupted secrets files (TestDecryptCorruptedFile) 5. Truncated secrets files (TestDecryptTruncatedFile) 6. Random data as secrets (TestDecryptRandomData) 7. Concurrent config writes (TestConfigCache_ConcurrentWrites)
Add sync.RWMutex to protect the global configDir variable: - setConfigDir() now uses Lock() - getConfigDir() now uses RLock() Fix direct configDir access in tests: - cmd/config_test.go: use setConfigDir() instead of direct assignment - cmd/default_test.go: use getConfigDir() instead of direct read The race detector tests still skip due to complex goroutine synchronization needs in switch_run_test.go - the mutex protects the global state but the test structure needs refactoring.
Add comprehensive unit tests to improve code coverage: - internal/audit: Test LogMigration, file reopening after close, splitLines edge cases, corrupted JSON handling, session ID generation - internal/backup: Test backup creation with missing files, directory handling, overwrite behavior, error cases - internal/config: Test SaveConfig and LoadConfig edge cases, empty files, invalid YAML, ParseSecrets edge cases - cmd: Test config transaction functions (backup, rollback, commit) Coverage improvements: - config_tx.go createConfigBackup: 75% -> 87.5% - config_tx.go rollbackConfig: 66.7% -> 88.9% - config_tx.go withConfigTransaction: 80% -> 90% - backup RestoreBackup: 62.5% -> 70.8% - audit LogMigration: 0% -> 100%
Use write lock instead of read lock to prevent check-then-act race between modtime check and cache invalidation. Direct delete instead of calling Invalidate() to hold lock for entire operation.
Signal handler now only propagates exit code. Deferred cleanup() handles resource cleanup on all exit paths (normal return, signal interrupt, or any error). Prevents race between signal handler and deferred cleanup both trying to execute cleanup().
Add logging when ParseSecrets() skips malformed entries (empty key/value or newlines in key/value) to help users diagnose missing secrets.
- ConfigCache: add Cleanup() method for long-running processes to evict expired entries - Audit: add RotateLog() method with size/age limits to prevent unbounded growth - PowerShell: escape additional characters (&, ;, |, %) to prevent injection - Update: log audit events when config is auto-migrated
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary