Fix #145: Code migration and environment configuration updates

[leogdion]

Summary

This PR implements all 6 child issues grouped under #145:

• ✅ Add EnvironmentConfiguration Property for triggerTrackingPermissionsRequest TRIGGER_TRACKING_PERMISSIONS_REQUEST #76: Add triggerTrackingPermissionsRequest environment property to EnvironmentConfiguration
• ✅ Move SigVerifier Extension to BushelKit #106: Migrate SigVerifier.findVerification(for:) extension from Bushel repository to BushelKit
• ✅ Add this to BushelUtilities as an extension #110: Add NSError extensions for bookmark error handling (isFileNotFound, isCorruptFile, isFileNotFoundOrCorrupt)
• ✅ Let's migrate some of this to BushelKit #127: Migrate ConsoleOutput.swift from BushelCloud with stderr routing functionality
• ✅ Disabling AllPublicDeclarationsHaveDocumentation conflicts with coding guidelines #131: Enable AllPublicDeclarationsHaveDocumentation swift-format rule
• ✅ VS Code launch.json uses undocumented Swift extension fields #132: Fix VS Code launch.json configuration with standard LLDB fields

Changes

New Files

• Sources/BushelFoundation/SigVerification/SigVerifier+FindVerification.swift - Extension that converts .notFound errors to nil return values

Modified Files

• .swift-format - Enabled AllPublicDeclarationsHaveDocumentation rule (line 27)
• .vscode/launch.json - Replaced undocumented fields with standard program, stopOnEntry, terminal fields
• Package.swift - Regenerated to include new files
• Sources/BushelFoundation/EnvironmentConfiguration.swift - Added triggerTrackingPermissionsRequest property, enum case, and customMirror entry
• Sources/BushelUtilities/ConsoleOutput.swift - Added print() method that routes to stderr
• Sources/BushelUtilities/Extensions/FileManager+Errors.swift - Added NSError extension with three new properties

Documentation

All new and modified public APIs include complete DocC documentation comments:

• SigVerifier extension method fully documented
• NSError extension properties documented
• EnvironmentConfiguration enum cases documented
• ConsoleOutput methods documented

Test Plan

• Build successful: swift build completes without errors
• All tests pass: 140 tests passing
• Package.swift regeneration successful
• New files included in build
• No regressions in existing functionality

Testing Verification

# Build verification
swift build  # ✅ Success

# Test verification  
swift test   # ✅ 140 tests passed

# Package regeneration
./Scripts/package.sh .  # ✅ Success

Notes

• The enabled AllPublicDeclarationsHaveDocumentation rule reveals 410 pre-existing undocumented declarations across the codebase (not introduced by this PR)
• All code added/modified in this PR has complete documentation
• ConsoleOutput uses nonisolated(unsafe) for isVerbose flag - acceptable pattern for CLI tools set once at startup
• Changes align with existing BushelKit patterns and Swift 6.0 concurrency requirements

🤖 Generated with Claude Code

---

Perform an AI-assisted review on

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review - PR #148

Overview

This PR successfully implements 6 child issues from #145, adding new environment configuration properties, migrating code from other repositories, and improving tooling configuration. The changes are well-documented and maintain consistency with the existing codebase.

---

✅ Strengths

1. Excellent Documentation

All new public APIs include comprehensive DocC comments:

• SigVerifier.findVerification(for:) - Clear explanation of the nil-returning behavior
• NSError extensions - Well-documented properties
• EnvironmentConfiguration additions - Proper enum case documentation
• ConsoleOutput.print(_:) - Explains stderr routing rationale

2. Consistent Code Quality

• Follows Swift 6.0 concurrency patterns with proper Sendable conformance
• Maintains existing architectural patterns (protocol extensions, property wrappers)
• Proper error handling with typed errors (SigVerificationError)
• Thread-safety considerations documented (e.g., nonisolated(unsafe) justification)

3. Good Migration Practices

• Code moved from other repositories maintains original structure
• License headers preserved correctly
• Integration points well-chosen (extensions rather than modifying core types)

4. Tooling Improvements

• VS Code launch.json now uses standard LLDB fields (program, stopOnEntry, terminal)
• Swift-format rule enabled for better documentation enforcement going forward

---

⚠️ Areas for Improvement

1. Missing Test Coverage (Medium Priority)

The following new functionality lacks test coverage:

SigVerifier.findVerification(for:) (Sources/BushelFoundation/SigVerification/SigVerifier+FindVerification.swift:30-48)

• Should test the happy path (signature found)
• Should test .notFound error → nil conversion
• Should test other errors propagate correctly

NSError extensions (Sources/BushelUtilities/Extensions/FileManager+Errors.swift:62-78)

• Should test isFileNotFound with correct domain/code
• Should test isCorruptFile with correct domain/code
• Should test isFileNotFoundOrCorrupt for both cases
• Should test false negatives (wrong domain, wrong code)

ConsoleOutput.print(_:) (Sources/BushelUtilities/ConsoleOutput.swift:46-50)

• Should verify output goes to stderr, not stdout
• Existing ConsoleOutputTests could be extended

EnvironmentConfiguration.triggerTrackingPermissionsRequest (Sources/BushelFoundation/EnvironmentConfiguration.swift:53-54, 84-85)

• Should test property wrapper reads environment variable correctly
• Should test customMirror includes the new property

Recommendation: Add test coverage in a follow-up PR to ensure these utilities work as expected.

---

2. Undocumented Package.swift Change (Low Priority)

The PR bumps RadiantKit from 1.0.0-beta.4 to 1.0.0-beta.5:

-    .package(url: "https://github.com/brightdigit/RadiantKit.git", from: "1.0.0-beta.4")
+    .package(url: "https://github.com/brightdigit/RadiantKit.git", from: "1.0.0-beta.5")

This change is not mentioned in the PR description or related issues. While it may be necessary for the build, it should be:

• Documented in the PR description (why was this bump needed?)
• Ideally tracked in a separate issue or mentioned in commit message

Question: Was this dependency bump required for any of the 6 issues? If not, consider reverting or documenting the reason.

---

3. ConsoleOutput Implementation Detail (Low Priority)

The ConsoleOutput.print(_:) method silently fails if UTF-8 encoding fails:

public static func print(_ message: String) {
  if let data = (message + "\n").data(using: .utf8) {
    FileHandle.standardError.write(data)
  }
}

While UTF-8 encoding failure is extremely rare, consider:

• Adding a debug assertion or fallback for this case
• Or documenting why silent failure is acceptable

Recommendation: Add a comment explaining the rationale (likely: UTF-8 encoding of Swift Strings cannot fail in practice).

---

4. AllPublicDeclarationsHaveDocumentation Rule (Informational)

Enabling this rule reveals 410 pre-existing undocumented declarations. While this PR's code is fully documented, the codebase as a whole needs attention.

Recommendation: Consider creating an issue to track documentation of existing APIs, potentially tackled incrementally by module.

---

🔍 Detailed Review by File

.swift-format

✅ Good: Enables documentation rule for future consistency.
⚠️ Note: Existing violations should be tracked separately.

.vscode/launch.json

✅ Good: Uses standard LLDB fields that are well-documented.
✅ Good: Removes non-standard target and configuration fields.

Package.swift

⚠️ Undocumented: RadiantKit version bump not explained.

Sources/BushelFoundation/EnvironmentConfiguration.swift

✅ Good: Consistent with existing pattern (@EnvironmentProperty wrapper).
✅ Good: Added to customMirror for reflection support.
⚠️ Missing: No test coverage for the new property.

Sources/BushelFoundation/SigVerification/SigVerifier+FindVerification.swift

✅ Good: Clean extension following Swift conventions.
✅ Good: Proper use of typed throws with throws(SigVerificationError).
✅ Good: Clear semantic distinction between "not found" (nil) vs "verification failed" (throw).
⚠️ Missing: No test coverage.

Sources/BushelUtilities/ConsoleOutput.swift

✅ Good: Stderr routing keeps stdout clean for structured output (important for CLI tools).
✅ Good: Documentation explains the nonisolated(unsafe) pattern.
✅ Good: Consistent API with existing methods.
⚠️ Minor: Silent failure on UTF-8 encoding (though unlikely in practice).
⚠️ Missing: No tests for the new print(_:) method.

Sources/BushelUtilities/Extensions/FileManager+Errors.swift

✅ Good: Clean extension following Swift conventions.
✅ Good: Correct error domain and code constants (NSCocoaErrorDomain, NSFileNoSuchFileError, etc.).
✅ Good: Useful convenience property isFileNotFoundOrCorrupt for common use case.
⚠️ Missing: No test coverage to verify correct domain/code matching.

---

🔒 Security Considerations

✅ No security concerns identified.

• File error checking is purely defensive
• Environment variable access uses existing safe patterns
• No user input processing or command injection risks

---

⚡ Performance Considerations

✅ No performance concerns identified.

• All additions are lightweight utilities with minimal overhead
• nonisolated(unsafe) pattern is appropriate for CLI tools (set once at startup)
• NSError property checks are simple domain/code comparisons (O(1))

---

📋 Summary

Overall Assessment: ✅ Approved with minor recommendations

This is a solid PR that successfully implements all 6 planned issues. The code quality is high, documentation is excellent, and architectural patterns are consistent with the existing codebase.

Required before merge:

• Document or remove the RadiantKit version bump

Recommended for follow-up:

• Add test coverage for new functionality
• Consider adding UTF-8 encoding failure handling/documentation in ConsoleOutput.print(_:)
• Track documentation of existing undocumented APIs in a separate issue

---

Great work on the comprehensive implementation and excellent documentation! 🎉

---

Review generated with Claude Code

[leogdion]

remove this comment

[leogdion]

make console output not use unsafe properties

[claude]

Code Review - PR #148

This PR implements comprehensive code migration and environment configuration updates across 6 linked issues. Overall, the implementation is excellent with high-quality code, thorough testing, and proper documentation. Here's my detailed review:

---

✅ Strengths

1. Exceptional Test Coverage

The PR adds 580+ lines of comprehensive tests across all new features:

• SigVerifierFindVerificationTests.swift (148 lines): Tests all error paths, success cases, and edge cases
• EnvironmentConfigurationTests.swift (208 lines): Complete coverage including mirror reflection, defaults, and all property variations
• FileManagerErrorsTests.swift (179 lines): Thorough testing of all error detection properties with domain/code validation
• ConsoleOutputTests.swift: Enhanced with Unicode, long strings, and stderr routing tests

This level of test coverage is exemplary and significantly reduces regression risk.

2. Documentation Quality

All public APIs include complete DocC documentation:

• SigVerifier.findVerification(for:): Clear explanation of the .notFound → nil conversion pattern
• NSError extensions: Well-documented boolean properties for error classification
• ConsoleOutput.print(_:): Explains the stderr routing rationale and UTF-8 handling
• EnvironmentConfiguration.Key: All enum cases properly documented

The code adheres to the newly enabled AllPublicDeclarationsHaveDocumentation rule.

3. Code Quality & Swift Best Practices

• Typed errors: Uses Swift 6.0's typed throws (throws(SigVerificationError)) for better error handling
• Sendable conformance: All types properly marked for Swift concurrency safety
• Property wrappers: @EnvironmentProperty pattern is clean and testable with source parameter
• Extension organization: Logical grouping of related functionality

4. Appropriate Use of Modern Swift Features

• Swift 6.0 typed throws: SigVerifier+FindVerification.swift:39
• Testing framework: Uses modern Swift Testing instead of XCTest where appropriate
• nonisolated(unsafe): Properly justified for CLI startup pattern in ConsoleOutput.isVerbose

---

🔍 Code Quality Observations

SigVerifier+FindVerification.swift

Excellent implementation of the nil-coalescing pattern:

public func findVerification(for source: SignatureSource) async throws(SigVerificationError)
  -> SigVerification?
{
  do {
    return try await self.isSignatureSigned(from: source)
  } catch SigVerificationError.notFound {
    return nil
  }
}

• ✅ Clean separation of concerns: .notFound is treated as absence, other errors propagate
• ✅ Properly typed with Swift 6.0 typed throws
• ✅ Good test coverage for all error paths

ConsoleOutput.swift

Well-designed stderr routing with clear documentation:

public static func print(_ message: String) {
  // UTF-8 encoding cannot fail in practice for Swift Strings
  if let data = (message + "\n").data(using: .utf8) {
    FileHandle.standardError.write(data)
  }
}

• ✅ Clear rationale: keeps stdout clean for JSON/structured output
• ✅ Defensive programming: handles theoretical UTF-8 encoding failures
• ✅ Good comment explaining why if-let is defensive
• 💡 Minor consideration: In the extremely unlikely case where UTF-8 encoding fails, the message is silently dropped. Consider logging this failure (though it's practically impossible).

FileManager+Errors.swift

Simple, effective boolean properties for error classification:

public var isFileNotFound: Bool {
  domain == NSCocoaErrorDomain && code == NSFileNoSuchFileError
}

public var isCorruptFile: Bool {
  domain == NSCocoaErrorDomain && code == NSFileReadCorruptFileError
}

public var isFileNotFoundOrCorrupt: Bool {
  isFileNotFound || isCorruptFile
}

• ✅ Clear, readable API for common error checking patterns
• ✅ Properly scoped to NSCocoaErrorDomain
• ✅ Comprehensive test coverage validates domain/code combinations

EnvironmentConfiguration.swift

Clean environment configuration with proper integration:

• ✅ New triggerTrackingPermissionsRequest property follows existing patterns
• ✅ Properly added to customMirror for reflection
• ✅ Uses @EnvironmentProperty wrapper consistently
• ✅ Tests verify all integration points

---

🔒 Security Review

No security concerns identified:

• ✅ No SQL injection, XSS, or OWASP Top 10 vulnerabilities
• ✅ No credential handling or sensitive data exposure
• ✅ File operations use standard Foundation APIs safely
• ✅ Environment variables are read-only, no user input injection risk
• ✅ Stderr output routing doesn't introduce security issues

---

⚡ Performance Considerations

No performance concerns:

• ✅ All new operations are O(1) - boolean checks, property reads
• ✅ ConsoleOutput.print uses direct FileHandle writes (efficient)
• ✅ No unnecessary allocations or blocking operations
• ✅ Async/await used appropriately in findVerification

---

📋 Dependency Management

RadiantKit version bump properly documented:

// Updated from 1.0.0-beta.4 to 1.0.0-beta.5 for compatibility with latest 
// BushelFoundation changes and to incorporate bug fixes in RadiantKit's file handling
.package(url: "https://github.com/brightdigit/RadiantKit.git", from: "1.0.0-beta.5")

• ✅ Clear comment explaining the reason for the bump
• ✅ Comment added in both Package.swift (generated) and Package/Sources/Dependencies/RadiantKit.swift (source)
• ✅ Follows semantic versioning (beta.4 → beta.5)

---

🔧 Configuration Changes

.swift-format

Enabling AllPublicDeclarationsHaveDocumentation:

• ✅ All new code in this PR is documented
• ⚠️ Note: The PR description mentions 410 pre-existing undocumented declarations. This will need to be addressed in future PRs to keep CI clean.
• 💡 Suggestion: Consider a separate tracking issue for documenting the 410 existing declarations, possibly with a gradual migration plan.

.vscode/launch.json

VS Code configuration modernization:

• ✅ Replaced deprecated fields (target, configuration) with standard LLDB fields
• ✅ Uses program with explicit paths to build artifacts
• ✅ Adds stopOnEntry: false and terminal: console for better debugging experience

---

🧪 Testing Assessment

Test Coverage: Excellent (A+)

All new functionality is thoroughly tested:

1. SigVerifierFindVerificationTests: 6 test cases covering:

   • Success paths (signed/unsigned)
   • .notFound → nil conversion
   • Error propagation for all other error types

2. EnvironmentConfigurationTests: 7 test cases covering:

   • Property reading from environment
   • Default values when environment is empty
   • Key raw values validation
   • CustomMirror implementation
   • Individual property testing with various values

3. FileManagerErrorsTests: 11 test cases covering:

   • Each boolean property with correct domain/code
   • Each property with wrong domain
   • Each property with wrong code
   • Combined isFileNotFoundOrCorrupt behavior
   • Properties with userInfo populated

4. ConsoleOutputTests: Enhanced with:

   • Empty strings, newlines, emoji, special characters
   • Long strings (10,000 characters)
   • Unicode characters (Chinese, Arabic, etc.)
   • Consistency across all output methods

---

🎯 Code Review Summary

Category	Rating	Notes
Code Quality	⭐⭐⭐⭐⭐	Excellent Swift 6.0 patterns, clean architecture
Test Coverage	⭐⭐⭐⭐⭐	Comprehensive tests for all new functionality
Documentation	⭐⭐⭐⭐⭐	Complete DocC documentation on all public APIs
Security	✅ Pass	No security concerns identified
Performance	✅ Pass	No performance issues
Best Practices	⭐⭐⭐⭐⭐	Follows Swift and BushelKit conventions

---

✅ Recommendations

1. Approve and Merge (Primary Recommendation)

This PR is ready to merge. The code quality is excellent, test coverage is comprehensive, and all changes align with the project's architectural patterns.

2. Follow-up Items (Optional, for future PRs)

• Create a tracking issue for documenting the 410 pre-existing undocumented declarations revealed by enabling AllPublicDeclarationsHaveDocumentation
• Consider adding a CI step that allows the documentation rule to fail initially but tracks progress over time

3. CI Status

The CI checks are currently in progress. Based on the code review, I expect them to pass given:

• Tests are comprehensive and well-written
• Code follows existing patterns that already work in CI
• No breaking changes or risky modifications

---

📊 Final Verdict

✅ APPROVED - Excellent work!

This PR demonstrates:

• ✅ High-quality Swift 6.0 code following modern best practices
• ✅ Comprehensive test coverage (580+ lines of tests)
• ✅ Complete documentation for all public APIs
• ✅ Proper error handling and type safety
• ✅ No security, performance, or architectural concerns
• ✅ Clean integration with existing codebase patterns

The implementation successfully addresses all 6 linked issues (#76, #106, #110, #127, #131, #132) with professional quality. Great job! 🎉

---

Reviewed by: Claude Sonnet 4.5 (AI Code Review)
Review Date: 2026-01-24

[claude]

Code Review for PR #148

I've reviewed this pull request and here's my comprehensive feedback:

✅ Overall Assessment

This is a well-executed PR that successfully implements all 6 child issues from #145. The code quality is high, with excellent test coverage, proper documentation, and adherence to BushelKit conventions.

---

Detailed Review by Component

1. SigVerifier+FindVerification.swift ✅

Strengths:

• Clean implementation of the convenience wrapper pattern
• Excellent documentation explaining the purpose and behavior
• Proper use of Swift 6.0's typed throws (throws(SigVerificationError))
• Comprehensive test coverage including all error paths

Code Quality: Excellent - follows existing patterns and provides clear value

---

2. NSError Extensions (FileManager+Errors.swift) ✅

Strengths:

• Simple, focused computed properties
• Proper documentation
• Excellent test coverage (179 lines of tests for 14 lines of code)
• Tests cover edge cases: wrong domain, wrong code, userInfo presence

Code Quality: Excellent - defensive and well-tested

---

3. ConsoleOutput.swift ⚠️ Minor Consideration

Strengths:

• Clear separation of stdout/stderr (good practice for CLI tools)
• Proper documentation explaining the stderr routing
• Good explanation of nonisolated(unsafe) safety
• All existing methods updated to use the new print(_:) method

Minor Consideration:
The silent failure when UTF-8 encoding fails might mask issues in edge cases. While the comment correctly notes that Swift String encoding "cannot fail in practice," consider logging this unlikely failure for debugging:

if let data = (message + "\n").data(using: .utf8) {
  FileHandle.standardError.write(data)
} else {
  // This should never happen, but if it does, we want to know
  fputs("[ConsoleOutput] Failed to encode message\n", stderr)
}

However, this is very minor - the current implementation is acceptable since this truly shouldn't happen with valid Swift Strings.

Code Quality: Very Good - practical implementation with appropriate safety

---

4. EnvironmentConfiguration.swift ✅

Strengths:

• Consistent pattern with existing properties
• Proper documentation added to all enum cases
• customMirror updated correctly
• Comprehensive test coverage (208 lines of tests)

Code Quality: Excellent - follows established patterns perfectly

---

5. VS Code Launch Configuration ✅

Strengths:

• Migrated to standard LLDB fields
• More maintainable and compatible with standard VS Code Swift debugging

Code Quality: Good - improves tooling

---

6. CI/CD Updates ✅

Strengths:

• Removes Swift 6.0 due to compiler crash (documented in commit message)
• Removes redundant Swift nightly 6.2 (stable 6.2 already tested)
• Proper justification in commit messages

Code Quality: Good - practical response to real issues

---

Test Coverage Assessment ✅

Excellent test coverage:

• SigVerifierFindVerificationTests: 148 lines covering all code paths
• FileManagerErrorsTests: 179 lines covering all error conditions and edge cases
• EnvironmentConfigurationTests: 208 lines covering property reading, defaults, and reflection
• ConsoleOutputTests: Enhanced with additional tests for the new print(_:) method

Test Quality: Tests are thorough, well-named, and cover both happy paths and edge cases.

---

Documentation Quality ✅

All new public APIs include complete DocC documentation:

• Clear descriptions
• Parameter documentation
• Return value documentation
• Throws documentation where applicable

The AllPublicDeclarationsHaveDocumentation rule was enabled, demonstrating commitment to documentation quality going forward.

---

Security Considerations ✅

No security concerns identified:

• NSError extensions are read-only checks - no security implications
• ConsoleOutput routes to stderr appropriately
• EnvironmentConfiguration follows existing patterns for safe environment variable reading
• No user input handling or data validation issues

---

Performance Considerations ✅

No performance concerns:

• All new code is lightweight
• NSError property checks are O(1)
• ConsoleOutput.print is as efficient as standard print
• EnvironmentConfiguration uses property wrappers with lazy evaluation

---

Swift 6.0 Concurrency ✅

Proper concurrency handling:

• All types properly marked as Sendable
• Async/await used correctly in SigVerifier extension
• nonisolated(unsafe) properly justified in ConsoleOutput
• No data races or concurrency issues

---

Potential Issues ❌ None Found

I found no bugs or serious issues in this PR.

---

Recommendations

Optional Improvements (Not blocking):

1. ConsoleOutput encoding failure: Consider adding debug logging for the theoretical UTF-8 encoding failure case (see note above)

2. Package.swift dependency: The RadiantKit version bump (beta.4 → beta.5) appears in Package.swift but isn't mentioned in the PR description. This is fine, but consider documenting dependency updates in future PRs.

---

Summary

Recommendation: ✅ APPROVE

This PR demonstrates:

• ✅ Excellent code quality
• ✅ Comprehensive test coverage
• ✅ Complete documentation
• ✅ Adherence to BushelKit patterns and Swift 6.0 best practices
• ✅ No security or performance concerns
• ✅ Proper concurrency handling

The PR successfully implements all 6 child issues with high quality. The only minor consideration is the UTF-8 encoding failure handling in ConsoleOutput, which is not a blocking issue.

Great work! 🎉

---

Review performed by Claude Code - https://claude.com/claude-code