Skip to content

feat(ccusage): add CCUsage collection service for usage tracking#106

Merged
AnnatarHe merged 9 commits intomainfrom
feat/ccusage-collection-service
Sep 27, 2025
Merged

feat(ccusage): add CCUsage collection service for usage tracking#106
AnnatarHe merged 9 commits intomainfrom
feat/ccusage-collection-service

Conversation

@AnnatarHe
Copy link
Copy Markdown
Contributor

Description

This PR adds a new CCUsage collection service that tracks Claude Code usage statistics when enabled.

Changes

  • Added CCUsage configuration: New optional CCUsage struct in config with Enabled field
  • Implemented CCUsage service:
    • Collects usage data every hour using bunx ccusage daily --json or npx ccusage daily --json
    • Falls back between bunx and npx based on availability
    • Sends collected data to server endpoint /api/v1/ccusage
  • Integrated into daemon: Service starts automatically when CCUsage is enabled in config

Configuration

To enable CCUsage collection, add to your config.toml:

[ccusage]
enabled = true

Implementation Details

  • Service checks for bunx first, falls back to npx if not available
  • Returns error if neither package runner is found
  • Enriches ccusage data with system metadata (hostname, username, OS info)
  • Runs collection every hour with proper lifecycle management

Testing

  • Service only runs when explicitly enabled (default: disabled)
  • Gracefully handles missing bunx/npx
  • Proper error handling and logging throughout

- Add CCUsage configuration struct with optional enabled field
- Implement CCUsage service that collects usage data hourly using bunx/npx
- Integrate service into daemon to run when enabled
- Split data collection (via ccusage CLI) and remote sending logic
- Support both bunx and npx package runners with fallback
@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello @AnnatarHe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a new feature for collecting Claude Code usage statistics. It provides a dedicated service that, when enabled through configuration, periodically gathers usage data from the system using available package runners (bunx or npx), enriches it with relevant system information, and transmits it to a backend API. This enhancement aims to provide valuable insights into application usage patterns without affecting core functionality if disabled.

Highlights

  • New CCUsage Collection Service: Introduced a new service to track Claude Code usage statistics, which can be enabled via configuration. This service periodically collects usage data and sends it to a specified API endpoint.
  • Configurable Usage Tracking: Added a new CCUsage configuration struct to model/types.go, allowing users to enable or disable usage tracking through the config.toml file.
  • Daemon Integration: The CCUsageService is now integrated into the daemon's main process, starting automatically if enabled in the configuration and ensuring proper lifecycle management (start/stop).
  • Flexible Data Collection: The service intelligently uses either bunx or npx to execute the ccusage command, falling back to npx if bunx is not available, and enriches the collected data with system metadata like hostname, username, and OS information.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Sep 26, 2025

Claude finished @AnnatarHe's task —— View job


Code Review for CCUsage Collection Service

I've completed a thorough review of the CCUsage collection service implementation. Here's my comprehensive feedback:

Todo List

  • Read and analyze changed files
  • Review code quality and best practices
  • Identify potential bugs or issues
  • Check performance considerations
  • Evaluate security concerns
  • Assess test coverage
  • Provide comprehensive feedback

Positive Aspects

  1. Clean Architecture: Well-structured service with clear separation of concerns using interfaces
  2. Graceful Fallback: Smart logic to try bunx first, then fallback to npx
  3. Proper Lifecycle Management: Good context handling and graceful shutdown mechanisms
  4. Privacy-Conscious: Only runs when explicitly enabled (default disabled)
  5. Error Handling: Comprehensive error handling with informative messages
  6. System Integration: Follows existing patterns in the codebase (uses existing HTTP client, config system)

⚠️ Issues & Recommendations

1. Critical: Missing Input Validation & Security

Location: model/ccusage_service.go:155-159

var ccusageOutput map[string]interface{}
if err := json.Unmarshal(output, &ccusageOutput); err != nil {
    return nil, fmt.Errorf("failed to parse ccusage output: %w", err)
}

Issue: Direct unmarshaling of external command output without validation could lead to memory exhaustion or unexpected data structures.

Recommendation: Add size limits and basic validation:

if len(output) > 10*1024*1024 { // 10MB limit
    return nil, fmt.Errorf("ccusage output too large: %d bytes", len(output))
}

2. Performance: Resource Leaks

Location: model/ccusage_service.go:88-93

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    close(s.stopChan)
}

Issue: Potential race condition - multiple calls to Stop() will panic due to closing an already closed channel.

Recommendation: Add protection:

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    select {
    case <-s.stopChan:
        // Already closed
    default:
        close(s.stopChan)
    }
}

3. Security: Command Injection Risk

Location: model/ccusage_service.go:138-144
Issue: While using exec.CommandContext is secure, the hardcoded arguments should be explicitly documented as safe.

Recommendation: Add a comment explaining the security model:

// Safe: arguments are hardcoded constants, no user input
cmd = exec.CommandContext(ctx, bunxPath, "ccusage", "daily", "--instances", "--json")

4. Error Handling: Insufficient Context

Location: model/ccusage_service.go:109-119
Issue: Network failures are logged but don't include enough context for debugging.

Recommendation: Add structured logging:

if err != nil {
    logrus.WithFields(logrus.Fields{
        "endpoint": s.config.APIEndpoint,
        "error": err,
    }).Warn("Failed to send usage data")
    return fmt.Errorf("failed to send usage data: %w", err)
}

5. Configuration: Potential NPE

Location: cmd/daemon/main.go:99-107

if cfg.CCUsage != nil && cfg.CCUsage.Enabled != nil && *cfg.CCUsage.Enabled {

Issue: Triple pointer dereferencing is error-prone and verbose.

Recommendation: Add a helper method in model/types.go:

func (c *CCUsage) IsEnabled() bool {
    return c != nil && c.Enabled != nil && *c.Enabled
}

6. Missing Test Coverage

Critical Issue: No tests found for the new CCUsage functionality.

Required Tests:

  • Unit tests for CCUsageService interface methods
  • Mock tests for external command execution
  • Integration tests for the service lifecycle
  • Error condition testing (missing binaries, network failures)

7. Documentation Gaps

Missing:

  • Godoc comments for public types and methods
  • Error condition documentation
  • Rate limiting considerations for the hourly collection

🔒 Security Assessment

Overall: ✅ Good - No major security vulnerabilities detected

  • ✅ External commands use safe exec.CommandContext
  • ✅ No user input in command arguments
  • ✅ Uses existing secure HTTP client
  • ✅ Opt-in functionality (disabled by default)
  • ⚠️ Minor: Should validate external command output size

🚀 Performance Considerations

  • ✅ Hourly collection is reasonable frequency
  • ✅ Uses context for cancellation
  • ⚠️ No timeout on external command execution
  • ⚠️ Large JSON responses could consume significant memory

Recommendation: Add command timeout:

ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()
cmd = exec.CommandContext(ctx, bunxPath, ...)

📋 Test Coverage Priority

  1. High: Service lifecycle (Start/Stop)
  2. High: Command execution with mocked binaries
  3. Medium: Network failure handling
  4. Medium: Configuration validation
  5. Low: System info collection

🎯 Final Recommendation

Approve with minor fixes. The implementation is solid and follows good practices. Address the resource leak issue and add basic tests before merging.

Priority Fixes:

  1. Fix Stop() method race condition
  2. Add command timeout
  3. Add basic unit tests
  4. Consider input validation for large outputs

The feature is well-implemented and ready for production with these minor improvements.

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 26, 2025

Codecov Report

❌ Patch coverage is 0% with 438 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
model/ccusage_service.go 0.00% 310 Missing ⚠️
model/api.base.go 0.00% 110 Missing ⚠️
cmd/daemon/main.go 0.00% 9 Missing ⚠️
model/dotfile.go 0.00% 9 Missing ⚠️
Flag Coverage Δ
unittests 20.95% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
cmd/daemon/main.go 0.00% <0.00%> (ø)
model/dotfile.go 0.00% <0.00%> (ø)
model/api.base.go 17.83% <0.00%> (-26.17%) ⬇️
model/ccusage_service.go 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new service for collecting Claude Code usage statistics. The implementation is well-structured, with a clear separation of concerns for data collection, service lifecycle management, and data submission. The changes to the configuration and daemon startup are appropriate.

My review includes several suggestions to improve maintainability, robustness, and consistency with the existing codebase. The key points are:

  • Standardizing on a single logging library (slog).
  • Improving type safety by using specific structs instead of map[string]interface{} for known data structures.
  • Increasing robustness in how system information like the username is retrieved.
  • Enhancing flexibility by making the collection interval configurable.
  • Refactoring for better readability and to reduce code duplication.

Overall, this is a solid addition. Addressing these points will make the new service even more robust and easier to maintain.

Comment thread model/ccusage_service.go
"os/user"
"time"

"github.com/sirupsen/logrus"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The project appears to be standardizing on slog for logging, as it's configured in cmd/daemon/main.go. This file introduces logrus, creating an inconsistency. To maintain a single logging framework for easier configuration and maintenance, please migrate the logging in this file from logrus to slog.

This will involve:

  1. Replacing the logrus import with log/slog.
  2. Updating log calls. For example:
    • logrus.Info("message") becomes slog.Info("message")
    • logrus.Warnf("message: %v", err) becomes slog.Warn("message", slog.Any("error", err))
Suggested change
"github.com/sirupsen/logrus"
"log/slog"

Comment thread cmd/daemon/main.go
go daemon.SocketTopicProccessor(msg)

// Start CCUsage service if enabled
if cfg.CCUsage != nil && cfg.CCUsage.Enabled != nil && *cfg.CCUsage.Enabled {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This condition to check if CCUsage is enabled is verbose and is duplicated in model/ccusage_service.go. To improve readability and avoid code duplication (Don't Repeat Yourself - DRY), add a helper method to the CCUsage struct.

In model/types.go:

func (c *CCUsage) IsEnabled() bool {
	return c != nil && c.Enabled != nil && *c.Enabled
}

Then you can simplify this line and the check in ccusage_service.go.

Suggested change
if cfg.CCUsage != nil && cfg.CCUsage.Enabled != nil && *cfg.CCUsage.Enabled {
if cfg.CCUsage.IsEnabled() {

Comment thread model/ccusage_service.go Outdated
Username string `json:"username" msgpack:"username"`
OS string `json:"os" msgpack:"os"`
OSVersion string `json:"osVersion" msgpack:"osVersion"`
Data map[string]interface{} `json:"data" msgpack:"data"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using map[string]interface{} for the Data field bypasses type safety. Define a specific struct for the ccusage command's output to make the code more robust and self-documenting.

For example, you could define a struct like this and use it here:

// CCUsageStats defines the structure of the data from the ccusage command
type CCUsageStats struct {
    Completions int `json:"completions"`
    Users       int `json:"users"`
    // ... other fields
}

This would improve type safety and make the code easier to maintain.

Suggested change
Data map[string]interface{} `json:"data" msgpack:"data"`
Data CCUsageStats `json:"data" msgpack:"data"`

Comment thread model/ccusage_service.go
logrus.Info("Starting CCUsage collection service")

// Create a ticker for hourly collection
s.ticker = time.NewTicker(1 * time.Hour)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The collection interval is hardcoded to 1 hour. This reduces flexibility. Consider making this value configurable in config.toml under the [ccusage] section. This would allow operators to adjust the collection frequency without changing the code.

You would need to add a field to the CCUsage struct in model/types.go, for example:

// In model/types.go
type CCUsage struct {
    Enabled *bool `toml:"enabled"`
    CollectionInterval string `toml:"collectionInterval"` // e.g. "1h", "30m"
}
interval := 1 * time.Hour
if s.config.CCUsage != nil && s.config.CCUsage.CollectionInterval != "" {
	if parsedInterval, err := time.ParseDuration(s.config.CCUsage.CollectionInterval); err == nil {
		interval = parsedInterval
	} else {
		logrus.Warnf("Invalid CCUsage.CollectionInterval '%s', using default 1h: %v", s.config.CCUsage.CollectionInterval, err)
	}
}
s.ticker = time.NewTicker(interval)

Comment thread model/ccusage_service.go
Comment on lines +168 to +177
username := os.Getenv("USER")
if username == "" {
currentUser, err := user.Current()
if err != nil {
logrus.Warnf("Failed to get username: %v", err)
username = "unknown"
} else {
username = currentUser.Username
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Getting the username from the USER environment variable is not always reliable. The user.Current() function is a more robust, cross-platform method and should be the primary method for retrieving the username.

var username string
currentUser, err := user.Current()
if err != nil {
	logrus.Warnf("Failed to get current user: %v, falling back to USER env var", err)
	username = os.Getenv("USER")
} else {
	username = currentUser.Username
}

if username == "" {
	logrus.Warnf("Failed to get username")
	username = "unknown"
}

- Add CCUsageProjectDailyOutput struct for strongly typed JSON parsing
- Replace generic map with specific type for better type safety
- Auto-generated from actual ccusage command output structure
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Sep 26, 2025

Claude finished @AnnatarHe's task —— View job


Code Review for CCUsage Collection Service

I've completed a thorough review of the CCUsage collection service implementation. Here's my comprehensive feedback:

Todo List

  • Read and analyze changed files
  • Review code quality and best practices
  • Identify potential bugs or issues
  • Check performance considerations
  • Evaluate security concerns
  • Assess test coverage
  • Provide comprehensive feedback

Positive Aspects

  1. Clean Architecture: Well-structured service with clear separation of concerns using interfaces
  2. Graceful Fallback: Smart logic to try bunx first, then fallback to npx
  3. Proper Lifecycle Management: Good context handling and graceful shutdown mechanisms
  4. Privacy-Conscious: Only runs when explicitly enabled (default disabled)
  5. Error Handling: Comprehensive error handling with informative messages
  6. System Integration: Follows existing patterns in the codebase (uses existing HTTP client, config system)

⚠️ Issues & Recommendations

1. Critical: Missing Input Validation & Security

Location: model/ccusage_service.go:155-159

var ccusageOutput CCUsageProjectDailyOutput
if err := json.Unmarshal(output, &ccusageOutput); err != nil {
    return nil, fmt.Errorf("failed to parse ccusage output: %w", err)
}

Issue: Direct unmarshaling of external command output without validation could lead to memory exhaustion or unexpected data structures.

Recommendation: Add size limits and basic validation:

if len(output) > 10*1024*1024 { // 10MB limit
    return nil, fmt.Errorf("ccusage output too large: %d bytes", len(output))
}

2. Performance: Resource Leaks

Location: model/ccusage_service.go:88-93

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    close(s.stopChan)
}

Issue: Potential race condition - multiple calls to Stop() will panic due to closing an already closed channel.

Recommendation: Add protection:

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    select {
    case <-s.stopChan:
        // Already closed
    default:
        close(s.stopChan)
    }
}

3. Security: Command Injection Risk

Location: model/ccusage_service.go:138-144
Issue: While using exec.CommandContext is secure, the hardcoded arguments should be explicitly documented as safe.

Recommendation: Add a comment explaining the security model:

// Safe: arguments are hardcoded constants, no user input
cmd = exec.CommandContext(ctx, bunxPath, "ccusage", "daily", "--instances", "--json")

4. Error Handling: Insufficient Context

Location: model/ccusage_service.go:109-119
Issue: Network failures are logged but don't include enough context for debugging.

Recommendation: Add structured logging:

if err != nil {
    logrus.WithFields(logrus.Fields{
        "endpoint": s.config.APIEndpoint,
        "error": err,
    }).Warn("Failed to send usage data")
    return fmt.Errorf("failed to send usage data: %w", err)
}

5. Configuration: Potential NPE

Location: cmd/daemon/main.go:99-107

if cfg.CCUsage != nil && cfg.CCUsage.Enabled != nil && *cfg.CCUsage.Enabled {

Issue: Triple pointer dereferencing is error-prone and verbose.

Recommendation: Add a helper method in model/types.go:

func (c *CCUsage) IsEnabled() bool {
    return c != nil && c.Enabled != nil && *c.Enabled
}

6. Missing Test Coverage

Critical Issue: No tests found for the new CCUsage functionality.

Required Tests:

  • Unit tests for CCUsageService interface methods
  • Mock tests for external command execution
  • Integration tests for the service lifecycle
  • Error condition testing (missing binaries, network failures)

7. Documentation Gaps

Missing:

  • Godoc comments for public types and methods
  • Error condition documentation
  • Rate limiting considerations for the hourly collection

🔒 Security Assessment

Overall: ✅ Good - No major security vulnerabilities detected

  • ✅ External commands use safe exec.CommandContext
  • ✅ No user input in command arguments
  • ✅ Uses existing secure HTTP client
  • ✅ Opt-in functionality (disabled by default)
  • ⚠️ Minor: Should validate external command output size

🚀 Performance Considerations

  • ✅ Hourly collection is reasonable frequency
  • ✅ Uses context for cancellation
  • ⚠️ No timeout on external command execution
  • ⚠️ Large JSON responses could consume significant memory

Recommendation: Add command timeout:

ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()
cmd = exec.CommandContext(ctx, bunxPath, ...)

📋 Test Coverage Priority

  1. High: Service lifecycle (Start/Stop)
  2. High: Command execution with mocked binaries
  3. Medium: Network failure handling
  4. Medium: Configuration validation
  5. Low: System info collection

🎯 Final Recommendation

Approve with minor fixes. The implementation is solid and follows good practices. Address the resource leak issue and add basic tests before merging.

Priority Fixes:

  1. Fix Stop() method race condition
  2. Add command timeout
  3. Add basic unit tests
  4. Consider input validation for large outputs

The feature is well-implemented and ready for production with these minor improvements.


- Add SendGraphQLRequest helper function for consistent GraphQL operations
- Implement getLastSyncTimestamp to fetch last sync time from server
- Update collectData to support since parameter for incremental data collection
- Refactor dotfile.go to use new SendGraphQLRequest helper
- Reduce duplicate code and improve maintainability
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Sep 26, 2025

Claude finished @AnnatarHe's task —— View job


Claude Code Review: CCUsage Collection Service

Todo List

  • Read and analyze all changed files
  • Review code quality and best practices
  • Identify potential bugs or issues
  • Check performance considerations
  • Evaluate security concerns
  • Assess test coverage
  • Provide comprehensive feedback

Positive Aspects

  1. Clean Architecture: Well-structured service with clear separation of concerns using interfaces
  2. Graceful Fallback: Smart logic to try bunx first, then fallback to npx
  3. Proper Lifecycle Management: Good context handling and graceful shutdown mechanisms
  4. Privacy-Conscious: Only runs when explicitly enabled (default disabled)
  5. Incremental Sync: Intelligent since parameter support for efficient data collection
  6. System Integration: Follows existing patterns in the codebase (uses existing HTTP client, config system)

⚠️ Critical Issues

1. Resource Leak - Race Condition

Location: model/ccusage_service.go:88-93

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    close(s.stopChan) // ⚠️ Panic if called multiple times
}

Issue: Multiple calls to Stop() will panic due to closing an already closed channel.
Fix: Add protection:

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    select {
    case <-s.stopChan:
        // Already closed
    default:
        close(s.stopChan)
    }
}

2. Missing Command Timeout

Location: model/ccusage_service.go:214-220
Issue: External command execution has no timeout, could hang indefinitely.
Fix: Add timeout:

ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()
cmd = exec.CommandContext(ctx, bunxPath, args...)

3. Logging Inconsistency

Location: model/ccusage_service.go:13
Issue: Uses logrus while the rest of the project uses slog (as seen in cmd/daemon/main.go).
Fix: Migrate to slog for consistency.

⚠️ Medium Priority Issues

4. Configuration Helper Missing

Location: cmd/daemon/main.go:99

if cfg.CCUsage != nil && cfg.CCUsage.Enabled != nil && *cfg.CCUsage.Enabled {

Issue: Verbose triple pointer dereferencing repeated in code.
Fix: Add helper method to model/types.go:

func (c *CCUsage) IsEnabled() bool {
    return c != nil && c.Enabled != nil && *c.Enabled
}

5. Input Validation Missing

Location: model/ccusage_service.go:232-235
Issue: No size limits on external command output.
Fix: Add validation:

if len(output) > 10*1024*1024 { // 10MB limit
    return nil, fmt.Errorf("ccusage output too large: %d bytes", len(output))
}

6. Username Retrieval Logic

Location: model/ccusage_service.go:244-253
Issue: Should prioritize user.Current() over environment variable for reliability.
Fix: Reverse the order - try user.Current() first.

🔒 Security Assessment

Overall: ✅ Good - No major security vulnerabilities

  • ✅ Uses safe exec.CommandContext with hardcoded arguments
  • ✅ No user input in command execution
  • ✅ Uses existing secure HTTP client infrastructure
  • ✅ Opt-in functionality (disabled by default)
  • ⚠️ Minor: Should validate external command output size

🚀 Performance Considerations

  • ✅ Hourly collection frequency is appropriate
  • ✅ Uses context for proper cancellation
  • ✅ Incremental sync reduces data transfer
  • ⚠️ No timeout on external command execution
  • ⚠️ Large JSON responses could consume significant memory

📋 Test Coverage

Critical Gap: 0% test coverage for new functionality (164 lines missing)

Required Tests:

  1. High Priority: Service lifecycle (Start/Stop)
  2. High Priority: Command execution with mocked binaries
  3. Medium Priority: Network failure handling
  4. Medium Priority: Configuration validation
  5. Low Priority: System info collection

🏗️ Architecture & Design

Strengths:

  • Good separation of concerns with interfaces
  • Follows existing codebase patterns
  • Proper error handling throughout
  • Smart fallback mechanism between bunx and npx

Areas for Improvement:

  • Consider making collection interval configurable
  • Add structured logging with context
  • Consider retry logic for network failures

🎯 Final Recommendation

✅ Approve with fixes. The implementation is solid and well-architected. Address the critical resource leak and add basic tests before merging.

Priority Fixes:

  1. Critical: Fix Stop() method race condition
  2. High: Add command timeout (30s)
  3. High: Add basic unit tests
  4. Medium: Standardize on slog logging
  5. Medium: Add configuration helper method

The CCUsage service is a valuable addition that will provide important usage insights while maintaining privacy and security standards. With these minor improvements, it's ready for production use.


- Add generic GraphQLResponse[T] struct for consistent response handling
- Add GraphQLError struct for proper error representation
- Update ccusage and dotfile services to use generic response types
- Add GraphQL error checking in SendGraphQLRequest
- Reduce code duplication and improve type safety
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Sep 26, 2025

Claude finished @AnnatarHe's task —— View job


Claude Code Review: CCUsage Collection Service

Todo List

  • Read and analyze all changed files
  • Review code quality and best practices
  • Identify potential bugs or issues
  • Check performance considerations
  • Evaluate security concerns
  • Assess test coverage
  • Provide comprehensive feedback

Positive Aspects

  1. Clean Architecture: Well-structured service with clear separation of concerns using interfaces
  2. Graceful Fallback: Smart logic to try bunx first, then fallback to npx
  3. Proper Lifecycle Management: Good context handling and graceful shutdown mechanisms
  4. Privacy-Conscious: Only runs when explicitly enabled (default disabled)
  5. Incremental Sync: Intelligent since parameter support for efficient data collection
  6. System Integration: Follows existing patterns in the codebase (uses existing HTTP client, config system)

⚠️ Critical Issues

1. Resource Leak - Race Condition

Location: model/ccusage_service.go:88-93

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    close(s.stopChan) // ⚠️ Panic if called multiple times
}

Issue: Multiple calls to Stop() will panic due to closing an already closed channel.
Fix: Add protection:

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    select {
    case <-s.stopChan:
        // Already closed
    default:
        close(s.stopChan)
    }
}

2. Missing Command Timeout

Location: model/ccusage_service.go:216-221
Issue: External command execution has no timeout, could hang indefinitely.
Fix: Add timeout:

ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()
cmd = exec.CommandContext(ctx, bunxPath, args...)

3. Logging Inconsistency

Location: model/ccusage_service.go:13
Issue: Uses logrus while the rest of the project uses slog (as seen in cmd/daemon/main.go).
Fix: Migrate to slog for consistency.

⚠️ Medium Priority Issues

4. Configuration Helper Missing

Location: cmd/daemon/main.go:99

if cfg.CCUsage != nil && cfg.CCUsage.Enabled != nil && *cfg.CCUsage.Enabled {

Issue: Verbose triple pointer dereferencing repeated in code.
Fix: Add helper method to model/types.go:

func (c *CCUsage) IsEnabled() bool {
    return c != nil && c.Enabled != nil && *c.Enabled
}

5. Input Validation Missing

Location: model/ccusage_service.go:225-228
Issue: No size limits on external command output.
Fix: Add validation:

if len(output) > 10*1024*1024 { // 10MB limit
    return nil, fmt.Errorf("ccusage output too large: %d bytes", len(output))
}

6. Username Retrieval Logic

Location: model/ccusage_service.go:237-246
Issue: Should prioritize user.Current() over environment variable for reliability.
Fix: Reverse the order - try user.Current() first.

🔒 Security Assessment

Overall: ✅ Good - No major security vulnerabilities

  • ✅ Uses safe exec.CommandContext with hardcoded arguments
  • ✅ No user input in command execution
  • ✅ Uses existing secure HTTP client infrastructure
  • ✅ Opt-in functionality (disabled by default)
  • ⚠️ Minor: Should validate external command output size

🚀 Performance Considerations

  • ✅ Hourly collection frequency is appropriate
  • ✅ Uses context for proper cancellation
  • ✅ Incremental sync reduces data transfer
  • ⚠️ No timeout on external command execution
  • ⚠️ Large JSON responses could consume significant memory

📋 Test Coverage

Critical Gap: 0% test coverage for new functionality (299 lines missing)

Required Tests:

  1. High Priority: Service lifecycle (Start/Stop)
  2. High Priority: Command execution with mocked binaries
  3. Medium Priority: Network failure handling
  4. Medium Priority: Configuration validation
  5. Low Priority: System info collection

🏗️ Architecture & Design

Strengths:

  • Good separation of concerns with interfaces
  • Follows existing codebase patterns
  • Proper error handling throughout
  • Smart fallback mechanism between bunx and npx

Areas for Improvement:

  • Consider making collection interval configurable
  • Add structured logging with context
  • Consider retry logic for network failures

🎯 Final Recommendation

✅ Approve with fixes. The implementation is solid and well-architected. Address the critical resource leak and add basic tests before merging.

Priority Fixes:

  1. Critical: Fix Stop() method race condition
  2. High: Add command timeout (30s)
  3. High: Add basic unit tests
  4. Medium: Standardize on slog logging
  5. Medium: Add configuration helper method

The CCUsage service is a valuable addition that will provide important usage insights while maintaining privacy and security standards. With these minor improvements, it's ready for production use.


AnnatarHe and others added 2 commits September 27, 2025 01:35
- Changed API path from /api/v1/ccusage to /api/v1/ccusage/batch
- Transformed data structure to match server's CCUsageBatchPayload format
- Updated response handling to include success count and failed projects
- Added proper batch entry transformation for multiple projects and dates

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Added new SendHTTPRequestJSON function for JSON-based HTTP requests
- Marked SendHTTPRequest as deprecated (uses msgpack) for legacy support
- Refactored SendGraphQLRequest to use SendHTTPRequestJSON internally
- Updated ccusage service to use the new JSON-based function
- Eliminated code duplication in GraphQL request handling

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Sep 26, 2025

Claude finished @AnnatarHe's task —— View job


Claude Code Review: CCUsage Collection Service

Todo List

  • Read and analyze all changed files
  • Review code quality and best practices
  • Identify potential bugs or issues
  • Check performance considerations
  • Evaluate security concerns
  • Assess test coverage
  • Provide comprehensive feedback

Positive Aspects

  1. Clean Architecture: Well-structured service with clear separation of concerns using interfaces
  2. Graceful Fallback: Smart logic to try bunx first, then fallback to npx
  3. Proper Lifecycle Management: Good context handling and graceful shutdown mechanisms
  4. Privacy-Conscious: Only runs when explicitly enabled (default disabled)
  5. Incremental Sync: Intelligent since parameter support for efficient data collection
  6. System Integration: Follows existing patterns in the codebase (uses existing HTTP client, config system)

⚠️ Critical Issues

1. Resource Leak - Race Condition

Location: model/ccusage_service.go:88-93

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    close(s.stopChan) // ⚠️ Panic if called multiple times
}

Issue: Multiple calls to Stop() will panic due to closing an already closed channel.
Fix: Add protection:

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    select {
    case <-s.stopChan:
        // Already closed
    default:
        close(s.stopChan)
    }
}

2. Missing Command Timeout

Location: model/ccusage_service.go:216-221
Issue: External command execution has no timeout, could hang indefinitely.
Fix: Add timeout:

ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()
cmd = exec.CommandContext(ctx, bunxPath, args...)

3. Logging Inconsistency

Location: model/ccusage_service.go:13
Issue: Uses logrus while the rest of the project uses slog (as seen in cmd/daemon/main.go).
Fix: Migrate to slog for consistency.

⚠️ Medium Priority Issues

4. Configuration Helper Missing

Location: cmd/daemon/main.go:99

if cfg.CCUsage != nil && cfg.CCUsage.Enabled != nil && *cfg.CCUsage.Enabled {

Issue: Verbose triple pointer dereferencing repeated in code.
Fix: Add helper method to model/types.go:

func (c *CCUsage) IsEnabled() bool {
    return c != nil && c.Enabled != nil && *c.Enabled
}

5. Input Validation Missing

Location: model/ccusage_service.go:225-228
Issue: No size limits on external command output.
Fix: Add validation:

if len(output) > 10*1024*1024 { // 10MB limit
    return nil, fmt.Errorf("ccusage output too large: %d bytes", len(output))
}

6. Username Retrieval Logic

Location: model/ccusage_service.go:237-246
Issue: Should prioritize user.Current() over environment variable for reliability.
Fix: Reverse the order - try user.Current() first.

🔒 Security Assessment

Overall: ✅ Good - No major security vulnerabilities

  • ✅ Uses safe exec.CommandContext with hardcoded arguments
  • ✅ No user input in command execution
  • ✅ Uses existing secure HTTP client infrastructure
  • ✅ Opt-in functionality (disabled by default)
  • ⚠️ Minor: Should validate external command output size

🚀 Performance Considerations

  • ✅ Hourly collection frequency is appropriate
  • ✅ Uses context for proper cancellation
  • ✅ Incremental sync reduces data transfer
  • ⚠️ No timeout on external command execution
  • ⚠️ Large JSON responses could consume significant memory

📋 Test Coverage

Critical Gap: 0% test coverage for new functionality (299 lines missing)

Required Tests:

  1. High Priority: Service lifecycle (Start/Stop)
  2. High Priority: Command execution with mocked binaries
  3. Medium Priority: Network failure handling
  4. Medium Priority: Configuration validation
  5. Low Priority: System info collection

🏗️ Architecture & Design

Strengths:

  • Good separation of concerns with interfaces
  • Follows existing codebase patterns
  • Proper error handling throughout
  • Smart fallback mechanism between bunx and npx

Areas for Improvement:

  • Consider making collection interval configurable
  • Add structured logging with context
  • Consider retry logic for network failures

🎯 Final Recommendation

✅ Approve with fixes. The implementation is solid and well-architected. Address the critical resource leak and add basic tests before merging.

Priority Fixes:

  1. Critical: Fix Stop() method race condition
  2. High: Add command timeout (30s)
  3. High: Add basic unit tests
  4. Medium: Standardize on slog logging
  5. Medium: Add configuration helper method

The CCUsage service is a valuable addition that will provide important usage insights while maintaining privacy and security standards. With these minor improvements, it's ready for production use.


…y hostname

- Fix GraphQL query structure to match server schema (User.ccusage.lastSyncAt)
- Add hostname parameter to getLastSyncTimestamp to filter CCUsage data by current host
- Ensure incremental sync is host-specific to prevent cross-host data conflicts

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Sep 27, 2025

Claude finished @AnnatarHe's task —— View job


Code Review for CCUsage Collection Service

Todo List

  • Read and analyze all changed files
  • Review code quality and best practices
  • Identify potential bugs or issues
  • Check performance considerations
  • Evaluate security concerns
  • Assess test coverage
  • Provide comprehensive feedback

Positive Aspects

  1. Clean Architecture: Well-structured service with clear separation of concerns using interfaces
  2. Graceful Fallback: Smart logic to try bunx first, then fallback to npx
  3. Proper Lifecycle Management: Good context handling and graceful shutdown mechanisms
  4. Privacy-Conscious: Only runs when explicitly enabled (default disabled)
  5. Incremental Sync: Intelligent since parameter support for efficient data collection
  6. System Integration: Follows existing patterns in the codebase (uses existing HTTP client, config system)

⚠️ Critical Issues

1. Resource Leak - Race Condition

Location: model/ccusage_service.go:88-93

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    close(s.stopChan) // ⚠️ Panic if called multiple times
}

Issue: Multiple calls to Stop() will panic due to closing an already closed channel.
Fix: Add protection:

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    select {
    case <-s.stopChan:
        // Already closed
    default:
        close(s.stopChan)
    }
}

2. Missing Command Timeout

Location: model/ccusage_service.go:225-230
Issue: External command execution has no timeout, could hang indefinitely.
Fix: Add timeout:

ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()
cmd = exec.CommandContext(ctx, bunxPath, args...)

3. Logging Inconsistency

Location: model/ccusage_service.go:13
Issue: Uses logrus while the rest of the project uses slog (as seen in cmd/daemon/main.go).
Fix: Migrate to slog for consistency.

⚠️ Medium Priority Issues

4. Configuration Helper Missing

Location: cmd/daemon/main.go:99

if cfg.CCUsage != nil && cfg.CCUsage.Enabled != nil && *cfg.CCUsage.Enabled {

Issue: Verbose triple pointer dereferencing repeated in code.
Fix: Add helper method to model/types.go:

func (c *CCUsage) IsEnabled() bool {
    return c != nil && c.Enabled != nil && *c.Enabled
}

5. Input Validation Missing

Location: model/ccusage_service.go:243-246
Issue: No size limits on external command output.
Fix: Add validation:

if len(output) > 10*1024*1024 { // 10MB limit
    return nil, fmt.Errorf("ccusage output too large: %d bytes", len(output))
}

6. Username Retrieval Logic

Location: model/ccusage_service.go:255-264
Issue: Should prioritize user.Current() over environment variable for reliability.
Fix: Reverse the order - try user.Current() first.

🔒 Security Assessment

Overall: ✅ Good - No major security vulnerabilities

  • ✅ Uses safe exec.CommandContext with hardcoded arguments
  • ✅ No user input in command execution
  • ✅ Uses existing secure HTTP client infrastructure
  • ✅ Opt-in functionality (disabled by default)
  • ⚠️ Minor: Should validate external command output size

🚀 Performance Considerations

  • ✅ Hourly collection frequency is appropriate
  • ✅ Uses context for proper cancellation
  • ✅ Incremental sync reduces data transfer
  • ⚠️ No timeout on external command execution
  • ⚠️ Large JSON responses could consume significant memory

📋 Test Coverage

Critical Gap: 0% test coverage for new functionality (299 lines missing)

Required Tests:

  1. High Priority: Service lifecycle (Start/Stop)
  2. High Priority: Command execution with mocked binaries
  3. Medium Priority: Network failure handling
  4. Medium Priority: Configuration validation
  5. Low Priority: System info collection

🏗️ Architecture & Design

Strengths:

  • Good separation of concerns with interfaces
  • Follows existing codebase patterns
  • Proper error handling throughout
  • Smart fallback mechanism between bunx and npx

Areas for Improvement:

  • Consider making collection interval configurable
  • Add structured logging with context
  • Consider retry logic for network failures

🎯 Final Recommendation

✅ Approve with fixes. The implementation is solid and well-architected. Address the critical resource leak and add basic tests before merging.

Priority Fixes:

  1. Critical: Fix Stop() method race condition
  2. High: Add command timeout (30s)
  3. High: Add basic unit tests
  4. Medium: Standardize on slog logging
  5. Medium: Add configuration helper method

The CCUsage service is a valuable addition that will provide important usage insights while maintaining privacy and security standards. With these minor improvements, it's ready for production use.


- Changed lastSyncAt from int64 to string in GraphQL response
- Parse RFC3339 timestamp format from server
- Filter out timestamps before 2023 as invalid
- Use time.Time instead of *int64 for better type safety

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Sep 27, 2025

Claude finished @AnnatarHe's task —— View job


Code Review for CCUsage Collection Service

Todo List

  • Read and analyze all changed files
  • Review code quality and best practices
  • Identify potential bugs or issues
  • Check performance considerations
  • Evaluate security concerns
  • Assess test coverage
  • Provide comprehensive feedback

Positive Aspects

  1. Clean Architecture: Well-structured service with clear separation of concerns using interfaces
  2. Graceful Fallback: Smart logic to try bunx first, then fallback to npx
  3. Proper Lifecycle Management: Good context handling and graceful shutdown mechanisms
  4. Privacy-Conscious: Only runs when explicitly enabled (default disabled)
  5. Incremental Sync: Intelligent since parameter support for efficient data collection
  6. System Integration: Follows existing patterns in the codebase (uses existing HTTP client, config system)

⚠️ Critical Issues

1. Resource Leak - Race Condition

Location: model/ccusage_service.go:88-93

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    close(s.stopChan) // ⚠️ Panic if called multiple times
}

Issue: Multiple calls to Stop() will panic due to closing an already closed channel.
Fix: Add protection:

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    select {
    case <-s.stopChan:
        // Already closed
    default:
        close(s.stopChan)
    }
}

2. Missing Command Timeout

Location: model/ccusage_service.go:225-230
Issue: External command execution has no timeout, could hang indefinitely.
Fix: Add timeout:

ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()
cmd = exec.CommandContext(ctx, bunxPath, args...)

3. Logging Inconsistency

Location: model/ccusage_service.go:13
Issue: Uses logrus while the rest of the project uses slog (as seen in cmd/daemon/main.go).
Fix: Migrate to slog for consistency.

⚠️ Medium Priority Issues

4. Configuration Helper Missing

Location: cmd/daemon/main.go:99

if cfg.CCUsage != nil && cfg.CCUsage.Enabled != nil && *cfg.CCUsage.Enabled {

Issue: Verbose triple pointer dereferencing repeated in code.
Fix: Add helper method to model/types.go:

func (c *CCUsage) IsEnabled() bool {
    return c != nil && c.Enabled != nil && *c.Enabled
}

5. Input Validation Missing

Location: model/ccusage_service.go:243-246
Issue: No size limits on external command output.
Fix: Add validation:

if len(output) > 10*1024*1024 { // 10MB limit
    return nil, fmt.Errorf("ccusage output too large: %d bytes", len(output))
}

6. Username Retrieval Logic

Location: model/ccusage_service.go:255-264
Issue: Should prioritize user.Current() over environment variable for reliability.
Fix: Reverse the order - try user.Current() first.

🔒 Security Assessment

Overall: ✅ Good - No major security vulnerabilities

  • ✅ Uses safe exec.CommandContext with hardcoded arguments
  • ✅ No user input in command execution
  • ✅ Uses existing secure HTTP client infrastructure
  • ✅ Opt-in functionality (disabled by default)
  • ⚠️ Minor: Should validate external command output size

🚀 Performance Considerations

  • ✅ Hourly collection frequency is appropriate
  • ✅ Uses context for proper cancellation
  • ✅ Incremental sync reduces data transfer
  • ⚠️ No timeout on external command execution
  • ⚠️ Large JSON responses could consume significant memory

📋 Test Coverage

Critical Gap: 0% test coverage for new functionality (299 lines missing)

Required Tests:

  1. High Priority: Service lifecycle (Start/Stop)
  2. High Priority: Command execution with mocked binaries
  3. Medium Priority: Network failure handling
  4. Medium Priority: Configuration validation
  5. Low Priority: System info collection

🏗️ Architecture & Design

Strengths:

  • Good separation of concerns with interfaces
  • Follows existing codebase patterns
  • Proper error handling throughout
  • Smart fallback mechanism between bunx and npx

Areas for Improvement:

  • Consider making collection interval configurable
  • Add structured logging with context
  • Consider retry logic for network failures

🎯 Final Recommendation

✅ Approve with fixes. The implementation is solid and well-architected. Address the critical resource leak and add basic tests before merging.

Priority Fixes:

  1. Critical: Fix Stop() method race condition
  2. High: Add command timeout (30s)
  3. High: Add basic unit tests
  4. Medium: Standardize on slog logging
  5. Medium: Add configuration helper method

The CCUsage service is a valuable addition that will provide important usage insights while maintaining privacy and security standards. With these minor improvements, it's ready for production use.


Change timestamp logging from %d to %v for better readability

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Sep 27, 2025

Claude finished @AnnatarHe's task —— View job


Code Review for CCUsage Collection Service

Todo List

  • Read and analyze all changed files
  • Review code quality and best practices
  • Identify potential bugs or issues
  • Check performance considerations
  • Evaluate security concerns
  • Assess test coverage
  • Provide comprehensive feedback

Positive Aspects

  1. Clean Architecture: Well-structured service with clear separation of concerns using interfaces
  2. Graceful Fallback: Smart logic to try bunx first, then fallback to npx
  3. Proper Lifecycle Management: Good context handling and graceful shutdown mechanisms
  4. Privacy-Conscious: Only runs when explicitly enabled (default disabled)
  5. Incremental Sync: Intelligent since parameter support for efficient data collection
  6. System Integration: Follows existing patterns in the codebase (uses existing HTTP client, config system)

⚠️ Critical Issues

1. Resource Leak - Race Condition

Location: model/ccusage_service.go:88-93

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    close(s.stopChan) // ⚠️ Panic if called multiple times
}

Issue: Multiple calls to Stop() will panic due to closing an already closed channel.
Fix: Add protection:

func (s *ccUsageService) Stop() {
    if s.ticker != nil {
        s.ticker.Stop()
    }
    select {
    case <-s.stopChan:
        // Already closed
    default:
        close(s.stopChan)
    }
}

2. Missing Command Timeout

Location: model/ccusage_service.go:235-240
Issue: External command execution has no timeout, could hang indefinitely.
Fix: Add timeout:

ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()
cmd = exec.CommandContext(ctx, bunxPath, args...)

3. Logging Inconsistency

Location: model/ccusage_service.go:13
Issue: Uses logrus while the daemon uses slog (as seen in cmd/daemon/main.go). The project has 27 files using logrus vs only 5 using slog, but the daemon specifically uses slog.
Fix: For consistency with the daemon that manages this service, consider migrating to slog.

⚠️ Medium Priority Issues

4. Configuration Helper Missing

Location: cmd/daemon/main.go:99

if cfg.CCUsage != nil && cfg.CCUsage.Enabled != nil && *cfg.CCUsage.Enabled {

Issue: Verbose triple pointer dereferencing repeated in code.
Fix: Add helper method to model/types.go:

func (c *CCUsage) IsEnabled() bool {
    return c != nil && c.Enabled != nil && *c.Enabled
}

5. Input Validation Missing

Location: model/ccusage_service.go:253-256
Issue: No size limits on external command output.
Fix: Add validation:

if len(output) > 10*1024*1024 { // 10MB limit
    return nil, fmt.Errorf("ccusage output too large: %d bytes", len(output))
}

6. Username Retrieval Logic

Location: model/ccusage_service.go:265-274
Issue: Should prioritize user.Current() over environment variable for reliability.
Fix: Reverse the order - try user.Current() first.

🔒 Security Assessment

Overall: ✅ Good - No major security vulnerabilities

  • ✅ Uses safe exec.CommandContext with hardcoded arguments
  • ✅ No user input in command execution
  • ✅ Uses existing secure HTTP client infrastructure
  • ✅ Opt-in functionality (disabled by default)
  • ⚠️ Minor: Should validate external command output size

🚀 Performance Considerations

  • ✅ Hourly collection frequency is appropriate
  • ✅ Uses context for proper cancellation
  • ✅ Incremental sync reduces data transfer
  • ⚠️ No timeout on external command execution
  • ⚠️ Large JSON responses could consume significant memory

📋 Test Coverage

Critical Gap: 0% test coverage for new functionality (299 lines missing)

Required Tests:

  1. High Priority: Service lifecycle (Start/Stop)
  2. High Priority: Command execution with mocked binaries
  3. Medium Priority: Network failure handling
  4. Medium Priority: Configuration validation
  5. Low Priority: System info collection

🏗️ Architecture & Design

Strengths:

  • Good separation of concerns with interfaces
  • Follows existing codebase patterns
  • Proper error handling throughout
  • Smart fallback mechanism between bunx and npx

Areas for Improvement:

  • Consider making collection interval configurable
  • Add structured logging with context
  • Consider retry logic for network failures

🎯 Final Recommendation

✅ Approve with fixes. The implementation is solid and well-architected. Address the critical resource leak and add basic tests before merging.

Priority Fixes:

  1. Critical: Fix Stop() method race condition
  2. High: Add command timeout (30s)
  3. High: Add basic unit tests
  4. Medium: Standardize on slog logging
  5. Medium: Add configuration helper method

The CCUsage service is a valuable addition that will provide important usage insights while maintaining privacy and security standards. With these minor improvements, it's ready for production use.


@AnnatarHe AnnatarHe merged commit d930d8e into main Sep 27, 2025
2 of 3 checks passed
@AnnatarHe AnnatarHe deleted the feat/ccusage-collection-service branch September 27, 2025 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant