Skip to content

chore: address deferred low-severity findings from validate-policy / test-policy review #235

Description

@yvonnedevlinrh

Context

During the review of #227 (feat: add validate-policy and test-policy CLI commands), several low-severity findings were identified and intentionally deferred to keep the PR focused. This issue tracks them.

Related: #227, #178, #87

Findings

L2: lintError JSON key is conditionally present

Location: cmd/complypack/cli/validate_policy.go:235–250 (writeValidatePolicyJSON)

The valid, syntaxErrors, contractViolations, and lintWarnings keys are always present in the JSON output, but lintError is only emitted when result.lintErr != nil. This inconsistency forces consumers to use existence-checking for one field while the others are guaranteed. The conditional-presence contract is also untested — TestWriteValidatePolicyJSON_Valid does not assert lintError is absent.

Suggested fix: Either always include lintError (as null when absent) for consistency, or document the conditional behavior and add a test asserting absence when nil.

L3: hasFailed and convertValidatePolicyResult lack direct tests

Location:

  • cmd/complypack/cli/test_policy.go:156 (hasFailed)
  • cmd/complypack/cli/validate_policy.go:170 (convertValidatePolicyResult)

Both unexported helpers contain meaningful logic:

  • hasFailed returns false when !r.TestsExecuted || r.Results == nil, otherwise checks Failed > 0 || len(Errors) > 0
  • convertValidatePolicyResult performs a data mapping that could silently lose fields

Neither has direct unit tests; both are only exercised implicitly through E2E tests. Table-driven tests would improve confidence in edge cases.

L4: Duplicate Rego fixture strings between CLI and MCP test packages

Location:

  • CLI: cmd/complypack/cli/helpers_test.go:22–30 (regoValidPolicyCLI, regoSyntaxErrorPolicyCLI, regoContractViolationPolicyCLI)
  • MCP: internal/mcp/tools_test.go:26–55 (regoValidPolicy, regoSyntaxErrorPolicy, regoContractViolationPolicy)

The Rego logic in each triplet is identical except for the package declaration. If the test policy logic changes, it must be updated in two places. A shared internal/testutil/ package with a helper that injects the package name could eliminate this duplication.

Acceptance Criteria

  • L2: lintError presence/absence behavior is documented or normalized, with test coverage
  • L3: hasFailed and convertValidatePolicyResult have direct table-driven unit tests
  • L4: Rego fixture duplication is reduced or explicitly justified with a code comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    Priority

    None yet

    Effort

    None yet

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions