Architecture Review Findings#

Overview#

The opnDossier architecture review was conducted on January 14, 2026, by an architecture analysis team evaluating a codebase of approximately 40,000 lines of Go code with 61 test files. The project received an overall architecture grade of A- (Strong architecture with minor improvement opportunities), indicating a mature codebase with well-designed foundational patterns but some areas requiring refinement.

The review identified significant architectural strengths including an exemplary plugin architecture demonstrating clean interface definitions, loose coupling, and thread-safe registry patterns, comprehensive security-first parser design with XML bomb protection and XXE attack prevention, and clean package organization with clear separation of concerns across 13 logical domains. The codebase demonstrates modern Go patterns including proper error wrapping with %w, context propagation, and interface-based design.

However, the review also documented technical debt requiring attention: global state in cmd/ and audit/plugin.go, context underutilization in long-running operations, sequential plugin execution limiting parallelization opportunities, converter pattern fragmentation, and unclear LRU cache dependency usage. The converter pattern fragmentation was subsequently resolved through PRs #400, #409, #431, and #434, which consolidated format dispatch logic through the FormatRegistry pattern. A follow-up review is scheduled for Q2 2026.

Architectural Strengths#

Plugin Architecture (Best Practice Example)#

The opnDossier plugin architecture demonstrates exceptional design as a best practice reference for extensible compliance systems. The compliance.Plugin interface provides a standardized contract with clean separation between metadata methods (Name, Version, Description), execution (RunChecks), and introspection (GetControls, GetControlByID). The architecture supports both built-in plugins (firewall, SANS, STIG) and dynamic .so plugins loaded at runtime.

The plugin registry implements thread-safe operations using sync.RWMutex for concurrent access, allowing parallel reads during plugin retrieval while ensuring exclusive write access during registration. Plugin validation occurs at registration time, preventing invalid plugins from entering the system. The global registry uses sync.Once for thread-safe lazy initialization, ensuring single initialization across concurrent access.

Panic Recovery and Isolation: The plugin execution architecture implements panic recovery boundaries around each plugin's RunChecks() invocation (PR #442, issue #309). Each iteration wraps the plugin call in defer recover(), preventing misbehaving plugins—particularly dynamically-loaded .so plugins—from crashing the entire audit process. Panicked plugins are logged via *slog.Logger and retained in results with zero findings, ensuring downstream consumers can see all requested plugins were evaluated. This isolation pattern, documented in AGENTS.md §8.2 and GOTCHAS.md §2.2, provides critical stability guarantees for extensible systems where third-party plugin quality cannot be guaranteed.

Parser Security Architecture#

The XML parser implements security-first design with multiple protective layers: 10MB default size limits prevent XML bombs, entity expansion is disabled via empty entity map to prevent XXE attacks, and streaming processing minimizes memory footprint. The parser actively checks context for cancellation during token processing, supporting timeouts and graceful shutdown.

Package Organization#

The project demonstrates clean separation of concerns across 13 logical domains: analysis (canonical finding types), audit (security plugins), config (configuration management), converter (JSON/YAML/Markdown), display (terminal output), log (centralized logging), markdown (generation), metrics (performance), parser (XML parsing with security), plugin (interfaces), plugins (compliance implementations), processor (core analysis), and validator (configuration validation). No circular dependencies exist, with clear boundaries between parsing, processing, conversion, and output. The introduction of the analysis package improves code consistency by eliminating duplicate Finding type definitions across modules.

Interface Segregation (SOLID Principles)#

Issue #323 identified that the ReportBuilder interface violated the Interface Segregation Principle (ISP) with 20 methods bundled into a monolithic interface. PR #431 resolved this by splitting it into three focused interfaces:

• SectionBuilder (9 methods): Build*Section methods and SetIncludeTunables for rendering individual configuration domains
• TableWriter (11 methods): Write*Table methods for appending formatted tables to markdown instances
• ReportComposer (2 methods): BuildStandardReport and BuildComprehensiveReport for assembling complete reports

The public ReportBuilder interface now composes all three sub-interfaces, maintaining full backward compatibility. The refactoring implements the consumer-local interface narrowing pattern documented in AGENTS.md section 5.9a: HybridGenerator declares unexported reportGenerator and auditBuilder interfaces listing only the methods it actually calls (SetIncludeTunables, BuildAuditSection, BuildStandardReport, BuildComprehensiveReport), while the public API continues accepting the broad ReportBuilder interface. This architectural improvement demonstrates evolutionary design that enhances code quality without breaking existing integrations.

The project follows Go best practices by exposing a public API surface through pkg/ packages while keeping implementation details in internal/. The platform-agnostic CommonDevice model lives in pkg/model/, vendor-specific parsers in pkg/parser/, and configuration schemas in pkg/schema/, enabling external Go projects to import and use opnDossier's core functionality. The parser factory (renamed to Factory following Go naming conventions) creates device-specific parsers with injected dependencies, maintaining clean separation between the public API and internal implementation.

Public Package Boundary Enforcement: Issue #301 identified violations where pkg/ packages improperly imported internal/ packages, breaking Go's module-level access boundary for external consumers. These violations were resolved through interface injection (defining OPNsenseXMLDecoder in pkg/parser/ and injecting cfgparser.NewXMLParser() at the cmd/ layer) and constant extraction (duplicating string literals like NetworkAny into pkg/schema/opnsense/). The solutions are documented in docs/solutions/architecture-issues/pkg-internal-import-boundary.md. Boundary verification is now part of architectural standards: grep -rn 'internal/' --include='*.go' pkg/ | grep -v _test.go catches violations before commits. The interface injection pattern established in this effort is the canonical approach for handling pkg/ packages that need internal/ functionality.

Testing Framework#

The project maintains 61 test files with comprehensive coverage including unit tests, benchmark tests (*_bench_test.go), integration tests marked with build tags, and legacy comparison tests. Development standards require >80% test coverage for all new functionality.

Technical Debt and Issues#

1. Global State in cmd/ and audit/plugin.go (Moderate Concern)#

Issue Description: The cmd/root.go package contains global variables including cfgFile, Cfg (application configuration), and logger, while internal/audit/plugin.go contains a GlobalRegistry initialized with sync.Once.

Impact: Testing complexity requires explicit setup/teardown, concurrency risks from shared mutable state, and increased package coupling through implicit dependencies.

Thread-Safety Guarantees: The global plugin registry is explicitly documented with comprehensive thread-safety guarantees. The sync.Once initialization provides happens-before semantics per the Go memory model, ensuring all writes within the initialization function are visible to any goroutine calling GetGlobalRegistry() without additional synchronization. The intended usage pattern follows a lifecycle contract: register-at-startup (sequential initialization phase) followed by read-during-operation (concurrent runtime phase). While the internal sync.RWMutex makes concurrent reads and writes technically safe, the architectural design expects all plugin registration to complete during sequential application startup before concurrent access begins.

Architectural Independence (RESOLVED — Historical): PluginManager previously maintained its own internal PluginRegistry instance that was independent of the global singleton, and plugins registered through PluginManager.InitializePlugins() did not populate the global registry returned by GetGlobalRegistry(). This registry independence led to silent "plugin registered in one, queried from the other" bugs and was documented in GOTCHAS.md section 2.1 as a critical gotcha.

As of todo #143 (resolved in Phase 7), there is now a single registry path: NewPluginManager(logger, reg) accepts an explicit *PluginRegistry parameter. Pass nil to allocate a fresh private registry, or pass a shared *PluginRegistry when multiple managers or subsystems must observe the same plugin set. GetGlobalRegistry, RegisterGlobalPlugin, GetGlobalPlugin, and ListGlobalPlugins are now deprecated and scheduled for removal in v2.0. GOTCHAS.md section 2.1 has been rewritten as "Registry Consolidation (Historical — Resolved)" to document the resolution.

General Mitigation: CLI globals follow acceptable Cobra conventions, and no mutation races are evident.

Current Solution (CommandContext Pattern): The codebase has implemented a CommandContext pattern for dependency injection that encapsulates Config and Logger, storing them in Cobra's command context rather than package globals. Commands retrieve dependencies using GetCommandContext(), reducing global state usage while maintaining Cobra compatibility.

Recommended Solution: Refactor GlobalRegistry from singleton pattern to dependency injection, keep CLI globals acceptable for the main package, and add tests demonstrating thread safety. The registry consolidation has been completed (see "Architectural Independence" above). A CommandContext pattern has been proposed to further reduce global state.

2. Context Underutilization (Low-Medium Priority)#

Issue Description: Many functions accept context.Context parameters but do not utilize them for cancellation, leaving them named as _ to indicate intentional non-use. Examples include MarkdownConverter.ToMarkdown, CoreProcessor.analyze, and multiple audit report generation methods.

Impact: Long-running operations such as analysis and large file parsing cannot be cancelled, and no timeout support exists. Operations that need context checks include analyzeDeadRules, analyzeInterfaceRules (O(n²) complexity), analyzeUnusedInterfaces, and analyzeSecurityIssues.

Good Example: The XML parser demonstrates proper context usage with periodic ctx.Done() checks during token streaming, and CoreProcessor.Process checks context between phases but not within individual operations.

Recommended Solution: Add context checks to long-running operations using select statements checking ctx.Done(), and document context policy including when to check ctx.Done() and recommended timeouts.

3. Sequential Plugin Execution (Performance Optimization Opportunity)#

Issue Description: Plugin checks execute sequentially, with each plugin's RunChecks() call wrapped in panic recovery boundaries (PR #442, issue #309). The for loop includes defer recover() protection to prevent misbehaving plugins from crashing the audit engine, with panicked plugins logged and retained in results with zero findings. The architecture review confirms concurrency is underutilized in the current implementation, though sequential execution with panic isolation provides strong reliability guarantees.

Current Approach: Sequential execution is correct and sufficient for typical configuration sizes. The plugin registry is thread-safe (using sync.RWMutex), enabling future parallelization without registry modifications. The panic recovery boundary ensures plugin failures are isolated and cannot propagate to the audit engine or affect other plugins.

Optimization Recommendation: Add parallel processing for large-scale batch operations using worker pools to limit concurrency, though this is a performance optimization rather than an architectural requirement. The codebase includes a worker pool implementation in internal/processor/concurrent.go demonstrating proper context-aware concurrent patterns. Any parallelization implementation must preserve the panic recovery boundary around each plugin to maintain isolation guarantees.

4. LRU Cache Dependency (Code Clarity Issue)#

Issue Description: The project includes a hashicorp/golang-lru/v2 dependency with unclear usage - no obvious cache usage was found in scanned files.

Recommended Solution: Audit where the LRU cache is used, document cache sizing and eviction policy, or remove the dependency if unused.

5. Charset Reader Simplifications (Known Technical Debt)#

Issue Description: The XML parser has a TODO comment to use golang.org/x/text/encoding rather than the current implementation that treats ISO-8859-1 as UTF-8 (a simplification).

Impact: Limited encoding support beyond UTF-8, US-ASCII, ISO-8859-1, and Windows-1252.

Workaround: Most OPNsense configurations use UTF-8, making the current approach acceptable.

Recommended Solution: Track the charset TODO in the backlog, document the limitation, and defer implementation until encoding compatibility issues arise.

6. Converter Pattern Consolidation (Resolved)#

Issue Description: The project previously contained redundant markdown generation implementations with unclear boundaries. Two approaches existed: converter/markdown.go for legacy direct conversion (used only in tests) and converter/hybrid_generator.go for modern programmatic generation (used in production).

Progress (PR #400): PR #400 addressed part of this technical debt by moving audit report rendering from cmd/audit_handler.go to the internal/converter/builder/ layer. The new BuildAuditSection() method consolidated audit-specific markdown generation into the builder pattern, eliminating format-specific code in the cmd layer. Audit mode produces multi-format output (markdown, JSON, YAML) through the unified converter architecture by mapping audit.Report to model.ComplianceResults and attaching it to CommonDevice before generation.

Progress (PR #409 - Shared Analysis Package): PR #409 eliminated mirrored analysis logic between converter/enrichment.go and processor/analyze.go by extracting shared detection, statistics, rule comparison, and helper functions into a new internal/analysis/ package. The converter no longer mirrors processor logic—both modules now import and call the same canonical implementations (analysis.ComputeStatistics(), analysis.ComputeAnalysis(), analysis.DetectDeadRules(), etc.).

Progress (PR #431 - Interface Segregation): PR #431 improved the builder pattern's separation of concerns by splitting the monolithic 20-method ReportBuilder interface into focused sub-interfaces (SectionBuilder, TableWriter, ReportComposer). This refactoring implemented the Interface Segregation Principle and introduced the consumer-local interface narrowing pattern, where consumers declare narrow private interfaces depending only on methods they actually call.

Resolution (PR #434 - FormatRegistry): PR #434 completed the converter consolidation by introducing FormatRegistry as the single source of truth for format dispatch. The registry pattern replaced scattered switch statements and constant definitions across 8+ locations in the codebase. DefaultRegistry centralizes format names, aliases, file extensions, validation, and generation dispatch — adding a new format requires only registering a FormatHandler in newDefaultRegistry(). All format handler parameter signatures were normalized to accept Options uniformly, eliminating inconsistent parameter decomposition. The processor.Transform() method adds alias resolution via DefaultRegistry.Canonical(), ensuring aliases (txt, htm, md, yml) work consistently across all code paths. The registry is documented in AGENTS.md section 5.9b and includes comprehensive test coverage (76 registry test cases with 100% coverage). See internal/converter/registry.go.

Plugin Architecture Details#

CompliancePlugin Interface#

The compliance.Plugin interface defines a contract for compliance checking with methods:

• Metadata: Name(), Version(), Description()
• Execution: RunChecks(device *model.CommonDevice) - returns three values: findings slice, evaluated control IDs slice, and error
• Introspection: GetControls(), GetControlByID(id string)
• Validation: ValidateConfiguration() - ensures plugin configuration is valid

The Finding type is defined canonically in internal/analysis/finding.go and standardizes analysis findings across audit, compliance, and processor modules with fields for type, title, description, recommendation, component, reference, and metadata. The compliance.Finding type is a type alias for analysis.Finding, ensuring consistency across the codebase. The Control struct defines security controls with ID, title, category, severity, and remediation guidance.

Built-in Plugins#

Three built-in plugins are registered during initialization:

1. STIG Plugin (stig.go) - Implements DoD Security Technical Implementation Guide checks
2. SANS Plugin (sans.go) - Implements SANS security best practices
3. Firewall Plugin (firewall.go) - Checks firewall-specific security configurations

Plugin Execution Flow#

The execution flow follows these steps:

1. PluginManager.InitializePlugins() registers all plugins at startup
2. PluginRegistry.RunComplianceChecks() iterates through requested plugins sequentially, with each plugin's RunChecks() call wrapped in defer recover() panic recovery boundaries
3. Each plugin's RunChecks() method analyzes the CommonDevice configuration in a single pass, returning findings, evaluated control IDs, and error; panicked plugins are logged and retained with zero findings
4. Findings are aggregated into a ComplianceResult with summary statistics
5. In audit mode, cmd/audit_handler.go maps audit.Report to model.ComplianceResults and attaches it to a shallow copy of CommonDevice
6. The standard converter pipeline handles rendering via the builder pattern (markdown), or direct serialization (JSON/YAML)

Dynamic Plugin Loading#

The registry supports loading .so plugins from directories using Go's plugin system, enabling extensibility without recompilation.

Parser Security Features#

Input Protection Mechanisms#

The parser implements multiple security layers:

• 10MB default size limits prevent XML bombs via io.LimitReader
• Entity expansion disabled via empty entity map prevents XXE attacks
• Streaming processing with token-based parsing minimizes memory footprint
• Automatic charset conversion handles UTF-8, US-ASCII, ISO-8859-1, and Windows-1252

Memory Efficiency#

Token-based streaming processes XML without loading complete documents, and explicit garbage collection is triggered after large sections to manage memory usage. The parser checks context.Done() periodically during token processing, enabling graceful cancellation.

Testing Coverage#

Test Organization#

The project includes 61 test files with multiple coverage patterns:

• Unit tests for individual functions and components
• Benchmark tests (*_bench_test.go files) for performance tracking
• Integration tests marked with build tags
• Legacy comparison tests validating output consistency

Development Standards#

The CONTRIBUTING.md guide requires >80% test coverage for all new functionality. Tests must demonstrate thread safety for concurrent components, with explicit setup/teardown for global state.

Roadmap for Addressing Issues#

Resolved Issues#

1. Public Package Boundary Violations (Issue #301) - Resolved through interface injection and constant extraction patterns. The pkg/ packages no longer import internal/ packages, maintaining Go's module-level access boundary for external consumers. Documented in docs/solutions/architecture-issues/pkg-internal-import-boundary.md.

2. Mirrored Analysis Logic (PR #409) - The converter enrichment pipeline previously mirrored processor analysis logic, creating maintenance burden and inconsistency risks. PR #409 resolved this by extracting shared functionality into internal/analysis/ package, providing canonical implementations of ComputeStatistics(), ComputeAnalysis(), DetectDeadRules(), DetectUnusedInterfaces(), and rule comparison helpers. Both converter and processor now import this shared package, eliminating duplication and establishing a clear architectural pattern for shared analysis functionality.

3. ReportBuilder Interface Segregation (Issue #323, PR #431) - The monolithic 20-method ReportBuilder interface violated the Interface Segregation Principle by bundling unrelated responsibilities. PR #431 resolved this by splitting it into three focused interfaces: SectionBuilder (9 methods for rendering configuration sections), TableWriter (11 methods for table formatting), and ReportComposer (2 methods for assembling complete reports). The public ReportBuilder interface now composes all three, maintaining full backward compatibility. The refactoring introduced the consumer-local interface narrowing pattern (documented in AGENTS.md section 5.9a), where HybridGenerator declares narrow private interfaces (reportGenerator, auditBuilder) depending only on methods actually called. This demonstrates evolutionary architecture that improves code quality without breaking integrations.

4. Converter Pattern Consolidation (PR #434) - The codebase previously contained scattered switch statements and format constant definitions across 8+ locations for handling different output formats. PR #434 resolved this by introducing the FormatRegistry pattern, establishing DefaultRegistry as the single source of truth for format dispatch. Format validation, handler selection, alias resolution, and file extension determination now occur through centralized registry methods. Handler parameter signatures were normalized to accept Options uniformly. The processor.Transform() method gained alias resolution via DefaultRegistry.Canonical(), ensuring consistent behavior for format aliases (md, yml, txt, htm) across all code paths. Adding new output formats requires only registering a FormatHandler in newDefaultRegistry(). The pattern is documented in AGENTS.md section 5.9b.

5. DeviceParser Registry Pattern (Issue #302, PR #437) - Replaced hardcoded switch-based factory dispatch with a thread-safe DeviceParserRegistry enabling compile-time extensibility. Parser packages now self-register via init() functions, eliminating the need to modify factory.go when adding new device types. Follows the database/sql driver pattern and provides runtime discovery via registry.List(). The registry uses the same thread-safe singleton pattern (sync.Once + sync.RWMutex) as the existing plugin and format registries, demonstrating consistent architectural patterns across the codebase. Implementation documented in docs/solutions/architecture-issues/pluggable-deviceparser-registry-pattern.md and AGENTS.md section 5.25a.

6. Plugin Panic Recovery (Issue #309, PR #442) - Implemented panic recovery boundaries around plugin execution to address the stability concern of misbehaving plugins crashing the audit engine. Each plugin's RunChecks() call in RunComplianceChecks is wrapped in defer recover(), preventing dynamically-loaded or third-party plugins from propagating panics. Panicked plugins are logged via *slog.Logger and retained in results with zero findings, ensuring downstream consumers receive complete plugin information. This architectural pattern, documented in AGENTS.md §8.2 and GOTCHAS.md §2.2, establishes critical isolation boundaries for extensible systems where plugin quality cannot be guaranteed.

High Priority (Architectural)#

6. Reduce Global State - Low-medium effort, medium impact. Implement CommandContext pattern more broadly for dependency injection.

Medium Priority (Quality)#

7. Improve Context Usage - Low effort, medium impact. Add context checks to long-running operations with select statements.

Low Priority (Enhancement)#

8. Add Architecture Diagrams - Low effort, high documentation impact. Create mermaid diagrams for data flow and plugin architecture.
9. Optimize Parallelization - Medium effort, low performance impact. Add concurrent plugin execution for large-scale operations.
10. Audit Cache Usage - Low effort, code clarity impact. Find LRU cache usage or remove unused dependency.

Deferred (Acceptable Workarounds)#

11. Charset Reader Enhancement - Defer until encoding compatibility issues arise; current UTF-8 simplification is acceptable.

Summary#

The opnDossier project demonstrates mature software architecture with strong separation of concerns, an extensible plugin system, secure-by-default XML parsing, comprehensive testing, and modern Go idioms. The A- architecture grade reflects minor technical debt that does not compromise core functionality. No major architectural changes are required - the codebase is well-designed for long-term maintainability.

The converter pattern consolidation identified in the architecture review has been completed through PRs #400, #409, #431, and #434. Plugin panic recovery (PR #442) addresses the stability concern identified in the original review regarding plugin execution reliability. The next architecture review, scheduled for Q2 2026, will assess progress on remaining issues and validate architectural decisions made during refactoring.

Relevant Code Files#

Related Topics#

• Plugin Architecture Best Practices - Design patterns for extensible compliance systems
• Dependency Injection in Go - CommandContext pattern as alternative to global state
• Context Propagation Patterns - Best practices for cancellation and timeout support
• Secure XML Parsing - XXE prevention and streaming architecture
• Thread-Safe Registry Patterns - Using sync.RWMutex and sync.Once for concurrent access
• Public Package Boundary Enforcement - Interface injection pattern for pkg/ packages requiring internal/ functionality
• Shared Analysis Package - Canonical implementations of statistics, detection, and rule comparison logic to eliminate code duplication
• FormatRegistry Pattern - Single source of truth for format dispatch, validation, and alias resolution (AGENTS.md §5.9b)
• DeviceParser Registry Pattern - Thread-safe parser registry with self-registration and runtime discovery (AGENTS.md §5.25a)