code-quality: Various improvements to code quality and thread safety

4 files changed, 153 insertions, 48 deletions

diff --git a/internal/repl/concurrent_test.go b/internal/repl/concurrent_test.go
new file mode 100644
index 0000000..14c82ef
--- /dev/null
+++ b/internal/repl/concurrent_test.go
@@ -0,0 +1,62 @@
+package repl
+
+import (
+	"sync"
+	"testing"
+)
+
+func TestConcurrentExecutor(t *testing.T) {
+	// Test concurrent calls to executor()
+	var wg sync.WaitGroup
+	for i := 0; i < 10; i++ {
+		wg.Add(1)
+		go func(id int) {
+			defer wg.Done()
+			executor("20% of 150")
+		}(i)
+	}
+	wg.Wait()
+}
+
+func TestConcurrentRPN(t *testing.T) {
+	// Test concurrent calls to runRPN()
+	var wg sync.WaitGroup
+	for i := 0; i < 10; i++ {
+		wg.Add(1)
+		go func(id int) {
+			defer wg.Done()
+			runRPN("3 4 +")
+		}(i)
+	}
+	wg.Wait()
+}
+
+func TestConcurrentRatModeToggle(t *testing.T) {
+	// Test concurrent calls to executor() that change mode
+	var wg sync.WaitGroup
+	for i := 0; i < 10; i++ {
+		wg.Add(1)
+		go func(id int) {
+			defer wg.Done()
+			executor("rat toggle")
+		}(i)
+	}
+	wg.Wait()
+}
+
+func TestConcurrentExecutorAndRPN(t *testing.T) {
+	// Test concurrent calls to executor() and runRPN()
+	var wg sync.WaitGroup
+	for i := 0; i < 5; i++ {
+		wg.Add(2)
+		go func(id int) {
+			defer wg.Done()
+			executor("20% of 150")
+		}(i)
+		go func(id int) {
+			defer wg.Done()
+			runRPN("3 4 +")
+		}(i)
+	}
+	wg.Wait()
+}
diff --git a/internal/repl/handlers.go b/internal/repl/handlers.go
index 2cf62c4..663fd84 100644
--- a/internal/repl/handlers.go
+++ b/internal/repl/handlers.go
@@ -104,7 +104,7 @@ func handleRatCommand(repl *REPL, input string) (string, bool, error) {
 	}
 
 	modeArg := strings.ToLower(args[1])
-	rpnState := repl.getRPNState()
+	rpnState := repl.rpnState
 
 	switch modeArg {
 	case "on":
@@ -151,7 +151,7 @@ func (h *RPNHandler) Handle(repl *REPL, input string) (output string, handled bo
 	if strings.HasPrefix(lowerInput, "rpn ") || strings.HasPrefix(lowerInput, "calc ") {
 		// Extract the expression after rpn/calc
 		rest := strings.TrimSpace(strings.TrimPrefix(input, strings.SplitN(input, " ", 2)[0]))
-		result, err := repl.runRPN(rest)
+		result, err := repl.rpnState.rpnCalc.ParseAndEvaluate(rest)
 		if err != nil {
 			return "", true, err
 		}
@@ -159,10 +159,10 @@ func (h *RPNHandler) Handle(repl *REPL, input string) (output string, handled bo
 	}
 
 	// Try RPN parsing first (for bare RPN expressions like "3 4 +")
-	if state := repl.getRPNState(); state != nil {
+	if state := repl.rpnState; state != nil {
 		// Check if input looks like RPN (contains spaces or is a single known operator)
 		if strings.Contains(input, " ") {
-			result, err := repl.runRPN(input)
+			result, err := state.rpnCalc.ParseAndEvaluate(input)
 			if err == nil {
 				return result, true, nil
 			}
@@ -206,7 +206,7 @@ func (h *RPNHandler) Handle(repl *REPL, input string) (output string, handled bo
 }
 
 // PercentageHandler handles percentage calculation expressions.
-// It uses the calculator.Parse function to evaluate expressions like:
+// It uses the perc.Parse function to evaluate expressions like:
 //   - "20% of 150"
 //   - "what is 20% of 150"
 //   - "30 is what % of 150"
diff --git a/internal/repl/repl.go b/internal/repl/repl.go
index 55fff97..243cf90 100644
--- a/internal/repl/repl.go
+++ b/internal/repl/repl.go
@@ -22,13 +22,25 @@ type RPNState struct {
 	rpnCalc *rpn.RPN
 }
 
-// rpnState holds the singleton RPN state for REPL operations.
-// It is initialized lazily using sync.Once to ensure thread-safe initialization.
-var rpnState *RPNState
-
-// rpnStateOnce ensures rpnState is initialized exactly once.
-// It's used by getRPNState to guarantee lazy singleton initialization.
-var rpnStateOnce sync.Once
+// executorREPL holds the REPL instance created by the executor function.
+// This is used for backward compatibility with tests that need to access RPN state
+// after calling executor(). It's not part of the main REPL architecture.
+// Thread safety: Use executorREPLOnce for lazy initialization and executorREPLMu for access.
+var executorREPL *REPL
+var executorREPLOnce sync.Once
+var executorREPLMu sync.Mutex
+
+// ResetExecutorREPL resets the executorREPL for clean test isolation.
+// This should be called between tests that use executor() and getRPNState()
+// to ensure each test starts with a fresh RPN state.
+//
+// Note: This function is intended for test use only and should not be used
+// in production code. For production use, create new REPL instances with NewREPL().
+func ResetExecutorREPL() {
+	executorREPLMu.Lock()
+	defer executorREPLMu.Unlock()
+	executorREPL = nil
+}
 
 // REPL manages the interactive command-line interface for the percentage calculator.
 // It provides an interactive prompt with history, tab-completion, signal handling,
@@ -39,12 +51,14 @@ var rpnStateOnce sync.Once
 //   - HistoryManager: manages command history persistence
 //   - SignalHandler: handles SIGINT (Ctrl+C)
 //   - commandChain: processes commands via chain of responsibility
+//   - rpnState: provides RPN state for calculations
 type REPL struct {
 	ttyChecker    *TTYChecker
 	historyMgr    *HistoryManager
 	signalHandler *SignalHandler
 	prompt        *prompt.Prompt
 	commandChain  CommandHandler
+	rpnState      *RPNState
 }
 
 // NewREPL creates a new REPL instance with default components.
@@ -54,11 +68,19 @@ type REPL struct {
 // The executor function is called for each non-empty input line.
 // The completer function provides tab-completion suggestions for the prompt.
 func NewREPL(executor func(string), completer func(prompt.Document) []prompt.Suggest) *REPL {
+	// Initialize RPN state via dependency injection
+	vars := rpn.NewVariables()
+	rpnState := &RPNState{
+		vars:    vars,
+		rpnCalc: rpn.NewRPN(vars),
+	}
+
 	repl := &REPL{
 		ttyChecker:    &TTYChecker{},
 		historyMgr:    NewHistoryManager(".gt_history"),
 		signalHandler: NewSignalHandler(),
 		commandChain:  NewCommandChain(),
+		rpnState:      rpnState,
 	}
 
 	// Set up executor - if nil, use default
@@ -226,59 +248,68 @@ func RunREPL() error {
 // commands via the chain of responsibility pattern, including percentage
 // calculations, RPN expressions, and built-in commands.
 func executor(input string) {
-	// Create a minimal REPL instance without building a prompt
-	r := &REPL{
-		ttyChecker:    &TTYChecker{},
-		historyMgr:    NewHistoryManager(".gt_history"),
-		signalHandler: NewSignalHandler(),
-		commandChain:  NewCommandChain(),
-	}
-	defaultExecutor(r, input)
-}
-
-// getRPNState returns or creates the RPN state using lazy initialization.
-// It's thread-safe using sync.Once to ensure the RPN state is initialized exactly once.
-// The RPN state is shared across all REPL instances.
-//
-// Returns the RPNState instance for performing RPN calculations
-func getRPNState() *RPNState {
-	rpnStateOnce.Do(func() {
+	// Initialize executorREPL only once using sync.Once for thread-safe lazy initialization
+	executorREPLOnce.Do(func() {
 		vars := rpn.NewVariables()
-		rpnState = &RPNState{
+		rpnState := &RPNState{
 			vars:    vars,
 			rpnCalc: rpn.NewRPN(vars),
 		}
+
+		// Create a minimal REPL instance without building a prompt
+		executorREPL = &REPL{
+			ttyChecker:    &TTYChecker{},
+			historyMgr:    NewHistoryManager(".gt_history"),
+			signalHandler: NewSignalHandler(),
+			commandChain:  NewCommandChain(),
+			rpnState:      rpnState,
+		}
 	})
-	return rpnState
-}
 
-// getRPNState returns the RPN state.
-// This is a REPL instance method for backward compatibility that delegates to the package-level getRPNState.
-//
-// Returns the RPNState instance for performing RPN calculations
-func (r *REPL) getRPNState() *RPNState {
-	return getRPNState()
+	// Use mutex to protect access to executorREPL during execution
+	executorREPLMu.Lock()
+	repl := executorREPL
+	executorREPLMu.Unlock()
+
+	defaultExecutor(repl, input)
 }
 
 // runRPN parses and evaluates an RPN (Reverse Polish Notation) expression.
-// It uses the shared RPN state to maintain stack state across multiple calls.
+// This is a package-level wrapper for backward compatibility that delegates to
+// the executor's REPL runRPN method.
 //
-// input: the RPN expression to evaluate (e.g., "3 4 +" or "x 5 = x x +")
+// input: the RPN expression to evaluate
 // Returns the result string and an error if the expression is invalid
 func runRPN(input string) (string, error) {
-	state := getRPNState()
-	return state.rpnCalc.ParseAndEvaluate(input)
+	executorREPLMu.Lock()
+	defer executorREPLMu.Unlock()
+
+	if executorREPL != nil {
+		return executorREPL.rpnState.rpnCalc.ParseAndEvaluate(input)
+	}
+	return "", fmt.Errorf("no executor REPL available - call executor() first")
 }
 
-// runRPN parses and evaluates an RPN (Reverse Polish Notation) expression.
-// This is a REPL instance method for backward compatibility that delegates to the package-level runRPN.
+// getRPNState returns the RPN state from the executor's REPL.
+// This is a package-level helper for backward compatibility with tests that need
+// to access RPN state after calling executor(). It's not part of the main REPL
+// architecture.
 //
-// input: the RPN expression to evaluate
-// Returns the result string and an error if the expression is invalid
-func (r *REPL) runRPN(input string) (string, error) {
-	return runRPN(input)
+// Returns the RPNState instance from the last executor() call, or nil if executor() hasn't been called
+func getRPNState() *RPNState {
+	executorREPLMu.Lock()
+	defer executorREPLMu.Unlock()
+
+	if executorREPL != nil {
+		return executorREPL.rpnState
+	}
+	return nil
 }
 
+
+
+
+
 // getHistoryPath returns the absolute path to the history file.
 // This is a package-level wrapper for backward compatibility.
 // The history file is stored in the user's home directory.
diff --git a/internal/repl/repl_test.go b/internal/repl/repl_test.go
index 2200b2a..cd062cb 100644
--- a/internal/repl/repl_test.go
+++ b/internal/repl/repl_test.go
@@ -470,11 +470,17 @@ func TestRPNHandlerWithPercentageExpression(t *testing.T) {
 func TestRPNHandlerWithRPNExpression(t *testing.T) {
 	// Test RPN expressions
 	chain := NewCommandChain()
+	vars := rpn.NewVariables()
+	rpnState := &RPNState{
+		vars:    vars,
+		rpnCalc: rpn.NewRPN(vars),
+	}
 	r := &REPL{
 		ttyChecker:    &TTYChecker{},
 		historyMgr:    NewHistoryManager(".gt_history"),
 		signalHandler: NewSignalHandler(),
 		commandChain:  chain,
+		rpnState:      rpnState,
 	}
 
 	// Test RPN expression
@@ -490,11 +496,17 @@ func TestRPNHandlerWithRPNExpression(t *testing.T) {
 func TestRPNHandlerWithSingleNumber(t *testing.T) {
 	// Test single number input (RPN - pushes number onto stack)
 	chain := NewCommandChain()
+	vars := rpn.NewVariables()
+	rpnState := &RPNState{
+		vars:    vars,
+		rpnCalc: rpn.NewRPN(vars),
+	}
 	r := &REPL{
 		ttyChecker:    &TTYChecker{},
 		historyMgr:    NewHistoryManager(".gt_history"),
 		signalHandler: NewSignalHandler(),
 		commandChain:  chain,
+		rpnState:      rpnState,
 	}
 
 	// Test single number