Nsm modernize with tests#1
Merged
Merged
Conversation
I used claude to review the code and manually checked every change - Update Go 1.15 → 1.23, CBOR library v2.2.0 → v2.7.0 - Fix potential panic on empty slice access in IOCTL operations - Fix resource leaks: only clear fd on successful close - Fix infinite loop in Read() entropy generation - Fix inconsistent nil checking patterns throughout codebase - Fix duplicate JSON tags in response structs - Document blocking IOCTL behavior and performance implications - Add basic GitHub Actions CI workflow Co-Authored-By: Claude <noreply@anthropic.com>
- Fix buffer overrun: validate response length before slicing - Fix type assertion panics: add safe checks for pool operations - Fix race condition: use defer for cleanup in Close() method - Add input validation: check request/response sizes and nil values - Add error context: wrap CBOR and encoding errors with details - Fix README examples: correct defer placement after error checks - Update README: use consistent Go idioms (err != nil) Addresses memory safety issues that could cause crashes or data corruption in production AWS Nitro Enclave environments. Co-Authored-By: Claude <noreply@anthropic.com>
- Add tests validating empty buffer protection against panics
- Add tests for nil session handling and proper error returns
- Add tests for resource cleanup with defer in Close() method
- Add tests for type-safe pool operations without panics
- Add tests for input validation and closed session detection
- Fix nil pool access: check pools before use in Send/Read
- Modernize: use 'any' instead of 'interface{}' for Go 1.18+
- All tests pass, validating production safety of security fixes
Co-Authored-By: Claude <noreply@anthropic.com>
- Add test coverage for ioc package: IOCTL command generation (100%) - Add test coverage for request package: all Encoded() methods (100%) - Add test coverage for response package: CBOR unmarshaling (100%) - Extend nsm package tests: sendMarshaled validation, successful paths - Improve main package coverage from 44.6% to 63.4% - Add tests for OpenDefaultSession, pool type safety, error handling - Tests validate all security fixes and core functionality - All tests are focused, human-reviewable, and production-ready Co-Authored-By: Claude <noreply@anthropic.com>
- Add GitHub Actions workflow with lint, test, and coverage checks - Pin all dependencies: golangci-lint v1.61.0, actions@v4/v5/v6 - Use go.mod version (1.23) for consistent builds - Enforce 60% minimum test coverage with built-in tooling - Add Makefile for local development with same standards - Self-contained pipeline - no external services or tokens required - Add .gitignore with https://github.com/github/gitignore/blob/main/Go.gitignore Co-Authored-By: Claude <noreply@anthropic.com>
- Fix unused parameters in test functions by replacing with underscore - Organize constants and variables at top of nsm.go file - Add tools.go for development dependencies with go run approach - Update Makefile to use go run for tools instead of install - Update CI workflow to use Makefile targets for consistency Co-Authored-By: Claude <noreply@anthropic.com>
- Add make fmt command with gofmt and goimports - Make lint target depend on fmt to ensure proper formatting - Fix unused parameters in test functions by using underscore - Fix nil pointer dereference warning with else-if pattern - Fix allocation warnings by using original pool objects - Add tools.go for development dependencies with go:build tools tag - Update Makefile to use go run for tools instead of installation - Ensure consistent code formatting across all files Co-Authored-By: Claude <noreply@anthropic.com>
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.
Same as hf#6 but I want to make sure CI runs