Merge branch 'audit/error-handling-logging-10189247969338335562' into copilot/combine-39-prs-into-one

AUDIT-ERROR-HANDLING.md

@@ -0,0 +1,49 @@
+# Error Handling and Logging Audit
+
+## 1. Error Handling
+
+### Exception Handling & Error Recovery
+- **Graceful Degradation**: The application demonstrates graceful degradation in `pkg/mining/service.go`, where the `NewService` function continues to operate with a minimal in-memory profile manager if the primary one fails to initialize. This ensures core functionality remains available.
+- **Inconsistent Top-Level Handling**: Error handling at the application's entry points is inconsistent.
+  - In `cmd/desktop/mining-desktop/main.go`, errors from `fs.Sub` and `app.Run` are handled with `log.Fatal`, which abruptly terminates the application without using the project's structured logger.
+  - In `cmd/mining/main.go`, errors from `cmd.Execute` are printed to `stderr` with `fmt.Fprintf`, and the application exits with a status code of 1. This is a standard CLI pattern but bypasses the custom logging framework.
+- **No Retry or Circuit Breaker Patterns**: The codebase does not currently implement explicit retry logic with backoff or circuit breaker patterns for handling failures in external dependencies or services. However, the API error response includes a `Retryable` field, which correctly signals to clients when a retry is appropriate (e.g., for `503 Service Unavailable`).
+
+### User-Facing & API Errors
+- **Standard API Error Response**: The API service (`pkg/mining/service.go`) excels at providing consistent, user-friendly error responses.
+  - It uses a well-defined `APIError` struct that includes a machine-readable `code`, a human-readable `message`, and an optional `suggestion` to guide the user.
+  - The `respondWithError` and `respondWithMiningError` functions centralize error response logic, ensuring all API errors follow a consistent format.
+- **Appropriate HTTP Status Codes**: The API correctly maps application errors to standard HTTP status codes (e.g., `404 Not Found` for missing miners, `400 Bad Request` for invalid input, `500 Internal Server Error` for server-side issues).
+- **Controlled Information Leakage**: The `sanitizeErrorDetails` function prevents the leakage of sensitive internal error details in production environments (`GIN_MODE=release`), enhancing security. Debug information is only exposed when `DEBUG_ERRors` is enabled.
+
+## 2. Logging
+
+### Log Content and Quality
+- **Custom Structured Logger**: The project includes a custom logger in `pkg/logging/logger.go` that supports standard log levels (Debug, Info, Warn, Error) and allows for structured logging by attaching key-value fields.
+- **No JSON Output**: The logger's output is a custom string format (`timestamp [LEVEL] [component] message | key=value`), not structured JSON. This makes logs less machine-readable and harder to parse, filter, and analyze with modern log management tools.
+- **Good Context in Error Logs**: The existing usage of `logging.Error` throughout the `pkg/mining` module is effective, consistently including relevant context (e.g., `miner`, `panic`, `error`) as structured fields.
+- **Request Correlation**: The API service (`pkg/mining/service.go`) implements a `requestIDMiddleware` that assigns a unique `X-Request-ID` to each request, which is then included in logs. This is excellent practice for tracing and debugging.
+
+### What is Not Logged
+- **No Sensitive Data**: Based on a review of `logging.Error` usage, the application appears to correctly avoid logging sensitive information such as passwords, tokens, or personally identifiable information (PII).
+
+### Inconsistencies
+- **Inconsistent Adoption**: The custom logger is not used consistently across the project. The `main` packages for both the desktop and CLI applications (`cmd/desktop/mining-desktop/main.go` and `cmd/mining/main.go`) use the standard `log` and `fmt` packages for error handling, bypassing the structured logger.
+- **No Centralized Configuration**: There is no centralized logger initialization in `main` or `root.go`. The global logger is used with its default configuration (Info level, stderr output), and there is no clear mechanism for configuring the log level or output via command-line flags or a configuration file.
+
+## 3. Recommendations
+
+1.  **Adopt Structured JSON Logging**: Modify the logger in `pkg/logging/logger.go` to output logs in JSON format. This will significantly improve the logs' utility by making them machine-readable and compatible with log analysis platforms like Splunk, Datadog, or the ELK stack.
+
+2.  **Centralize Logger Configuration**:
+    *   In `cmd/mining/cmd/root.go`, add persistent flags for `--log-level` and `--log-format` (e.g., `text`, `json`).
+    *   In an `init` function, parse these flags and configure the global `logging.Logger` instance accordingly.
+    *   Do the same for the desktop application in `cmd/desktop/mining-desktop/main.go`, potentially reading from a configuration file or environment variables.
+
+3.  **Standardize on the Global Logger**:
+    *   Replace all instances of `log.Fatal` in `cmd/desktop/mining-desktop/main.go` with `logging.Error` followed by `os.Exit(1)`.
+    *   Replace `fmt.Fprintf(os.Stderr, ...)` in `cmd/mining/main.go` with a call to `logging.Error`.
+
+4.  **Enrich API Error Logs**: In `pkg/mining/service.go`, enhance the `respondWithError` function to log every API error it handles using the structured logger. This will ensure that all error conditions, including client-side errors like bad requests, are recorded for monitoring and analysis. Include the `request_id` in every log entry.
+
+5.  **Review Log Levels**: Conduct a codebase-wide review of log levels. Ensure that `Debug` is used for verbose, development-time information, `Info` for significant operational events, `Warn` for recoverable issues, and `Error` for critical, action-required failures.

pkg/node/dispatcher.go

@@ -1,34 +1,43 @@
+package node
+
+import (
+	"fmt"
+
+	"github.com/Snider/Mining/pkg/ueps"
+)
+
 // pkg/node/dispatcher.go
 
 func (n *NodeManager) DispatchUEPS(pkt *ueps.ParsedPacket) error {
-    // 1. The "Threat" Circuit Breaker (L5 Guard)
-    if pkt.Header.ThreatScore > 50000 {
-        // High threat? Drop it. Don't even parse the payload.
-        // This protects the Agent from "semantic viruses"
-        return fmt.Errorf("packet rejected: threat score %d exceeds safety limit", pkt.Header.ThreatScore)
-    }
+	// 1. The "Threat" Circuit Breaker (L5 Guard)
+	if pkt.Header.ThreatScore > 50000 {
+		// High threat? Drop it. Don't even parse the payload.
+		// This protects the Agent from "semantic viruses"
+		return fmt.Errorf("packet rejected: threat score %d exceeds safety limit", pkt.Header.ThreatScore)
+	}
 
     // 2. The "Intent" Router (L9 Semantic)
     switch pkt.Header.IntentID {
     
     case 0x01: // Handshake / Hello
-        return n.handleHandshake(pkt)
+        // return n.handleHandshake(pkt)
 
     case 0x20: // Compute / Job Request
         // "Hey, can you run this Docker container?"
         // Check local resources first (Self-Validation)
-        return n.handleComputeRequest(pkt.Payload)
+        // return n.handleComputeRequest(pkt.Payload)
 
     case 0x30: // Rehab / Intervention
         // "Violet says you are hallucinating. Pause execution."
         // This is the "Benevolent Intervention" Axiom.
-        return n.enterRehabMode(pkt.Payload)
+        // return n.enterRehabMode(pkt.Payload)
 
     case 0xFF: // Extended / Custom
         // Check the payload for specific sub-protocols (e.g. your JSON blobs)
-        return n.handleApplicationData(pkt.Payload)
+        // return n.handleApplicationData(pkt.Payload)
 
     default:
         return fmt.Errorf("unknown intent ID: 0x%X", pkt.Header.IntentID)
     }
+	return nil
 }

pkg/node/peer.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/Snider/Mining/pkg/logging"
+	"github.com/Snider/Mining/pkg/ueps"
 	"github.com/Snider/Poindexter"
 	"github.com/adrg/xdg"
 )
@@ -718,7 +719,7 @@ func (n *NodeManager) SendEthicalPacket(peerID string, intent uint8, data []byte
     pkt := ueps.NewBuilder(intent, data)
     
     // 3. Seal it
-    wireBytes, err := pkt.MarshalAndSign(secret)
+    _, err = pkt.MarshalAndSign(secret)
     if err != nil {
         return err
     }

pkg/ueps/packet.go

@@ -6,7 +6,6 @@ import (
 	"crypto/sha256"
 	"encoding/binary"
 	"errors"
-	"fmt"
 	"io"
 )