-
Notifications
You must be signed in to change notification settings - Fork 10
test: improve test coverage from 37.6% to 40.0% #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
marcus
wants to merge
8
commits into
main
Choose a base branch
from
feat/improve-test-coverage
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
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
- Fix 8 errcheck violations in run.go: - displayPreflight: assign fmt.Fprintf/Fprintln errors to _ (lines 475-530) - ensurePATH: assign os.Setenv error to _ (line 918) - Fix 4 errcheck violations in run_test.go: - TestConfirmRun_TTYAcceptsY: assign w.Close error to _ (line 864) - TestConfirmRun_TTYAcceptsYes: assign w.Close error to _ (line 886) - TestConfirmRun_TTYDefaultRejectsEmpty: assign w.Close error to _ (line 930) - captureStdout: assign w.Close error to _ (line 956) - Fix 1 staticcheck QF1008 violation in stats.go: - MarshalJSON: use d.Seconds() directly instead of d.Duration.Seconds() (line 31) Co-Authored-By: Claude Haiku 4.5 <[email protected]>
## Security Fixes 1. Fix dangerous default configurations - Change DangerouslySkipPermissions default from true → false - Change DangerouslyBypassApprovalsAndSandbox default from true → false - Users now must explicitly opt-in to skip security prompts rather than opt-out 2. Fix database directory permissions - Change DB directory mode from 0755 to 0700 - Restricts access to owner only (was: world-readable) - Database contains sensitive execution history and token data 3. Add shell path escaping in PATH configuration - New escapeShellPath() function prevents shell injection - Properly quotes and escapes special characters ($, \`, ", \) - Prevents shell startup failures with unusual path names ## Security Audit Summary Identified 10 security anti-patterns in codebase: - 2 HIGH severity: dangerous defaults, missing path validation - 6 MEDIUM severity: file permissions, path traversal, error handling - 1 LOW severity: hardcoded artifact names See SECURITY_AUDIT.md for full analysis and recommendations for remaining items. ## Testing - ✅ Code compiles without errors - ✅ Configuration defaults now false (safer) - ✅ DB directory uses 0700 permissions - ✅ Shell paths properly escaped Co-Authored-By: Claude Haiku 4.5 <[email protected]>
Add tests and documentation for backward compatibility of v0.3.1 security fixes: 1. Config backward compatibility: - Old v0.3.0 configs load correctly with new security defaults - Dangerous flags now default to false (opt-in required) - Explicit flag values are preserved - Environment overrides still work - Validation rules unchanged - Config merging preserved 2. Database backward compatibility: - New database directories use 0700 permissions (stricter) - Old databases with 0755 permissions still work - Migrations are idempotent (safe to re-run) - Schema additions (provider column, reset times) backward compatible - Path expansion still works 3. Shell path escaping backward compatibility: - Paths without special chars continue to work - Proper escaping prevents shell injection - Path detection algorithms unchanged - Symlink resolution preserved 4. Documentation: - Migration guide explaining breaking changes - CHANGELOG updated with backward compat notes - Clear guidance for unattended execution All tests pass. CLI interface stable for scripts/automation. Co-Authored-By: Claude Haiku 4.5 <[email protected]>
Add comprehensive bus-factor analysis to identify single-person dependencies and code ownership concentration risks: - internal/analysis package: git history parser, concentration metrics calculator - Metrics: Herfindahl Index, Gini Coefficient, Top-N ownership percentages - Risk assessment: critical/high/medium/low based on concentration and contributor count - Database schema: store analysis results with historical tracking - CLI command: nightshift busfactor with filtering options (date range, file pattern) - Report generation: markdown and JSON output with recommendations - Comprehensive test coverage: unit and integration tests - Documentation: metrics interpretation, usage examples, best practices Bus Factor = minimum contributors needed for 50% of commits. Helps identify knowledge concentration risks and track sustainability over time. Co-Authored-By: Claude Haiku 4.5 <[email protected]>
…or report
Fixed a critical bug in RenderMarkdown that prevented proper formatting of
recommendations. The original code was checking if rec[0:4] matched specific
prefixes ('GOOD', 'HIGH', 'CRIT', 'MEDI'), which failed for:
1. 'CRITICAL' (8 chars, not 4)
2. Other recommendations not starting with these prefixes (e.g., 'Pair...', 'Target...', etc.)
Changed to use bytes.HasPrefix() to properly check full keyword prefixes, ensuring:
- Risk level keywords (CRITICAL, HIGH, MEDIUM, GOOD) are correctly identified and bolded
- Other recommendations are properly formatted as bullet points
- All recommendation types render correctly in the markdown output
Co-Authored-By: Claude Haiku 4.5 <[email protected]>
…analysis packages
…erages, and Prune
Adds update-homebrew-tap job to prevent formula drift. Same pattern as sidecar. Refs marcus/sidecar#122.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Improved overall test coverage from 37.6% to 40.0% by adding comprehensive unit tests across multiple packages. Focus was on high-value, frequently-used functions and critical business logic.
Test Improvements
New Test Files Created
cmd/nightshift/commands/budget_test.go- Tests for budget command helpers and formatting functionsinternal/setup/presets_test.go- Tests for setup package preset selection logicinternal/reporting/run_report_test.go- Tests for run report generationinternal/reporting/run_results_test.go- Tests for run results persistenceinternal/analysis/db_test.go- Tests for analysis database operationsinternal/snapshots/collector_extended_test.go- Additional snapshot collector testsCoverage Improvements by Package
Test Coverage Details
Budget Command Tests (27 tests)
formatTokens64: Tests for token formatting at different scalesformatBudgetMeta: Tests for budget metadata formattingformatResetLine: Tests for reset time displayprogressBar: Tests for progress bar renderingresolveProviderList: Tests for provider filtering and validationSetup Package Tests (18 tests)
DetectRepoSignals: Repository characteristic detection (release signals, ADR structure)PresetTasks: Task filtering logic for safe/balanced/aggressive presetshasAny: File/directory existence checkingReporting Package Tests (29 tests)
Analysis Package Tests (10 tests)
BusFactorResult.Store: Database persistenceLoadLatest: Retrieval of most recent analysisLoadAll: Bulk retrieval with date filteringSnapshots Package Tests (11 tests)
GetSinceWeekStart: Week-bounded snapshot retrievalGetHourlyAverages: Hourly aggregation with lookback windowsPrune: Retention-based cleanup with edge casesTest Execution
All tests pass successfully:
Implementation Notes
Tests follow existing patterns:
🤖 Generated with Claude Code