CI Pipeline Gotchas#

Overview#

The opnDossier project implements the EvilBit Labs Pipeline v2 Specification with a comprehensive CI/CD pipeline designed to enforce local/CI parity—ensuring that quality checks behave identically in both development and continuous integration environments. The primary CI gate is executed via a single command, just ci-check, which orchestrates pre-commit hooks, format checking, linting, unit tests, and integration tests in a coordinated sequence.

The project employs a three-tier quality enforcement system: pre-commit hooks for file format validation and structural checks, golangci-lint for comprehensive Go code quality analysis with over 70 enabled linters, and CI validation for testing coverage and security scanning. Tasks are considered incomplete until ci-check passes successfully, establishing a strict quality gate before any code integration.

This article documents critical, project-specific CI/testing pitfalls that commonly cause build failures and developer confusion. These gotchas range from test parallelism restrictions to race detection false positives in asynchronous test infrastructure, subtle tool integration conflicts, test data management issues, configuration synchronization challenges, merge queue integration issues, and file-split refactoring complications. Understanding these patterns is essential for efficient development and successful CI pipeline navigation.

§1.1: t.Parallel() Forbidden in cmd/ Package Tests#

The cmd/ package uses package-level CLI flag globals (sharedDeviceType, sharedAuditMode, outputFile, format, force, etc.) that are mutated during test setup. When tests run in parallel using t.Parallel(), they risk data races when sibling tests mutate these shared globals simultaneously.

Root Cause#

CLI flag variables in the cmd/ package exist at package scope to integrate with Cobra's flag binding system. Multiple test functions within the same package may read or write these globals during setup (resetFlags()) or test execution. When tests execute in parallel, concurrent mutations create race conditions that the Go race detector identifies.

Race Detector Behavior#

The race detector reports these data races, but the stack traces often point to collateral access in unrelated utility functions rather than the actual conflicting test code. This makes debugging extremely difficult, as the reported race location may be far removed from the root cause (the parallel tests mutating shared flag globals).

Policy#

Blanket ban on t.Parallel() in cmd/ tests enforced by the forbidigo linter. This policy eliminates an entire class of data races by preventing parallel test execution in code that relies on package-level mutable state.

Enforcement#

The .golangci.yml configuration includes a forbidigo rule with path-except scoping to apply only to cmd/:

forbidigo:
  analyze-types: true
  forbid:
    - pattern: '^testing\.T\.Parallel$'
      msg: 'cmd/ tests must not call t.Parallel(): cmd uses package-level CLI flag globals that race under parallel execution. See GOTCHAS §1.1.'

The rule uses type-aware matching to catch t.Parallel() regardless of the local variable name (t, tt, tc, etc.). The path-except exclusion rule restricts enforcement to the cmd/ directory:

- path-except: 'cmd/.*\.go$'
  linters: [forbidigo]

Implementation Notes#

This PR removed 26 existing t.Parallel() violations across 5 cmd/*_test.go files and armed the forbidigo linter to prevent future violations. All removed parallel calls were replaced with inline comments referencing this gotcha:

// Do NOT use t.Parallel() — cmd package uses package-level flag globals.
// See GOTCHAS §1.1.

Reference#

For linter configuration details and rule scoping patterns, see the Linter Patterns documentation.

§1.1: Race Detector In Tests#

Race detection is mandatory with the -race flag on all test runs in the opnDossier pipeline. However, race detection can fail on async test infrastructure components such as spinners and progress bars, which are not indicative of production bugs. This represents a known limitation where the race detector's sensitivity produces false positives in test-specific UI components.

Root Cause#

The progress spinner implementation uses a background goroutine (go s.spin()) for animation rendering, with mutex protection for shared state such as the current frame and message text. The race detector can flag data access patterns in test environments where spinners are rapidly started and stopped, even though cleanup channels ensure proper goroutine shutdown. These detections occur because test scenarios may violate the expected lifecycle timing that production usage maintains.

Solution and Enforcement#

The just test-race task can be executed locally for race detection testing. When the race detector fires on progress bar or spinner components, developers should verify that the flagged code path is test infrastructure rather than production logic. The codebase includes explicit race condition tests for critical business logic paths, and the processor implementation includes explicit race protection with mutex locks where concurrent access is genuinely required.

Enforcement: The pre-push hook in .pre-commit-config.yaml runs just ci-check (which includes test-race) before every push. CI cannot host the race detector reliably on GitHub runners, so this local hook is the enforcement point—a regression that reintroduces a cmd/ data race will be caught on git push, not in CI. See CONTRIBUTING.md § Git Hooks: Commit vs Push for install steps and the --no-verify escape hatch.

Modernize Tool and //go Directive Conflicts#

The Go modernize tool automatically applies modern Go idiom upgrades to the codebase, such as converting ioutil calls to their os and io equivalents. However, the tool adds //go:fix inline directives that must be manually removed afterward because they conflict with the gocheckcompilerdirectives linter.

Correct Workflow#

When applying modernization fixes, developers must follow a three-step process:

1. Run the modernize tool to identify and apply fixes:

   ⧉

   go run golang.org/x/tools/go/analysis/passes/modernize/cmd/modernize@latest -test -fix ./...
   

2. Manually remove all //go:fix directives from the codebase using text search and replace

3. Run just lint to verify gocheckcompilerdirectives passes

Technical Explanation#

The gocheckcompilerdirectives linter is enabled to validate that only recognized Go compiler directives (such as //go:build, //go:embed, and //go:generate) appear in the codebase. The modernize tool's //go:fix is a non-standard directive used solely by the tool itself, causing linting failures when these annotations remain in committed code.

Prevention in CI#

The modernize-check task runs in CI via just lint, but operates in -test mode (without -fix) to detect modernization opportunities without automatically applying them. This prevents the introduction of problematic directives while still identifying outdated code patterns.

Test Helper File Naming and revive var-naming Rules#

The file test_helpers.go is not suffixed with _test.go, which has significant implications for linting enforcement. Because it is treated as a regular Go source file rather than a test file, revive var-naming linting rules apply. This prohibits underscores in exported function names such as Test_Function_Name, which would be acceptable in *_test.go files.

Correct Naming Patterns#

The actual test_helpers.go file demonstrates proper Go naming conventions:

• GetCommonTestCases() - camelCase, no underscores
• RunConverterTests() - camelCase, no underscores
• newFieldsTestDevice() - camelCase (private), no underscores
• assertNewFieldsPresent() - camelCase (private), no underscores

Test helper files that are not suffixed with _test.go are regular Go source files and must follow standard Go naming conventions enforced by the revive linter. The underscore-separated naming style commonly used in test function names (e.g., TestParser_InvalidInput) is only permitted within actual *_test.go files.

Golden File Regeneration and complete.json#

The project uses golden file tests via sebdah/goldie/v2 for snapshot-based testing of generated output. When the complete.json test data file is modified, all golden files that reference this test data must be regenerated to reflect the updated output.

Regeneration Procedure#

Golden files are regenerated using the following command:

go test ./internal/converter -run TestGolden -update

This updates all golden file snapshots in internal/converter/testdata/golden/. The -update flag triggers regeneration mode, overwriting existing golden files with current test output.

Critical Implementation Detail#

Golden file tests use builder options for deterministic output. Dynamic values (timestamps, versions) are injected at construction time via builder options (builder.WithGeneratedTime, builder.WithVersion) so that goldie can compare bytes directly without post-hoc normalization. This eliminates the need for regex normalization helpers and ensures deterministic golden file generation across machines and build environments.

Impact Scope#

complete.json serves as comprehensive test data and is used extensively across:

• Benchmark tests for medium-sized configuration testing
• Golden tests for standard and comprehensive report generation
• Integration tests for complete configuration scenarios
• JSON/YAML golden tests for both redacted and unredacted output

Best Practices#

Developers should always review diffs after regenerating golden files using git diff internal/converter/testdata/golden/ to verify that changes are intentional and do not indicate regressions. The pre-commit hooks exclude .golden.md files from markdown formatting to preserve exact byte-for-byte comparisons. Tests should never be run with the -update flag in CI environments; CI should only validate against existing golden files to detect unexpected output changes.

Map Iteration Output Testing and Non-Determinism#

Go maps exhibit non-deterministic iteration order by language design. When testing output that involves map iteration, exact string equality comparisons will fail intermittently depending on the runtime's internal map structure, leading to flaky tests.

Solution Pattern#

The codebase consistently uses slices.Sorted(maps.Keys(...)) to ensure deterministic output across all map iterations:

Example from cmd/audit_handler.go:

// Add compliance plugin results if available (deterministic order)
for _, pluginName := range slices.Sorted(maps.Keys(report.Compliance)) {
    result := report.Compliance[pluginName]
    // ...
}

Example from internal/model/opnsense/converter.go:

for _, key := range slices.Sorted(maps.Keys(items)) {
    iface := items[key]
    // ...
}

Documentation in Code#

Code comments explicitly acknowledge non-determinism:

// Sort by ID for deterministic output (map iteration order is non-deterministic)
sort.Slice(reorders, func(i, j int) bool {
    return reorders[i].ID < reorders[j].ID
})

Testing Recommendations#

For tests involving maps, developers should:

• Use strings.Contains() for substring matching instead of exact equality when order doesn't matter semantically
• Sort map keys before iteration for both production and test code
• Prefer golden file tests with deterministic fixtures that use fixed timestamps and sorted output

//nolint Directive Placement and gofumpt Stripping#

//nolint: directives must be placed on a separate line above the code they apply to because inline comments are stripped by gofumpt formatting. This is a critical requirement that prevents linter suppression comments from being inadvertently removed during automated formatting.

Correct vs. Incorrect Placement#

Correct - Separate Line:

//nolint:gosec // This is test code, the binary path is controlled by the test
cmd := exec.CommandContext(ctx, "go", "build", "-o", s.binaryPath, ".")

(build_test.go:48)

Incorrect - Inline (gets stripped):

var configShowJSONOutput bool //nolint:gochecknoglobals // Cobra flag variable

Technical Explanation#

gofumpt is configured with extra-rules: true, providing stricter formatting rules than standard gofmt. This aggressive formatting behavior strips inline comments on certain declarations, particularly variable declarations and struct field definitions. Placing nolint directives on a separate preceding line ensures they survive formatting operations.

Common Usage Patterns#

The codebase demonstrates several established patterns:

• Test security exceptions: //nolint:gosec // Test file permissions are fine for testing
• Intentional duplication: //nolint:dupl // tunnel converter tests are structurally similar by design
• Parallel test exceptions: //nolint:tparallel // subtests share manager state from parent setup

Benchmark Timeout Issues with Large Files#

Benchmarks operating on large files can hang for extended periods, potentially blocking CI pipeline completion. The CI configuration employs timeout-minutes and continue-on-error: true settings to prevent benchmark hangs from delaying or blocking pull request merges.

Benchmark Performance Constraints#

Parser benchmarks enforce specific performance constraints:

// Ensure each parse completes in reasonable time (target < 2s as specified)
if duration > 2*time.Second {
    b.Errorf("Parse took %v, expected < 2s", duration)
}

Additionally, memory growth checks run every 10 iterations with an O(1) constraint requiring less than 10 MB growth, ensuring the streaming parser maintains constant memory usage.

Build Tag Isolation#

Legacy benchmarks use the //go:build legacy_bench build tag with a relaxed 5-second timeout (compared to 2 seconds for the streaming parser). This isolation pattern prevents slower legacy benchmarks from running in standard test suites.

Stress Test Timeouts#

Stress tests implement explicit 30-second timeouts:

timeout := time.After(30 * time.Second)
for completed.Load() < numJobs-errCount.Load() {
    select {
    case result := <-wp.Results():
        // process result
    case <-timeout:
        t.Fatalf("timeout: completed %d of %d jobs", ...)
    }
}

This pattern ensures that tests fail fast with clear timeout messages rather than hanging indefinitely.

CI Strategy#

The Performance Benchmarks job is configured as non-blocking with continue-on-error: true to prevent benchmark hangs or performance regressions from blocking PR merges. This allows the team to investigate performance issues asynchronously without disrupting development velocity.

Package Renames and Pre-commit Config Exclude Paths#

When renaming directories or packages, developers must update exclude paths in .pre-commit-config.yaml to match the new structure. Failure to do so can result in pre-commit hook failures or unintended validation of files that should be excluded.

Current Exclusion Patterns#

XML testdata exclusion:

- id: check-xml
  exclude: ^internal/cfgparser/testdata/

Golden markdown file exclusion:

types: [markdown]
exclude: \.golden\.md$

Rationale for Exclusions#

These exclusions exist for specific technical reasons:

• XML testdata may contain intentionally malformed XML for error handling and parser robustness testing
• Golden markdown files must not be reformatted as they serve as byte-for-byte test fixtures

Package Rename Checklist#

When renaming packages, developers should:

1. Update all exclude paths in .pre-commit-config.yaml to reflect new directory structures
2. Update import paths in all Go source files
3. Update documentation references and comments
4. Run just ci-check to verify all quality gates pass with the new structure

Mergify Merge Queue CI Integration#

The project uses Mergify merge queues for automated PR merging with five author-specific queues (dosubot, dependabot-workflows, dependabot, maintainer, external), each with tiered CI requirements. Misconfiguration of check names or queue conditions can cause PRs to hang indefinitely in the merge queue.

CI Check Name Matching#

Mergify check-success conditions must match the name: field in GitHub workflow job definitions, NOT the job ID. This is a critical distinction that causes silent failures when configured incorrectly.

Correct - uses job name:

merge_conditions:
  - check-success = Lint
  - check-success = Build
  - "check-success = Test (ubuntu-latest, stable)"
  - "check-success = Test (ubuntu-latest, oldstable)"
  - "check-success = Test (macos-latest, stable)"
  - "check-success = Test (macos-latest, oldstable)"
  - "check-success = Test (windows-latest, stable)"
  - "check-success = Test (windows-latest, oldstable)"
  - check-success = codecov/project

Incorrect - uses job ID (will never match):

merge_conditions:
  - check-success = lint # Wrong: this is the job ID
  - check-success = build # Wrong: this is the job ID
  - check-success = test # Wrong: this is the job ID

Matrix Job Check Names#

Check names with matrix dimensions use the format Job Name (matrix-value). The CI test job runs across six combinations of three platforms (ubuntu-latest, macos-latest, windows-latest) and two Go versions (stable, oldstable):

- "check-success = Test (ubuntu-latest, stable)"
- "check-success = Test (ubuntu-latest, oldstable)"
- "check-success = Test (macos-latest, stable)"
- "check-success = Test (macos-latest, oldstable)"
- "check-success = Test (windows-latest, stable)"
- "check-success = Test (windows-latest, oldstable)"

The check name includes both matrix dimensions in parentheses, comma-separated. Incorrect check names will cause PRs to hang in the merge queue indefinitely, as Mergify waits for checks that will never report.

DCO Sign-off is NOT a CI Check#

DCO sign-off is enforced by a GitHub App, not a CI workflow. Do not add check-success = DCO to Mergify conditions—this check name does not exist. The GitHub App automatically blocks PRs with unsigned commits at the branch protection level before they reach the merge queue.

Tiered CI Requirements#

The .mergify.yml configuration uses conditional merge protections to enforce different CI requirements based on PR author and file scope:

• Bot workflow PRs (dosubot, dependabot with only .github/workflows/ changes): require only Lint
• All other PRs (code changes, non-bot authors): require full CI suite (Lint, Build, six Test checks (stable and oldstable Go versions on three platforms), Integration Tests, Coverage, codecov/project)

Bot workflow-only PRs use the dependabot-workflows queue with a file pattern condition:

queue_conditions:
  - author = dependabot[bot]
  - "-files ~= ^(?!\\.github/workflows/)" # No files outside workflows directory

This pattern ensures that dependency updates to GitHub Actions workflows merge quickly without waiting for full test suite execution.

Autoqueue Behavior#

Automatic queueing is configured via auto_merge_conditions in merge_protections_settings, which reflects the queue_conditions of queue rules that previously used the deprecated autoqueue: true field. PRs are automatically enqueued when the conditions in auto_merge_conditions match. No explicit queue action is needed in pull_request_rules. Multiple queues can be active simultaneously—PRs are routed to the correct queue based on author and file scope.

The pull_request_rules section focuses on auto-approval for trusted bot accounts rather than explicit queueing:

- name: Auto-approve dosubot PRs
  conditions:
    - base = main
    - author = dosubot[bot]
  actions:
    review:
      type: APPROVE

Conventional Commit Enforcement#

Merge protections enforce conventional commit format for non-bot authors:

- name: Enforce conventional commit
  if:
    - base = main
    - author != dependabot[bot]
    - author != dosubot[bot]
  success_conditions:
    - "title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.+\))?:"

Bots (dependabot[bot], dosubot[bot]) are exempt from this check. Manual merges that bypass the queue will still be blocked by merge protections, ensuring consistent quality enforcement.

File-Split Refactoring and Pre-commit Hook Interference#

When splitting large files into domain-specific files within the same package, pre-commit hooks (gofumpt, linters) can silently rearrange helpers back into the original file, creating a different layout than intended. Additionally, changing unexported function signatures during the split can break test files that call those functions directly.

Scenario and Impact#

When a developer splits a large file (e.g., internal/validator/opnsense.go at 1,061 lines, 32% over the 800-line project maximum) into domain-specific files (validate_system.go, validate_network.go, validate_security.go), two issues can occur:

1. Hook rearrangement: After commit, pre-commit formatters may consolidate domain-specific helpers back into the orchestrator file (opnsense.go), defeating the purpose of the refactor
2. Test breakage: Unexported function signature changes break same-package test files that call those functions directly

The intended file organization is lost, potentially violating project file size limits and creating unexpected test failures.

Detection Indicators#

• Line counts don't match expectations after commit
• go build reports duplicate declarations
• Files contain functions that should have been moved to other files
• Tests fail with "not enough arguments" or type mismatch errors on unexported functions

Root Cause#

gofumpt with extra-rules: true and linters run via pre-commit hooks can reformat and reorder code based on their own heuristics. When both the orchestrator file and new domain files are modified simultaneously, the formatter may consolidate related declarations back into the larger file. Additionally, Go test files in the same package can call unexported functions directly, creating tight coupling that breaks when signatures change.

Prevention Strategies#

Following the file-split refactoring pattern documented in AGENTS.md section 5.26, developers should:

1. Always re-verify file contents after pre-commit hooks run:

   ⧉

   wc -l internal/package/*.go # Check line counts match intent
   go build ./internal/package/ # Verify no duplicate declarations
   

2. Before changing any unexported function signature, grep for call sites:

   ⧉

   grep -rn 'functionName(' internal/package/*_test.go
   

3. Run just ci-check after every file-split refactor to catch duplicate declarations, test failures, and lint issues

4. Follow the established naming convention: <base>_<domain>.go (e.g., validate_system.go, report_statistics.go)

Example: Signature Change Impact#

When optimizing validateInterface to accept a pre-computed map[string]struct{} instead of *schema.Interfaces, test call sites required updating:

// Before (old signature)
errors := validateInterface(&tt.iface, "test", interfaces)

// After (new signature with pre-computed map)
errors := validateInterface(&tt.iface, "test", collectInterfaceNames(interfaces))

Precedent and Documentation#

This pattern follows established examples:

• PR #415: report.go file split
• PR #417: opnsense.go file split
• pkg/parser/opnsense/ converter_*.go files

Detailed guidance is provided in docs/solutions/architecture-issues/file-split-refactor-gotchas.md.

Additional CI Configuration Details#

CI Check Command Structure#

The just ci-check command chains five sequential quality gates:

ci-check: check format-check lint test test-integration
    @echo "✅ All CI checks passed"

Each constituent task performs a distinct validation:

• check: Executes all pre-commit hooks on all files
• format-check: Validates code formatting without modifying files
• lint: Runs golangci-lint plus modernize-check analysis
• test: Executes unit tests
• test-integration: Runs integration tests with build tags

golangci-lint Configuration#

The project employs 70+ enabled linters with a 30-minute timeout, indicating comprehensive analysis. Notable configuration highlights include:

• Strategic gosec exclusions: G301 (man page permissions) and G304 (template loading) for legitimate use cases
• gochecknoglobals disabled to accommodate Cobra command-line framework patterns
• Complexity thresholds: cyclop max-complexity 50, package-average 10.0

Pre-commit Hook Framework#

Pre-commit v6.0.0 provides comprehensive validation including:

• File format validation (JSON, YAML, XML, TOML)
• Markdown formatting with extensive plugins for GitHub Flavored Markdown, admonitions, footnotes, and more
• GitHub Actions workflow validation via actionlint
• Line ending normalization for cross-platform consistency

Development Policy#

The project maintains a pre-existing issue policy requiring developers to fix pre-existing issues encountered during work (race conditions, bugs, etc.) rather than dismissing them as "not our problem." This ensures continuous quality improvement and prevents technical debt accumulation.

Summary of Gotchas#

Relevant Code Files#

Related Topics#

• Pipeline v2 Specification - The architectural specification defining the CI/CD pipeline structure
• Contributing Guide - Developer guidelines and contribution workflow
• Development Architecture - High-level architectural documentation