Code Review Workflow#

The Code Review Workflow in the opnDossier project is a comprehensive, automated code quality assurance process built around CodeRabbit, an AI-powered code review tool integrated with GitHub Actions, linting tools, and multi-platform testing. The workflow enforces strict quality gates through Pipeline v2 compliance, including mandatory acknowledgment of CodeRabbit findings before merging any pull request. This approach combines automated tooling with human oversight to maintain high code quality while enabling rapid development.

Unlike traditional manual code review processes, the opnDossier workflow leverages 51 integrated linting and security tools (including golangci-lint, gitleaks, semgrep, and checkov) and path-specific review instructions tailored to different parts of the codebase. Mergify automation manages five author-specific merge queues that differentiate between bot PRs with varying CI requirements, maintainer PRs, and external contributions, ensuring appropriate review depth and testing rigor for each category.

The workflow emphasizes triage effectiveness over exhaustive review coverage. CodeRabbit findings are classified by severity (critical, high, medium, low, info), with clear guidelines on what to fix immediately (potential issues, silent failures) versus what to defer (scope creep, over-engineering suggestions). Common false positive patterns have been documented, including "add validation methods", "unused context parameter", and "consolidate test helpers", helping developers distinguish actionable feedback from noise.

CodeRabbit Integration#

CodeRabbit is configured as the primary AI-assisted code review tool using an assertive review profile with automatic reviews enabled for all PRs to the main branch (excluding drafts and WIP PRs). The tool operates as a GitHub App that automatically triggers on PR events without requiring explicit workflow configuration.

Configuration Features#

The 516-line .coderabbit.yaml configuration file defines:

• Review Profile: Assertive mode with request changes workflow enabled
• Auto Review: Enabled for all PRs to main branch, excluding drafts
• Auto Labeling: Automatically applies labels based on PR content and type
• Auto Assignment: Automatically assigns reviewers based on configuration
• Conventional Commits: Enforces conventional commit format in PR titles
• Issue Enrichment: Can automatically enhance issue bodies with implementation details

Path-Specific Review Instructions#

Different code areas receive specialized review focus:

• CLI commands (cmd/**/*.go): Focus on UX, error messages, and flag handling
• XML parsing (internal/parser/**/*.go): Emphasize OPNsense schema accuracy and streaming efficiency
• Validation (internal/validator/**/*.go): Ensure meaningful validation without over-engineering
• Data models (internal/model/**/*.go): Verify struct tags and OPNsense schema alignment
• Format conversion (internal/converter/**/*.go): Focus on markdown/JSON/YAML generation quality
• Security audit (internal/audit/**/*.go): Validate security checks and compliance rules

Integrated Analysis Tools#

CodeRabbit integrates with 51 analysis tools, including:

• golangci-lint (Go linting)
• shellcheck (shell script analysis)
• markdownlint (markdown validation)
• yamllint (YAML validation)
• gitleaks (secret detection)
• checkov (infrastructure as code security)
• semgrep (static analysis)
• actionlint (GitHub Actions validation)

Review Automation Stack#

The opnDossier repository relies on an integrated set of tools for automated code review, without a separate "pr-review-toolkit" component. The review automation stack consists of:

1. CodeRabbit - AI-assisted code review with security-focused checks
2. Mergify - Automated merge queue management and enforcement
3. GitHub Actions CI - Automated tests, linting, and validation
4. DCO App - Developer Certificate of Origin sign-off enforcement

CodeRabbit reviews are automatically triggered on PR creation/update, while maintainers can trigger enhancements via @coderabbitai mentions. Performance and simplicity reviews happen as part of the standard CodeRabbit review cycle.

Triage Guidelines#

Severity Classification#

Issues and review findings are classified using four severity levels:

• Critical/High → Error level (blocking)
• Medium → Warning level
• Low → Note level
• Info → Informational

Labeling System#

The project uses a multi-dimensional labeling system to organize work:

Priority Labels (explicit priority classification):

• priority:critical - Critical issues and urgent fixes
• priority:high - High priority issues and important features
• priority:normal - Standard work and regular features
• priority:low - Low priority enhancements

Type Labels (PR type classification):

• type:bug - Bug fixes and defect corrections
• type:feature - New functionality and enhancements

Category Labels (domain organization):

• performance - Performance optimizations and improvements
• refactoring - Code refactoring without functional changes (NOT tech-debt)
• Additional: go, dependencies, documentation, testing, ci, breaking_change, audit

Triage Decision Process#

Issues are triaged based on five criteria:

1. User impact: Does it affect production audit reports?
2. Core functionality: Is it a regression in existing features?
3. Silent failure risk: Does it hide errors or produce misleading output?
4. Compliance implications: Does it affect audit trail reliability?
5. Quick fix feasibility: Can it be resolved with clear, low-risk changes?

What to Fix Immediately#

• Potential issues: Security vulnerabilities, silent failures, data loss risks
• Correctness bugs: Logic errors, type mismatches, incorrect calculations
• Performance regressions: Timeouts, memory leaks, unnecessary allocations
• Breaking changes: API changes without migration path

Example: Issue #310 identified severity counters always returning zero due to type mismatch, causing misleading audit reports—an immediate fix priority.

What to Skip or Defer#

• Scope creep: Suggestions to add features unrelated to PR focus
• Over-engineering: Adding abstractions without clear benefit
• Nitpicks: Style preferences beyond linting rules
• False positives: Documented patterns that don't apply

Example: PR #216 removed unused depth-tracking infrastructure identified by CodeRabbit, but only after confirming it was truly unused—not all "unused code" suggestions are valid.

Common False Positive Patterns#

"Add Validation Methods"#

CodeRabbit often suggests adding validation methods to structs or functions when the context already provides sufficient validation. Developers should evaluate whether additional validation actually improves correctness or just adds complexity.

"Unused Context Parameter"#

Functions with context.Context parameters that don't explicitly use the context may still need it for:

• Future cancellation support
• Interface compliance
• Consistency with similar functions
• Third-party library requirements

"Consolidate Test Helpers"#

Suggestions to merge similar test helper functions often ignore important differences in:

• Setup requirements
• Cleanup behavior
• Error handling patterns
• Test isolation needs

GitHub Actions Security Alerts#

OpenSSF Scorecard identifies "unpinned dependencies" in GitHub Actions workflows when actions use mutable version tags like @v4 instead of SHA hashes. These are policy violations rather than actual vulnerabilities. The repository addresses these by pinning all actions to full SHA commits.

Documentation Coverage Gaps#

CodeRabbit may flag insufficient docstring coverage (e.g., 34.48% vs. required 80%). While legitimate, this is treated as documentation debt rather than a functional problem requiring immediate action on unrelated branches.

Review Scope and Focus#

Performance Review Findings#

Performance optimization suggestions should become GitHub issues, not immediate fixes on unrelated branches. This ensures:

• Proper tracking and prioritization
• Discussion of trade-offs before implementation
• Separate testing and validation cycles
• Clear commit history without mixed concerns

Example: PR #216 addressed benchmark timeout issues by identifying that running ALL benchmarks with -bench=. included slow parser tests generating 50MB+ files taking 37+ seconds each, with -count=3 tripling runtime unnecessarily. The fix reduced timeout to 5 minutes, used -count=1 and -benchtime=1s, and excluded parser/processor benchmarks from CI.

Simplicity Review Scope#

When reviewing a branch for simplicity, focus on commits after the base branch, not the full PR diff. This prevents:

• Suggesting refactors of unchanged upstream code
• Creating confusion about what's actually being changed in this PR
• Scope creep from reviewing existing technical debt

The review should evaluate:

• Are new changes simpler than they need to be?
• Is new complexity justified by requirements?
• Are new abstractions necessary?

Example: PR #234 added semaphore pattern for concurrency control—appropriate complexity for resource management, not over-engineering.

Duplicate Test Detection#

To identify duplicate tests covering the same scenario:

1. Grep for test functions: grep -r "func Test" **/*_test.go
2. Compare test names: Look for patterns like TestFoo vs TestFoo_Bar
3. Analyze test setup: Check if tests set up identical preconditions
4. Review assertions: Determine if tests verify the same behavior

Example test assertion specificity: PR #218 verified markdown output format, checking that cells contain [wan], #wan-interface, and separators like , —not just content presence.

Pull Request Workflow#

Pre-Submission Checklist#

Before submitting a PR:

1. All checks pass: just check (pre-commit hooks, linting, tests)
2. Commits signed off with DCO: git commit -s
3. Documentation updated for user-facing changes
4. Tests required for new functionality (>80% coverage)

Review Process#

Pull requests require:

• At least one review (human or CodeRabbit acknowledgment)
• All CI checks passing (golangci-lint, gofumpt, tests on stable and oldstable Go versions across three platforms, govulncheck, CodeQL, Trivy)
• Documentation updates if requested
• Acknowledging CodeRabbit findings is mandatory before merging

The documented review process includes:

1. Automated Review: CodeRabbit comments on PRs automatically
2. Human Review: At least one review required (human or CodeRabbit acknowledgment)
3. CI Validation: All checks must pass (lint, test, security scan)
4. Merge Queue: Mergify manages merge order and enforcement

Merge Queue System#

Mergify manages five author-specific merge queues, with automatic queueing configured via auto_merge_conditions in merge_protections_settings to automatically enqueue PRs when conditions match. The system uses distinct queues based on author type and file scope:

Queue 1 - dosubot:

• Author: dosubot[bot]
• Auto-approved via Mergify
• CI requirement: Lint only
• No manual approval needed

Queue 2 - dependabot-workflows:

• Author: dependabot[bot] with changes only in .github/workflows/
• Auto-approved via Mergify
• CI requirement: Lint only
• Handles dependency updates to workflow files with minimal testing

Queue 3 - dependabot:

• Author: dependabot[bot] with non-workflow changes
• Auto-approved via Mergify
• CI requirement: Full suite (Lint, Build, Test x6, Integration Tests, Coverage, codecov/project)
• Handles dependency updates affecting application code

Queue 4 - maintainer:

• Author: unclesp1d3r (sole maintainer)
• Queue when maintainer adds lgtm label (self-labeling workflow)
• CI requirement: Full suite (Lint, Build, Test x6, Integration Tests, Coverage, codecov/project)
• No separate approval required (addresses sole-maintainer self-merge scenario)

Queue 5 - external:

• Author: Anyone except dependabot[bot], dosubot[bot], or unclesp1d3r
• Requires maintainer approval (branch-protection-review-decision = APPROVED)
• CI requirement: Full suite (Lint, Build, Test x6, Integration Tests, Coverage, codecov/project)
• Must have approved review from maintainer before queueing

Merge Protections:

• Conventional commit format enforced for all PRs except bots (author != dependabot[bot] and author != dosubot[bot])
• CI requirements tiered based on author and file scope: lint-only for bot workflow PRs, full CI (including Test x6 across stable and oldstable Go versions) for all others
• PRs must be within 10 commits of base branch
• codecov/patch removed from merge gate due to unreliable status posting; coverage requirements enforced through GitHub Actions Coverage job

Pipeline v2 Compliance#

The complete merge criteria includes:

1. Pass all linters (golangci-lint)
2. Pass format checks (gofumpt, goimports)
3. Pass all tests with race detection and >80% coverage (codecov/project gate; codecov/patch removed from Mergify due to unreliable status posting)
4. Acknowledge CodeRabbit.ai findings ✅
5. Pass security gates (govulncheck, CodeQL, Trivy)
6. Use valid Conventional Commits
7. Pass license compliance (FOSSA)

Review Finding Examples#

Performance Optimization#

PR #216: Benchmark timeout reduction

• Finding: CI timing out after 15 minutes due to slow parser benchmarks
• Root cause: Running all benchmarks with -count=3, including tests generating 50MB+ files
• Resolution: Reduced timeout to 5 minutes, used -count=1, excluded slow benchmarks from CI
• Triage: Immediate fix (blocking CI)

PR #216: Unused depth-tracking removal

• Finding: Unused maxDepth field and related methods in Generator
• Resolution: Removed DefaultMaxDepth constant, field, and depth parameters
• Triage: Safe simplification (no functional impact)

Concurrency and Resource Management#

PR #234: Semaphore pattern for file operations

• Finding: Need to limit concurrent file operations to prevent resource exhaustion
• Resolution: Added buffered channel sized by CPU cores as semaphore
• Triage: Appropriate complexity (prevents real issues)

PR #234: Goroutine leak fix

• Finding: Goroutine leak in ProcessBatch when job submission fails
• Resolution: Track submitted job count and propagate submission errors
• Triage: Critical fix (memory leak)

Security and File Operations#

PR #234: File write safety

• Finding: File writes not guaranteed to persist on system crash
• Resolution: Call file.Sync() before Close() to flush data to disk
• Triage: Important correctness fix

PR #194: Configuration file permissions

• Finding: Configuration files created with default permissions
• Resolution: Use restrictive permissions (0600) for config files
• Triage: Security best practice

Silent Failures#

Issue #310: Severity breakdown always zero

• Finding: Severity counters in audit reports always showing zero
• Root cause: Type mismatch in severity comparison logic
• Impact: Misleading audit reports suggesting no issues found
• Triage: Critical fix (incorrect output)

Issue #296: Placeholder compliance checks

• Finding: Compliance check helpers returning true regardless of configuration
• Root cause: Placeholder implementation not replaced with actual validation
• Impact: False sense of compliance
• Triage: High priority fix

Documentation Accuracy#

PR #267: Documentation inaccuracies

• Finding: Fabricated API examples, non-existent CLI flags, wrong binary names
• Resolution: Removed incorrect examples, corrected license references, updated Go version
• Triage: Important (prevents user confusion)

Code Quality Improvements#

PR #234: Standard library usage

• Finding: Custom implementations of functionality in standard library
• Resolution: Use slices.Equal(), maps.Copy(), xml.EscapeText from stdlib
• Triage: Good simplification (reduces maintenance burden)

Development Standards#

Code Review Checklist#

PR template includes automated review findings:

Security: ✅ PASSED

• No hardcoded secrets or credentials
• No debug statements in production code

Code Quality: ✅ PASSED

• Thread-safe implementation (sync.Once)
• Proper error handling with fallbacks
• Cross-platform compatibility

TODO Comments: Track and document future work

• Mark with target version for removal

Conventional Commits#

The project uses Conventional Commits:

Format: <type>(<scope>): <description>

Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore

Examples:

feat(parser): add support for new XML schema
fix(config): resolve environment variable precedence
docs(readme): update configuration examples
chore(deps): update dependencies

Dependabot PRs use the chore(deps) prefix for standardized commit formatting.

Quality Standards#

All code must meet these requirements:

• All code must pass golangci-lint
• Tests required for new functionality (>80% coverage)
• Documentation updates for user-facing changes
• Follow Go best practices and project conventions
• All pre-commit checks must pass before submitting PR

Development Workflow Commands#

Comprehensive quality checks:

just test # Run tests
just lint # Run linters
just check # Run all pre-commit checks
just ci-check # Run comprehensive CI checks

Relevant Code Files#

Project Governance#

Decision-Making Process#

The project uses maintainer-driven governance:

• Bug fixes and minor changes: Any maintainer can review and merge
• New features: Discussed in GitHub issue before implementation; maintainer approval required
• Architecture changes: Require maintainer approval with rationale documented
• Breaking changes: Discussed in GitHub issue with community input

Roles#

• Maintainer: Merge PRs, manage releases, set project direction (@UncleSp1d3r)
• Security Contact: Triage vulnerability reports (support@evilbitlabs.io)
• Contributor: Anyone following the contributing guide

Related Topics#

• Pipeline v2 Compliance: Comprehensive quality assurance specification used by EvilBit Labs
• Conventional Commits: Standardized commit message format enforced by CodeRabbit
• OSSF Best Practices: Security and quality standards the project adheres to
• Developer Certificate of Origin (DCO): Sign-off requirement for all commits
• Mergify Automation: Merge queue management and enforcement system