# Codebase Concerns

**Analysis Date:** 2026-05-18

## Tech Debt

**Blocking async calls at CLI boundary:**
- Issue: 56+ `.GetAwaiter().GetResult()` calls block async operations synchronously
- Files: `Program.cs` (lines 90, 101, 122, 142, 166, 200, 221, 240, 262), machine classes
- Why: Main() was not async when initially written; .NET 4.7.2 supports async Main via C# 7.1
- Impact: Thread pool starvation risk, deadlock risk in certain SynchronizationContext scenarios, harder debugging
- Fix: Refactor `Program.Main` to `static async Task<int> Main(string[] args)`, propagate `await` throughout

**Silent exception handlers (catch without logging):**
- Issue: `try { ... } catch { }` swallows exceptions with no trace
- Files: `Cnc/Fanuc/FanucMachine.cs` (disconnect cleanup), `Cnc/Siemens/SiemensMachine.cs`, `Cnc/Heidenhain/HeidenhainMachine.cs`, `Cnc/Heidenhain/Lsv2Client.cs`
- Why: Cleanup paths (disconnect, logout) should not propagate to caller; silence was intentional
- Impact: Resource leaks masked, production issues undiagnosable, disconnect failures invisible
- Fix: Log at minimum to debug/trace: `catch (Exception ex) { /* log ex */ }`. Add logging framework.

**No structured logging framework:**
- Issue: Entire codebase uses `Console.WriteLine` (98+ calls in `Program.cs` alone)
- Files: `Program.cs`, all machine implementations
- Why: MVP simplicity; tool was always a CLI
- Impact: No log levels, no timestamps, no file logging, production issues undiagnosable
- Fix: Add Serilog or NLog. Replace Console.WriteLine with `_logger.Information()`. Add request/response tracing to FOCAS calls.

**Large monolithic FanucMachine:**
- Issue: `Cnc/Fanuc/FanucMachine.cs` is ~740 lines with complex state management
- Files: `Cnc/Fanuc/FanucMachine.cs`
- Why: FOCAS protocol has many edge cases; chunked transfer and offset handling grew organically
- Impact: Hard to test individual branches; buffer handling edge cases unclear
- Fix: Extract `IFocasTransferHandler` for chunk upload/download logic; separate offset data reader

**`string.Format()` over `$""` interpolation:**
- Issue: Old string formatting style throughout validator and some machine code
- Files: `Cnc/NcProgramValidator.cs`, `Program.cs`
- Why: Legacy code predates C# 6 adoption on this project
- Impact: Cosmetic; harder to read format strings
- Fix: Opportunistic migration to `$""` during edits

## Known Issues / Incomplete Implementations

**Heidenhain ReadActiveProgramAsync / SelectMainProgram not implemented:**
- Symptoms: Returns `CncResult.Fail("Not supported via basic LSV2")`
- Files: `Cnc/Heidenhain/HeidenhainMachine.cs`
- Root cause: LSV2 protocol complexity; feature not required for initial release
- Workaround: Users specify program path explicitly

**Siemens ReadActiveProgramAsync / SelectMainProgram not implemented:**
- Symptoms: Returns stub failure
- Files: `Cnc/Siemens/SiemensMachine.cs`
- Root cause: FTP protocol doesn't expose active program info
- Workaround: Users specify program path explicitly

**MachineSnapshot returns stub data for Heidenhain/Siemens/Mitsubishi:**
- Files: `Cnc/Heidenhain/HeidenhainMachine.cs`, `Cnc/Siemens/SiemensMachine.cs`, `Cnc/Mitsubishi/MitsubishiMachine.cs`
- Issue: All snapshot fields return Unknown/empty; no real machine state read
- Impact: No diagnostic info available for non-Fanuc machines

## Security Considerations

**RSA public key hardcoded in binary:**
- Risk: Key embedded as const string in binary; cannot be rotated without recompile and redeploy
- Files: `Licensing/LicenseValidator.cs`
- Current mitigation: Asymmetric RSA — private key is vendor-only; hardcoded public key is acceptable for this use case
- Recommendation: Document key rotation procedure; consider adding license version/expiration metadata for future revocation

**MachineFingerprint null dereference on unusual hardware:**
- Risk: `mo.Properties["processorID"].Value.ToString()` throws NullReferenceException if WMI property missing
- Files: `Licensing/MachineFingerprint.cs`
- Current mitigation: None
- Fix: `mo.Properties["processorID"]?.Value?.ToString() ?? "unknown"`

**Obsolete crypto APIs:**
- Risk: `RSACryptoServiceProvider` and `SHA256CryptoServiceProvider` are obsolete in .NET 6+
- Files: `Licensing/LicenseValidator.cs`
- Current mitigation: .NET 4.7.2 target — obsolete warning only, not broken
- Fix when upgrading: Use `RSA.Create()` + `VerifyData(data, signature, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1)`

**SHA256 provider not explicitly disposed:**
- Risk: Minor resource leak in license validation path
- Files: `Licensing/LicenseValidator.cs`
- Fix: Wrap in `using (var sha = new SHA256CryptoServiceProvider()) { ... }`

## Platform Constraints

**Windows-only (hard constraint):**
- FOCAS1 DLL (`Fwlib32.dll`) is 32-bit Windows native — no Linux/macOS support
- WMI (`System.Management`) Windows-only — licensing system non-functional on Linux
- Files: `Cnc/Fanuc/libs/`, `Licensing/MachineFingerprint.cs`, `fwlib32.cs`
- Impact: Cannot run on Linux/macOS; CI on Linux can only test non-FOCAS paths
- Action if cross-platform needed: Abstract fingerprinting behind `IMachineFingerprint`; already done — add Linux implementation using `/proc/cpuinfo` or `dmidecode`

**FOCAS DLL version sensitivity:**
- Risk: FOCAS API changes between Fanuc control versions; `FanucDialectDetector` auto-detects but may fail on unknown versions
- Files: `Cnc/Fanuc/FanucDialectDetector.cs`, `Cnc/Fanuc/FanucMachine.cs`
- Mitigation: Dialect abstraction layer (`IFocasDialect`) isolates version differences

## Performance Bottlenecks

**In-memory accumulation for large NC programs:**
- Issue: `UploadProgramByIdentifier` appends to `StringBuilder` for entire program before writing
- Files: `Cnc/Fanuc/FanucMachine.cs`
- Impact: For programs >10MB, high memory allocation; streaming to file would be more efficient
- Fix: Write chunks directly to `FileStream` instead of accumulating in memory

**Polling with 100ms busy-wait during transfer:**
- Issue: `ct.WaitHandle.WaitOne(100)` loops during buffer waits
- Files: `Cnc/Fanuc/FanucMachine.cs`
- Impact: Wastes thread-pool slot; CPU cycles during transfer
- Fix: Use `await Task.Delay(100, ct)` instead of blocking WaitOne

**Network connection timeout via `.Wait()` with boolean return:**
- Issue: `_tcp.ConnectAsync().Wait(timeout)` — if task never completes, does not abort it
- Files: `Cnc/Heidenhain/Lsv2Client.cs`
- Fix: `await Task.WhenAny(connectTask, Task.Delay(timeout, ct))` + cancellation

## Fragile Areas

**FOCAS dialect detection (`FanucDialectDetector`):**
- Files: `Cnc/Fanuc/FanucDialectDetector.cs`
- Why fragile: Relies on probing specific FOCAS API calls to detect version; untested against full range of Fanuc control generations
- Safe modification: Always test against physical hardware after changes; log detected dialect version

**License validation (no recovery path):**
- Files: `Licensing/LicenseValidator.cs`, `Licensing/MachineFingerprint.cs`
- Why fragile: Any WMI failure or file read error produces a hard exit with no diagnostic for user
- Safe modification: Add exception handling with user-facing error message; distinguish "no license file" from "invalid license" from "fingerprint failure"

**Integration tests with hardcoded IPs:**
- Files: `NcProgramManager.Tests/Integration/SiemensMachineIntegrationTests.cs`, `HeidenhainMachineIntegrationTests.cs`, `MitsubishiMachineIntegrationTests.cs`
- Why fragile: Tests will fail in CI without physical hardware at specific IPs
- Fix: Make IPs configurable via environment variables; skip integration tests in CI by default

## Missing / Not Covered

- No CLI-level tests (no test for `Program.cs` orchestration or exit code behavior)
- No tests for `MachineFingerprint` (WMI, no mock tests)
- No tests for `LicenseFile` parsing edge cases (corrupt Base64, empty file)
- No structured logging or diagnostics framework
- No error code documentation in code (only in `README.md`)
- No architecture documentation (FOCAS dialect selection logic undocumented in code)

---

**Overall Assessment:** Architecture is solid — clean machine abstraction, well-structured interfaces, good error propagation via `CncResult<T>`. Main concerns are operational (logging, blocking async, empty catch blocks) rather than structural. Platform constraints (Windows, FOCAS DLL) are intentional for the target environment.

---

*Concerns analysis: 2026-05-18*
*Update as issues are resolved or discovered*