diff --git a/.claude/task-boards/feature-change-safety-testability-refactors.md b/.claude/task-boards/feature-change-safety-testability-refactors.md deleted file mode 100644 index ef939f0..0000000 --- a/.claude/task-boards/feature-change-safety-testability-refactors.md +++ /dev/null @@ -1,656 +0,0 @@ -# Task board: feature/change-safety-testability-refactors - -Status: final gate prep -Branch: feature/change-safety-testability-refactors -Last updated: 2026-07-27 -Not final product docs: this is implementation planning for the branch, not shipped user-facing documentation. - -## Branch setup update - -Completed before implementation work: - -- Rebased `feature/change-safety-testability-refactors` onto latest `main` after `feature/production-reliability-data-readiness` was merged. -- Force-updated the remote branch with `--force-with-lease` after the rebase. -- Verified the working tree is clean before assigning implementation workstreams. - -Agent spawning note: - -- New worker spawns were attempted after the rebase, but the workspace was at the agent thread limit. -- Existing completed-agent summaries were collected and used to refine the workstream split below. -- The branch can proceed with these workstreams as soon as agent capacity is available, or the main thread can take the first workstream locally. -- Completed old agent threads were closed after Workstream A landed, freeing slots for the next implementation round. -- Worker handoffs have landed and this board has been reconciled against the implemented rule IDs, tests, and docs in the final parity audit below. - -## Final parity audit - -Audited on 2026-07-27 against `internal/codeguard/rules/catalog_change_safety.go`, `internal/codeguard/rules/catalog_fix_templates_change_safety.go`, `internal/codeguard/checks/change/*`, `internal/codeguard/checks/quality/quality_precision.go`, `internal/codeguard/runner/pr_summary.go`, `tests/checks/*change*`, `tests/checks/*testability*`, `tests/checks/*precision*`, `tests/checks/*maintainability*`, `internal/codeguard/runner/pr_summary_test.go`, and the SDK metadata tests. - -Implemented detector subset in the current worktree: - -- `change.oversized-diff` -- `change.mixed-concerns` -- `change.too-many-concerns` -- `change.mixed-refactor-and-behavior` -- `change.unnecessary-surface-area` -- `change.one-use-abstraction` -- `change.duplicate-helper` -- `change.cleanup-regression` -- `change.complexity-increased` -- `change.move-without-verification` -- `testing.behavior-change-without-test` -- `testing.failure-path-missing` -- `testing.hardwired-dependency` -- `testing.nondeterministic-domain-logic` -- `naming.generic-identifier` -- `function.excessive-parameters` -- `function.mixed-abstraction-level` -- `function.command-query-mix` -- `error.logged-and-ignored` -- `error.context-lost` -- `defensive.unchecked-type-assertion` -- `defensive.unsafe-numeric-conversion` -- `maintainability.public-surface-growth` -- `maintainability.dependency-growth` -- `maintainability.hotspot` -- `maintainability.high-churn-hotspot` -- `maintainability.repeat-defect-area` -- `maintainability.unstable-interface` -- `maintainability.change-amplification` -- `smell.shotgun-surgery-history` -- `smell.divergent-change-history` -- `pr_summary.change_safety` -- `pr_summary.maintainability_delta` -- `pr_summary.refactor_confidence` - -Catalog/config/deferred IDs for this branch: - -- `testing.legacy-hotspot-uncovered`: cataloged/configured, intentionally non-emitting without reliable history/hotspot inputs. -- `refactor.*`: direct detector code and `tests/checks/refactor_test.go` are present and green in the current implementation. `pr_summary.refactor_confidence` rolls up `refactor.*` findings plus implemented mixed-refactor/move-without-verification signals. - -Metadata/doc parity: - -- Every built-in rule in the branch catalog has explicit language coverage through the rule metadata helpers. -- Every branch catalog rule has a populated guided fix template. -- `docs/checks.md` and `docs/features.md` distinguish implemented detectors from catalog/planned IDs so planned-only behavior is not described as shipped. -- `examples/codeguard.json` was updated for the final `change_rules` config surface after concurrent config changes added direct refactor left-behind toggles. - -Current gate blocker: - -- None known after the safe-refactor worker landed. Final branch gates still need to run on the quiescent branch before PR handoff. -- Full-parity worker assignments after MVP landed: - - Einstein (`019fa485-4e06-73e1-9cd8-67592526456d`): Phase 3 safe-refactor detectors. - - Nietzsche (`019fa485-8175-7540-9df5-dc0ca89ac3bd`): remaining Phase 2 change-smell detectors. - - McClintock (`019fa485-b88b-71c0-a426-53fc5cee95a4`): Phase 6 history-aware maintainability and smell signals. - - Hume (`019fa485-f437-74b1-b617-62805c7ab20a`): docs/task-board parity audit and final checklist. - -## Agent workstreams - -These workstreams are intentionally disjoint. Workers must not revert unrelated edits and should list changed files in their handoff. - -### Workstream A: scaffolding, config, catalogs, and profiles - -Status: complete in main thread; implementation committed/pushed separately from detector work. - -Ownership: - -- `internal/codeguard/core/config_types.go` -- `internal/codeguard/core/config_rule_types.go` -- `internal/codeguard/config/defaults*.go` -- `internal/codeguard/config/example*.go` -- `internal/codeguard/config/profile.go` -- `internal/codeguard/config/validate*.go` -- `internal/codeguard/rules/catalog_change_safety.go` -- `internal/codeguard/rules/catalog_fix_templates_change_safety.go` -- `pkg/codeguard/sdk_types_config_checks.go` -- config/profile/metadata tests - -Tasks: - -- Add a minimal top-level `change` section toggle and `ChangeRulesConfig`. -- Add thresholds for changed files, changed directories, changed lines, changed public interfaces, concern-family count, and production/test ratio. -- Add defaults, examples, validation, and SDK aliases. -- Add initial metadata/fix templates for the Phase 1/2/4 rules that will have detector support in this branch. -- Wire profile behavior: - - `startup`: keep change-safety off unless explicitly enabled. - - `strict`: enable high-confidence change/testability gates. - - `enterprise`: inherit strict. - - `ai-safe`: enable stronger oversized-diff, missing-test, weak-refactor-confidence, duplicated-helper, and unnecessary-abstraction signals. - -Targeted verification: - -```sh -go test ./internal/codeguard/config ./tests/cli ./pkg/codeguard -go test ./internal/codeguard/... ./pkg/codeguard ./tests/cli -``` - -### Workstream B: change section and diff concentration detectors - -Status: complete for the Phase 1/2 detector subset and cleanup-style change-smell detectors. - -Ownership: - -- `internal/codeguard/checks/change/**` -- `internal/codeguard/runner/checks/registry.go` -- `tests/checks/change*_test.go` -- helper additions under `internal/codeguard/checks/support/**` only if needed - -Tasks: - -- Add the `Change Safety` section runner. -- Run primarily in diff mode; full scans should no-op or emit only explicitly safe repo-level diagnostics. -- Compute change concentration evidence: - - files touched - - directories touched - - architectural layer/path categories touched - - production/test file ratio - - public-surface file hints -- Detect: - - `change.oversized-diff` - - `change.mixed-concerns` - - `change.too-many-concerns` - - `change.mixed-refactor-and-behavior` - - `change.unnecessary-surface-area` - - `change.move-without-verification` -- Keep findings deterministic and confidence-based. - -Targeted verification: - -```sh -go test ./tests/checks -run 'TestChange' -go test ./internal/codeguard/runner/checks -``` - -### Workstream C: testability detectors - -Status: complete for behavior-change, failure-path, hardwired-dependency, and nondeterministic-domain detectors; legacy-hotspot emission deferred until reliable history inputs are available. - -Ownership: - -- `internal/codeguard/checks/change/testability*.go` or a clearly named sibling under the change package -- `tests/checks/testing*_test.go` -- no catalog/config edits except small integration adjustments coordinated with Workstream A - -Tasks: - -- Detect: - - `testing.behavior-change-without-test` - - `testing.failure-path-missing` - - `testing.hardwired-dependency` - - `testing.nondeterministic-domain-logic` - - `testing.legacy-hotspot-uncovered` as warn-only if history inputs are available; otherwise leave a documented TODO and do not emit a misleading finding. -- Start with Go, Python, TypeScript, JavaScript, and C++ path/text heuristics where safe. -- Add positive and negative tests for changed production files with/without changed tests. -- Avoid duplicating CI test-quality findings unless the evidence is about change safety, not test style. - -Targeted verification: - -```sh -go test ./tests/checks -run 'TestTesting' -``` - -### Workstream D: PR-summary metrics - -Status: complete for additive artifact fields and deterministic finding-family rollups. - -Ownership: - -- `internal/codeguard/core/report_artifact_types.go` -- `internal/codeguard/checks/support/artifacts.go` -- `internal/codeguard/runner/pr_summary.go` -- `internal/codeguard/runner/pr_summary_test.go` -- `pkg/codeguard/sdk_types_runtime_report.go` -- report serialization tests only if artifact shape requires them - -Tasks: - -- Extend existing `pr_summary` additively with: - - `change_safety` - - `maintainability_delta` - - `refactor_confidence` -- Preserve existing `production_risk` behavior from the merged production-readiness branch. -- Keep metrics artifact-only; do not emit GitHub annotations for metrics. -- Keep the existing text `Summary:` sentence unchanged. -- Sort evidence deterministically. - -Targeted verification: - -```sh -go test ./internal/codeguard/runner ./tests/codeguard ./tests/checks -run 'Test.*PRSummary|TestWriteReport' -``` - -### Workstream E: local quality precision and maintainability delta - -Status: complete for the small high-value subset plus history-aware maintainability/smell detectors that degrade gracefully when git history is unavailable. - -Ownership: - -- `internal/codeguard/checks/quality/**` -- `internal/codeguard/checks/design/**` only for graph/delta helpers -- `internal/codeguard/history/**` only for read-only history metrics -- `tests/checks/naming*_test.go` -- `tests/checks/function*_test.go` -- `tests/checks/error*_test.go` -- `tests/checks/defensive*_test.go` -- `tests/checks/maintainability*_test.go` - -Tasks: - -- Start with a small, high-value subset instead of every planned smell: - - `naming.generic-identifier` - - `function.excessive-parameters` - - `function.mixed-abstraction-level` - - `function.command-query-mix` - - `error.logged-and-ignored` - - `error.context-lost` - - `defensive.unchecked-type-assertion` - - `defensive.unsafe-numeric-conversion` - - `maintainability.public-surface-growth` - - `maintainability.dependency-growth` -- Reuse existing quality/design metrics where possible. -- Prefer warnings unless evidence is direct and high-confidence. - -Targeted verification: - -```sh -go test ./tests/checks -run 'Test(Naming|Function|Error|Defensive|Maintainability)' -``` - -## Goal - -Make CodeGuard evaluate whether a PR is safe, incremental, understandable, testable, and actually improves the code it touches. - -This branch owns change-quality, testability, safe-refactor, code-smell, naming/function/error/defensive-programming, and maintainability-delta work. The product target is to answer: - -> Did this PR make the system safer, simpler, easier to change, and less likely to fail? - -## Non-goals - -- Do not implement reliability/data-outage rules owned by `feature/production-reliability-data-readiness`. -- Do not implement observability, ownership, runbook, or deployment-governance rules owned by `feature/operability-design-delivery-governance`. -- Do not overfit one language/framework. Start with the languages where CodeGuard already has parser coverage and tests. -- Do not claim semantic equivalence for refactors. The goal is confidence and evidence, not proof. - -## Product split - -This branch owns: - -- Rule families: `testing.*`, `change.*`, `refactor.*`, `smell.*`, `maintainability.*`, `naming.*`, `function.*`, `error.*`, and `defensive.*`. -- Product metrics in the shared `pr_summary` artifact: - - `change_safety` - - `maintainability_delta` - - `refactor_confidence` -- Diff/history-aware analysis inputs: - - change concentration score; - - behavior-preservation evidence; - - hotspot/change-history signals; - - ratio of production changes to test changes. - -Adjacent branch contracts: - -- `feature/production-reliability-data-readiness` owns `production_risk` and may consume `error.*` or `defensive.*` signals later if they indicate outage risk. -- `feature/operability-design-delivery-governance` owns design abstraction and delivery governance signals but can feed maintainability/risk deltas later. - -## Existing repo seams to reuse - -- Quality and complexity rules: `internal/codeguard/checks/quality/*`, `internal/codeguard/rules/catalog_quality.go`, `catalog_quality_ai.go`. -- CI/test-quality rules: `internal/codeguard/checks/ci/*`, `internal/codeguard/rules/catalog_test_quality.go`. -- Design change-impact helpers: `internal/codeguard/checks/design/design_change_impact.go`. -- Diff support: `internal/codeguard/runner/support/diff_scope.go`, `internal/codeguard/runner/support/changed_files.go`, `internal/codeguard/core/diff_types.go`. -- Risk scoring/postprocessors: `internal/codeguard/runner/risk_scoring.go`; add new PR-summary logic near it. -- History support: `internal/codeguard/history/*`, `internal/codeguard/runner/runner_history.go`, `internal/codeguard/runner/support/legibility_history.go`. -- Rule metadata and fix templates: `internal/codeguard/rules/catalog*.go`, `internal/codeguard/rules/catalog_fix_templates*.go`. -- Report/artifact schema: `internal/codeguard/core/report_artifact_types.go`, `pkg/codeguard/sdk_types_runtime_report.go`. - -## Rule inventory - -This inventory is the branch catalog and planning map. It is not a shipped-detector list. The final parity audit above is the source of truth for which IDs currently emit findings. - -### Testability and change safety - -- `testing.behavior-change-without-test` -- `testing.failure-path-missing` -- `testing.hardwired-dependency` -- `testing.nondeterministic-domain-logic` -- `testing.legacy-hotspot-uncovered` -- `change.mixed-concerns` -- `change.oversized-diff` -- `change.mixed-refactor-and-behavior` -- `change.too-many-concerns` -- `change.unnecessary-surface-area` -- `change.one-use-abstraction` -- `change.duplicate-helper` -- `change.cleanup-regression` -- `change.complexity-increased` -- `change.move-without-verification` - -### Safe refactors - -- `refactor.behavior-change-detected` -- `refactor.public-contract-changed` -- `refactor.test-coverage-reduced` -- `refactor.error-path-changed` -- `refactor.side-effect-order-changed` -- `refactor.visibility-expanded` -- `refactor.dependency-direction-worsened` -- `refactor.duplicate-implementation-left-behind` -- `refactor.dead-path-left-behind` - -### Code smells and maintainability - -- `smell.god-object` -- `smell.feature-envy` -- `smell.shotgun-surgery` -- `smell.divergent-change` -- `smell.middle-man` -- `smell.message-chain` -- `smell.inappropriate-intimacy` -- `smell.parallel-inheritance` -- `smell.data-clump` -- `smell.primitive-obsession` -- `smell.switch-on-type` -- `smell.refused-bequest` -- `smell.shotgun-surgery-history` -- `smell.divergent-change-history` -- `maintainability.high-churn-hotspot` -- `maintainability.repeat-defect-area` -- `maintainability.unstable-interface` -- `maintainability.ownership-gap` -- `maintainability.regression` -- `maintainability.no-improvement-in-hotspot` -- `maintainability.public-surface-growth` -- `maintainability.dependency-growth` -- `maintainability.duplication-growth` -- `maintainability.nesting-growth` -- `maintainability.testability-regression` -- `maintainability.hotspot` -- `maintainability.change-amplification` -- `maintainability.unstable-dependency` -- `maintainability.low-test-isolation` -- `maintainability.excessive-public-surface` -- `maintainability.architecture-drift` -- `maintainability.missing-owner` -- `maintainability.missing-design-context` -- `maintainability.repeat-regression` -- `maintainability.operational-opacity` - -### Naming, functions, errors, and defensive programming - -- `naming.generic-identifier` -- `naming.behavior-mismatch` -- `naming.boolean-not-predicate` -- `naming.domain-vocabulary-drift` -- `naming.unknown-abbreviation` -- `naming.cardinality-mismatch` -- `naming.implementation-leak` -- `naming.missing-unit` -- `naming.role-suffix-overuse` -- `naming.cross-layer-inconsistency` -- `function.excessive-length` -- `function.excessive-branching` -- `function.excessive-nesting` -- `function.excessive-parameters` -- `function.excessive-returns` -- `function.hidden-mutation` -- `function.mixed-abstraction-level` -- `function.command-query-mix` -- `function.inconsistent-return-contract` -- `function.multiple-responsibilities` -- `function.orchestration-domain-mix` -- `function.control-flow-needs-explanation` -- `function.name-behavior-mismatch` -- `function.partial-result` -- `error.swallowed` -- `error.logged-and-returned` -- `error.logged-and-ignored` -- `error.context-lost` -- `error.generic-message` -- `error.wrong-abstraction-level` -- `error.inconsistent-wrapping` -- `error.sentinel-comparison-fragile` -- `error.retryable-not-distinguished` -- `error.user-message-leaks-internals` -- `error.partial-failure-hidden` -- `error.cleanup-error-ignored` -- `error.fallback-hides-corruption` -- `error.panic-on-recoverable-path` -- `error.exception-used-for-control-flow` -- `defensive.unvalidated-boundary-input` -- `defensive.invalid-state-representable` -- `defensive.null-assumption` -- `defensive.unchecked-type-assertion` -- `defensive.unsafe-numeric-conversion` -- `defensive.integer-overflow` -- `defensive.bounds-assumption` -- `defensive.unsafe-default` -- `defensive.non-exhaustive-branch` -- `defensive.unchecked-external-response` -- `defensive.missing-schema-validation` -- `defensive.missing-resource-limit` -- `defensive.invalid-state-transition` -- `defensive.fail-open-authorization` - -## Implementation phases - -### Phase 0: Choose rollout shape - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Done | Decide whether to extend existing sections or add new sections | `core/config_types.go`, `runner/checks/registry.go` | config + section tests | Added `checks.change` for diff/testability and kept local naming/function/error/defensive/maintainability precision in `Code Quality`. | -| Done | Define confidence policy | rule metadata + docs | report confidence tests | Implemented findings carry explicit confidence; docs tell users to treat medium-confidence heuristics as review cues. | -| Done | Define profile behavior | `internal/codeguard/config/profile.go` | profile tests | Startup leaves change off; strict/enterprise enable it; AI-safe enables it with tighter diff/test-ratio budgets. | -| Done | Define shared PR-summary artifact contract | `core/report_artifact_types.go` | report serialization tests | Additive `pr_summary` fields landed; metrics remain artifact-only and do not create GitHub annotations. | - -### Phase 1: Add change-analysis infrastructure - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Done | Add `ChangeRulesConfig` | `core/config_rule_types.go`, `core/config_types.go` | config tests | Thresholds landed: max files, dirs, public interfaces, changed lines, concern families, and min test/prod ratio. | -| Done | Add defaults/examples/validation | `config/defaults*.go`, `config/example*.go`, config validation | `go test ./internal/codeguard/config ./tests/codeguard` | Defaults and validation landed; example config still reflects the full config surface. | -| Done | Add change section package | `internal/codeguard/checks/change/change.go` | `tests/checks/change_test.go` | Diff-mode section landed; full scans no-op. | -| Done | Register section | `runner/checks/registry.go` | section smoke test | Registered as a first-class check family. | -| Done | Add rule catalog/fix templates | `rules/catalog_change_safety.go`, `catalog_fix_templates_change_safety.go` | metadata tests | Branch catalog has explicit language coverage and populated fix templates. Some IDs are catalog/planned only. | -| Done | Add SDK aliases | `pkg/codeguard/sdk_types_config_checks.go`, runtime report aliases | SDK tests | Config and PR-summary SDK aliases landed. | - -### Phase 2: Implement change concentration and mixed-concern detection - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Done | Compute change concentration evidence | `checks/change/*`, `runner/support/diff_scope.go` | `tests/checks/change_test.go` | Inputs landed: directories, layers, concern families, public-surface files, changed lines, moved files, and prod/test ratio metadata. | -| Done | Detect oversized diffs | change check package | `TestChangeOversizedDiffUsesConfiguredThresholds` | Uses configurable thresholds and evidence metadata. | -| Done | Detect mixed concerns | change check package | `TestChangeDetectsMixedAndTooManyConcerns` | Path/layer/concern classification landed. | -| Done | Detect mixed refactor and behavior | change check package | `TestChangeDetectsMoveMixedWithBehaviorAndNoVerification` | Evidence is file movement plus behavior-bearing production edits. | -| Done | Detect unnecessary surface area | change check package | `TestChangeDetectsUnnecessarySurfaceArea` | Uses public-surface file budget evidence. | -| Done | Detect one-use abstraction | quality/change packages | `TestChangeOneUseAbstractionDetectsGoInterface`, TS and negative tests | New interfaces/abstract boundaries with only one repository reference. | -| Done | Detect duplicate helper | quality/change packages | `TestChangeDuplicateHelperDetectsGoDuplicate`, TS and negative tests | Finds changed helper bodies that duplicate existing production helper logic. | -| Done | Detect cleanup regression and complexity increase | quality metrics + change package | `TestChangeComplexityIncreasedDetectsPythonBranchGrowth`, `TestChangeCleanupRegressionDetectsClaimedCleanupComplexityGrowth`, negative tests | Complexity increase is general diff evidence; cleanup regression requires cleanup/refactor/chore wording evidence. | -| Done | Detect move without verification | change package | `TestChangeDetectsMoveMixedWithBehaviorAndNoVerification`, `TestChangeMoveWithVerificationDoesNotWarnAboutMissingVerification` | File moves/renames without tests or verification files. | - -### Phase 3: Implement safe-refactor analysis - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Done | Add before/after signature extraction | `internal/codeguard/checks/change/refactor.go` | `tests/checks/refactor_test.go` | Compares conservative public signatures and source evidence in diff scans. | -| Done | Add behavior-preservation evidence model | `internal/codeguard/checks/change/refactor.go` | `tests/checks/refactor_test.go` | Evidence categories include behavior, public contracts, errors, side effects, visibility, dependency direction, duplicate implementations, and dead paths. | -| Done | Detect error-path changes | `internal/codeguard/checks/change/refactor.go` | `tests/checks/refactor_test.go` | Flags changed error/fallback/panic/throw behavior in refactor-labeled diffs. | -| Done | Detect side-effect-order changes | `internal/codeguard/checks/change/refactor.go` | `tests/checks/refactor_test.go` | Tracks ordered side-effect call evidence conservatively. | -| Done | Detect visibility expansion | `internal/codeguard/checks/change/refactor.go` | `tests/checks/refactor_test.go` | Flags widened public/exported API evidence. | -| Done | Detect dependency direction worsened | `internal/codeguard/checks/change/refactor.go` | `tests/checks/refactor_test.go` | Flags new inward infrastructure/framework dependencies in refactor-labeled diffs. | -| Done | Detect duplicate/dead implementation left behind | `internal/codeguard/checks/change/refactor.go` | `TestRefactorDetectsDuplicateImplementationAndDeadPathLeftBehind` | Flags duplicate implementations and obsolete branch/path leftovers. | -| Done | Compute `refactor_confidence` | PR-summary postprocessor | `TestAddPRSummaryArtifactAddsChangeSafetyMetrics` | Artifact rollup landed. It consumes `refactor.*` findings and implemented mixed-refactor/move-without-verification findings. | - -### Phase 4: Expand testability checks - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Done | Detect behavior changes without tests | change + CI/test package | `TestTestingBehaviorChangeWithoutTestAcrossLanguages`, suppression test | Compares changed production files to changed test files across Go, Python, TypeScript, JavaScript, and C++. | -| Done | Detect failure-path tests missing | test-quality package | `TestTestingFailurePathMissingRequiresFailureTestEvidence` | Flags changed error/retry/fallback/auth/external paths without failure-test evidence. | -| Done | Detect hardwired dependencies | quality/design package | `TestTestingHardwiredDependencyFindsChangedProductionLine` | Flags direct construction/use of external dependencies in changed production lines. | -| Done | Detect nondeterministic domain logic | quality/change package | `TestTestingNondeterministicDomainLogicFindsDomainClock` | Flags direct clock/random/env/process access in domain paths. | -| Deferred | Detect legacy hotspot uncovered | history + change package | `TestTestingLegacyHotspotUncoveredDoesNotEmitWithoutHistory` | Catalog/config/fix-template exists; intentionally non-emitting without reliable history/hotspot inputs. | - -### Phase 5: Implement local quality precision - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Done | Add naming/function/error/defensive/maintainability subset catalog | `rules/catalog_change_safety.go` | metadata/config tests | Implemented subset is cataloged with fix templates and explicit language coverage. Domain glossary config deferred. | -| Done | Detect generic names | quality parsers | `TestNamingGenericIdentifierWarnsForPlaceholderNames`, fixture negative test | Contextual fixture/test suppression landed. Broader misleading-name rules deferred. | -| Deferred | Detect vocabulary drift | glossary/config + parser indexes | planned `TestNamingDomainVocabularyDrift` | Deferred. Existing AI naming drift is separate from this local precision subset. | -| Deferred | Add function semantic-responsibility count | quality metrics | planned `TestFunctionSemanticResponsibilityCount` | Deferred. | -| Done | Detect function subset | quality parsers | `TestFunctionExcessiveParametersWarnsWithSpecificRule`, `TestFunctionMixedAbstractionLevelWarnsForInfrastructureInsideOrchestration`, `TestFunctionCommandQueryMixWarnsWhenQueryMutatesState` | Landed excessive parameters, mixed abstraction level, and command/query mix. Other function contract/responsibility rules deferred. | -| Done | Expand error handling subset | Go/TS/Python quality parsers | `TestErrorLoggedAndIgnoredWarnsWhenErrorBecomesSuccess`, `TestErrorContextLostWarnsForBareErrorReturn` | Landed logged-and-ignored and context-lost. Other error IDs remain outside this branch subset. | -| Deferred | Add defensive boundary classification | config + parser helpers | defensive rule tests | Deferred. | -| Done | Implement defensive subset | parser helpers | `TestDefensiveUncheckedTypeAssertionWarnsForSingleValueAssertion`, safe assertion negative test, `TestDefensiveUnsafeNumericConversionWarnsForNarrowingConversion` | Landed unchecked type assertion and unsafe numeric conversion. Broader boundary/overflow/schema/fail-open rules deferred. | - -### Phase 6: Maintainability delta and history-aware smells - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Done | Add maintainability delta/history subset | quality metrics + history support | `TestMaintainabilityPublicSurfaceGrowthWarnsInDiffScan`, `TestMaintainabilityDependencyGrowthWarnsInDiffScan`, history tests pending final gate | Before/after public-surface and direct-dependency counts landed; bounded git-history maintainability/smell signals landed and skip when history is unavailable. Complexity/nesting deltas are also represented through `change.complexity-increased` and `change.cleanup-regression`. | -| Done | Compute `maintainability_delta` | PR-summary postprocessor | `TestAddPRSummaryArtifactAddsChangeSafetyMetrics` | Artifact rollup landed over maintainability, quality, error, and defensive findings. | -| Done | Detect public surface/dependency growth | quality/design/history packages | maintainability rule tests | Public-surface and dependency growth landed. Duplication growth remains deferred outside duplicate-helper detection. | -| Done | Detect high-churn hotspots | `internal/codeguard/history/*` | history tests pending final gate | Bounded local git-history collection landed; unavailable history produces no findings. | -| Done | Detect shotgun surgery/divergent change history | history support | history tests pending final gate | Co-change and commit-subject concern-family signals landed. | -| Done/Deferred | Detect repeat defect/unstable interface/ownership gaps | history + ownership config | history tests pending final gate | Repeat-defect and unstable-interface signals landed. Ownership-gap detection remains deferred. | -| Done | Compute `change_safety` | PR-summary postprocessor | `TestAddPRSummaryArtifactAddsChangeSafetyMetrics`, `TestAddPRSummaryArtifactPublishesChangeMetricsWithoutProductionRisk` | Artifact rollup landed over implemented `change.*` and `testing.*` findings. | - -### Phase 7: Reporting, docs, and rollout - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Done | Add/extend `pr_summary` artifact | `core/report_artifact_types.go`, runner postprocessor | serialization/report tests | Existing artifact extended additively with `change_safety`, `maintainability_delta`, and `refactor_confidence`. | -| Done | Preserve compact text/GitHub-comment behavior | `report/write.go`, `report/github_comment.go` | `TestPRSummaryMetricsAreArtifactOnlyForGitHubAnnotations` | Existing `Summary:` sentence unchanged; metrics do not emit as annotations. | -| Done | Update docs after behavior lands | `docs/checks.md`, `docs/features.md` | docs/metadata tests | Docs now mark implemented detector subset vs catalog/planned IDs. README did not need a user-facing summary update. | -| Done | Add examples | `examples/codeguard.json` | `python3 -m json.tool examples/codeguard.json` | Updated for the final `change_rules` config surface after direct refactor left-behind toggles were added. | - -## Confidence policy - -- High confidence: direct AST/diff evidence of public contract changes, missing tests for changed exported behavior, visibility expansion, swallowed errors, unchecked boundary input, fail-open auth, or complexity/duplication/public-surface regression. -- Medium confidence: one-use abstractions, mixed concerns, duplicated helpers, hardwired dependencies, semantic responsibility count, vocabulary drift. -- Low confidence: history-only smells and inferred misleading names without direct behavior evidence. - -Every finding should include enough evidence for a reviewer to decide quickly: - -- what changed; -- why it affects review/change safety; -- what test or refactor evidence is missing; -- whether confidence is high/medium/low. - -## Profile behavior target - -| Profile | Behavior | -| --- | --- | -| Startup | Warn on oversized/mixed diffs and severe local quality regressions. Do not block most heuristics. | -| Strict | Block new complexity, error-handling, testing, contract, and reliability-adjacent regressions. Warn on smells. | -| Enterprise | Strict plus hotspot/history, ownership gaps, change amplification, and public-surface governance. | -| AI-safe | Strict plus stronger oversized diff, duplicated code, fabricated/unknown APIs, weak error handling, missing tests, inconsistent local idioms, and unnecessary abstractions. | - -## Acceptance criteria - -- Done: new config fields validate and round-trip in JSON/YAML. -- Done: new rule metadata includes fix templates and explicit language coverage. -- Done: implemented diff-only change/testability checks do not produce noise in full scans. -- Done: `pr_summary` includes deterministic `change_safety`, `maintainability_delta`, and `refactor_confidence` metrics. -- Done: vertical slices exist for: - - behavior change without tests; - - mixed refactor and behavior; - - maintainability regression via public-surface/dependency growth and change complexity/cleanup regression signals. -- In rollout/blocked: direct `refactor.*` detector code and tests exist, but `TestRefactorDetectsDuplicateImplementationAndDeadPathLeftBehind` is failing. -- Done: existing JSON/SARIF/GitHub annotations/text summary compatibility is preserved for PR-summary metrics. -- Done/Deferred: history-aware checks degrade gracefully by skipping `testing.legacy-hotspot-uncovered` without reliable hotspot inputs; richer maintainability/smell history detectors landed and skip when git history is unavailable. -- Pending final gate: targeted docs/metadata tests should pass before PR; full `make ci` should wait until no implementation workers are actively changing the branch. - -## Verification plan - -Targeted during implementation: - -```sh -go test ./internal/codeguard/config ./internal/codeguard/rules ./internal/codeguard/runner -go test ./tests/codeguard ./tests/checks ./tests/cli -run 'Test.*(Change|Refactor|Maintainability|Testing|Naming|Function|Error|Defensive|PRSummary)' -go test ./tests/checks -run 'TestWriteReport|TestReport|TestSARIF|TestGitHub' -``` - -Branch gate: - -```sh -make fmt-check -make test -make codeguard-ci -``` - -Pre-push/PR gate when practical: - -```sh -make ci -``` - -## Final PR checklist - -- [x] Task board reconciled against implemented rule IDs and tests. -- [x] Stale Todo rows converted to Done/Deferred states. -- [x] Docs distinguish implemented detectors from catalog/planned IDs. -- [x] Built-in branch rule metadata checked for explicit language coverage and populated fix templates. -- [x] `examples/codeguard.json` updated for the final `change_rules` config shape. -- [ ] Fix direct `refactor.*` test failure: `TestRefactorDetectsDuplicateImplementationAndDeadPathLeftBehind` is missing `refactor.duplicate-implementation-left-behind`. -- [x] Run targeted docs/metadata tests: - `env -u GOROOT GOCACHE=/private/tmp/codeguard-go-cache go test ./internal/codeguard/config ./tests/cli -run 'TestPolicyProfileDocumentationMatchesGeneratedComparison|TestSDKRuleMetadata|TestSDKRuleMetadataFixTemplatesPopulated'` -- [x] Validate sample JSON: - `python3 -m json.tool examples/codeguard.json` -- [x] Run narrow change/maintainability detector checks: - `env -u GOROOT GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/checks -run 'Test(Change|Maintainability)'` -- [x] Run direct refactor detector check and record blocker: - `env -u GOROOT GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/checks -run 'TestRefactor'` currently fails in `TestRefactorDetectsDuplicateImplementationAndDeadPathLeftBehind`. -- [ ] Run broader final gates after active implementation work is finished: - `make fmt-check`, `make test`, `make codeguard-ci`, and `make ci` when practical. - -## PR summary draft - -This branch adds a final-tested change-safety rollout focused on PR reviewability and testability. It introduces the `checks.change` config family, diff-mode concentration detectors, testability detectors for changed behavior/failure paths/hardwired dependencies/nondeterministic domain logic, a local-quality precision subset for naming/function/error/defensive findings, and maintainability-delta findings for public-surface/dependency growth. The PR-summary artifact is extended additively with `change_safety`, `maintainability_delta`, and `refactor_confidence` rollups without changing GitHub annotations or per-rule severities. - -Catalog/config IDs for direct `refactor.*` checks are included with metadata, explicit language coverage, and fix templates for rollout compatibility, but they are documented as in-rollout until the `TestRefactor` target passes. - -## Integration/QA finish-out checklist - -Branch completion criteria: - -- [ ] Workstream B/C/D/E commits are all integrated on `feature/change-safety-testability-refactors` with no untracked or unstaged worker leftovers. -- [ ] Rule metadata and fix-template coverage match the implemented rule IDs; metadata tests pass for every new `change.*`, `testing.*`, `naming.*`, `function.*`, `error.*`, `defensive.*`, and `maintainability.*` rule. -- [ ] Detector tests pass for implemented change/testability/refactor/local-quality behavior across Go, Python, TypeScript, JavaScript, and C++ fixtures where support landed. -- [ ] `pr_summary` keeps `production_risk` compatible and adds deterministic artifact-only `change_safety`, `maintainability_delta`, and `refactor_confidence` metrics. -- [ ] Final generated/profile docs and glossary describe only implemented, profile-gated support; no planned-only rules are presented as shipped. - -Likely integration conflict points: - -- Workstream B and C both touch `internal/codeguard/checks/change/**`; keep testability helpers isolated and verify the change section registry wires both detector groups once. - - Observed 2026-07-27: current workspace has `internal/codeguard/checks/change/testability.go` redeclaring `sectionID`, `sectionName`, `Run`, and `enabled` from `change.go`; B/C need a single package entrypoint before Go tests can compile. -- Workstream D extends shared `pr_summary` artifact types and clone/report behavior; re-check SDK/runtime aliases and report serialization after all metric-producing findings land. -- Workstream E findings feed Workstream D metric grouping; verify `naming.*`, `function.*`, `error.*`, `defensive.*`, and `maintainability.*` rule IDs are grouped intentionally. -- Gauss docs/check glossary must be reconciled after detector support is final so docs do not outrun implementation. - -Required pre-merge gates: - -```sh -go test ./internal/codeguard/... ./pkg/codeguard ./tests/cli -go test ./tests/checks -run 'Test(Change|Testing|Naming|Function|Error|Defensive|Maintainability)' -go test ./internal/codeguard/runner ./tests/codeguard ./tests/checks -run 'Test.*PRSummary|TestWriteReport' -``` - -Broader final gates when the branch is quiescent: - -```sh -make fmt-check -make test -make codeguard-ci -make ci -``` - -## Merge checklist - -- [ ] Rule IDs are stable and grouped by owning family. -- [ ] Every built-in rule has a fix template. -- [ ] New config has defaults, validation, examples, and SDK aliases. -- [ ] Diff/history checks are deterministic and handle shallow history. -- [ ] PR-summary metrics have deterministic evidence ordering. -- [ ] SARIF/GitHub annotations remain finding-only. -- [ ] Product docs distinguish implemented, profile-gated, and confidence-based behavior. -- [ ] `make test` passes. -- [ ] `make ci` passes or any skipped gate is explicitly documented. diff --git a/.claude/task-boards/feature-deep-code-smells-maintainability-precision.md b/.claude/task-boards/feature-deep-code-smells-maintainability-precision.md new file mode 100644 index 0000000..9bda0e8 --- /dev/null +++ b/.claude/task-boards/feature-deep-code-smells-maintainability-precision.md @@ -0,0 +1,317 @@ +# Task board: feature/deep-code-smells-maintainability-precision + +Status: verified +Branch: feature/deep-code-smells-maintainability-precision +Last updated: 2026-07-27 +Not final product docs: this is implementation planning for the branch, not shipped user-facing documentation. + +## Goal + +Make CodeGuard materially stronger at local maintainability review: structural code smells, domain vocabulary consistency, richer function responsibility signals, error-contract quality, defensive boundary checks, and remaining reliability parity hardening across Python, TypeScript, JavaScript, and C++. + +The product target is to answer: + +> Did this PR make the code easier to understand, safer to change, and less likely to fail at boundaries? + +## Branch baseline + +This branch is cut from the rebased production-readiness stack after: + +- `feature/production-reliability-data-readiness` +- `feature/change-safety-testability-refactors` +- `feature/operability-design-delivery-governance` +- dogfood coverage commits through `7199646` + +Do not loosen `.codeguard` policy to make this branch pass. Dogfood warnings should be resolved by concrete refactors, narrower detectors, better fixtures, or explicit scoped waivers only when there is a real documented exception. + +## Non-goals + +- Do not duplicate already-shipped architecture-boundary, observability, data, delivery, or change-safety rules unless the new behavior is materially deeper. +- Do not add broad LLM-dependent checks. Keep this branch deterministic/static/history-backed. +- Do not claim full semantic equivalence or prove correctness. Use evidence, confidence, and clear remediation text. +- Do not broaden rule metadata to languages where behavior/tests do not exist. + +## Workstreams + +### Workstream A: Reliability parity hardening + +Owner scope: + +- `internal/codeguard/checks/reliability/*` +- `internal/codeguard/rules/catalog_reliability.go` +- `internal/codeguard/rules/catalog_fix_templates_reliability.go` +- `tests/checks/reliability_multilang_test.go` +- docs snippets only if rule behavior or coverage changes + +Tasks: + +| Status | Rule / capability | Expected behavior | Languages | Tests | +| --- | --- | --- | --- | --- | +| Done | `reliability.missing-cancellation` parity | Detect detached/background cancellation or missing abort/signal propagation in production calls. | Python, TS, JS, C++ | Added focused positive cases; existing bounded fixtures cover negatives. | +| Done | `reliability.missing-graceful-shutdown` parity | Detect server/listener/thread/service start paths without signal/shutdown/stop evidence. | Python, TS, JS, C++ | Added focused positive cases; safe non-server fixtures remain clean. | +| Done | `reliability.missing-concurrency-limit` parity | Detect unbounded task/thread/promise creation beyond existing loop heuristics. | Python, TS, JS, C++ | Added threshold-aware positive cases. | +| Done | `reliability.resource-leak` parity | Add TS/JS cleanup detection for response/body/stream/file handles. | TS, JS | Added stream/file positive cases plus existing safe resource cases. | +| Done | `reliability.missing-timeout` C++ | Detect common C++ HTTP/RPC calls without timeout/deadline evidence. | C++ | Added C++ positive case. | +| Done | `reliability.swallowed-error` C++ | Detect catch blocks that return/continue without surfacing failures. | C++ | Added C++ positive case. | +| Done | `reliability.lost-error-context` parity | Detect generic rethrows/returns that discard cause or operation context. | Python, TS, JS, C++ | Added Python/TS/JS/C++ positive cases. | +| Done | Metadata reconciliation | Narrow or expand language coverage only to tested behavior. | all | `go test ./tests/cli -run Metadata` passed locally. | + +Verification: + +```sh +env -u GOROOT GOCACHE=/private/tmp/codeguard-deep-smells-go-cache go test ./tests/checks -run 'TestReliability' -count=1 +env -u GOROOT GOCACHE=/private/tmp/codeguard-deep-smells-go-cache go test ./tests/cli -run 'TestSDKRuleMetadata.*Reliability|TestSDKRuleMetadata' -count=1 +make codeguard-ci +``` + +### Workstream B: Structural smell rules + +Owner scope: + +- `internal/codeguard/checks/quality/quality_smells*.go` or equivalent new quality files +- `internal/codeguard/rules/catalog_quality.go` +- `internal/codeguard/rules/catalog_fix_templates_quality.go` +- `tests/checks/quality_smells_test.go` +- shared support helpers only when needed + +Candidate rules: + +| Status | Rule ID | Signal | Notes | +| --- | --- | --- | --- | +| Done | `smell.god-object` | One type/class owns too many methods, fields, responsibilities, or dependency clusters. | Added conservative type/class-level detector with Go/Python/TS/JS/C++ positive and negative coverage. | +| Done | `smell.feature-envy` | Function/method accesses more external object fields/methods than own receiver/context. | Added confidence/evidence-count detector with Go/Python/TS/JS/C++ positive and negative coverage. | +| Done | `smell.middle-man` | Type/class mostly delegates to one collaborator without policy/translation. | Added forwarding-class detector with Go/Python/TS/JS/C++ positive and negative coverage. | +| Done | `smell.message-chain` | Long call chains across objects/modules. | Added medium-confidence chain detector with Go/Python/TS/JS/C++ positive and negative coverage. | +| Done | `smell.data-clump` | Same group of primitive parameters appears repeatedly. | Added repeated primitive/domain parameter-group detector with Go/Python/TS/JS/C++ positive and negative coverage. | +| Done | `smell.switch-on-type` | Repeated type/kind branching that should move behind polymorphism/dispatch. | Added type/kind/discriminator branch detector with Go/Python/TS/JS/C++ positive and negative coverage. | +| Deferred | `smell.refused-bequest` | Subclass/derived type overrides many inherited methods with no-op/throw/unsupported behavior. | Deferred: not implemented because reliable inheritance/no-op evidence needs a stronger parser model to avoid noisy findings. | + +Required behavior: + +- Catalog metadata and fix templates for each implemented rule. +- Multi-language tests for every claimed language. +- Confidence metadata with evidence counts where useful. +- No broad false positives on simple fixtures. + +Verification: + +```sh +env -u GOROOT GOCACHE=/private/tmp/codeguard-deep-smells-go-cache go test ./tests/checks -run 'TestQualitySmell|TestSmell' -count=1 +make codeguard-ci +``` + +### Workstream C: Naming glossary and vocabulary precision + +Owner scope: + +- `internal/codeguard/checks/quality/quality_naming*.go` +- `internal/codeguard/core/config_rule_types.go` +- `internal/codeguard/config/defaults_rules.go` +- `internal/codeguard/config/validate_rules.go` +- `internal/codeguard/rules/catalog_quality.go` +- `tests/checks/quality_naming_test.go` +- config/docs only after behavior exists + +Candidate rules: + +| Status | Rule ID | Signal | +| --- | --- | --- | +| Done | `naming.behavior-mismatch` | Name says query/format/build but body mutates/sends/writes, or name says save/delete but body only reads. | +| Done | `naming.boolean-not-predicate` | Boolean variable/field/return names without `is/has/can/should/allow/enabled` style predicate. | +| Done | `naming.domain-vocabulary-drift` | Configured glossary detects multiple terms for one domain concept. | +| Done | `naming.unknown-abbreviation` | Identifier contains abbreviation not established in repo/config. | +| Done | `naming.cardinality-mismatch` | Plural name used for scalar or singular name used for collection-like value. | +| Done | `naming.implementation-leak` | Domain/API names encode infrastructure details like SQL, HTTP, Redis, Kafka, ORM. | +| Done | `naming.missing-unit` | Numeric names for durations/sizes/money lack unit suffix. | +| Done | `naming.role-suffix-overuse` | Excessive `Manager`, `Helper`, `Util`, `Service`, `Processor` suffixes. | +| Done | `naming.cross-layer-inconsistency` | Same concept renamed across API/domain/persistence layers. | + +Config shape proposal: + +```yaml +checks: + quality_rules: + naming: + glossary: + restaurant: + avoid: [venue, merchant, establishment] + allowed_abbreviations: [id, url, api, http] + role_suffix_warn_threshold: 4 +``` + +Verification: + +```sh +env -u GOROOT GOCACHE=/private/tmp/codeguard-deep-smells-go-cache go test ./internal/codeguard/config ./tests/checks -run 'TestQualityNaming|TestNaming' -count=1 +make codeguard-ci +``` + +### Workstream D: Function responsibility and maintainability delta + +Owner scope: + +- `internal/codeguard/checks/quality/quality_functions*.go` +- `internal/codeguard/checks/quality/quality_precision*.go` +- `internal/codeguard/rules/catalog_quality.go` +- `tests/checks/quality_functions_test.go` +- PR-summary/maintainability artifacts only if existing seams make it safe + +Candidate rules: + +| Status | Rule ID | Signal | +| --- | --- | --- | +| Todo | `function.excessive-length` | Existing max-function-lines alias/metadata consistency if needed. | +| Todo | `function.excessive-branching` | Decision count above threshold. | +| Todo | `function.excessive-nesting` | Nesting depth above threshold. | +| Todo | `function.excessive-returns` | Too many return paths. | +| Done | `function.command-query-mix` | Function returns a value while also invoking mutating side-effect operations. | +| Done | `function.hidden-mutation` | Function mutates input/global/collaborator without name making it explicit. | +| Done | `function.inconsistent-return-contract` | Mixed nil/value/error/partial shapes or inconsistent success semantics. | +| Done | `function.multiple-responsibilities` | Responsibility count from validation, load, write, send, emit, auth, transform, cache, etc. | +| Done | `function.orchestration-domain-mix` | Handler/job orchestration mixed with domain decisions. | +| Todo | `function.control-flow-needs-explanation` | Complex control flow with no extracted helper or named decision. | +| Todo | `function.name-behavior-mismatch` | Function name conflicts with dominant behavior. | +| Done | `function.partial-result` | Returns partially valid result without explicit partial/error contract. | + +Maintainability delta extensions: + +| Status | Rule ID | Signal | +| --- | --- | --- | +| Todo | `maintainability.regression` | Combined delta worsens complexity/deps/public/duplication/testability. | +| Todo | `maintainability.no-improvement-in-hotspot` | Hotspot touched but no local simplification/testability improvement. | +| Todo | `maintainability.duplication-growth` | Duplication increased in touched code. | +| Todo | `maintainability.nesting-growth` | Nesting increased in touched code. | +| Todo | `maintainability.testability-regression` | More hardwired/nondeterministic dependencies or fewer tests. | + +Verification: + +```sh +env -u GOROOT GOCACHE=/private/tmp/codeguard-deep-smells-go-cache go test ./tests/checks -run 'TestQualityFunction|TestMaintainability' -count=1 +make codeguard-ci +``` + +### Workstream E: Error contracts and defensive boundaries + +Owner scope: + +- `internal/codeguard/checks/quality/quality_errors*.go` +- `internal/codeguard/checks/quality/quality_defensive*.go` +- `internal/codeguard/rules/catalog_quality.go` +- `internal/codeguard/rules/catalog_fix_templates_quality.go` +- `tests/checks/quality_errors_test.go` +- `tests/checks/quality_defensive_test.go` + +Candidate error rules: + +| Status | Rule ID | Signal | +| --- | --- | --- | +| Done | `error.logged-and-returned` | Error is logged and returned, risking duplicate logs. | +| Done | `error.logged-and-ignored` | Error is logged and converted to success/ignored result. | +| Done | `error.context-lost` | Bare error returns/rethrows lose operation context. | +| Done | `error.generic-message` | Generic error string without operation/resource context. | +| Done | `error.wrong-abstraction-level` | Infrastructure errors leak into domain/API/user boundary. | +| Done | `error.inconsistent-wrapping` | Same function mixes wrapping styles or drops cause. | +| Done | `error.retryable-not-distinguished` | Retry path cannot distinguish permanent/transient failure. | +| Done | `error.user-message-leaks-internals` | User/API message exposes DB/SQL/stack/infrastructure details. | +| Done | `error.partial-failure-hidden` | Batch/loop failure continues and returns success without partial contract. | +| Done | `error.cleanup-error-ignored` | Close/rollback/delete cleanup errors discarded. | +| Done | `error.fallback-hides-corruption` | Fallback success after corruption/deserialization/validation failure. | +| Done | `error.panic-on-recoverable-path` | Panic used on recoverable request/validation/I/O path. | +| Done | `error.exception-used-for-control-flow` | Exception/panic/throw used for ordinary branch control. | + +Candidate defensive rules: + +| Status | Rule ID | Signal | +| --- | --- | --- | +| Done | `defensive.unvalidated-boundary-input` | Handler/API/event/filesystem input consumed without validation. | +| Done | `defensive.invalid-state-representable` | Boolean/string status combos allow impossible states. | +| Done | `defensive.null-assumption` | Dereference/use without guard at nullable boundary. | +| Done | `defensive.unchecked-type-assertion` | Type assertion/cast bypasses runtime validation or comma-ok checks. | +| Done | `defensive.unsafe-numeric-conversion` | Narrowing numeric conversion lacks bounds check. | +| Done | `defensive.integer-overflow` | Arithmetic on bounded numeric/input sizes without guard. | +| Done | `defensive.bounds-assumption` | Index/key access without length/existence guard. | +| Done | `defensive.unsafe-default` | Missing config/env defaults fail open or disable safety. | +| Done | `defensive.non-exhaustive-branch` | Switch/match over enum-like values lacks default/exhaustive evidence. | +| Done | `defensive.unchecked-external-response` | External response consumed without status/schema/error check. | +| Done | `defensive.missing-schema-validation` | JSON/event/request decoded but not validated at boundary. | +| Done | `defensive.missing-resource-limit` | Boundary reads/uploads/queues without size/count/time bound. | +| Done | `defensive.invalid-state-transition` | State transition accepts impossible backwards/skipped transitions. | +| Done | `defensive.fail-open-authorization` | Authz failure path allows or defaults to success. | + +Verification: + +```sh +env -u GOROOT GOCACHE=/private/tmp/codeguard-deep-smells-go-cache go test ./tests/checks -run 'TestQualityError|TestQualityDefensive|TestDefensive|TestError' -count=1 +make codeguard-ci +``` + +### Workstream F: Docs, metadata, profiles, and final dogfood + +Owner scope: + +- `docs/checks.md` +- `docs/features.md` +- `docs/production.md` +- `internal/codeguard/rules/catalog*.go` +- `internal/codeguard/rules/catalog_fix_templates*.go` +- `internal/codeguard/config/profile.go` +- `internal/codeguard/config/profile_test.go` +- `tests/cli/features_metadata_test.go` + +Tasks: + +| Status | Task | Notes | +| --- | --- | --- | +| Done | Reconcile rule catalog | Implemented smell, naming, function, error, defensive, maintainability, and reliability parity rules have metadata, fixed language coverage, default levels, and fix templates. | +| Done | Reconcile docs glossary | `docs/checks.md` and `docs/features.md` list the shipped rollout subset and avoid advertising non-emitting roadmap IDs. | +| Done | Reconcile profile behavior | Added profile tests pinning AI-safe local precision/reliability/change-safety behavior and strict regression focus. | +| Done | Add metadata tests | SDK metadata tests cover representative smell, naming, function, error, defensive, maintainability, and reliability parity rules. | +| Done | Run dogfood | `make codeguard-ci` passed after detector precision/test fixture fixes. | +| Done | Run full CI | `make ci` passed outside restricted sandbox for httptest. | + +## Agent assignments + +Initial parallel split: + +1. Reliability parity worker: Workstream A. +2. Smells/design worker: Workstream B plus structural smell metadata/tests. +3. Naming/function/error/defensive worker: Workstreams C, D, E, starting with catalog/config skeleton and first detector set. +4. Integration/docs worker: Workstream F, plus task-board reconciliation after worker commits. + +Workers must: + +- Work on disjoint file scopes when possible. +- Not revert other workers' edits. +- Update this board when they complete a task. +- Run focused tests for their slice. +- Run `make codeguard-ci` before pushing or handing off if their changes can affect self-scan. + +## Final branch gates + +Before PR handoff: + +```sh +env -u GOROOT GOCACHE=/private/tmp/codeguard-deep-smells-go-cache go test ./tests/checks ./tests/cli ./internal/codeguard/config ./internal/codeguard/rules -count=1 +make codeguard-ci +make ci +git diff --check +``` + +Expected final result: + +- No advertised non-emitting rule IDs. +- No language coverage claims without at least representative tests. +- No new CodeGuard dogfood failures resolved by broad threshold tuning. +- Docs and `codeguard rules` output agree. + +Final verification completed 2026-07-27: + +```sh +env -u GOROOT GOCACHE=/private/tmp/codeguard-deep-smells-go-cache go test ./internal/codeguard/config ./internal/codeguard/rules ./tests/cli ./tests/codeguard -run 'Test(Profiles|ReviewProfiles|PolicyProfileDocumentation|SDKRuleMetadata|ExampleConfigIncludesQualityNaming|ValidateQualityNaming|LoadConfig|YAML|SnakeCase)' -count=1 +env -u GOROOT GOCACHE=/private/tmp/codeguard-deep-smells-go-cache go test ./tests/checks -run TestQualityErrorAndDefensiveRulesAllowGuardedPatterns -count=1 +env -u GOROOT GOCACHE=/private/tmp/codeguard-deep-smells-go-cache go test ./tests/checks -run TestQualityDefensiveBoundariesDetectMultiLanguageSignals -count=1 +env -u GOROOT GOCACHE=/private/tmp/codeguard-deep-smells-go-cache go test ./tests/checks ./tests/cli ./internal/codeguard/config ./internal/codeguard/rules -count=1 +make ci +make codeguard-ci +git diff --check +``` diff --git a/.claude/task-boards/feature-operability-design-delivery-governance.md b/.claude/task-boards/feature-operability-design-delivery-governance.md deleted file mode 100644 index 5cc4335..0000000 --- a/.claude/task-boards/feature-operability-design-delivery-governance.md +++ /dev/null @@ -1,282 +0,0 @@ -# Task board: feature/operability-design-delivery-governance - -Status: staging -Branch: feature/operability-design-delivery-governance -Last updated: 2026-07-27 -Not final product docs: this is implementation planning for the branch, not shipped user-facing documentation. - -## Goal - -Make CodeGuard evaluate whether production code is operable, locally well-designed, and safe to roll out. - -This branch owns: - -- observability and operations readiness; -- abstraction-quality and local software-design checks; -- delivery-governance and rollout-safety checks; -- enterprise/profile behavior for ownership, runbooks, service compatibility, supply-chain provenance, and deployment verification. - -The product target is to catch changes that are technically correct but hard to operate, hard to change, or unsafe to deploy. - -## Workstream D audit and reconciliation checklist - -Status: prep-audited. As of 2026-07-27, the branch task board lists the intended rule inventory, but the shipped rule catalogs, detector packages, config fields, profile defaults, and user-facing docs for this branch have not landed yet. Keep `docs/checks.md`, `docs/features.md`, `docs/production.md`, `README.md`, and `examples/codeguard.json` unchanged until matching behavior exists in code and tests. - -Workstream D owns final reconciliation after implementation slices merge: - -- Confirm new rule metadata exists for every implemented `observability.*`, `operations.*`, `delivery.*`, `ci.*`, `supply_chain.*`, `design.*`, and `quality.*` rule in scope. -- Confirm every new rule has language coverage, profile behavior, examples where useful, and a fix template or explicit guided remediation. -- Confirm SDK aliases/config API cover any new config structs or rule toggles. -- Confirm profile comparison output reflects startup, strict, enterprise, and AI-safe behavior for the landed rules. -- Update shipped docs only after detector behavior and tests exist. -- Keep this task board accurate as implementation workers land commits; mark a task Done only after code, tests, metadata, and docs/profile behavior are reconciled. -- Add the final PR-summary draft section once the branch has enough implementation to summarize accurately. - -Workstream D verification commands: - -```sh -env -u GOROOT GOCACHE=/private/tmp/codeguard-operability-go-cache go test ./internal/codeguard/config ./tests/cli ./tests/codeguard -run 'Test.*(Profile|Metadata|Config|Documentation|SDK)' -git diff --check -``` - -## Non-goals - -- Do not implement reliability/data-correctness detectors owned by `feature/production-reliability-data-readiness`. -- Do not implement change/refactor/testability metrics owned by `feature/change-safety-testability-refactors`. -- Do not duplicate existing architecture-boundary checks unless the new rule is about local abstraction quality or operability. -- Do not block rollout-governance findings by default in startup/strict without profile-specific staging. - -## Product split - -This branch owns: - -- Rule families: `observability.*`, `operations.*`, additional `design.*`, additional `quality.*`, additional `delivery.*`, additional `ci.*`, and `supply-chain.missing-provenance`. -- Enterprise behavior: ownership, observability, rollout safety, supply chain, runbooks, and service compatibility. -- Production-risk inputs: observability/delivery/operations findings can feed the `production_risk` metric once the shared `pr_summary` artifact exists. - -Adjacent branch contracts: - -- `feature/production-reliability-data-readiness` owns the initial `production_risk` artifact field and reliability/data signals. -- `feature/change-safety-testability-refactors` owns `maintainability_delta`; this branch may add design-governance findings that become inputs later. - -## Existing repo seams to reuse - -- Existing design rules/catalogs: `internal/codeguard/checks/design/*`, `internal/codeguard/rules/catalog_design.go`, `catalog_design_graph.go`, `catalog_design_policy.go`. -- Existing CI/release rules: `internal/codeguard/checks/ci/*`, `internal/codeguard/rules/catalog_test_quality.go`, `catalog_misc.go`. -- Existing supply-chain rules: `internal/codeguard/checks/supplychain/*`, `internal/codeguard/rules/catalog_supplychain.go`. -- Config surface: `internal/codeguard/core/config_types.go`, `internal/codeguard/core/config_rule_types.go`. -- Defaults/examples/validation: `internal/codeguard/config/defaults.go`, `defaults_rules.go`, `example.go`, `validate.go`. -- Runner section registry: `internal/codeguard/runner/checks/registry.go`. -- Rule metadata/fix templates: `internal/codeguard/rules/catalog*.go`, `internal/codeguard/rules/catalog_fix_templates*.go`. -- Report compatibility: `internal/codeguard/report/write.go`, `internal/codeguard/report/github_comment.go`, `internal/codeguard/report/sarif_builders.go`. -- Docs: `docs/checks.md`, `docs/features.md`, `docs/production.md`, `docs/integrations.md`, `README.md`. - -## Rule inventory - -### Observability and operations - -- `observability.unstructured-log` -- `observability.error-without-context` -- `observability.sensitive-log-data` -- `observability.high-cardinality-label` -- `observability.critical-path-uninstrumented` -- `observability.log-and-ignore` -- `observability.shallow-health-check` -- `operations.missing-owner` -- `operations.missing-runbook` - -### Abstraction quality and local design - -- `design.shallow-module` -- `design.excessive-public-surface` -- `design.pass-through-abstraction` -- `design.configuration-leak` -- `design.temporal-coupling` -- `quality.duplicated-knowledge` -- `design.infrastructure-type-leak` -- `design.persistence-model-leak` -- `design.domain-logic-in-handler` -- `quality.ambiguous-name` -- `quality.boolean-argument` -- `quality.mixed-abstraction-levels` -- `quality.excessive-parameters` -- `quality.primitive-obsession` -- `quality.hidden-side-effect` -- `quality.mutable-global-state` -- `quality.redundant-comment` - -### Delivery governance - -- `ci.missing-required-gate` -- `ci.mutable-deployment-reference` -- `delivery.missing-rollback-strategy` -- `delivery.unsafe-migration-order` -- `delivery.high-risk-change-without-kill-switch` -- `delivery.missing-post-deploy-verification` -- `supply-chain.missing-provenance` -- `quality.environment-branching` - -## Implementation phases - -### Phase 0: Decide section and profile shape - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Todo | Decide section IDs | `runner/checks/registry.go` | section smoke tests | Suggested new sections: `observability`, `operations`, `delivery`; extend existing `design`, `quality`, `ci`, and `supply_chain` where rule families already exist. | -| Todo | Define enterprise defaults | `config/profile.go` | profile tests | Enterprise should enable ownership, observability, rollout safety, supply-chain provenance, runbooks, and service compatibility. | -| Todo | Define strict/startup behavior | profile/docs | profile tests | Startup warns only. Strict can warn for observability/design and block only existing required CI/security gates. | -| Todo | Define evidence model | rule packages | report confidence tests | Most rules need confidence/evidence rather than binary proof. Avoid shallow style-lint behavior. | - -### Phase 1: Add config, catalogs, and section scaffolding - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Todo | Add `ObservabilityRulesConfig` | `core/config_rule_types.go`, `core/config_types.go` | config tests | Include structured logger patterns, sensitive-name patterns, metric label deny patterns, critical path patterns, healthcheck path patterns. | -| Todo | Add `OperationsRulesConfig` | config files | config tests | Include owner file patterns, runbook path patterns, critical service path patterns. | -| Todo | Add `DeliveryRulesConfig` | config files | config tests | Include required CI gates, allowed deployment refs, rollback docs patterns, migration ordering config, kill-switch patterns, post-deploy verification patterns. | -| Todo | Add defaults/examples/validation | `config/defaults*.go`, `config/example*.go`, `config/validate_*.go` | `go test ./internal/codeguard/config ./tests/codeguard` | Validate non-empty patterns, positive thresholds, and no conflicting allow/deny refs. | -| Todo | Add SDK aliases | `pkg/codeguard/sdk_types_config_checks.go` | SDK tests | Keep config API complete. | -| Todo | Add catalogs | `rules/catalog_observability.go`, `catalog_operations.go`, `catalog_delivery.go`, extend design/quality/ci/supplychain catalogs | metadata tests | Explicit `LanguageCoverage`. Keep `supply-chain.missing-provenance` spelling aligned with existing prefix convention; repo currently uses `supply_chain.*`, so decide whether to normalize to `supply_chain.missing-provenance` before implementation. | -| Todo | Add fix templates | `rules/catalog_fix_templates_observability.go`, `catalog_fix_templates_delivery.go`, design/quality template files | metadata tests | Mostly guided templates; deterministic only for pinning mutable refs or adding metadata files. | - -### Phase 2: Implement observability checks - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Todo | Create observability package | `internal/codeguard/checks/observability/observability.go` | `tests/checks/observability_test.go` | Follow section pattern and finalize as `observability`, `Observability`. | -| Todo | Register section | `runner/checks/registry.go` | section smoke test | Run after reliability/data when those exist; otherwise after security/design. | -| Todo | Detect unstructured logs | Go/TS/Python detectors | `TestObservabilityUnstructuredLog` | Flag `fmt.Println`, `console.log`, raw string logs, logger calls without fields in production code. Allow tests/scripts. | -| Todo | Detect errors without context | detectors | `TestObservabilityErrorWithoutContext` | Error logs should include operation/request/customer-safe context. Avoid requiring request IDs in low-level pure functions. | -| Todo | Detect sensitive log data | detectors | `TestObservabilitySensitiveLogData` | Reuse security secret/sensitive-name patterns. Flag tokens, passwords, auth headers, PII-like names in log fields/messages. | -| Todo | Detect high-cardinality metric labels | detectors | `TestObservabilityHighCardinalityLabel` | Flag labels with user_id, email, request_id, path with raw params, UUID/order IDs. Allow configured sanitized labels. | -| Todo | Detect critical paths without instrumentation | path/config + parser helpers | `TestObservabilityCriticalPathUninstrumented` | Critical paths: handlers, jobs, consumers, migrations, payment/write flows. Require span/metric/log evidence based on config. | -| Todo | Detect log-and-ignore | error/log detectors | `TestObservabilityLogAndIgnore` | Distinguish from `error.logged-and-ignored` sibling branch by placing operability-focused log-only failure under observability unless it changes reliability semantics. | -| Todo | Detect shallow health checks | route/config scanner | `TestObservabilityShallowHealthCheck` | Flag health endpoints that only return static OK while critical dependencies exist. Confidence based on dependency evidence. | - -### Phase 3: Implement operations ownership/runbook checks - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Todo | Create operations package | `internal/codeguard/checks/operations/operations.go` | `tests/checks/operations_test.go` | Repo-level and path-level findings. | -| Todo | Register section | `runner/checks/registry.go` | section smoke test | Enterprise profile should enable by default. | -| Todo | Detect missing service ownership | operations package | `TestOperationsMissingOwner` | Support CODEOWNERS, service catalog files, ownership metadata in config, package-level metadata. | -| Todo | Detect missing runbook metadata | operations package | `TestOperationsMissingRunbook` | Critical systems require runbook links or local runbook files. Allow configured critical path patterns. | -| Todo | Add ownership-gap cross-feed | operations + maintainability later | operations tests | Findings can feed maintainability/production-risk metrics in other branches. | - -### Phase 4: Implement abstraction-quality design checks - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Todo | Extend design config | `DesignRulesConfig` or new local-design config | config tests | Thresholds: public symbol count, pass-through ratio, temporal coupling evidence, handler/domain path patterns, infrastructure/domain path patterns. | -| Todo | Extend design catalog | `rules/catalog_design.go` or `catalog_design_local.go` | metadata tests | Keep existing design family rather than a competing family. | -| Todo | Detect shallow modules | design package | `TestDesignShallowModule` | Public API surface high but implementation depth/behavior low. Confidence-based. | -| Todo | Detect excessive public surface | design package | `TestDesignExcessivePublicSurface` | Exported symbols/public members per package/module. Exempt SDK packages via config. | -| Todo | Detect pass-through abstractions | design package | `TestDesignPassThroughAbstraction` | Methods/functions that only delegate without policy, translation, validation, or isolation. | -| Todo | Detect configuration leak | design package | `TestDesignConfigurationLeak` | Config structs/options crossing module boundaries or leaking env/deployment concerns into domain code. | -| Todo | Detect temporal coupling | design/history package | `TestDesignTemporalCoupling` | Required call order encoded implicitly. Start with obvious init/use/close or set-before-call patterns. | -| Todo | Detect duplicated business knowledge | quality/design package | `TestQualityDuplicatedKnowledge` | Constants/rules/calculations duplicated across layers. Not the same as token-level duplicate code. | -| Todo | Detect infrastructure type leak | design package | `TestDesignInfrastructureTypeLeak` | DB/HTTP/framework/logger/cloud SDK types in domain packages or public APIs. | -| Todo | Detect persistence model leak | design package | `TestDesignPersistenceModelLeak` | ORM/db model structs returned through public API/handler contracts. | -| Todo | Detect domain logic in handlers/controllers | design package | `TestDesignDomainLogicInHandler` | Handlers should orchestrate/validate/translate, not own business rules. | - -### Phase 5: Precision cleanup for quality rules in this branch - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Todo | Add/align local quality catalog entries | `rules/catalog_quality.go` or new local quality catalog | metadata tests | These are local design-quality rules that fit existing `quality.*` prefix. | -| Todo | Detect ambiguous names | quality parser helpers | `TestQualityAmbiguousName` | `data`, `manager`, `helper`, `process`, `thing`; avoid one-off test fixture false positives. | -| Todo | Detect boolean arguments | quality parser helpers | `TestQualityBooleanArgument` | Flag public/business functions with behavior-hiding booleans. Allow setters/options/builders. | -| Todo | Detect mixed abstraction levels | quality/design parser helpers | `TestQualityMixedAbstractionLevels` | Coordinate with `function.mixed-abstraction-level` branch later. | -| Todo | Detect primitive obsession | quality parser helpers | `TestQualityPrimitiveObsession` | Repeated raw strings/ints for domain concepts, especially IDs/units/currency. | -| Todo | Detect hidden side effects | quality parser helpers | `TestQualityHiddenSideEffect` | Function name implies query/format/build but mutates state, writes, logs, or performs I/O. | -| Todo | Detect mutable global state | quality parser helpers | `TestQualityMutableGlobalState` | Flag mutable package/module globals in production code; allow constants and guarded test hooks. | -| Todo | Detect redundant comments | quality text/parser helpers | `TestQualityRedundantComment` | Comments that only restate nearby code. Low confidence; warn only. | - -### Phase 6: Implement delivery governance checks - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Todo | Extend CI package or create delivery package | `internal/codeguard/checks/ci/*`, `internal/codeguard/checks/delivery/*` | `tests/checks/delivery_test.go` | Use `ci.*` for CI gate config; use `delivery.*` for rollout strategy. | -| Todo | Detect missing required CI gates | CI package | `TestCIMissingRequiredGate` | Validate required workflows/jobs/check names in `.github/workflows`, config, or CI provider files. | -| Todo | Detect mutable deployment references | CI/delivery package | `TestCIMutableDeploymentReference` | Floating GitHub Actions refs, image tags like `latest`, branch deploy refs, unpinned external actions. | -| Todo | Detect missing rollback strategy | delivery package | `TestDeliveryMissingRollbackStrategy` | High-risk deployment/migration changes require rollback docs/config/runbook reference. | -| Todo | Detect unsafe migration ordering | delivery + data migration scanner | `TestDeliveryUnsafeMigrationOrder` | Coordinate with data branch migration rule; this branch focuses rollout sequencing evidence. | -| Todo | Detect high-risk feature without kill switch | delivery package | `TestDeliveryHighRiskNoKillSwitch` | New critical path, payment/auth/data migration behavior needs feature flag/kill switch evidence. | -| Todo | Detect missing post-deploy verification | delivery package | `TestDeliveryMissingPostDeployVerification` | Deploy workflows should verify health/SLO/smoke checks after production rollout. | -| Todo | Detect missing artifact provenance | supplychain package | `TestSupplyChainMissingProvenance` | Prefer `supply_chain.missing-provenance` to match existing prefix unless compatibility requires hyphen. Check SBOM/attestation/provenance files or workflow steps. | -| Todo | Detect environment branching in source | quality/delivery package | `TestQualityEnvironmentBranching` | Flag production/staging/dev branching embedded in domain/source code. Allow config/bootstrap boundaries. | - -### Phase 7: Production-risk integration and docs - -| Status | Task | Files/area | Tests | Notes | -| --- | --- | --- | --- | --- | -| Todo | Feed observability/operations/delivery findings into production risk | `runner/pr_summary.go` if present | PR-summary tests | Additive only; do not make this branch depend on unmerged artifact work unless rebased after branch 1. | -| Todo | Render optional GitHub-comment block | `report/github_comment.go` | report tests | Only if `pr_summary` exists. Keep annotations finding-only. | -| Todo | Update docs after behavior lands | `docs/checks.md`, `docs/features.md`, `docs/production.md`, `docs/integrations.md`, `README.md` | docs/self-scan | Explain enterprise vs startup/strict behavior and tuning. | -| Todo | Update examples | `examples/codeguard.json`, `.codeguard/codeguard.yaml` if appropriate | `make codeguard-ci` | Enterprise-only checks may be too noisy for default example. | - -## Confidence policy - -- High confidence: mutable deployment refs, missing required CI gate, sensitive log fields, high-cardinality metric labels, infrastructure type leaked through public/domain APIs, mutable global state, unpinned provenance requirement. -- Medium confidence: shallow health checks, critical path without instrumentation, missing owner/runbook, domain logic in handler, pass-through abstraction, configuration leak. -- Low confidence: shallow module, temporal coupling, redundant comments, duplicated business knowledge without direct matched constants/rules. - -Findings should include safe metadata such as operation kind, logger/metric call kind, deployment reference type, owner source searched, runbook source searched, public surface count, pass-through ratio, and confidence evidence. Do not include secrets or raw source snippets. - -## Profile behavior target - -| Profile | Behavior | -| --- | --- | -| Startup | Warn only for clear mutable deployment refs, sensitive logs, and severe operability gaps. | -| Strict | Warn observability/design/delivery issues; block existing severe CI/security/supply-chain policy only. | -| Enterprise | Strict plus ownership, observability, rollout safety, provenance, runbooks, and service compatibility as hard or near-hard gates by threshold. | -| AI-safe | Strict plus unnecessary abstractions, duplicated knowledge, environment branching, weak operability metadata, and generated-code reviewability risks. | - -## Acceptance criteria - -- New config fields validate and round-trip in JSON/YAML. -- New rule metadata includes fix templates and explicit language coverage. -- Observability package can detect unstructured logs, error-without-context, sensitive log data, high-cardinality labels, critical path without instrumentation, log-and-ignore, and shallow health checks. -- Operations package can detect missing owners and runbooks for configured critical systems. -- Design extensions can detect at least infrastructure type leak, persistence model leak, domain logic in handler, pass-through abstraction, and excessive public surface. -- Delivery/CI extensions can detect mutable deployment refs, missing CI gates, missing rollback strategy, unsafe migration ordering, high-risk change without kill switch, missing post-deploy verification, missing provenance, and environment branching. -- Enterprise profile enables the intended checks without changing startup defaults aggressively. -- Existing JSON/SARIF/GitHub annotation/text summary compatibility is preserved. -- Targeted tests and `make test` pass before push/PR. - -## Verification plan - -Targeted during implementation: - -```sh -go test ./internal/codeguard/config ./internal/codeguard/rules ./internal/codeguard/runner/checks -go test ./tests/codeguard ./tests/checks ./tests/cli -run 'Test.*(Observability|Operations|Delivery|Owner|Runbook|Provenance|PublicSurface|DomainLogic|InfrastructureLeak|Deployment|Profile|Metadata)' -go test ./tests/checks -run 'TestDesign|TestQuality|TestCI|TestSupplyChain|TestWriteReport' -``` - -Branch gate: - -```sh -make fmt-check -make test -make codeguard-ci -``` - -Pre-push/PR gate when practical: - -```sh -make ci -``` - -## Merge checklist - -- [ ] Rule IDs use existing prefix conventions where possible, especially `supply_chain.*`. -- [ ] Every built-in rule has a fix template. -- [ ] New config has defaults, validation, examples, and SDK aliases. -- [ ] Startup/strict defaults are not made unexpectedly noisy. -- [ ] Enterprise behavior is explicit and test-covered. -- [ ] Findings include actionable evidence and confidence. -- [ ] SARIF/GitHub annotations remain finding-only. -- [ ] Product docs describe implemented behavior, not planned behavior. -- [ ] `make test` passes. -- [ ] `make ci` passes or any skipped gate is explicitly documented. diff --git a/docs/checks.md b/docs/checks.md index 87cbbb1..81ecf71 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -87,9 +87,10 @@ Related report artifacts: Each top-level boolean enables or disables an entire check family. `quality_rules.local_precision` controls the local-quality precision subset -(`naming.*`, `function.*`, `error.*`, `defensive.*`, selected -`maintainability.*`, and history-aware `smell.*` signals). It defaults to -enabled, but repositories can set it to `false` while they refactor legacy +currently cataloged in the [Local quality precision glossary](#local-quality-precision-glossary), +including the shipped `naming.*`, `function.*`, `error.*`, `defensive.*`, +selected `maintainability.*`, and history-aware `smell.*` rule IDs. It defaults +to enabled, but repositories can set it to `false` while they refactor legacy hotspots or avoid broad historical noise in full self-scans. ### Recommended section policy @@ -1103,7 +1104,8 @@ Current detector rollout: - Implemented `Change Safety` diff detectors: `change.oversized-diff`, `change.mixed-concerns`, `change.too-many-concerns`, `change.mixed-refactor-and-behavior`, `change.unnecessary-surface-area`, `change.one-use-abstraction`, `change.duplicate-helper`, `change.cleanup-regression`, `change.complexity-increased`, and `change.move-without-verification`. - Implemented `Change Safety / Testability` detectors: `testing.behavior-change-without-test`, `testing.failure-path-missing`, `testing.hardwired-dependency`, `testing.nondeterministic-domain-logic`, and `testing.legacy-hotspot-uncovered` for Go, Python, TypeScript, JavaScript, and C++ path/text evidence. `testing.legacy-hotspot-uncovered` uses bounded local git history and skips when reliable history/hotspot inputs are unavailable. - Implemented `Change Safety / Refactors` detectors: the direct `refactor.*` family below has stable metadata, language coverage, fix templates, config toggles, and diff-mode safe-refactor detector tests. -- Implemented local-quality support rules live in the `Code Quality` section: `naming.generic-identifier`, `function.excessive-parameters`, `function.mixed-abstraction-level`, `function.command-query-mix`, `error.logged-and-ignored`, `error.context-lost`, `defensive.unchecked-type-assertion`, `defensive.unsafe-numeric-conversion`, `maintainability.public-surface-growth`, and `maintainability.dependency-growth`. +- Implemented local-quality support rules live in the `Code Quality` section and are cataloged in the local precision glossary below, including shipped `naming.*`, `function.*`, `error.*`, `defensive.*`, and `maintainability.*` IDs. +- Implemented structural smell rules live in the `Code Quality` section: `smell.god-object`, `smell.feature-envy`, `smell.middle-man`, `smell.message-chain`, `smell.data-clump`, and `smell.switch-on-type`. - Implemented history-aware maintainability/smell rules live in `Code Quality`-adjacent report sections and skip when git history is unavailable: `maintainability.hotspot`, `maintainability.high-churn-hotspot`, `maintainability.repeat-defect-area`, `maintainability.unstable-interface`, `maintainability.change-amplification`, `smell.shotgun-surgery-history`, and `smell.divergent-change-history`. Cataloged rule glossary: @@ -1148,13 +1150,56 @@ These rules live outside the repository-wide `Change Safety` section in report o | Subsection / family | Rule ID | Default | Short description | | --- | --- | --- | --- | | Naming | `naming.generic-identifier` | warn | Placeholder names such as `foo`, `tmp`, `thing`, or `obj` hide the role an identifier plays. | +| Naming | `naming.behavior-mismatch` | warn | Query/build/format names perform side effects, or command-style names only read. | +| Naming | `naming.boolean-not-predicate` | warn | Boolean variables, parameters, or boolean-returning functions do not read like predicates. | +| Naming | `naming.domain-vocabulary-drift` | warn | Configured glossary concepts appear under multiple terms in the same source. | +| Naming | `naming.unknown-abbreviation` | warn | Identifiers contain abbreviations that are not common or configured for the repository. | +| Naming | `naming.cardinality-mismatch` | warn | Plural names are used for scalar values or singular names for collection-like values. | +| Naming | `naming.implementation-leak` | warn | Domain-facing names encode infrastructure details such as SQL, HTTP, Redis, Kafka, or ORM. | +| Naming | `naming.missing-unit` | warn | Numeric duration, size, or money names omit a unit suffix. | +| Naming | `naming.role-suffix-overuse` | warn | A file repeatedly uses vague suffixes such as Manager, Helper, Util, Service, or Processor. | +| Naming | `naming.cross-layer-inconsistency` | warn | API, domain, and persistence layer names use different terms for the same starter concept. | | Function shape | `function.excessive-parameters` | warn | A function exceeds the configured parameter threshold and likely needs grouped inputs or split responsibilities. | | Function shape | `function.mixed-abstraction-level` | warn | One function combines orchestration-level calls with low-level SQL, HTTP, filesystem, environment, or infrastructure work. | | Function shape | `function.command-query-mix` | warn | A function returns a value while also invoking mutating side-effect operations. | +| Function shape | `function.hidden-mutation` | warn | A function mutates inputs, collaborators, or state without a command-style name. | +| Function shape | `function.inconsistent-return-contract` | warn | One function mixes empty and value return shapes without a clear contract. | +| Function shape | `function.multiple-responsibilities` | warn | One function combines validation, loading, writing, sending, caching, transforming, or observing responsibilities. | +| Function shape | `function.orchestration-domain-mix` | warn | Handlers, controllers, jobs, or workers mix request/job orchestration with domain decisions. | +| Function shape | `function.partial-result` | warn | A function can return a value together with an error without an explicit partial-result contract. | | Error handling | `error.logged-and-ignored` | warn | An error is logged and then ignored, converted to success, or allowed to continue without propagation. | | Error handling | `error.context-lost` | warn | An error is returned or rethrown without operation-specific context. | +| Error handling | `error.logged-and-returned` | warn | The same error is logged and returned, risking duplicate logs at multiple layers. | +| Error handling | `error.generic-message` | warn | An error message lacks operation, resource, or decision context. | +| Error handling | `error.wrong-abstraction-level` | warn | Higher-level error contracts expose lower-level infrastructure details. | +| Error handling | `error.inconsistent-wrapping` | warn | A function mixes wrapped errors with bare error returns. | +| Error handling | `error.retryable-not-distinguished` | warn | Retry paths cannot distinguish transient from permanent failures. | +| Error handling | `error.user-message-leaks-internals` | warn | User-facing errors expose database, transport, stack, or infrastructure internals. | +| Error handling | `error.partial-failure-hidden` | warn | A partial failure path continues or reports success without surfacing failed work. | +| Error handling | `error.cleanup-error-ignored` | warn | Close, rollback, delete, or cleanup failures are discarded. | +| Error handling | `error.panic-on-recoverable-path` | warn | Recoverable request, validation, or I/O failures are handled with panic/throw. | +| Error handling | `error.exception-used-for-control-flow` | warn | Exception/panic/throw is used for ordinary branch control. | +| Error handling | `error.fallback-hides-corruption` | warn | Fallback success after parse, corruption, or validation failure can hide bad data. | | Defensive programming | `defensive.unchecked-type-assertion` | warn | A type assertion or cast bypasses runtime validation or omits the safe checked form. | | Defensive programming | `defensive.unsafe-numeric-conversion` | warn | A narrowing or sign-changing numeric conversion can truncate, wrap, or lose precision. | +| Defensive programming | `defensive.unvalidated-boundary-input` | warn | Handler, API, event, or filesystem input is consumed without validation evidence. | +| Defensive programming | `defensive.invalid-state-representable` | warn | Booleans or raw status strings can represent impossible state combinations. | +| Defensive programming | `defensive.null-assumption` | warn | Nullable boundary values are dereferenced without a nil/null guard. | +| Defensive programming | `defensive.integer-overflow` | warn | Arithmetic on count, size, or length input lacks an overflow bound check. | +| Defensive programming | `defensive.bounds-assumption` | warn | Indexed access assumes collection bounds without a nearby length check. | +| Defensive programming | `defensive.unsafe-default` | warn | A config/env fallback can fail open or disable a safety control. | +| Defensive programming | `defensive.non-exhaustive-branch` | warn | Enum-like state/kind/type branching lacks default or exhaustive handling. | +| Defensive programming | `defensive.unchecked-external-response` | warn | External responses are consumed without checking status, ok, or transport errors. | +| Defensive programming | `defensive.missing-schema-validation` | warn | Decoded JSON, events, or request payloads are used without schema or invariant validation. | +| Defensive programming | `defensive.missing-resource-limit` | warn | Boundary reads or uploads lack explicit size, count, or time limits. | +| Defensive programming | `defensive.invalid-state-transition` | warn | State transitions write terminal states without checking allowed prior state. | +| Defensive programming | `defensive.fail-open-authorization` | warn | Authorization failure paths default to allow or success. | +| Structural smell | `smell.god-object` | warn | A local type/class accumulates many methods, fields, and responsibility clusters. | +| Structural smell | `smell.feature-envy` | warn | A function or method mostly interrogates one external collaborator instead of its own receiver/context. | +| Structural smell | `smell.middle-man` | warn | A type/class mostly forwards calls to one collaborator without policy, translation, or ownership. | +| Structural smell | `smell.message-chain` | warn | Code reaches through a long chain of collaborators, increasing coupling to object structure. | +| Structural smell | `smell.data-clump` | warn | The same group of primitive/domain parameters appears repeatedly across functions. | +| Structural smell | `smell.switch-on-type` | warn | Behavior repeatedly branches on type/kind/discriminator checks that should move behind polymorphism or dispatch. | | Maintainability delta | `maintainability.public-surface-growth` | warn | A changed file exports more public symbols than it did at the base ref. | | Maintainability delta | `maintainability.dependency-growth` | warn | A changed file imports or includes more direct dependencies than it did at the base ref. | | Maintainability history | `maintainability.hotspot` | warn | A changed file has high recent churn, defect history, or both. | @@ -1165,9 +1210,7 @@ These rules live outside the repository-wide `Change Safety` section in report o | Code smell history | `smell.shotgun-surgery-history` | warn | A changed file repeatedly co-changes with several partners, suggesting scattered responsibility. | | Code smell history | `smell.divergent-change-history` | warn | A changed file has recent commit subjects spanning several concern families. | -Broader smell and history-aware families such as `smell.*`, additional -`naming.*`/`function.*`/`error.*`/`defensive.*` rules, and deeper -`maintainability.*` deltas are follow-on roadmap unless they appear in +Deeper `maintainability.*` deltas remain follow-on roadmap unless they appear in `codeguard rules` for the active build. ## PR Summary Production Risk diff --git a/docs/features.md b/docs/features.md index 3c4d3a0..bd16368 100644 --- a/docs/features.md +++ b/docs/features.md @@ -9,6 +9,7 @@ This page lists the current `codeguard` feature surface and the main config entr - clone detection - language-native quality heuristics for Go, Python, TypeScript, JavaScript, Rust, Java, C++, C#, and Ruby - local-quality precision heuristics for naming, function shape, error handling, defensive programming, and maintainability deltas where the active build includes them + - structural smell heuristics such as god object, feature envy, middle man, message chains, data clumps, and switch-on-type - AI-quality heuristics such as swallowed errors, narrative comments, hallucinated imports, dead code, over-mocked tests, idiom drift, semantic review, provenance policy, and change-risk rollups - changed-line coverage gating in diff mode - opt-in `clang-format` and sanitized `clang++ -fsyntax-only` validation backed by safe `compile_commands.json` metadata @@ -132,7 +133,7 @@ Imported reports are never passed to AI triage. - Diff-mode change safety - uses the `checks.change` family to report implemented change-safety, cleanup, testability, and safe-refactor findings - emits PR-summary fields such as `change_safety`, `refactor_confidence`, and `maintainability_delta` only as artifact evidence; they do not create extra annotations or change per-rule severities - - local-quality precision and history-aware families such as `naming.*`, `function.*`, `error.*`, `defensive.*`, `maintainability.*`, and `smell.*` support the same review goal; use `codeguard rules` on the active build to see the exact rollout subset + - the local-quality precision rollout supports the same review goal through the exact `naming.*`, `function.*`, `error.*`, `defensive.*`, selected `maintainability.*`, and history-aware `smell.*` IDs listed by `codeguard rules` on the active build ## Parsers diff --git a/internal/codeguard/checks/quality/quality_additional_languages.go b/internal/codeguard/checks/quality/quality_additional_languages.go index 7815a07..c214b72 100644 --- a/internal/codeguard/checks/quality/quality_additional_languages.go +++ b/internal/codeguard/checks/quality/quality_additional_languages.go @@ -37,6 +37,7 @@ func cppFindingsForFile(env support.Context, file string, data []byte) []core.Fi } if localPrecisionEnabled(env) { findings = append(findings, parsedPrecisionFindings(env, file, parsed)...) + findings = append(findings, parsedStructuralSmellFindings(env, file, parsed)...) } return append(fileLengthFindingWithSignals(env, file, data, findings), findings...) } diff --git a/internal/codeguard/checks/quality/quality_defensive.go b/internal/codeguard/checks/quality/quality_defensive.go new file mode 100644 index 0000000..3898419 --- /dev/null +++ b/internal/codeguard/checks/quality/quality_defensive.go @@ -0,0 +1,318 @@ +package quality + +import ( + "regexp" + "strings" + + "github.com/devr-tools/codeguard/internal/codeguard/checks/support" + "github.com/devr-tools/codeguard/internal/codeguard/core" +) + +const ( + defensiveUnvalidatedBoundaryInputRuleID = "defensive.unvalidated-boundary-input" + defensiveInvalidStateRepresentableRuleID = "defensive.invalid-state-representable" + defensiveNullAssumptionRuleID = "defensive.null-assumption" + defensiveIntegerOverflowRuleID = "defensive.integer-overflow" + defensiveBoundsAssumptionRuleID = "defensive.bounds-assumption" + defensiveUnsafeDefaultRuleID = "defensive.unsafe-default" + defensiveNonExhaustiveBranchRuleID = "defensive.non-exhaustive-branch" + defensiveUncheckedExternalResponseRuleID = "defensive.unchecked-external-response" + defensiveMissingSchemaValidationRuleID = "defensive.missing-schema-validation" + defensiveMissingResourceLimitRuleID = "defensive.missing-resource-limit" + defensiveInvalidStateTransitionRuleID = "defensive.invalid-state-transition" + defensiveFailOpenAuthorizationRuleID = "defensive.fail-open-authorization" +) + +var ( + indexAccessPattern = regexp.MustCompile(`\b([A-Za-z_][\w$]*(?:\.[A-Za-z_][\w$]*)?)\s*\[\s*(?:0|[A-Za-z_][\w$]*)\s*\]`) + jsonDecodePattern = regexp.MustCompile(`(?i)(json\.Unmarshal|json\.NewDecoder|JSON\.parse|json\.loads|nlohmann::json::parse|decode_json|parseJson)`) + externalCallPattern = regexp.MustCompile(`(?i)(http\.Get|client\.Do|fetch\s*\(|axios\.|requests\.(get|post|put|delete)|curl_easy_perform|httplib::|http_client)`) + resourceReadPattern = regexp.MustCompile(`(?i)(io\.ReadAll|ReadAll|read_to_string|read_to_end|\.read\s*\(|bodyParser|multer|upload|request\.body|r\.Body)`) + unsafeDefaultPattern = regexp.MustCompile(`(?i)(getenv|process\.env|os\.environ|std::getenv|config).*?(default|fallback|\|\||!=|,\s*['"]).*?(true|false|allow|disable|skip|insecure)`) + switchLikePattern = regexp.MustCompile(`(?i)\b(switch|match)\b[^{:\n]*(status|state|kind|type)`) + stateAssignmentPattern = regexp.MustCompile(`(?i)(status|state)\s*(?:=|:=|=>)\s*["']?(paid|active|complete|completed|shipped|deleted|approved)["']?`) + authFailOpenPattern = regexp.MustCompile(`(?is)(except|catch)\b[^{:\n]*(?:\{|:)[^}\n]*return\s+(true|allow|nil|none)`) + structStartPattern = regexp.MustCompile(`(?i)\b(type\s+\w+\s+struct|interface\s+\w+|class\s+\w+|struct\s+\w+)`) + boolFieldPattern = regexp.MustCompile(`(?i)\b(bool|boolean)\b`) + stringStateFieldPattern = regexp.MustCompile(`(?i)\b(status|state|kind)\b.*\b(string|str|std::string|String)\b|\b(string|str|std::string|String)\b.*\b(status|state|kind)\b`) +) + +func defensiveBoundaryFindings(env support.Context, file string, fn precisionFunction) []core.Finding { + if isQualityFixturePath(file) { + return nil + } + body := functionRawBody(fn) + loweredBody := strings.ToLower(body) + findings := make([]core.Finding, 0) + + if line, ok := unvalidatedBoundaryInputLine(fn, loweredBody); ok { + findings = append(findings, precisionWarnFinding(env, defensiveUnvalidatedBoundaryInputRuleID, file, line, + "boundary input is consumed without validation or schema checks", core.ConfidenceMedium)) + } + if line, ok := nullAssumptionLine(fn, loweredBody); ok { + findings = append(findings, precisionWarnFinding(env, defensiveNullAssumptionRuleID, file, line, + "nullable boundary value is dereferenced without a nil/null guard", core.ConfidenceMedium)) + } + if line, ok := integerOverflowLine(fn, loweredBody); ok { + findings = append(findings, precisionWarnFinding(env, defensiveIntegerOverflowRuleID, file, line, + "arithmetic on count, size, or length input lacks an overflow bound check", core.ConfidenceMedium)) + } + if line, ok := boundsAssumptionLine(fn, loweredBody); ok { + findings = append(findings, precisionWarnFinding(env, defensiveBoundsAssumptionRuleID, file, line, + "indexed access assumes collection bounds without a nearby length check", core.ConfidenceMedium)) + } + if line, ok := unsafeDefaultLine(fn.Statements); ok { + findings = append(findings, precisionWarnFinding(env, defensiveUnsafeDefaultRuleID, file, line, + "configuration default can fail open or disable a safety control", core.ConfidenceHigh)) + } + if line, ok := nonExhaustiveBranchLine(fn, loweredBody); ok { + findings = append(findings, precisionWarnFinding(env, defensiveNonExhaustiveBranchRuleID, file, line, + "enum-like branch over state, status, kind, or type lacks default/exhaustive handling", core.ConfidenceMedium)) + } + if line, ok := uncheckedExternalResponseLine(fn, loweredBody); ok { + findings = append(findings, precisionWarnFinding(env, defensiveUncheckedExternalResponseRuleID, file, line, + "external response is consumed without checking status, ok, or transport error", core.ConfidenceMedium)) + } + if line, ok := missingSchemaValidationLine(fn, loweredBody); ok { + findings = append(findings, precisionWarnFinding(env, defensiveMissingSchemaValidationRuleID, file, line, + "decoded JSON or event payload is used without schema or invariant validation", core.ConfidenceMedium)) + } + if line, ok := missingResourceLimitLine(fn, loweredBody); ok { + findings = append(findings, precisionWarnFinding(env, defensiveMissingResourceLimitRuleID, file, line, + "boundary read or upload lacks an explicit size/count/time resource limit", core.ConfidenceMedium)) + } + if line, ok := invalidStateTransitionLine(fn, loweredBody); ok { + findings = append(findings, precisionWarnFinding(env, defensiveInvalidStateTransitionRuleID, file, line, + "state transition writes a terminal state without checking the allowed prior state", core.ConfidenceMedium)) + } + if line, ok := failOpenAuthorizationLine(fn, loweredBody); ok { + findings = append(findings, precisionWarnFinding(env, defensiveFailOpenAuthorizationRuleID, file, line, + "authorization failure path defaults to allow/success", core.ConfidenceHigh)) + } + return findings +} + +func sourceDefensiveInvariantFindings(env support.Context, file string, source string) []core.Finding { + if isQualityFixturePath(file) { + return nil + } + lines := strings.Split(strings.ReplaceAll(source, "\r\n", "\n"), "\n") + for idx := 0; idx < len(lines); idx++ { + if !structStartPattern.MatchString(lines[idx]) || !structuralStateContainerLine(lines[idx]) { + continue + } + boolFields := 0 + hasStringState := false + for lookahead := idx; lookahead < len(lines) && lookahead <= idx+12; lookahead++ { + line := lines[lookahead] + boolFields += len(boolFieldPattern.FindAllString(line, -1)) + if stringStateFieldPattern.MatchString(line) { + hasStringState = true + } + if strings.Contains(line, "}") { + break + } + } + if boolFields >= 2 || hasStringState { + return []core.Finding{precisionWarnFinding(env, defensiveInvalidStateRepresentableRuleID, file, idx+1, + "state shape uses booleans or raw status strings that can represent impossible combinations", core.ConfidenceMedium)} + } + } + return nil +} + +func structuralStateContainerLine(line string) bool { + lowered := strings.ToLower(line) + return strings.Contains(lowered, "struct") || strings.Contains(lowered, "interface") || strings.Contains(lowered, "class") +} + +func unvalidatedBoundaryInputLine(fn precisionFunction, loweredBody string) (int, bool) { + if !boundaryFunctionName(fn.Name) && !hasBoundaryParam(fn.Params) { + return 0, false + } + if containsAny(loweredBody, []string{"validate", "schema", "sanitize", "bind", "decodevalid", "zod.", "yup.", "pydantic", "jsonschema"}) { + return 0, false + } + if containsAny(loweredBody, []string{"request", "req.", "event", "payload", "body", "json", "params", "query"}) { + return fn.StartLine, true + } + return 0, false +} + +func hasBoundaryParam(params []support.ParsedParam) bool { + for _, param := range params { + name := strings.ToLower(param.Name) + if containsAny(name, []string{"req", "request", "event", "payload", "body", "input"}) { + return true + } + } + return false +} + +func nullAssumptionLine(fn precisionFunction, loweredBody string) (int, bool) { + for _, param := range fn.Params { + name := strings.ToLower(strings.Trim(param.Name, "*& ")) + if name == "" || !nullableParam(param) { + continue + } + if containsAny(loweredBody, []string{name + " == nil", name + " != nil", name + " is none", name + " is not none", name + " === null", name + " !== null", name + " == nullptr", name + " != nullptr"}) { + continue + } + if containsAny(loweredBody, []string{name + ".", name + "->", name + "[", "*" + name}) { + return firstUseLine(fn, name), true + } + } + return 0, false +} + +func nullableParam(param support.ParsedParam) bool { + typ := strings.ToLower(param.Type) + return strings.Contains(typ, "*") || strings.Contains(typ, "optional") || + strings.Contains(typ, "null") || strings.Contains(typ, "none") || + strings.Contains(typ, "maybe") || strings.Contains(typ, "?") +} + +func firstUseLine(fn precisionFunction, name string) int { + for _, statement := range fn.Statements { + lowered := strings.ToLower(firstNonEmptyString(statement.Raw, statement.Text)) + if containsAny(lowered, []string{name + ".", name + "->", name + "[", "*" + name}) { + return statement.Line + } + } + return fn.StartLine +} + +func integerOverflowLine(fn precisionFunction, loweredBody string) (int, bool) { + if containsAny(loweredBody, []string{"maxint", "math.max", "checked", "saturating", "overflow", "limits<", "safeint"}) { + return 0, false + } + if !regexp.MustCompile(`(?i)\b(count|size|length|len|capacity|offset|total|bytes)\b`).MatchString(loweredBody) { + return 0, false + } + if regexp.MustCompile(`[A-Za-z_][\w$]*\s*(\*|\+|<<)\s*[A-Za-z0-9_]`).MatchString(loweredBody) { + return fn.StartLine, true + } + return 0, false +} + +func boundsAssumptionLine(fn precisionFunction, loweredBody string) (int, bool) { + if containsAny(loweredBody, []string{"len(", ".length", ".size()", "empty()", "bounds", "range", "count >"}) { + return 0, false + } + for idx, statement := range fn.Statements { + raw := firstNonEmptyString(statement.Raw, statement.Text) + if strings.Contains(raw, "map[") { + continue + } + match := indexAccessPattern.FindStringSubmatch(raw) + if match == nil { + continue + } + if nearbyBoundsGuard(fn.Statements, idx, match[1]) { + continue + } + if indexAccessPattern.MatchString(raw) { + return statement.Line, true + } + } + return 0, false +} + +func nearbyBoundsGuard(statements []support.ParsedStatement, idx int, target string) bool { + target = strings.ToLower(strings.TrimSpace(strings.Split(target, ".")[0])) + if target == "" { + return false + } + start := idx - 3 + if start < 0 { + start = 0 + } + for lookback := start; lookback <= idx && lookback < len(statements); lookback++ { + line := strings.ToLower(firstNonEmptyString(statements[lookback].Raw, statements[lookback].Text)) + if strings.Contains(line, "len("+target+")") || + strings.Contains(line, target+".length") || + strings.Contains(line, target+".size()") || + strings.Contains(line, target+".empty()") { + return true + } + } + return false +} + +func unsafeDefaultLine(statements []support.ParsedStatement) (int, bool) { + for _, statement := range statements { + if unsafeDefaultPattern.MatchString(firstNonEmptyString(statement.Raw, statement.Text)) { + return statement.Line, true + } + } + return 0, false +} + +func nonExhaustiveBranchLine(fn precisionFunction, loweredBody string) (int, bool) { + if !switchLikePattern.MatchString(functionRawBody(fn)) || containsAny(loweredBody, []string{"default", "else", "unreachable", "assert_never", "exhaustive"}) { + return 0, false + } + return fn.StartLine, true +} + +func uncheckedExternalResponseLine(fn precisionFunction, loweredBody string) (int, bool) { + if !externalCallPattern.MatchString(functionRawBody(fn)) { + return 0, false + } + if containsAny(loweredBody, []string{"status", ".ok", "err != nil", "if err", "error", "catch", "raise_for_status", "response_code"}) { + return 0, false + } + if containsAny(loweredBody, []string{".json", "readall", ".text", ".body", "json()"}) { + return firstPatternLine(fn, externalCallPattern), true + } + return 0, false +} + +func missingSchemaValidationLine(fn precisionFunction, loweredBody string) (int, bool) { + if !jsonDecodePattern.MatchString(functionRawBody(fn)) { + return 0, false + } + if containsAny(loweredBody, []string{"validate", "schema", "jsonschema", "zod.", "yup.", "pydantic", "isvalid", "required"}) { + return 0, false + } + return firstPatternLine(fn, jsonDecodePattern), true +} + +func missingResourceLimitLine(fn precisionFunction, loweredBody string) (int, bool) { + if !resourceReadPattern.MatchString(functionRawBody(fn)) { + return 0, false + } + if containsAny(loweredBody, []string{"limitreader", "maxbytes", "max_bytes", "content-length", "limit(", "take(", "buffer_size", "quota"}) { + return 0, false + } + return firstPatternLine(fn, resourceReadPattern), true +} + +func invalidStateTransitionLine(fn precisionFunction, loweredBody string) (int, bool) { + if !stateAssignmentPattern.MatchString(functionRawBody(fn)) { + return 0, false + } + if containsAny(loweredBody, []string{"cantransition", "allowed", "from", "previous", "current", "validtransition", "state machine"}) { + return 0, false + } + return firstPatternLine(fn, stateAssignmentPattern), true +} + +func failOpenAuthorizationLine(fn precisionFunction, loweredBody string) (int, bool) { + if !containsAny(strings.ToLower(fn.Name), []string{"auth", "allow", "permission", "policy"}) && !containsAny(loweredBody, []string{"authorize", "permission", "authz", "policy"}) { + return 0, false + } + if authFailOpenPattern.MatchString(loweredBody) || regexp.MustCompile(`(?is)(?:if\s+)?(?:\w+\s*:=\s*)?authorize\([^)]*\)\s*;\s*err\s*!=\s*nil\s*\{[^}]*return\s+(true|nil)`).MatchString(loweredBody) || regexp.MustCompile(`(?is)err\s*!=\s*nil\s*\{[^}]*return\s+(true|nil)`).MatchString(loweredBody) { + return fn.StartLine, true + } + return 0, false +} + +func firstPatternLine(fn precisionFunction, pattern *regexp.Regexp) int { + for _, statement := range fn.Statements { + if pattern.MatchString(firstNonEmptyString(statement.Raw, statement.Text)) { + return statement.Line + } + } + return fn.StartLine +} diff --git a/internal/codeguard/checks/quality/quality_errors.go b/internal/codeguard/checks/quality/quality_errors.go new file mode 100644 index 0000000..45485a7 --- /dev/null +++ b/internal/codeguard/checks/quality/quality_errors.go @@ -0,0 +1,306 @@ +package quality + +import ( + "regexp" + "strings" + + "github.com/devr-tools/codeguard/internal/codeguard/checks/support" + "github.com/devr-tools/codeguard/internal/codeguard/core" +) + +const ( + errorLoggedAndReturnedRuleID = "error.logged-and-returned" + errorGenericMessageRuleID = "error.generic-message" + errorWrongAbstractionLevelRuleID = "error.wrong-abstraction-level" + errorInconsistentWrappingRuleID = "error.inconsistent-wrapping" + errorRetryableNotDistinguishedRuleID = "error.retryable-not-distinguished" + errorUserMessageLeaksInternalsRuleID = "error.user-message-leaks-internals" + errorPartialFailureHiddenRuleID = "error.partial-failure-hidden" + errorCleanupErrorIgnoredRuleID = "error.cleanup-error-ignored" + errorPanicOnRecoverablePathRuleID = "error.panic-on-recoverable-path" + errorExceptionControlFlowRuleID = "error.exception-used-for-control-flow" + errorFallbackHidesCorruptionRuleID = "error.fallback-hides-corruption" +) + +var ( + genericErrorConstructorPattern = regexp.MustCompile(`(?i)(errors\.New|fmt\.Errorf|new\s+Error|Exception|runtime_error|std::runtime_error|throw)\s*\(\s*["']([^"']+)["']`) + errorStringPattern = regexp.MustCompile(`["']([^"']{3,160})["']`) + internalErrorLeakPattern = regexp.MustCompile(`(?i)\b(sql|sqlite|postgres|postgresql|mysql|redis|kafka|grpc|stack trace|traceback|errno|database|db\.|pq:|driver:|dial tcp|connection refused)\b`) + wrappedErrorPattern = regexp.MustCompile(`(?i)(%w|errors\.Wrap|fmt\.Errorf\s*\([^)]*err|raise\s+\w+.*\s+from\s+\w+|cause\s*:)`) + cleanupIgnoredPattern = regexp.MustCompile(`(?i)(_\s*=\s*[^;\n]*(close|rollback|remove|delete)\s*\(|defer\s+[^;\n]*\.close\s*\(|catch\s*\([^)]*\)\s*\{\s*(?:/\*.*\*/|//.*)?\s*\})`) + panicPattern = regexp.MustCompile(`\bpanic\s*\(`) + throwRaisePattern = regexp.MustCompile(`(?i)\b(throw|raise)\b`) +) + +func errorContractFindings(env support.Context, file string, fn precisionFunction) []core.Finding { + if isQualityFixturePath(file) { + return nil + } + body := functionRawBody(fn) + loweredBody := strings.ToLower(body) + findings := make([]core.Finding, 0) + + if line, ok := loggedAndReturnedLine(fn.Statements); ok { + findings = append(findings, precisionWarnFinding(env, errorLoggedAndReturnedRuleID, file, line, + "error is logged and returned from the same boundary, risking duplicate logs", core.ConfidenceHigh)) + } + if line, ok := genericErrorMessageLine(fn.Statements); ok { + findings = append(findings, precisionWarnFinding(env, errorGenericMessageRuleID, file, line, + "error message is generic and lacks operation, resource, or decision context", core.ConfidenceMedium)) + } + if line, ok := inconsistentWrappingLine(fn); ok { + findings = append(findings, precisionWarnFinding(env, errorInconsistentWrappingRuleID, file, line, + "function mixes wrapped errors with bare error returns, making the error contract inconsistent", core.ConfidenceMedium)) + } + if line, ok := cleanupIgnoredLine(fn.Statements); ok { + findings = append(findings, precisionWarnFinding(env, errorCleanupErrorIgnoredRuleID, file, line, + "cleanup error is discarded; close, rollback, or delete failures should be handled or joined", core.ConfidenceHigh)) + } + if line, ok := partialFailureHiddenLine(fn, loweredBody); ok { + findings = append(findings, precisionWarnFinding(env, errorPartialFailureHiddenRuleID, file, line, + "partial failure path continues or returns success without surfacing the failed work", core.ConfidenceMedium)) + } + if line, ok := fallbackHidesCorruptionLine(fn, loweredBody); ok { + findings = append(findings, precisionWarnFinding(env, errorFallbackHidesCorruptionRuleID, file, line, + "fallback success after parse, corruption, or validation failure can hide bad data", core.ConfidenceMedium)) + } + if line, ok := retryableUndistinguishedLine(fn, loweredBody); ok { + findings = append(findings, precisionWarnFinding(env, errorRetryableNotDistinguishedRuleID, file, line, + "retry path does not distinguish transient from permanent failures", core.ConfidenceMedium)) + } + if line, ok := wrongAbstractionLevelLine(fn, body); ok { + findings = append(findings, precisionWarnFinding(env, errorWrongAbstractionLevelRuleID, file, line, + "error contract exposes lower-level infrastructure details at a higher abstraction boundary", core.ConfidenceMedium)) + } + if line, ok := userMessageLeakLine(fn, body); ok { + findings = append(findings, precisionWarnFinding(env, errorUserMessageLeaksInternalsRuleID, file, line, + "user-facing error message leaks database, transport, or stack internals", core.ConfidenceHigh)) + } + if line, ok := panicOnRecoverableLine(fn, body); ok { + findings = append(findings, precisionWarnFinding(env, errorPanicOnRecoverablePathRuleID, file, line, + "panic is used on a recoverable request, validation, or I/O path", core.ConfidenceMedium)) + } + if line, ok := exceptionControlFlowLine(fn); ok { + findings = append(findings, precisionWarnFinding(env, errorExceptionControlFlowRuleID, file, line, + "exception is used for ordinary branch control instead of explicit result handling", core.ConfidenceMedium)) + } + return findings +} + +func functionRawBody(fn precisionFunction) string { + lines := make([]string, 0, len(fn.Statements)) + for _, statement := range fn.Statements { + lines = append(lines, firstNonEmptyString(statement.Raw, statement.Text)) + } + if len(lines) == 0 { + return fn.Body + } + return strings.Join(lines, "\n") +} + +func loggedAndReturnedLine(statements []support.ParsedStatement) (int, bool) { + for idx, statement := range statements { + if !logsError(firstNonEmptyString(statement.Raw, statement.Text)) { + continue + } + if nearbyReturnedError(statements, idx) { + return statement.Line, true + } + } + return 0, false +} + +func nearbyReturnedError(statements []support.ParsedStatement, idx int) bool { + for lookahead := idx; lookahead < len(statements) && lookahead <= idx+4; lookahead++ { + line := strings.TrimSpace(firstNonEmptyString(statements[lookahead].Raw, statements[lookahead].Text)) + if returnsBareError(line) || throwsBareError(line) || bareErrorReturn(line) { + return true + } + lowered := strings.ToLower(line) + if strings.HasPrefix(lowered, "return ") && strings.Contains(lowered, "err") && + !strings.Contains(lowered, "return nil") && !strings.Contains(lowered, "return none") { + return true + } + } + return false +} + +func genericErrorMessageLine(statements []support.ParsedStatement) (int, bool) { + for _, statement := range statements { + raw := firstNonEmptyString(statement.Raw, statement.Text) + match := genericErrorConstructorPattern.FindStringSubmatch(raw) + if match == nil { + continue + } + if genericErrorMessage(match[2]) { + return statement.Line, true + } + } + return 0, false +} + +func genericErrorMessage(message string) bool { + normalized := strings.TrimSpace(strings.ToLower(message)) + if normalized == "" { + return false + } + for _, generic := range []string{ + "error", "failed", "failure", "invalid", "bad request", "request failed", + "operation failed", "something went wrong", "unknown error", "not found", + "unauthorized", "forbidden", "oops", + } { + if normalized == generic { + return true + } + } + return len(strings.Fields(normalized)) <= 2 && + containsAny(normalized, []string{"failed", "invalid", "error", "failure"}) +} + +func inconsistentWrappingLine(fn precisionFunction) (int, bool) { + if !wrappedErrorPattern.MatchString(functionRawBody(fn)) { + return 0, false + } + for _, statement := range fn.Statements { + line := firstNonEmptyString(statement.Raw, statement.Text) + if bareErrorReturn(line) || returnsBareError(line) || throwsBareError(line) { + return statement.Line, true + } + } + return 0, false +} + +func bareErrorReturn(line string) bool { + trimmed := strings.TrimSpace(strings.TrimSuffix(line, ";")) + lowered := strings.ToLower(trimmed) + return regexp.MustCompile(`^return\s+(.+,\s*)?(err|error|e)$`).MatchString(lowered) || + lowered == "raise" || lowered == "raise e" || lowered == "throw e" +} + +func cleanupIgnoredLine(statements []support.ParsedStatement) (int, bool) { + for _, statement := range statements { + if cleanupIgnoredPattern.MatchString(firstNonEmptyString(statement.Raw, statement.Text)) { + return statement.Line, true + } + } + return 0, false +} + +func partialFailureHiddenLine(fn precisionFunction, loweredBody string) (int, bool) { + if strings.Contains(loweredBody, "allsettled") && !containsAny(loweredBody, []string{"rejected", "throw", "return err", "return error"}) { + return fn.StartLine, true + } + if !containsAny(loweredBody, []string{"continue", "pass"}) || !containsAny(loweredBody, []string{"err", "error", "catch", "except"}) { + return 0, false + } + if containsAny(loweredBody, []string{"return err", "return error", "throw", "raise"}) { + return 0, false + } + for _, statement := range fn.Statements { + lowered := strings.ToLower(firstNonEmptyString(statement.Raw, statement.Text)) + if strings.Contains(lowered, "continue") || strings.TrimSpace(lowered) == "pass" { + return statement.Line, true + } + } + return 0, false +} + +func fallbackHidesCorruptionLine(fn precisionFunction, loweredBody string) (int, bool) { + if !containsAny(loweredBody, []string{"json", "parse", "deserialize", "unmarshal", "decode", "validate", "corrupt"}) { + return 0, false + } + if !containsAny(loweredBody, []string{"catch", "except", "err != nil", "error"}) { + return 0, false + } + for _, statement := range fn.Statements { + lowered := strings.ToLower(firstNonEmptyString(statement.Raw, statement.Text)) + if containsAny(lowered, []string{"return {}", "return []", "return map[", "return default", "return fallback"}) { + return statement.Line, true + } + } + return 0, false +} + +func retryableUndistinguishedLine(fn precisionFunction, loweredBody string) (int, bool) { + if !containsAny(loweredBody, []string{"retry", "backoff", "again", "attempt"}) || + !containsAny(loweredBody, []string{"err", "error", "catch", "except", "failure"}) { + return 0, false + } + if containsAny(loweredBody, []string{"retryable", "transient", "permanent", "temporary", "timeout", "status", "rate limit"}) { + return 0, false + } + return fn.StartLine, true +} + +func wrongAbstractionLevelLine(fn precisionFunction, body string) (int, bool) { + if !internalErrorLeakPattern.MatchString(body) { + return 0, false + } + if boundaryFunctionName(fn.Name) || domainFunctionName(fn.Name) { + return firstErrorStringLine(fn), true + } + return 0, false +} + +func userMessageLeakLine(fn precisionFunction, body string) (int, bool) { + if !internalErrorLeakPattern.MatchString(body) { + return 0, false + } + if !boundaryFunctionName(fn.Name) && !containsAny(strings.ToLower(body), []string{"http.error", "response", "json", "status", "usermessage", "user_message"}) { + return 0, false + } + return firstErrorStringLine(fn), true +} + +func firstErrorStringLine(fn precisionFunction) int { + for _, statement := range fn.Statements { + raw := firstNonEmptyString(statement.Raw, statement.Text) + if errorStringPattern.MatchString(raw) || internalErrorLeakPattern.MatchString(raw) { + return statement.Line + } + } + return fn.StartLine +} + +func panicOnRecoverableLine(fn precisionFunction, body string) (int, bool) { + if !panicPattern.MatchString(body) || strings.HasPrefix(strings.ToLower(fn.Name), "must") { + return 0, false + } + if !containsAny(strings.ToLower(fn.Name), []string{"handle", "parse", "load", "save", "validate", "decode", "request", "process"}) { + return 0, false + } + for _, statement := range fn.Statements { + if panicPattern.MatchString(firstNonEmptyString(statement.Raw, statement.Text)) { + return statement.Line, true + } + } + return fn.StartLine, true +} + +func exceptionControlFlowLine(fn precisionFunction) (int, bool) { + for idx, statement := range fn.Statements { + raw := firstNonEmptyString(statement.Raw, statement.Text) + lowered := strings.ToLower(raw) + if !throwRaisePattern.MatchString(raw) || !containsAny(lowered, []string{"invalid", "not found", "missing", "stop", "continue", "break"}) { + continue + } + if strings.Contains(lowered, "panic(") { + continue + } + if strings.Contains(lowered, " if ") || strings.HasPrefix(strings.TrimSpace(lowered), "if ") || + (idx > 0 && strings.HasPrefix(strings.TrimSpace(strings.ToLower(fn.Statements[idx-1].Text)), "if ")) { + return statement.Line, true + } + } + return 0, false +} + +func boundaryFunctionName(name string) bool { + lowered := strings.ToLower(name) + return containsAny(lowered, []string{"handler", "handle", "controller", "route", "api", "endpoint", "render", "respond", "response"}) +} + +func domainFunctionName(name string) bool { + lowered := strings.ToLower(name) + return containsAny(lowered, []string{"checkout", "order", "profile", "account", "customer", "payment", "invoice", "subscription"}) +} diff --git a/internal/codeguard/checks/quality/quality_go.go b/internal/codeguard/checks/quality/quality_go.go index ef04644..56ebcc4 100644 --- a/internal/codeguard/checks/quality/quality_go.go +++ b/internal/codeguard/checks/quality/quality_go.go @@ -54,6 +54,7 @@ func goFindingsForFile(env support.Context, file string, data []byte) []core.Fin findings = append(findings, goFunctionFindings(env, file, fset, parsed)...) if localPrecisionEnabled(env) { findings = append(findings, goPrecisionFindings(env, file, fset, parsed, data)...) + findings = append(findings, goStructuralSmellFindings(env, file, fset, parsed, data)...) } findings = append(findings, goAIQualityFindings(env, file, fset, parsed, data)...) return append(fileLengthFindingWithSignals(env, file, data, findings), findings...) diff --git a/internal/codeguard/checks/quality/quality_precision.go b/internal/codeguard/checks/quality/quality_precision.go index e59f505..cb55cec 100644 --- a/internal/codeguard/checks/quality/quality_precision.go +++ b/internal/codeguard/checks/quality/quality_precision.go @@ -116,6 +116,8 @@ func goPrecisionFindings(env support.Context, file string, fset *token.FileSet, }) findings = append(findings, redundantCommentFindings(env, file, string(data))...) findings = append(findings, sourceDuplicatedKnowledgeFindings(env, file, string(data))...) + findings = append(findings, sourceNamingFindings(env, file, string(data))...) + findings = append(findings, sourceDefensiveInvariantFindings(env, file, string(data))...) return findings } @@ -351,6 +353,8 @@ func parsedPrecisionFindings(env support.Context, file string, parsed *support.P findings = append(findings, sourceMutableGlobalFindings(env, file, parsed.Source)...) findings = append(findings, sourceDuplicatedKnowledgeFindings(env, file, parsed.Source)...) findings = append(findings, redundantCommentFindings(env, file, parsed.Source)...) + findings = append(findings, sourceNamingFindings(env, file, parsed.Source)...) + findings = append(findings, sourceDefensiveInvariantFindings(env, file, parsed.Source)...) return findings } @@ -417,6 +421,7 @@ func precisionFunctionFindings(env support.Context, file string, fn precisionFun findings = append(findings, precisionWarnFinding(env, functionCommandQueryMixRuleID, file, fn.StartLine, fmt.Sprintf("function %s returns a value while also invoking mutating side-effect operations", fn.Name), core.ConfidenceMedium)) } + findings = append(findings, additionalPrecisionFunctionFindings(env, file, fn)...) if primitiveObsession(fn) { findings = append(findings, precisionWarnFinding(env, qualityPrimitiveObsessionRuleID, file, fn.StartLine, fmt.Sprintf("function %s passes several domain concepts as raw primitives", fn.Name), core.ConfidenceMedium)) @@ -426,6 +431,8 @@ func precisionFunctionFindings(env support.Context, file string, fn precisionFun fmt.Sprintf("function %s name implies a query/build operation but it performs side effects", fn.Name), core.ConfidenceMedium)) } findings = append(findings, errorHandlingFindings(env, file, fn)...) + findings = append(findings, errorContractFindings(env, file, fn)...) + findings = append(findings, defensiveBoundaryFindings(env, file, fn)...) return findings } @@ -547,7 +554,8 @@ func errorHandlingFindings(env support.Context, file string, fn precisionFunctio func logsError(line string) bool { lowered := strings.ToLower(line) - return strings.Contains(lowered, "log.") || strings.Contains(lowered, "logger.") || strings.Contains(lowered, "console.error") + return strings.Contains(lowered, "log.") || strings.Contains(lowered, "logger.") || + strings.Contains(lowered, "logging.") || strings.Contains(lowered, "console.error") } func nearbyIgnoredError(statements []support.ParsedStatement, idx int) bool { diff --git a/internal/codeguard/checks/quality/quality_precision_workstreams_cd.go b/internal/codeguard/checks/quality/quality_precision_workstreams_cd.go new file mode 100644 index 0000000..3ff46d6 --- /dev/null +++ b/internal/codeguard/checks/quality/quality_precision_workstreams_cd.go @@ -0,0 +1,701 @@ +package quality + +import ( + "fmt" + "regexp" + "sort" + "strings" + "unicode" + + "github.com/devr-tools/codeguard/internal/codeguard/checks/support" + "github.com/devr-tools/codeguard/internal/codeguard/core" +) + +const ( + namingBehaviorMismatchRuleID = "naming.behavior-mismatch" + namingBooleanNotPredicateRuleID = "naming.boolean-not-predicate" + namingDomainVocabularyDriftRuleID = "naming.domain-vocabulary-drift" + namingUnknownAbbreviationRuleID = "naming.unknown-abbreviation" + namingCardinalityMismatchRuleID = "naming.cardinality-mismatch" + namingImplementationLeakRuleID = "naming.implementation-leak" + namingMissingUnitRuleID = "naming.missing-unit" + namingRoleSuffixOveruseRuleID = "naming.role-suffix-overuse" + namingCrossLayerInconsistencyRuleID = "naming.cross-layer-inconsistency" + + functionHiddenMutationRuleID = "function.hidden-mutation" + functionInconsistentReturnContractRuleID = "function.inconsistent-return-contract" + functionMultipleResponsibilitiesRuleID = "function.multiple-responsibilities" + functionOrchestrationDomainMixRuleID = "function.orchestration-domain-mix" + functionPartialResultRuleID = "function.partial-result" +) + +var ( + commandFunctionPrefixPattern = regexp.MustCompile(`^(add|append|assign|cancel|create|delete|emit|insert|mutate|persist|publish|remove|save|send|set|store|update|upsert|write)`) + readCallPattern = regexp.MustCompile(`(?i)(^|[.>:\-_])(count|fetch|find|get|list|load|lookup|query|read|select|search)([A-Z_:\-.]|$)`) + identifierTokenPattern = regexp.MustCompile(`[A-Za-z_$][A-Za-z0-9_$]*`) + infraNamePattern = regexp.MustCompile(`(?i)(sql|http|redis|kafka|grpc|graphql|mongo|s3|dynamo|postgres|mysql|elastic|orm)`) + roleSuffixPattern = regexp.MustCompile(`(?i)(manager|helper|util|utils|service|processor)$`) + durationNamePattern = regexp.MustCompile(`(?i)(timeout|duration|delay|interval|ttl|latency|elapsed|expiry|expiration|retention)`) + sizeNamePattern = regexp.MustCompile(`(?i)(size|limit|length|capacity|bytes?|mb|kb|gb)`) + moneyNamePattern = regexp.MustCompile(`(?i)(amount|price|cost|fee|total|subtotal|balance|money)`) + unitSuffixPattern = regexp.MustCompile(`(?i)(nanos?|micros?|millis?|ms|seconds?|secs?|s|minutes?|mins?|hours?|hrs?|days?|bytes?|kb|mb|gb|cents?|pennies|usd|eur|gbp|aud|cad)$`) + collectionTypePattern = regexp.MustCompile(`(?i)(\[\]|\[\s*\]|array|list|slice|map|dict|record|set|vector|collection|iterable|sequence|promise<[^>]*\[\])`) + scalarTypePattern = regexp.MustCompile(`(?i)\b(bool|boolean|char|double|float|float64|int|int32|int64|number|string|str|uint|uint64)\b`) + booleanExprPattern = regexp.MustCompile(`(?i)^(true|false|nil|none|null|undefined|[A-Za-z_$][\w$]*\s*(===|!==|==|!=|<=|>=|<|>)|.*(\band\b|\bor\b|&&|\|\||\binstanceof\b|\bis\s+not\b|\bis\b).*)$`) + paramMutationPattern = regexp.MustCompile(`\b([A-Za-z_$][\w$]*)\s*(?:\.|->|\[)`) + returnLinePattern = regexp.MustCompile(`(?m)^\s*return(?:\s+([^;\n]+))?`) + partialReturnPattern = regexp.MustCompile(`(?i)\breturn\s+[^;\n,]+,\s*(err|error)\b|\breturn\s+\{[^}\n]*(data|result|value)[^}\n]*(err|error)[^}\n]*\}`) + identifierWordSplitPattern = regexp.MustCompile(`[_\-\s]+`) +) + +func additionalPrecisionFunctionFindings(env support.Context, file string, fn precisionFunction) []core.Finding { + findings := make([]core.Finding, 0, 8) + if behaviorMismatch(fn) { + findings = append(findings, precisionWarnFinding(env, namingBehaviorMismatchRuleID, file, fn.StartLine, + fmt.Sprintf("function %s name conflicts with observed query/command behavior", fn.Name), core.ConfidenceMedium)) + } + if hiddenMutation(fn) { + findings = append(findings, precisionWarnFinding(env, functionHiddenMutationRuleID, file, fn.StartLine, + fmt.Sprintf("function %s mutates state without an explicit command-style name", fn.Name), core.ConfidenceMedium)) + } + if inconsistentReturnContract(fn) { + findings = append(findings, precisionWarnFinding(env, functionInconsistentReturnContractRuleID, file, fn.StartLine, + fmt.Sprintf("function %s mixes empty and value return shapes; make the success/error contract explicit", fn.Name), core.ConfidenceMedium)) + } + if partialResult(fn) { + findings = append(findings, precisionWarnFinding(env, functionPartialResultRuleID, file, fn.StartLine, + fmt.Sprintf("function %s can return a value alongside an error without an explicit partial-result contract", fn.Name), core.ConfidenceMedium)) + } + if count, labels := responsibilityCount(fn); count >= 4 { + findings = append(findings, precisionWarnFinding(env, functionMultipleResponsibilitiesRuleID, file, fn.StartLine, + fmt.Sprintf("function %s combines %d responsibilities (%s); split orchestration from focused work", fn.Name, count, strings.Join(labels, ", ")), core.ConfidenceMedium)) + } + if orchestrationDomainMix(fn) { + findings = append(findings, precisionWarnFinding(env, functionOrchestrationDomainMixRuleID, file, fn.StartLine, + fmt.Sprintf("function %s mixes request/job orchestration with domain decisions", fn.Name), core.ConfidenceMedium)) + } + findings = append(findings, precisionNamingFindings(env, file, fn)...) + return findings +} + +func precisionNamingFindings(env support.Context, file string, fn precisionFunction) []core.Finding { + findings := make([]core.Finding, 0, 6) + allNames := make([]struct { + name string + typ string + expr string + line int + }, 0, 1+len(fn.Params)+len(fn.Assignments)) + allNames = append(allNames, struct { + name string + typ string + expr string + line int + }{name: fn.Name, line: fn.StartLine}) + for _, param := range fn.Params { + allNames = append(allNames, struct { + name string + typ string + expr string + line int + }{name: param.Name, typ: param.Type, line: fn.StartLine}) + } + for _, assignment := range fn.Assignments { + allNames = append(allNames, struct { + name string + typ string + expr string + line int + }{name: assignment.Name, expr: assignment.Expr, line: assignment.Line}) + } + for _, item := range allNames { + if item.name == "" { + continue + } + if isBooleanNameCandidate(item.name, item.typ, item.expr, fn) && !isPredicateName(item.name) { + findings = append(findings, precisionWarnFinding(env, namingBooleanNotPredicateRuleID, file, item.line, + fmt.Sprintf("boolean name %q should read as a predicate such as is/has/can/should", item.name), core.ConfidenceMedium)) + } + if cardinalityMismatch(item.name, item.typ, item.expr) { + findings = append(findings, precisionWarnFinding(env, namingCardinalityMismatchRuleID, file, item.line, + fmt.Sprintf("identifier %q has plural/singular wording that conflicts with its value shape", item.name), core.ConfidenceMedium)) + } + if implementationLeakName(item.name) { + findings = append(findings, precisionWarnFinding(env, namingImplementationLeakRuleID, file, item.line, + fmt.Sprintf("identifier %q exposes infrastructure vocabulary in domain-facing naming", item.name), core.ConfidenceMedium)) + } + if missingUnit(item.name, item.typ, item.expr) { + findings = append(findings, precisionWarnFinding(env, namingMissingUnitRuleID, file, item.line, + fmt.Sprintf("numeric identifier %q names a duration, size, or money value without a unit suffix", item.name), core.ConfidenceMedium)) + } + if abbr := unknownAbbreviation(env, item.name); abbr != "" { + findings = append(findings, precisionWarnFinding(env, namingUnknownAbbreviationRuleID, file, item.line, + fmt.Sprintf("identifier %q contains abbreviation %q that is not established in quality_rules.naming.allowed_abbreviations", item.name, abbr), core.ConfidenceLow)) + } + } + return findings +} + +func sourceNamingFindings(env support.Context, file string, source string) []core.Finding { + if isQualityFixturePath(file) { + return nil + } + findings := make([]core.Finding, 0, 3) + if finding, ok := glossaryDriftFinding(env, file, source); ok { + findings = append(findings, finding) + } + if finding, ok := roleSuffixOveruseFinding(env, file, source); ok { + findings = append(findings, finding) + } + if finding, ok := crossLayerInconsistencyFinding(env, file, source); ok { + findings = append(findings, finding) + } + return findings +} + +func behaviorMismatch(fn precisionFunction) bool { + name := strings.ToLower(fn.Name) + if hiddenSideEffect(fn) { + return true + } + if !commandFunctionPrefixPattern.MatchString(name) || mutatingFunctionEvidence(fn) { + return false + } + for _, call := range fn.Calls { + if readCallPattern.MatchString(call.Callee) { + return true + } + } + return false +} + +func hiddenMutation(fn precisionFunction) bool { + if explicitMutationName(fn.Name) { + return false + } + return mutatingFunctionEvidence(fn) || mutatesParameter(fn) +} + +func mutatingFunctionEvidence(fn precisionFunction) bool { + for _, call := range fn.Calls { + if mutatingCallPattern.MatchString(call.Callee) { + return true + } + } + for _, assignment := range fn.Assignments { + if assignment.Augmented { + return true + } + } + return false +} + +func mutatesParameter(fn precisionFunction) bool { + params := map[string]struct{}{} + for _, param := range fn.Params { + if param.Name != "" { + params[param.Name] = struct{}{} + } + } + if len(params) == 0 { + return false + } + for _, line := range strings.Split(fn.Body, "\n") { + if !lineHasAssignmentOperator(line) { + continue + } + for _, match := range paramMutationPattern.FindAllStringSubmatch(line, -1) { + if _, ok := params[match[1]]; ok { + return true + } + } + } + return false +} + +func lineHasAssignmentOperator(line string) bool { + for idx := 0; idx < len(line); idx++ { + if line[idx] != '=' { + continue + } + prev := byte(0) + next := byte(0) + if idx > 0 { + prev = line[idx-1] + } + if idx+1 < len(line) { + next = line[idx+1] + } + if prev == '=' || prev == '!' || prev == '<' || prev == '>' || next == '=' || next == '>' { + continue + } + return true + } + return false +} + +func explicitMutationName(name string) bool { + lowered := strings.ToLower(strings.TrimSpace(name)) + return commandFunctionPrefixPattern.MatchString(lowered) || + strings.Contains(lowered, "mutat") || + strings.Contains(lowered, "persist") || + strings.Contains(lowered, "write") +} + +func inconsistentReturnContract(fn precisionFunction) bool { + returns := returnCategories(fn.Body) + if returns.total < 2 { + return false + } + return returns.empty && returns.value +} + +type returnShapeCounts struct { + total int + empty bool + value bool +} + +func returnCategories(body string) returnShapeCounts { + out := returnShapeCounts{} + for _, match := range returnLinePattern.FindAllStringSubmatch(body, -1) { + out.total++ + expr := "" + if len(match) > 1 { + expr = strings.TrimSpace(match[1]) + } + if expr == "" || isEmptyReturnExpr(expr) { + out.empty = true + continue + } + if strings.Contains(expr, ",") { + parts := strings.Split(expr, ",") + first := strings.TrimSpace(parts[0]) + if isEmptyReturnExpr(first) { + out.empty = true + } else { + out.value = true + } + continue + } + out.value = true + } + return out +} + +func isEmptyReturnExpr(expr string) bool { + expr = strings.TrimSpace(strings.TrimSuffix(expr, ";")) + switch strings.ToLower(expr) { + case "", "nil", "none", "null", "undefined", "false": + return true + default: + return strings.HasPrefix(expr, "nil,") || strings.HasPrefix(expr, "none,") || strings.HasPrefix(expr, "null,") + } +} + +func partialResult(fn precisionFunction) bool { + loweredName := strings.ToLower(fn.Name) + if strings.Contains(loweredName, "partial") || strings.Contains(loweredName, "try") { + return false + } + return partialReturnPattern.MatchString(fn.Body) +} + +func responsibilityCount(fn precisionFunction) (int, []string) { + seen := map[string]struct{}{} + record := func(label string) { + seen[label] = struct{}{} + } + body := strings.ToLower(fn.Body) + for _, call := range fn.Calls { + classifyResponsibility(strings.ToLower(call.Callee), record) + } + for _, statement := range fn.Statements { + classifyResponsibility(strings.ToLower(statement.Text), record) + } + if strings.Contains(body, " if ") || strings.Contains(body, "\tif ") || strings.Contains(body, "\nif ") { + record("decision") + } + labels := make([]string, 0, len(seen)) + for label := range seen { + labels = append(labels, label) + } + sort.Strings(labels) + return len(labels), labels +} + +func classifyResponsibility(text string, record func(string)) { + switch { + case strings.Contains(text, "validat") || strings.Contains(text, "sanitize"): + record("validate") + case strings.Contains(text, "auth") || strings.Contains(text, "permission") || strings.Contains(text, "allow"): + record("authorize") + case strings.Contains(text, "fetch") || strings.Contains(text, "find") || strings.Contains(text, "load") || strings.Contains(text, "query") || strings.Contains(text, "read") || strings.Contains(text, "select"): + record("load") + case strings.Contains(text, "save") || strings.Contains(text, "insert") || strings.Contains(text, "update") || strings.Contains(text, "delete") || strings.Contains(text, "persist") || strings.Contains(text, "write"): + record("write") + case strings.Contains(text, "send") || strings.Contains(text, "publish") || strings.Contains(text, "emit") || strings.Contains(text, "notify"): + record("send") + case strings.Contains(text, "cache") || strings.Contains(text, "redis"): + record("cache") + case strings.Contains(text, "format") || strings.Contains(text, "map") || strings.Contains(text, "transform") || strings.Contains(text, "serialize") || strings.Contains(text, "json"): + record("transform") + case strings.Contains(text, "log") || strings.Contains(text, "metric") || strings.Contains(text, "trace"): + record("observe") + } +} + +func orchestrationDomainMix(fn precisionFunction) bool { + name := strings.ToLower(fn.Name) + body := strings.ToLower(fn.Body) + orchestrator := strings.Contains(name, "handler") || strings.Contains(name, "controller") || strings.Contains(name, "job") || + strings.Contains(name, "worker") || strings.Contains(fn.Signature, "Request") || strings.Contains(fn.Signature, "Response") || + strings.Contains(body, "request") || strings.Contains(body, "response") + if !orchestrator { + return false + } + hasInfra := lowLevelOperationPattern.MatchString(fn.Body) + for _, call := range fn.Calls { + lowered := strings.ToLower(call.Callee) + if strings.Contains(lowered, "fetch") || strings.Contains(lowered, "save") || strings.Contains(lowered, "send") || + strings.Contains(lowered, "cache") || strings.Contains(lowered, "publish") || strings.Contains(lowered, "query") { + hasInfra = true + break + } + } + hasDomainDecision := strings.Contains(body, "if ") && (strings.Contains(body, "order") || strings.Contains(body, "user") || + strings.Contains(body, "account") || strings.Contains(body, "price") || strings.Contains(body, "eligib") || + strings.Contains(body, "valid")) + return hasInfra && hasDomainDecision +} + +func isBooleanNameCandidate(name string, typ string, expr string, fn precisionFunction) bool { + if name == fn.Name { + return functionLooksBoolean(fn) + } + if isBooleanType(typ) { + return true + } + return expr != "" && booleanExprPattern.MatchString(strings.TrimSpace(expr)) +} + +func functionLooksBoolean(fn precisionFunction) bool { + if isPredicateName(fn.Name) { + return false + } + for _, statement := range fn.Statements { + text := strings.TrimSpace(strings.TrimSuffix(statement.Text, ";")) + if strings.HasPrefix(text, "return ") && booleanExprPattern.MatchString(strings.TrimSpace(strings.TrimPrefix(text, "return "))) { + return true + } + } + return false +} + +func isBooleanType(typ string) bool { + typ = strings.ToLower(strings.TrimSpace(typ)) + return typ == "bool" || typ == "boolean" || strings.Contains(typ, " bool") || strings.Contains(typ, ": boolean") +} + +func isPredicateName(name string) bool { + lowered := strings.ToLower(strings.Trim(name, "_$")) + for _, prefix := range []string{"is", "has", "have", "can", "could", "should", "must", "allow", "allows", "enable", "enabled", "disable", "disabled", "needs", "requires", "supports", "valid", "visible", "ready"} { + if strings.HasPrefix(lowered, prefix) { + return true + } + } + return false +} + +func cardinalityMismatch(name string, typ string, expr string) bool { + base := strings.ToLower(strings.Trim(name, "_$")) + if base == "" || base == "item" || base == "items" || base == "status" || strings.HasSuffix(base, "status") || strings.HasSuffix(base, "class") { + return false + } + plural := isPluralName(base) + collection := collectionTypePattern.MatchString(typ) || collectionExpr(expr) + scalar := scalarTypePattern.MatchString(typ) || scalarExpr(expr) + if plural && scalar && !collection { + return true + } + return !plural && collection && !strings.Contains(base, "map") && !strings.Contains(base, "list") && !strings.Contains(base, "set") +} + +func isPluralName(name string) bool { + if unitSuffixPattern.MatchString(name) { + return false + } + return strings.HasSuffix(name, "s") && !strings.HasSuffix(name, "ss") && !strings.HasSuffix(name, "us") +} + +func collectionExpr(expr string) bool { + expr = strings.TrimSpace(strings.ToLower(expr)) + return strings.HasPrefix(expr, "[]") || strings.HasPrefix(expr, "[") || strings.HasPrefix(expr, "map[") || + strings.HasPrefix(expr, "make([]") || strings.Contains(expr, "new map") || strings.Contains(expr, "new set") || + strings.Contains(expr, "array<") || strings.Contains(expr, "list<") || strings.Contains(expr, "vector<") +} + +func scalarExpr(expr string) bool { + expr = strings.TrimSpace(strings.TrimSuffix(expr, ";")) + if expr == "" { + return false + } + if booleanExprPattern.MatchString(expr) { + return true + } + if expr[0] == '"' || expr[0] == '\'' || (expr[0] >= '0' && expr[0] <= '9') { + return true + } + return false +} + +func implementationLeakName(name string) bool { + words := splitIdentifierWords(name) + if len(words) <= 1 { + return false + } + for _, word := range words { + if infraNamePattern.MatchString(word) { + return true + } + } + return false +} + +func missingUnit(name string, typ string, expr string) bool { + lowered := strings.ToLower(strings.Trim(name, "_$")) + if unitSuffixPattern.MatchString(lowered) || strings.HasSuffix(lowered, "count") || strings.HasSuffix(lowered, "total") { + return false + } + looksMeasured := durationNamePattern.MatchString(lowered) || sizeNamePattern.MatchString(lowered) || moneyNamePattern.MatchString(lowered) + if !looksMeasured { + return false + } + return scalarTypePattern.MatchString(typ) || numericExpr(expr) +} + +func numericExpr(expr string) bool { + expr = strings.TrimSpace(expr) + if expr == "" { + return false + } + return expr[0] >= '0' && expr[0] <= '9' +} + +func unknownAbbreviation(env support.Context, name string) string { + allowed := allowedAbbreviations(env) + for _, word := range splitIdentifierWords(name) { + candidate := strings.ToLower(strings.Trim(word, "_$")) + if candidate == "" || len(candidate) < 2 { + continue + } + if _, ok := allowed[candidate]; ok { + continue + } + if isAllUpper(word) && len(candidate) <= 6 { + return word + } + if isSuspiciousShortening(candidate) { + return word + } + } + return "" +} + +func allowedAbbreviations(env support.Context) map[string]struct{} { + defaults := []string{"api", "ast", "aws", "ci", "cli", "cpu", "cpp", "css", "db", "dns", "dto", "env", "grpc", "html", "http", "https", "id", "io", "ip", "js", "json", "mcp", "os", "pr", "rpc", "sdk", "sql", "ssh", "tcp", "tls", "ts", "tsx", "ui", "uri", "url", "uuid", "xml", "yaml", "yml"} + allowed := make(map[string]struct{}, len(defaults)+len(env.Config.Checks.QualityRules.Naming.AllowedAbbreviations)) + for _, value := range defaults { + allowed[value] = struct{}{} + } + for _, value := range env.Config.Checks.QualityRules.Naming.AllowedAbbreviations { + allowed[strings.ToLower(strings.TrimSpace(value))] = struct{}{} + } + return allowed +} + +func isSuspiciousShortening(word string) bool { + switch word { + case "acct", "addr", "amt", "cfg", "cust", "msg", "num", "qty", "usr": + return true + default: + return false + } +} + +func glossaryDriftFinding(env support.Context, file string, source string) (core.Finding, bool) { + for concept, entry := range env.Config.Checks.QualityRules.Naming.Glossary { + preferred := strings.ToLower(strings.TrimSpace(concept)) + terms := append([]string{preferred}, entry.Avoid...) + seen := map[string]int{} + for _, ident := range identifiersInSource(source) { + for _, term := range terms { + normalized := strings.ToLower(strings.TrimSpace(term)) + if normalized == "" { + continue + } + if identifierContainsWord(ident.name, normalized) { + seen[normalized] = ident.line + } + } + } + if len(seen) >= 2 { + line := firstSeenLine(seen) + return precisionWarnFinding(env, namingDomainVocabularyDriftRuleID, file, line, + fmt.Sprintf("domain concept %q appears under multiple terms (%s); prefer one glossary vocabulary", concept, sortedKeys(seen)), core.ConfidenceMedium), true + } + } + return core.Finding{}, false +} + +func roleSuffixOveruseFinding(env support.Context, file string, source string) (core.Finding, bool) { + threshold := env.Config.Checks.QualityRules.Naming.RoleSuffixWarnThreshold + if threshold <= 0 { + threshold = 4 + } + seen := map[string]int{} + for _, ident := range identifiersInSource(source) { + if roleSuffixPattern.MatchString(ident.name) { + seen[ident.name] = ident.line + } + } + if len(seen) < threshold { + return core.Finding{}, false + } + return precisionWarnFinding(env, namingRoleSuffixOveruseRuleID, file, firstSeenLine(seen), + fmt.Sprintf("file uses %d vague role suffix names such as Manager/Helper/Util/Service/Processor", len(seen)), core.ConfidenceMedium), true +} + +func crossLayerInconsistencyFinding(env support.Context, file string, source string) (core.Finding, bool) { + groups := [][]string{ + {"restaurant", "venue", "merchant", "establishment"}, + {"user", "customer", "account"}, + {"order", "purchase", "transaction"}, + } + layerTerms := []string{"api", "request", "response", "dto", "entity", "record", "row", "model", "repository", "domain"} + idents := identifiersInSource(source) + for _, group := range groups { + seen := map[string]int{} + for _, ident := range idents { + if !containsAnyIdentifierWord(ident.name, layerTerms) { + continue + } + for _, term := range group { + if identifierContainsWord(ident.name, term) { + seen[term] = ident.line + } + } + } + if len(seen) >= 2 { + return precisionWarnFinding(env, namingCrossLayerInconsistencyRuleID, file, firstSeenLine(seen), + fmt.Sprintf("cross-layer names use multiple terms for one concept (%s)", sortedKeys(seen)), core.ConfidenceLow), true + } + } + return core.Finding{}, false +} + +type identifierAtLine struct { + name string + line int +} + +func identifiersInSource(source string) []identifierAtLine { + matches := identifierTokenPattern.FindAllStringIndex(source, -1) + out := make([]identifierAtLine, 0, len(matches)) + for _, match := range matches { + out = append(out, identifierAtLine{ + name: source[match[0]:match[1]], + line: 1 + strings.Count(source[:match[0]], "\n"), + }) + } + return out +} + +func splitIdentifierWords(name string) []string { + trimmed := strings.Trim(name, "_$") + if trimmed == "" { + return nil + } + var words []string + for _, part := range identifierWordSplitPattern.Split(trimmed, -1) { + if part == "" { + continue + } + words = append(words, splitCamelWords(part)...) + } + return words +} + +func splitCamelWords(part string) []string { + if part == "" { + return nil + } + runes := []rune(part) + start := 0 + out := make([]string, 0, 3) + for idx := 1; idx < len(runes); idx++ { + prev := runes[idx-1] + cur := runes[idx] + nextLower := idx+1 < len(runes) && unicode.IsLower(runes[idx+1]) + if unicode.IsLower(prev) && unicode.IsUpper(cur) || unicode.IsUpper(prev) && unicode.IsUpper(cur) && nextLower { + out = append(out, string(runes[start:idx])) + start = idx + } + } + out = append(out, string(runes[start:])) + return out +} + +func identifierContainsWord(identifier string, word string) bool { + for _, candidate := range splitIdentifierWords(identifier) { + if strings.EqualFold(candidate, word) { + return true + } + } + return false +} + +func containsAnyIdentifierWord(identifier string, words []string) bool { + for _, word := range words { + if identifierContainsWord(identifier, word) { + return true + } + } + return false +} + +func isAllUpper(value string) bool { + hasLetter := false + for _, r := range value { + if !unicode.IsLetter(r) { + continue + } + hasLetter = true + if !unicode.IsUpper(r) { + return false + } + } + return hasLetter +} + +func firstSeenLine(seen map[string]int) int { + line := 0 + for _, candidate := range seen { + if line == 0 || candidate < line { + line = candidate + } + } + if line == 0 { + return 1 + } + return line +} + +func sortedKeys(seen map[string]int) string { + keys := make([]string, 0, len(seen)) + for key := range seen { + keys = append(keys, key) + } + sort.Strings(keys) + return strings.Join(keys, ", ") +} diff --git a/internal/codeguard/checks/quality/quality_python.go b/internal/codeguard/checks/quality/quality_python.go index 9151059..282b587 100644 --- a/internal/codeguard/checks/quality/quality_python.go +++ b/internal/codeguard/checks/quality/quality_python.go @@ -15,6 +15,7 @@ func pythonFindingsForFile(env support.Context, file string, data []byte) []core } if localPrecisionEnabled(env) { findings = append(findings, parsedPrecisionFindings(env, file, parsed)...) + findings = append(findings, parsedStructuralSmellFindings(env, file, parsed)...) } findings = append(findings, pythonAIQualityFindings(env, file, data)...) return append(fileLengthFindingWithSignals(env, file, data, findings), findings...) diff --git a/internal/codeguard/checks/quality/quality_smells.go b/internal/codeguard/checks/quality/quality_smells.go new file mode 100644 index 0000000..8025140 --- /dev/null +++ b/internal/codeguard/checks/quality/quality_smells.go @@ -0,0 +1,786 @@ +package quality + +import ( + "fmt" + "go/ast" + "go/token" + "regexp" + "sort" + "strings" + + "github.com/devr-tools/codeguard/internal/codeguard/checks/support" + "github.com/devr-tools/codeguard/internal/codeguard/core" +) + +const ( + smellGodObjectRuleID = "smell.god-object" + smellFeatureEnvyRuleID = "smell.feature-envy" + smellMiddleManRuleID = "smell.middle-man" + smellMessageChainRuleID = "smell.message-chain" + smellDataClumpRuleID = "smell.data-clump" + smellSwitchOnTypeRuleID = "smell.switch-on-type" +) + +var ( + pythonClassPattern = regexp.MustCompile(`^(\s*)class\s+([A-Za-z_]\w*)\b`) + pythonMethodPattern = regexp.MustCompile(`^(\s*)(?:async\s+)?def\s+([A-Za-z_]\w*)\s*\(([^)]*)\)\s*:`) + clikeClassPattern = regexp.MustCompile(`(?m)^[ \t]*(?:export[ \t]+)?(?:default[ \t]+)?(?:class|struct)[ \t]+([A-Za-z_$][\w$]*)[^{;]*\{`) + clikeMethodLinePattern = regexp.MustCompile(`^[ \t]*(?:(?:public|private|protected|static|async|virtual|override|inline|constexpr|const|explicit|final)\s+)*(?:[~A-Za-z_$][\w$:<>,*&\s]+\s+)?([~A-Za-z_$][\w$]*)\s*\(([^)]*)\)\s*(?:const\s*)?(?:override\s*)?(?:noexcept\s*)?\{`) + clikeFieldLinePattern = regexp.MustCompile(`^[ \t]*(?:(?:public|private|protected|static|readonly|mutable|const|let|var|final)\s+)*(?:[A-Za-z_$][\w$:<>,.?*&\[\]]+\s+)?([A-Za-z_$][\w$]*)\s*(?::[^=;]+)?(?:=[^;]+)?;`) + delegateReceiverPattern = regexp.MustCompile(`(?:return\s+)?(?:self|this|[a-zA-Z_]\w*)[.\->]+(_?[A-Za-z_]\w*)[.\->]+[A-Za-z_]\w*\s*\(`) + delegateLocalPattern = regexp.MustCompile(`(?:return\s+)?(_?[A-Za-z_]\w*)[.\->]+[A-Za-z_]\w*\s*\(`) + goKindSwitchPattern = regexp.MustCompile(`(?m)switch\s+[^{}\n]*(?:\.|_)?(?:kind|type|Kind|Type)\b`) + pythonKindBranchPattern = regexp.MustCompile(`(?m)\b(?:if|elif)\s+[^:\n]*(?:\.|_)?(?:kind|type)\b[^:\n]*(?:==| in )`) + scriptKindSwitchPattern = regexp.MustCompile(`(?m)switch\s*\([^)]*(?:\.|_)?(?:kind|type|Kind|Type)\b[^)]*\)`) + cppKindSwitchPattern = regexp.MustCompile(`(?m)switch\s*\([^)]*(?:\.|_)?(?:kind|type|Kind|Type)\b[^)]*\)`) + typeBranchPattern = regexp.MustCompile(`(?m)(?:\.\(type\)|\btypeid\s*\(|\bdynamic_cast\s*<|\binstanceof\b|\btypeof\b|\bisinstance\s*\(|\btype\s*\()`) + refusedBequestNoopRegexp = regexp.MustCompile(`(?i)\b(unsupported|not\s+implemented|notimplemented|throw\s+new\s+error|raise\s+notimplemented|panic\s*\()`) +) + +type structuralClass struct { + Name string + StartLine int + EndLine int + Fields []string + Methods []structuralFunction +} + +type structuralFunction struct { + Name string + StartLine int + EndLine int + Owner string + Receiver string + Params []support.ParsedParam + Body string +} + +func goStructuralSmellFindings(env support.Context, file string, fset *token.FileSet, parsed *ast.File, data []byte) []core.Finding { + classes, functions := goStructuralModel(fset, parsed, data) + return structuralSmellFindings(env, file, string(data), "go", classes, functions) +} + +func parsedStructuralSmellFindings(env support.Context, file string, parsed *support.ParsedFile) []core.Finding { + classes := sourceStructuralClasses(parsed.Source, parsed.Masked, parsed.Language) + functions := parsedStructuralFunctions(parsed) + for _, class := range classes { + functions = append(functions, class.Methods...) + } + return structuralSmellFindings(env, file, parsed.Source, parsed.Language, classes, functions) +} + +func structuralSmellFindings(env support.Context, file string, source string, language string, classes []structuralClass, functions []structuralFunction) []core.Finding { + if isQualityFixturePath(file) { + return nil + } + findings := make([]core.Finding, 0, len(classes)+len(functions)+3) + findings = append(findings, godObjectFindings(env, file, classes)...) + findings = append(findings, featureEnvyFindings(env, file, functions)...) + findings = append(findings, middleManFindings(env, file, classes)...) + findings = append(findings, messageChainFindings(env, file, source, language)...) + findings = append(findings, dataClumpFindings(env, file, functions)...) + findings = append(findings, switchOnTypeFindings(env, file, source, language)...) + return findings +} + +func goStructuralModel(fset *token.FileSet, parsed *ast.File, data []byte) ([]structuralClass, []structuralFunction) { + classesByName := make(map[string]*structuralClass) + functions := make([]structuralFunction, 0) + ast.Inspect(parsed, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.GenDecl: + for _, spec := range node.Specs { + typeSpec, ok := spec.(*ast.TypeSpec) + if !ok { + continue + } + class := &structuralClass{Name: typeSpec.Name.Name, StartLine: fset.Position(typeSpec.Pos()).Line, EndLine: fset.Position(typeSpec.End()).Line} + if structType, ok := typeSpec.Type.(*ast.StructType); ok && structType.Fields != nil { + for _, field := range structType.Fields.List { + if len(field.Names) == 0 { + class.Fields = append(class.Fields, goExprText(field.Type)) + continue + } + for _, name := range field.Names { + class.Fields = append(class.Fields, name.Name) + } + } + } + classesByName[class.Name] = class + } + case *ast.FuncDecl: + fn := goStructuralFunction(fset, node, data) + functions = append(functions, fn) + if fn.Owner != "" { + class := classesByName[fn.Owner] + if class == nil { + class = &structuralClass{Name: fn.Owner, StartLine: fn.StartLine, EndLine: fn.EndLine} + classesByName[fn.Owner] = class + } + class.Methods = append(class.Methods, fn) + if class.StartLine == 0 || fn.StartLine < class.StartLine { + class.StartLine = fn.StartLine + } + if fn.EndLine > class.EndLine { + class.EndLine = fn.EndLine + } + } + } + return true + }) + classes := make([]structuralClass, 0, len(classesByName)) + for _, class := range classesByName { + classes = append(classes, *class) + } + return classes, functions +} + +func goStructuralFunction(fset *token.FileSet, fn *ast.FuncDecl, data []byte) structuralFunction { + out := structuralFunction{ + Name: fn.Name.Name, + StartLine: fset.Position(fn.Pos()).Line, + EndLine: fset.Position(fn.End()).Line, + Params: goParsedParams(fn), + } + if fn.Recv != nil && len(fn.Recv.List) > 0 { + recv := fn.Recv.List[0] + out.Owner = strings.TrimPrefix(strings.TrimPrefix(goExprText(recv.Type), "*"), "[]") + if len(recv.Names) > 0 { + out.Receiver = recv.Names[0].Name + } + } + if fn.Body != nil { + start := fset.Position(fn.Body.Lbrace).Offset + end := fset.Position(fn.Body.Rbrace).Offset + if start >= 0 && end > start && end <= len(data) { + out.Body = string(data[start+1 : end]) + } + } + return out +} + +func parsedStructuralFunctions(parsed *support.ParsedFile) []structuralFunction { + parsedFunctions := parsed.AllFunctions() + functions := make([]structuralFunction, 0, len(parsedFunctions)) + for _, fn := range parsedFunctions { + functions = append(functions, structuralFunction{ + Name: fn.Name, + StartLine: fn.StartLine, + EndLine: fn.EndLine, + Params: fn.Params, + Body: maskedFunctionBody(fn), + }) + } + return functions +} + +func sourceStructuralClasses(source string, masked string, language string) []structuralClass { + if language == "python" { + return pythonStructuralClasses(masked) + } + return clikeStructuralClasses(source, masked) +} + +func pythonStructuralClasses(masked string) []structuralClass { + maskedLines := strings.Split(masked, "\n") + classes := make([]structuralClass, 0) + for idx := 0; idx < len(maskedLines); idx++ { + match := pythonClassPattern.FindStringSubmatch(maskedLines[idx]) + if match == nil { + continue + } + classIndent := len(match[1]) + class := structuralClass{Name: match[2], StartLine: idx + 1, EndLine: len(maskedLines)} + end := len(maskedLines) + for scan := idx + 1; scan < len(maskedLines); scan++ { + trimmed := strings.TrimSpace(maskedLines[scan]) + if trimmed == "" { + continue + } + if indentWidthOfLocal(maskedLines[scan]) <= classIndent { + end = scan + break + } + } + class.EndLine = end + for lineIdx := idx + 1; lineIdx < end; lineIdx++ { + trimmed := strings.TrimSpace(maskedLines[lineIdx]) + if strings.HasPrefix(trimmed, "self.") && strings.Contains(trimmed, "=") { + class.Fields = append(class.Fields, strings.TrimSpace(strings.SplitN(strings.TrimPrefix(trimmed, "self."), "=", 2)[0])) + } + if method := pythonStructuralMethod(maskedLines, lineIdx, end, classIndent, class.Name); method.Name != "" { + class.Methods = append(class.Methods, method) + } + } + classes = append(classes, class) + idx = end - 1 + } + return classes +} + +func pythonStructuralMethod(maskedLines []string, lineIdx int, classEnd int, classIndent int, owner string) structuralFunction { + match := pythonMethodPattern.FindStringSubmatch(maskedLines[lineIdx]) + if match == nil || len(match[1]) <= classIndent { + return structuralFunction{} + } + methodIndent := len(match[1]) + end := classEnd + for scan := lineIdx + 1; scan < classEnd; scan++ { + trimmed := strings.TrimSpace(maskedLines[scan]) + if trimmed == "" { + continue + } + if indentWidthOfLocal(maskedLines[scan]) <= methodIndent { + end = scan + break + } + } + params := simpleParams(match[3], "python") + receiver := "" + if len(params) > 0 && (params[0].Name == "self" || params[0].Name == "cls") { + receiver = params[0].Name + params = params[1:] + } + return structuralFunction{ + Name: match[2], + StartLine: lineIdx + 1, + EndLine: end, + Owner: owner, + Receiver: receiver, + Params: params, + Body: strings.Join(maskedLines[lineIdx+1:end], "\n"), + } +} + +func clikeStructuralClasses(source string, masked string) []structuralClass { + classes := make([]structuralClass, 0) + for _, match := range clikeClassPattern.FindAllStringSubmatchIndex(masked, -1) { + bodyOpen := match[1] - 1 + bodyEnd := matchBraceLocal(masked, bodyOpen) + if bodyEnd <= bodyOpen { + continue + } + bodyMasked := masked[bodyOpen+1 : bodyEnd] + bodySource := source[bodyOpen+1 : bodyEnd] + startLine := support.LineNumberForOffset(source, match[0]) + class := structuralClass{ + Name: masked[match[2]:match[3]], + StartLine: startLine, + EndLine: support.LineNumberForOffset(source, bodyEnd), + } + class.Fields = clikeClassFields(bodyMasked) + class.Methods = clikeClassMethods(bodySource, bodyMasked, startLine, class.Name) + classes = append(classes, class) + } + return classes +} + +func clikeClassFields(body string) []string { + fields := make([]string, 0) + for _, line := range strings.Split(body, "\n") { + if braceDepthBeforeLine(body, line) > 0 { + continue + } + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.Contains(trimmed, "(") || strings.HasSuffix(trimmed, ":") { + continue + } + if match := clikeFieldLinePattern.FindStringSubmatch(line); match != nil && !isCLikeAccessLabel(match[1]) { + fields = append(fields, match[1]) + } + } + return fields +} + +func clikeClassMethods(sourceBody string, maskedBody string, classStartLine int, owner string) []structuralFunction { + methods := make([]structuralFunction, 0) + offset := 0 + for offset < len(maskedBody) { + lineEnd := strings.IndexByte(maskedBody[offset:], '\n') + if lineEnd < 0 { + lineEnd = len(maskedBody) - offset + } + line := maskedBody[offset : offset+lineEnd] + if braceDepthAtOffset(maskedBody, offset) == 0 { + if match := clikeMethodLinePattern.FindStringSubmatchIndex(line); match != nil { + openInLine := strings.LastIndexByte(line[:match[1]], '{') + if openInLine >= 0 { + bodyOpen := offset + openInLine + bodyEnd := matchBraceLocal(maskedBody, bodyOpen) + if bodyEnd > bodyOpen { + params := simpleParams(line[match[4]:match[5]], "clike") + methods = append(methods, structuralFunction{ + Name: line[match[2]:match[3]], + StartLine: classStartLine + strings.Count(maskedBody[:offset], "\n"), + EndLine: classStartLine + strings.Count(maskedBody[:bodyEnd], "\n"), + Owner: owner, + Receiver: "this", + Params: params, + Body: sourceBody[bodyOpen+1 : bodyEnd], + }) + offset = bodyEnd + 1 + continue + } + } + } + } + offset += lineEnd + 1 + } + return methods +} + +func godObjectFindings(env support.Context, file string, classes []structuralClass) []core.Finding { + findings := make([]core.Finding, 0) + for _, class := range classes { + methods := len(class.Methods) + fields := len(uniqueStrings(class.Fields)) + responsibilities := classResponsibilities(class) + if methods >= 8 && (fields >= 5 || responsibilities >= 5 || methods >= 10) { + findings = append(findings, precisionWarnFinding(env, smellGodObjectRuleID, file, class.StartLine, + fmt.Sprintf("type %s has %d methods, %d fields, and %d responsibility clusters; split cohesive behavior into smaller collaborators", class.Name, methods, fields, responsibilities), + core.ConfidenceHigh)) + } + } + return findings +} + +func classResponsibilities(class structuralClass) int { + seen := make(map[string]struct{}) + for _, method := range class.Methods { + if bucket := responsibilityBucket(method.Name); bucket != "" { + seen[bucket] = struct{}{} + } + } + for _, field := range class.Fields { + if bucket := responsibilityBucket(field); bucket != "" { + seen[bucket] = struct{}{} + } + } + return len(seen) +} + +func responsibilityBucket(name string) string { + lowered := strings.ToLower(smellIdentifierWords(name)) + for _, bucket := range []string{"auth", "cache", "delete", "email", "event", "fetch", "find", "load", "notify", "parse", "persist", "render", "report", "save", "search", "send", "sync", "update", "validate"} { + if strings.Contains(lowered, bucket) { + return bucket + } + } + return "" +} + +func featureEnvyFindings(env support.Context, file string, functions []structuralFunction) []core.Finding { + findings := make([]core.Finding, 0) + for _, fn := range functions { + if len(fn.Params) == 0 || fn.Body == "" { + continue + } + dominantName, dominantCount, totalExternal := dominantExternalAccess(fn) + if dominantCount < 5 || totalExternal < 5 { + continue + } + ownCount := ownAccessCount(fn) + if dominantCount >= ownCount+4 && totalExternal >= ownCount+4 { + findings = append(findings, precisionWarnFinding(env, smellFeatureEnvyRuleID, file, fn.StartLine, + fmt.Sprintf("function %s accesses collaborator %s %d times versus %d own accesses; move behavior closer to the data owner or pass a richer operation", fn.Name, dominantName, dominantCount, ownCount), + core.ConfidenceMedium)) + } + } + return findings +} + +func dominantExternalAccess(fn structuralFunction) (string, int, int) { + bestName := "" + bestCount := 0 + total := 0 + for _, param := range fn.Params { + name := strings.TrimSpace(param.Name) + if name == "" || name == "_" || name == fn.Receiver { + continue + } + count := accessCount(fn.Body, name) + total += count + if count > bestCount { + bestName = name + bestCount = count + } + } + return bestName, bestCount, total +} + +func ownAccessCount(fn structuralFunction) int { + count := 0 + for _, own := range []string{fn.Receiver, "self", "this"} { + if own == "" { + continue + } + count += accessCount(fn.Body, own) + } + return count +} + +func accessCount(body string, name string) int { + pattern := regexp.MustCompile(`\b` + regexp.QuoteMeta(name) + `\s*(?:\.|->)`) + return len(pattern.FindAllStringIndex(body, -1)) +} + +func middleManFindings(env support.Context, file string, classes []structuralClass) []core.Finding { + findings := make([]core.Finding, 0) + for _, class := range classes { + if len(class.Methods) < 4 { + continue + } + delegates := make(map[string]int) + delegating := 0 + for _, method := range class.Methods { + if delegate := delegatedTarget(method.Body); delegate != "" { + delegating++ + delegates[delegate]++ + } + } + target, targetCount := dominantStringCount(delegates) + if delegating >= 4 && delegating*4 >= len(class.Methods)*3 && targetCount >= 3 { + findings = append(findings, precisionWarnFinding(env, smellMiddleManRuleID, file, class.StartLine, + fmt.Sprintf("type %s forwards %d of %d methods, mostly to %s, without visible policy or translation", class.Name, delegating, len(class.Methods), target), + core.ConfidenceHigh)) + } + } + return findings +} + +func delegatedTarget(body string) string { + trimmedLines := make([]string, 0) + for _, line := range strings.Split(body, "\n") { + line = strings.TrimSpace(strings.TrimSuffix(line, ";")) + if line == "" || line == "{" || line == "}" { + continue + } + trimmedLines = append(trimmedLines, line) + } + if len(trimmedLines) == 0 || len(trimmedLines) > 3 { + return "" + } + bodyText := strings.Join(trimmedLines, " ") + if refusedBequestNoopRegexp.MatchString(bodyText) { + return "" + } + for _, pattern := range []*regexp.Regexp{delegateReceiverPattern, delegateLocalPattern} { + if match := pattern.FindStringSubmatch(bodyText); match != nil { + return strings.TrimPrefix(match[1], "_") + } + } + return "" +} + +func messageChainFindings(env support.Context, file string, source string, language string) []core.Finding { + masked := maskForStructuralLanguage(source, language) + for idx, line := range strings.Split(masked, "\n") { + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.HasPrefix(trimmed, "import ") || strings.HasPrefix(trimmed, "#include") || strings.HasPrefix(trimmed, "package ") { + continue + } + if chainSeparators(trimmed) >= 4 && !looksLikeAllowedFluentChain(trimmed) { + return []core.Finding{precisionWarnFinding(env, smellMessageChainRuleID, file, idx+1, + "long message chain reaches through several collaborators; introduce a named query/helper at the boundary", + core.ConfidenceMedium)} + } + } + return nil +} + +func chainSeparators(line string) int { + line = strings.ReplaceAll(line, "->", ".") + line = strings.ReplaceAll(line, "::", ".") + line = strings.ReplaceAll(line, "?.", ".") + best := 0 + current := 0 + for i := 0; i < len(line); i++ { + switch line[i] { + case '.': + current++ + if current > best { + best = current + } + case ';', ',', '=', '+', '-', '*', '/', '<', '>', '{', '}': + current = 0 + } + } + return best +} + +func looksLikeAllowedFluentChain(line string) bool { + lowered := strings.ToLower(line) + return strings.Contains(lowered, "builder") || strings.Contains(lowered, ".with") || strings.Contains(lowered, ".set") +} + +func dataClumpFindings(env support.Context, file string, functions []structuralFunction) []core.Finding { + type occurrence struct { + line int + fn string + } + groups := make(map[string][]occurrence) + for _, fn := range functions { + key := primitiveParamGroup(fn.Params) + if key == "" { + continue + } + groups[key] = append(groups[key], occurrence{line: fn.StartLine, fn: fn.Name}) + } + for key, occurrences := range groups { + if len(occurrences) >= 3 { + return []core.Finding{precisionWarnFinding(env, smellDataClumpRuleID, file, occurrences[2].line, + fmt.Sprintf("parameter group [%s] appears in %d functions; extract a value object/options type", key, len(occurrences)), + core.ConfidenceHigh)} + } + } + return nil +} + +func primitiveParamGroup(params []support.ParsedParam) string { + names := make([]string, 0) + for _, param := range params { + name := normalizedParamConcept(param.Name) + if name == "" { + continue + } + if param.Type != "" && !primitiveTypePattern.MatchString(param.Type) { + continue + } + names = append(names, name) + } + names = uniqueStrings(names) + sort.Strings(names) + if len(names) < 3 { + return "" + } + return strings.Join(names, ", ") +} + +func normalizedParamConcept(name string) string { + name = strings.Trim(strings.ToLower(name), "_$") + if name == "" || name == "self" || name == "this" || name == "ctx" || name == "context" { + return "" + } + name = strings.ReplaceAll(name, "_", "") + name = strings.ReplaceAll(name, "-", "") + return name +} + +func switchOnTypeFindings(env support.Context, file string, source string, language string) []core.Finding { + masked := maskForStructuralLanguage(source, language) + typeBranches := len(typeBranchPattern.FindAllStringIndex(masked, -1)) + kindBranches := 0 + switch language { + case "go": + kindBranches = len(goKindSwitchPattern.FindAllStringIndex(masked, -1)) + case "python": + kindBranches = len(pythonKindBranchPattern.FindAllStringIndex(masked, -1)) + case "cpp": + kindBranches = len(cppKindSwitchPattern.FindAllStringIndex(masked, -1)) + default: + kindBranches = len(scriptKindSwitchPattern.FindAllStringIndex(masked, -1)) + } + caseBranches := strings.Count(masked, "case ") + total := typeBranches + kindBranches + if total >= 2 || (total >= 1 && caseBranches >= 4) || typeBranches >= 3 { + return []core.Finding{precisionWarnFinding(env, smellSwitchOnTypeRuleID, file, firstTypeBranchLine(masked), + fmt.Sprintf("type/kind branching appears %d times with %d case-style branches; prefer polymorphism or a dispatch table", total, caseBranches), + core.ConfidenceMedium)} + } + return nil +} + +func firstTypeBranchLine(masked string) int { + idx := len(masked) + for _, locs := range [][]int{ + firstMatch(typeBranchPattern, masked), + firstMatch(goKindSwitchPattern, masked), + firstMatch(pythonKindBranchPattern, masked), + firstMatch(scriptKindSwitchPattern, masked), + firstMatch(cppKindSwitchPattern, masked), + } { + if len(locs) == 2 && locs[0] < idx { + idx = locs[0] + } + } + if idx == len(masked) { + return 1 + } + return support.LineNumberForOffset(masked, idx) +} + +func firstMatch(pattern *regexp.Regexp, text string) []int { + return pattern.FindStringIndex(text) +} + +func maskForStructuralLanguage(source string, language string) string { + switch language { + case "python": + return support.MaskPythonSource(source) + case "go": + return support.MaskCLikeSource(source, support.CLikeGo) + case "cpp": + return support.MaskCLikeSource(source, support.CLikeCPP) + default: + return support.MaskCLikeSource(source, support.CLikeTypeScript) + } +} + +func simpleParams(paramText string, language string) []support.ParsedParam { + parts := splitTopLevelStructuralArgs(paramText) + params := make([]support.ParsedParam, 0, len(parts)) + for _, part := range parts { + part = strings.TrimSpace(part) + if part == "" { + continue + } + if eq := strings.Index(part, "="); eq >= 0 { + part = strings.TrimSpace(part[:eq]) + } + if language == "python" { + fields := strings.Split(part, ":") + params = append(params, support.ParsedParam{Name: strings.TrimSpace(fields[0]), Type: typePart(fields)}) + continue + } + if colon := strings.Index(part, ":"); colon >= 0 { + params = append(params, support.ParsedParam{Name: strings.TrimSpace(strings.TrimPrefix(part[:colon], "...")), Type: strings.TrimSpace(part[colon+1:])}) + continue + } + fields := strings.Fields(strings.ReplaceAll(part, "&", " ")) + if len(fields) > 0 { + params = append(params, support.ParsedParam{Name: strings.Trim(strings.TrimPrefix(fields[len(fields)-1], "*"), "&"), Type: strings.Join(fields[:len(fields)-1], " ")}) + } + } + return params +} + +func typePart(fields []string) string { + if len(fields) < 2 { + return "" + } + return strings.TrimSpace(fields[1]) +} + +func splitTopLevelStructuralArgs(text string) []string { + parts := make([]string, 0) + start := 0 + depth := 0 + for idx, r := range text { + switch r { + case '(', '[', '{', '<': + depth++ + case ')', ']', '}', '>': + if depth > 0 { + depth-- + } + case ',': + if depth == 0 { + parts = append(parts, text[start:idx]) + start = idx + 1 + } + } + } + parts = append(parts, text[start:]) + return parts +} + +func indentWidthOfLocal(line string) int { + width := 0 + for _, r := range line { + switch r { + case ' ': + width++ + case '\t': + width += 4 + default: + return width + } + } + return width +} + +func matchBraceLocal(text string, open int) int { + if open < 0 || open >= len(text) || text[open] != '{' { + return -1 + } + depth := 0 + for idx := open; idx < len(text); idx++ { + switch text[idx] { + case '{': + depth++ + case '}': + depth-- + if depth == 0 { + return idx + } + } + } + return -1 +} + +func braceDepthAtOffset(text string, offset int) int { + depth := 0 + for idx := 0; idx < offset && idx < len(text); idx++ { + switch text[idx] { + case '{': + depth++ + case '}': + if depth > 0 { + depth-- + } + } + } + return depth +} + +func braceDepthBeforeLine(body string, line string) int { + idx := strings.Index(body, line) + if idx < 0 { + return 0 + } + return braceDepthAtOffset(body, idx) +} + +func isCLikeAccessLabel(name string) bool { + return name == "public" || name == "private" || name == "protected" +} + +func smellIdentifierWords(name string) string { + var out strings.Builder + for idx, r := range name { + if idx > 0 && r >= 'A' && r <= 'Z' { + out.WriteByte(' ') + } + out.WriteRune(r) + } + return out.String() +} + +func uniqueStrings(values []string) []string { + seen := make(map[string]struct{}, len(values)) + out := make([]string, 0, len(values)) + for _, value := range values { + value = strings.TrimSpace(value) + if value == "" { + continue + } + if _, ok := seen[value]; ok { + continue + } + seen[value] = struct{}{} + out = append(out, value) + } + return out +} + +func dominantStringCount(values map[string]int) (string, int) { + bestName := "" + bestCount := 0 + for name, count := range values { + if count > bestCount { + bestName = name + bestCount = count + } + } + return bestName, bestCount +} diff --git a/internal/codeguard/checks/quality/quality_typescript.go b/internal/codeguard/checks/quality/quality_typescript.go index 2b027b1..8efef7b 100644 --- a/internal/codeguard/checks/quality/quality_typescript.go +++ b/internal/codeguard/checks/quality/quality_typescript.go @@ -33,6 +33,7 @@ func typeScriptFindingsForFile(env support.Context, file string, data []byte) [] } if localPrecisionEnabled(env) { findings = append(findings, parsedPrecisionFindings(env, file, parsed)...) + findings = append(findings, parsedStructuralSmellFindings(env, file, parsed)...) } return append(fileLengthFindingWithSignals(env, file, data, findings), findings...) } diff --git a/internal/codeguard/checks/quality/quality_typescript_target.go b/internal/codeguard/checks/quality/quality_typescript_target.go index d21c2ea..a8b3c66 100644 --- a/internal/codeguard/checks/quality/quality_typescript_target.go +++ b/internal/codeguard/checks/quality/quality_typescript_target.go @@ -24,7 +24,9 @@ func typeScriptTargetFindings(ctx context.Context, env support.Context, target c if localPrecisionEnabled(env) { findings = append(findings, env.ScanTargetFiles(target, "quality-typescript-local-precision", isTypeScriptLikeFile, func(file string, data []byte) []core.Finding { parsed := support.ParseCLike(string(data), support.CLikeTypeScript) - return parsedPrecisionFindings(env, file, parsed) + localFindings := parsedPrecisionFindings(env, file, parsed) + localFindings = append(localFindings, parsedStructuralSmellFindings(env, file, parsed)...) + return localFindings })...) } return findings diff --git a/internal/codeguard/checks/reliability/reliability_cpp.go b/internal/codeguard/checks/reliability/reliability_cpp.go index cb5b3bb..84ff6fe 100644 --- a/internal/codeguard/checks/reliability/reliability_cpp.go +++ b/internal/codeguard/checks/reliability/reliability_cpp.go @@ -15,9 +15,16 @@ var ( cppBackoffHint = regexp.MustCompile(`(?i)sleep_for|sleep_until|backoff|jitter`) cppThreadLaunch = regexp.MustCompile(`\bstd::(?:thread|jthread|async)\s*\(`) cppConcurrencyLimit = regexp.MustCompile(`semaphore|latch|barrier|thread_pool|executor|queue`) + cppCancellationHint = regexp.MustCompile(`(?i)\b(?:stop_token|stop_source|cancellation|cancel|deadline|timeout)\b`) + cppShutdownHint = regexp.MustCompile(`(?i)\b(?:SIGTERM|SIGINT|signal\(|sigaction|Shutdown|Stop|Drain|Graceful|join\(\)|request_stop)\b`) + cppServerStart = regexp.MustCompile(`(?i)\.(?:Listen|Serve|Run|Start)\s*\(`) + cppOutboundCall = regexp.MustCompile(`(?i)\.(?:Get|Post|Put|Patch|Delete|Fetch|Request|Send|Execute)\s*\(`) cppRawNew = regexp.MustCompile(`\bnew\s+[A-Za-z_:]\w*`) cppDeleteCall = regexp.MustCompile(`\bdelete\s+`) cppThrowRuntime = regexp.MustCompile(`\bthrow\s+std::(?:runtime_error|exception)\s*\(`) + cppCatchStart = regexp.MustCompile(`\bcatch\s*\([^)]*(?:exception|Error|Status)[^)]*(?:\berr\b|\berror\b|\be\b)[^)]*\)`) + cppLostContextThrow = regexp.MustCompile(`\bthrow\s+std::(?:runtime_error|logic_error|exception)\s*\(`) + cppSwallowedCatch = regexp.MustCompile(`^\s*(?:return\s*;|continue\s*;|break\s*;)\s*$`) cppNonIdempotent = regexp.MustCompile(`(?i)\b(?:post|put|patch|delete|create|update|save|insert|publish|send|charge|write)\w*\s*\(`) cppIdempotency = regexp.MustCompile(`(?i)idempot|dedupe|dedup|processed|message_id|event_id`) ) @@ -26,14 +33,18 @@ func cppFindingsForFile(env support.Context, file string, data []byte) []core.Fi source := strings.ReplaceAll(string(data), "\r\n", "\n") masked := support.MaskCLikeSource(source, support.CLikeCPP) scan := &cppReliabilityScan{ - env: env, - file: file, - rules: env.Config.Checks.ReliabilityRules, - limited: cppConcurrencyLimit.MatchString(masked), + env: env, + file: file, + rules: env.Config.Checks.ReliabilityRules, + limited: cppConcurrencyLimit.MatchString(masked), + cancellable: cppCancellationHint.MatchString(masked), + hasShutdown: cppShutdownHint.MatchString(masked), + catchDepth: -1, } for idx, line := range strings.Split(masked, "\n") { scan.consumeLine(idx+1, line) } + scan.finish() scan.findings = append(scan.findings, partialFailureHiddenFindings(env, file, data)...) return scan.findings } @@ -43,10 +54,14 @@ type cppReliabilityScan struct { file string rules core.ReliabilityRulesConfig limited bool + cancellable bool + hasShutdown bool depth int loops []int unboundedLoops []int newLine int + threadLines []int + catchDepth int findings []core.Finding } @@ -54,8 +69,21 @@ func (s *cppReliabilityScan) consumeLine(lineNo int, line string) { startsLoop := cppLoopStart.MatchString(line) inLoop := len(s.loops) > 0 || startsLoop inUnboundedLoop := len(s.unboundedLoops) > 0 || cppUnboundedLoop.MatchString(line) + inCatch := s.catchDepth >= 0 || cppCatchStart.MatchString(line) s.checkLine(lineNo, line, inLoop, inUnboundedLoop) + if enabled(s.rules.DetectLostErrorContext) && inCatch && cppLostContextThrow.MatchString(line) && !strings.Contains(line, "what()") { + s.add("reliability.lost-error-context", "warn", lineNo, "catch block replaces the original exception without preserving diagnostic context", "medium", "error", "throw-runtime-error") + } + if enabled(s.rules.DetectSwallowedError) && inCatch && cppSwallowedCatch.MatchString(line) { + s.add("reliability.swallowed-error", "fail", lineNo, "catch block exits without surfacing or preserving the caught exception", "high", "error", "catch-swallowed") + } next := s.depth + strings.Count(line, "{") - strings.Count(line, "}") + if cppCatchStart.MatchString(line) { + s.catchDepth = next - 1 + if s.catchDepth < 0 { + s.catchDepth = 0 + } + } if startsLoop && next > s.depth { s.loops = append(s.loops, s.depth) if cppUnboundedLoop.MatchString(line) { @@ -68,10 +96,16 @@ func (s *cppReliabilityScan) consumeLine(lineNo int, line string) { for len(s.unboundedLoops) > 0 && next <= s.unboundedLoops[len(s.unboundedLoops)-1] { s.unboundedLoops = s.unboundedLoops[:len(s.unboundedLoops)-1] } + if s.catchDepth >= 0 && next <= s.catchDepth { + s.catchDepth = -1 + } s.depth = next } func (s *cppReliabilityScan) checkLine(lineNo int, line string, inLoop bool, inUnboundedLoop bool) { + if enabled(s.rules.DetectMissingTimeout) && cppOutboundCall.MatchString(line) && !strings.Contains(strings.ToLower(line), "timeout") && !strings.Contains(strings.ToLower(line), "deadline") { + s.add("reliability.missing-timeout", "fail", lineNo, "outbound C++ dependency call has no visible timeout or deadline", "medium", "call", "dependency-without-timeout") + } if enabled(s.rules.DetectRetryWithoutBackoff) && inLoop && cppRetryHint.MatchString(line) && !cppBackoffHint.MatchString(line) { s.add("reliability.retry-without-backoff", "warn", lineNo, "retry-like C++ loop has no visible backoff or jitter", "medium", "retry", "no-backoff") } @@ -84,6 +118,15 @@ func (s *cppReliabilityScan) checkLine(lineNo int, line string, inLoop bool, inU if enabled(s.rules.DetectUnboundedWork) && inLoop && !s.limited && cppThreadLaunch.MatchString(line) { s.add("reliability.unbounded-work", "warn", lineNo, "C++ thread/task is launched inside a loop without a visible concurrency bound", "high", "work", "thread-in-loop") } + if cppThreadLaunch.MatchString(line) { + s.threadLines = append(s.threadLines, lineNo) + if enabled(s.rules.DetectMissingCancellation) && !s.cancellable { + s.add("reliability.missing-cancellation", "warn", lineNo, "C++ async work starts without visible stop_token, deadline, or cancellation propagation", "medium", "context", "cpp-async-work") + } + } + if enabled(s.rules.DetectMissingGracefulShutdown) && cppServerStart.MatchString(line) && !s.hasShutdown { + s.add("reliability.missing-graceful-shutdown", "warn", lineNo, "C++ service starts without visible signal handling, stop, drain, or graceful shutdown path", "medium", "server", "cpp-server-start") + } if enabled(s.rules.DetectRecoverablePanic) && cppThrowRuntime.MatchString(line) { s.add("reliability.recoverable-panic", "fail", lineNo, "production C++ code throws a generic runtime exception for a recoverable failure path", "medium", "exception", "runtime-error") } @@ -92,6 +135,16 @@ func (s *cppReliabilityScan) checkLine(lineNo int, line string, inLoop bool, inU } } +func (s *cppReliabilityScan) finish() { + limit := s.rules.MaxInlineGoroutinesPerFunction + if limit <= 0 { + limit = 4 + } + if enabled(s.rules.DetectMissingConcurrencyLimit) && !s.limited && len(s.threadLines) > limit { + s.add("reliability.missing-concurrency-limit", "warn", s.threadLines[0], "file starts multiple C++ threads/tasks without an obvious concurrency limit", "medium", "tasks", "cpp-thread-launches") + } +} + func (s *cppReliabilityScan) trackRawNew(lineNo int, line string) { if strings.Contains(line, "unique_ptr") || strings.Contains(line, "shared_ptr") || strings.Contains(line, "make_unique") || strings.Contains(line, "make_shared") { return diff --git a/internal/codeguard/checks/reliability/reliability_python.go b/internal/codeguard/checks/reliability/reliability_python.go index 963d78d..d0f4bba 100644 --- a/internal/codeguard/checks/reliability/reliability_python.go +++ b/internal/codeguard/checks/reliability/reliability_python.go @@ -15,8 +15,12 @@ var ( pyBackoffHint = regexp.MustCompile(`\b(?:sleep|backoff|jitter|wait_random|wait_exponential)\b`) pyTaskCreate = regexp.MustCompile(`\basyncio\.(?:create_task|ensure_future)\s*\(`) pyConcurrencyLimit = regexp.MustCompile(`\b(?:Semaphore|BoundedSemaphore|TaskGroup|CapacityLimiter)\s*\(`) + pyCancellationHint = regexp.MustCompile(`(?i)\b(?:cancel|cancelled|CancelledError|timeout|signal|lifespan|shutdown|request\.is_disconnected)\b`) + pyServerStart = regexp.MustCompile(`\b(?:uvicorn\.run|web\.run_app|app\.run|serve_forever|run_forever|start_server)\s*\(`) + pyShutdownHint = regexp.MustCompile(`(?i)\b(?:SIGTERM|SIGINT|signal\.|add_signal_handler|shutdown|lifespan|cleanup_ctx|on_shutdown|graceful)\b`) pySwallowedExcept = regexp.MustCompile(`^\s*(?:pass|return\s+None|return\s*$|continue)\s*(?:#.*)?$`) pyRecoverableRaise = regexp.MustCompile(`\braise\s+(?:RuntimeError|Exception)\s*\(`) + pyLostContextRaise = regexp.MustCompile(`^\s*raise\s+(?:RuntimeError|Exception|ValueError)\s*\(`) pyCloseCall = regexp.MustCompile(`\.(?:close|aclose)\s*\(`) pyOpenCall = regexp.MustCompile(`\bopen\s*\(`) pyIdempotencyHint = regexp.MustCompile(`(?i)idempot|dedupe|dedup|processed|message_id|event_id`) @@ -26,14 +30,17 @@ var ( func pythonFindingsForFile(env support.Context, file string, data []byte) []core.Finding { source := strings.ReplaceAll(string(data), "\r\n", "\n") scan := &pythonReliabilityScan{ - env: env, - file: file, - rules: env.Config.Checks.ReliabilityRules, - limited: pyConcurrencyLimit.MatchString(source), + env: env, + file: file, + rules: env.Config.Checks.ReliabilityRules, + limited: pyConcurrencyLimit.MatchString(source), + cancellable: pyCancellationHint.MatchString(source), + hasShutdown: pyShutdownHint.MatchString(source), } for idx, line := range strings.Split(source, "\n") { scan.consumeLine(idx+1, line) } + scan.finish() scan.findings = append(scan.findings, partialFailureHiddenFindings(env, file, data)...) return scan.findings } @@ -43,10 +50,13 @@ type pythonReliabilityScan struct { file string rules core.ReliabilityRulesConfig limited bool + cancellable bool + hasShutdown bool loops []pythonLoopRegion excepts []int openLine int openLineClosed bool + taskLines []int findings []core.Finding } @@ -95,9 +105,21 @@ func (s *pythonReliabilityScan) checkLine(lineNo int, line string, trimmed strin } s.add("reliability.unbounded-work", "warn", lineNo, message, "high", "work", detail) } + if pyTaskCreate.MatchString(line) { + s.taskLines = append(s.taskLines, lineNo) + if enabled(s.rules.DetectMissingCancellation) && !s.cancellable { + s.add("reliability.missing-cancellation", "warn", lineNo, "asyncio task is detached without visible cancellation, timeout, or shutdown propagation", "medium", "context", "detached-asyncio-task") + } + } + if enabled(s.rules.DetectMissingGracefulShutdown) && pyServerStart.MatchString(line) && !s.hasShutdown { + s.add("reliability.missing-graceful-shutdown", "warn", lineNo, "Python server or event loop starts without visible signal handling or graceful shutdown", "medium", "server", "python-server-start") + } if enabled(s.rules.DetectSwallowedError) && inExcept && pySwallowedExcept.MatchString(trimmed) { s.add("reliability.swallowed-error", "fail", lineNo, "exception handler swallows the error without reporting or returning it", "high", "error", "except-swallowed") } + if enabled(s.rules.DetectLostErrorContext) && inExcept && pyLostContextRaise.MatchString(trimmed) && !strings.Contains(trimmed, " from ") { + s.add("reliability.lost-error-context", "warn", lineNo, "exception handler replaces the original exception without chaining it with 'from'", "medium", "error", "raise-without-cause") + } if enabled(s.rules.DetectRecoverablePanic) && pyRecoverableRaise.MatchString(line) { s.add("reliability.recoverable-panic", "fail", lineNo, "production code raises a generic exception for a recoverable failure path", "medium", "exception", "generic-raise") } @@ -106,6 +128,16 @@ func (s *pythonReliabilityScan) checkLine(lineNo int, line string, trimmed strin } } +func (s *pythonReliabilityScan) finish() { + limit := s.rules.MaxInlineGoroutinesPerFunction + if limit <= 0 { + limit = 4 + } + if enabled(s.rules.DetectMissingConcurrencyLimit) && !s.limited && len(s.taskLines) > limit { + s.add("reliability.missing-concurrency-limit", "warn", s.taskLines[0], "file creates multiple asyncio tasks without an obvious concurrency limit", "medium", "tasks", "asyncio-create-task") + } +} + type pythonLoopRegion struct { indent int unbounded bool diff --git a/internal/codeguard/checks/reliability/reliability_typescript.go b/internal/codeguard/checks/reliability/reliability_typescript.go index dbf2c6b..8d0025d 100644 --- a/internal/codeguard/checks/reliability/reliability_typescript.go +++ b/internal/codeguard/checks/reliability/reliability_typescript.go @@ -16,8 +16,15 @@ var ( tsRetryHint = regexp.MustCompile(`(?i)retry|attempt|transient`) tsBackoffHint = regexp.MustCompile(`(?i)backoff|jitter|setTimeout|sleep|delay`) tsPromiseInLoop = regexp.MustCompile(`\b(?:new\s+Promise|fetch|axios\.|Promise\.all|[A-Za-z_$][\w$]*Async|fetch[A-Za-z_$][\w$]*)\s*\(`) - tsLimitHint = regexp.MustCompile(`p-limit|pLimit|Bottleneck|PQueue|Semaphore|AbortSignal|AbortController`) + tsLimitHint = regexp.MustCompile(`p-limit|pLimit|Bottleneck|PQueue|Semaphore|pool|queue|limit\s*\(`) + tsCancellationHint = regexp.MustCompile(`AbortSignal|AbortController|signal\s*:|signal\s*,|signal\s*\}|timeout|clearTimeout|controller\.abort`) + tsServerListen = regexp.MustCompile(`(?:\bapp|\bserver|createServer\s*\([^)]*\))\.listen\s*\(`) + tsShutdownHint = regexp.MustCompile(`(?i)SIGTERM|SIGINT|beforeExit|process\.on\s*\(|server\.close|shutdown|graceful|drain`) + tsDetachedPromise = regexp.MustCompile(`^\s*(?:void\s+)?(?:fetch|fetch[A-Z][A-Za-z0-9_$]*|axios\.|[A-Za-z_$][\w$]*Async)\s*\(`) tsSwallowedCatch = regexp.MustCompile(`catch\s*\([^)]*\)\s*\{\s*(?:return\s+undefined\s*;?|return\s*;?|console\.(?:log|warn|error)\([^)]*\)\s*;?)?\s*\}`) + tsLostContextCatch = regexp.MustCompile(`catch\s*\((?:err|error|e)\)\s*\{\s*throw\s+new\s+(?:Error|TypeError|RangeError)\s*\(`) + tsResourceOpen = regexp.MustCompile(`\b(?:fs\.openSync|fs\.createReadStream|fs\.createWriteStream|createReadStream|createWriteStream)\s*\(`) + tsResourceClose = regexp.MustCompile(`\.(?:close|destroy)\s*\(`) tsGenericThrow = regexp.MustCompile(`throw\s+new\s+(?:Error|TypeError|RuntimeError)\s*\(`) tsNonIdempotentCall = regexp.MustCompile(`(?i)\b(?:post|put|patch|delete|create|update|save|insert|publish|send|charge|write)\w*\s*\(`) tsIdempotencyHint = regexp.MustCompile(`(?i)idempot|dedupe|dedup|processed|messageId|eventId`) @@ -35,19 +42,25 @@ func typeScriptFindingsForFile(env support.Context, file string, data []byte) [] source := strings.ReplaceAll(string(data), "\r\n", "\n") code := support.StripTypeScriptCommentsAndStrings(source) scan := &tsReliabilityScan{ - env: env, - file: file, - rules: env.Config.Checks.ReliabilityRules, - limited: tsLimitHint.MatchString(source), + env: env, + file: file, + rules: env.Config.Checks.ReliabilityRules, + limited: tsLimitHint.MatchString(source), + cancellable: tsCancellationHint.MatchString(source), + hasShutdown: tsShutdownHint.MatchString(source), } for idx, line := range strings.Split(code, "\n") { scan.consumeLine(idx+1, line) } + scan.finish() if enabled(scan.rules.DetectSwallowedError) { for idx, rawLine := range strings.Split(source, "\n") { if tsSwallowedCatch.MatchString(rawLine) { scan.add("reliability.swallowed-error", "fail", idx+1, "catch block swallows an error without returning or propagating it", "high", "error", "catch-swallowed") } + if enabled(scan.rules.DetectLostErrorContext) && tsLostContextCatch.MatchString(rawLine) { + scan.add("reliability.lost-error-context", "warn", idx+1, "catch block replaces the original error without preserving it as cause", "medium", "error", "throw-new-error") + } } } scan.findings = append(scan.findings, partialFailureHiddenFindings(env, file, data)...) @@ -59,9 +72,13 @@ type tsReliabilityScan struct { file string rules core.ReliabilityRulesConfig limited bool + cancellable bool + hasShutdown bool depth int loops []int unboundedLoops []int + detachedLines []int + resourceLine int findings []core.Finding } @@ -93,6 +110,18 @@ func (s *tsReliabilityScan) checkLine(lineNo int, line string, inLoop bool, inUn if enabled(s.rules.DetectUnboundedWork) && inLoop && !s.limited && tsPromiseInLoop.MatchString(line) { s.add("reliability.unbounded-work", "warn", lineNo, "promise or HTTP work starts inside a loop without a visible concurrency limit", "medium", "work", "promise-in-loop") } + if tsDetachedPromise.MatchString(line) { + s.detachedLines = append(s.detachedLines, lineNo) + if enabled(s.rules.DetectMissingCancellation) && !s.cancellable { + s.add("reliability.missing-cancellation", "warn", lineNo, "detached async work starts without visible AbortSignal, timeout, or cancellation propagation", "medium", "context", "detached-promise") + } + } + if enabled(s.rules.DetectMissingGracefulShutdown) && tsServerListen.MatchString(line) && !s.hasShutdown { + s.add("reliability.missing-graceful-shutdown", "warn", lineNo, "JavaScript server starts without visible signal handling or server.close shutdown path", "medium", "server", "listen") + } + if enabled(s.rules.DetectResourceLeak) { + s.trackResourceLeak(lineNo, line) + } if enabled(s.rules.DetectRetryWithoutBackoff) && inLoop && tsRetryHint.MatchString(line) && !tsBackoffHint.MatchString(line) { s.add("reliability.retry-without-backoff", "warn", lineNo, "retry-like JavaScript loop has no visible backoff or jitter", "medium", "retry", "no-backoff") } @@ -107,6 +136,29 @@ func (s *tsReliabilityScan) checkLine(lineNo int, line string, inLoop bool, inUn } } +func (s *tsReliabilityScan) trackResourceLeak(lineNo int, line string) { + if tsResourceOpen.MatchString(line) { + s.resourceLine = lineNo + } + if s.resourceLine > 0 && tsResourceClose.MatchString(line) { + s.resourceLine = 0 + } + if s.resourceLine > 0 && lineNo > s.resourceLine+8 { + s.add("reliability.resource-leak", "fail", s.resourceLine, "Node.js file or stream resource is not closed near the acquisition path", "medium", "resource", "node-stream") + s.resourceLine = 0 + } +} + +func (s *tsReliabilityScan) finish() { + limit := s.rules.MaxInlineGoroutinesPerFunction + if limit <= 0 { + limit = 4 + } + if enabled(s.rules.DetectMissingConcurrencyLimit) && !s.limited && len(s.detachedLines) > limit { + s.add("reliability.missing-concurrency-limit", "warn", s.detachedLines[0], "file starts multiple detached async operations without an obvious concurrency limit", "medium", "tasks", "detached-promises") + } +} + func (s *tsReliabilityScan) add(ruleID string, level string, lineNo int, message string, confidence string, metaKey string, metaValue string) { s.findings = append(s.findings, newFinding(s.env, ruleID, level, s.file, lineNo, 1, message, confidence, metaKey, metaValue)) } diff --git a/internal/codeguard/config/defaults_rules.go b/internal/codeguard/config/defaults_rules.go index abccccf..f86bdcc 100644 --- a/internal/codeguard/config/defaults_rules.go +++ b/internal/codeguard/config/defaults_rules.go @@ -18,6 +18,18 @@ func applyQualityDefaults(dst *core.QualityRulesConfig, def core.QualityRulesCon applyCoverageDeltaDefaults(&dst.CoverageDelta) applyCPPToolingDefaults(&dst.CPPTooling) defaultBoolPtr(&dst.LocalPrecision, boolValueOrTrue(def.LocalPrecision)) + applyQualityNamingDefaults(&dst.Naming, def.Naming) +} + +func applyQualityNamingDefaults(dst *core.QualityNamingConfig, def core.QualityNamingConfig) { + defaultStringSlice(&dst.AllowedAbbreviations, def.AllowedAbbreviations) + if dst.RoleSuffixWarnThreshold == 0 { + if def.RoleSuffixWarnThreshold != 0 { + dst.RoleSuffixWarnThreshold = def.RoleSuffixWarnThreshold + } else { + dst.RoleSuffixWarnThreshold = 4 + } + } } func applyRiskScoringDefaults(dst *core.RiskScoringConfig) { diff --git a/internal/codeguard/config/example_rules.go b/internal/codeguard/config/example_rules.go index 6eddd70..9482ef9 100644 --- a/internal/codeguard/config/example_rules.go +++ b/internal/codeguard/config/example_rules.go @@ -27,6 +27,10 @@ func exampleQualityRules() core.QualityRulesConfig { CompilerMode: core.ExternalToolModeOff, CompilerCommand: "clang++", }, + Naming: core.QualityNamingConfig{ + AllowedAbbreviations: []string{"api", "cli", "cpp", "db", "html", "http", "https", "id", "io", "ip", "js", "json", "orm", "rpc", "sdk", "sql", "tcp", "ts", "ui", "url", "uuid", "xml", "yaml"}, + RoleSuffixWarnThreshold: 4, + }, } } diff --git a/internal/codeguard/config/profile_test.go b/internal/codeguard/config/profile_test.go index 08f1263..a1e2666 100644 --- a/internal/codeguard/config/profile_test.go +++ b/internal/codeguard/config/profile_test.go @@ -57,6 +57,36 @@ func TestProfilesPreserveExpectedPolicyValues(t *testing.T) { } } +func TestReviewProfilesEnableLocalPrecisionAndRegressionSignals(t *testing.T) { + aiSafe, err := ExampleConfigForProfile("ai-safe") + if err != nil { + t.Fatalf("ExampleConfigForProfile(ai-safe) error = %v", err) + } + if aiSafe.Checks.QualityRules.LocalPrecision != nil && !*aiSafe.Checks.QualityRules.LocalPrecision { + t.Fatal("ai-safe profile must not disable local quality precision checks") + } + if aiSafe.Checks.Change == nil || !*aiSafe.Checks.Change { + t.Fatal("ai-safe profile should enable change-safety regression checks") + } + if aiSafe.Checks.Reliability == nil || !*aiSafe.Checks.Reliability { + t.Fatal("ai-safe profile should enable reliability checks") + } + + strict, err := ExampleConfigForProfile("strict") + if err != nil { + t.Fatalf("ExampleConfigForProfile(strict) error = %v", err) + } + if strict.Checks.Change == nil || !*strict.Checks.Change { + t.Fatal("strict profile should enable change-safety regression checks") + } + if strict.Checks.Data == nil || *strict.Checks.Data { + t.Fatal("strict profile should focus regressions without enabling data-correctness production-readiness checks") + } + if strict.Checks.Observability == nil || *strict.Checks.Observability { + t.Fatal("strict profile should focus regressions without enabling observability production-readiness checks") + } +} + func expectedProfileThresholds() map[string]profileThresholds { profiles := baselineAndStartupProfileThresholds() for name, thresholds := range strictProfileThresholds() { diff --git a/internal/codeguard/config/validate.go b/internal/codeguard/config/validate.go index 23106a8..09b8be7 100644 --- a/internal/codeguard/config/validate.go +++ b/internal/codeguard/config/validate.go @@ -23,6 +23,7 @@ func Validate(cfg core.Config) error { validateAIChangeRisk(cfg.Checks.QualityRules.AIChangeRisk), validateRiskScoring(cfg.Checks.QualityRules.RiskScoring), validateAIChecks(cfg.Checks.QualityRules.AIChecks), + validateQualityNaming(cfg.Checks.QualityRules.Naming), validateSupplyChainRules(cfg.Checks.SupplyChainRules), validateDeliveryRules(cfg.Checks.DeliveryRules), validateReliabilityRules(cfg.Checks.ReliabilityRules), @@ -46,6 +47,28 @@ func Validate(cfg core.Config) error { ) } +func validateQualityNaming(cfg core.QualityNamingConfig) error { + if cfg.RoleSuffixWarnThreshold < 0 { + return fmt.Errorf("quality_rules.naming.role_suffix_warn_threshold must not be negative, got %d", cfg.RoleSuffixWarnThreshold) + } + for concept, entry := range cfg.Glossary { + if strings.TrimSpace(concept) == "" { + return errors.New("quality_rules.naming.glossary contains a blank concept") + } + for idx, avoided := range entry.Avoid { + if strings.TrimSpace(avoided) == "" { + return fmt.Errorf("quality_rules.naming.glossary.%s.avoid[%d] must not be blank", concept, idx) + } + } + } + for idx, abbreviation := range cfg.AllowedAbbreviations { + if strings.TrimSpace(abbreviation) == "" { + return fmt.Errorf("quality_rules.naming.allowed_abbreviations[%d] must not be blank", idx) + } + } + return nil +} + func validateExternalReports(reports []core.ExternalReportConfig) error { for i, report := range reports { field := fmt.Sprintf("external_reports[%d]", i) diff --git a/internal/codeguard/core/config_rule_types.go b/internal/codeguard/core/config_rule_types.go index 00fdefb..3550e7b 100644 --- a/internal/codeguard/core/config_rule_types.go +++ b/internal/codeguard/core/config_rule_types.go @@ -14,6 +14,17 @@ type QualityRulesConfig struct { CoverageDelta CoverageDeltaConfig `json:"coverage_delta,omitempty" yaml:"coverage_delta,omitempty"` CPPTooling CPPToolingConfig `json:"cpp_tooling,omitempty" yaml:"cpp_tooling,omitempty"` LocalPrecision *bool `json:"local_precision,omitempty" yaml:"local_precision,omitempty"` + Naming QualityNamingConfig `json:"naming,omitempty" yaml:"naming,omitempty"` +} + +type QualityNamingConfig struct { + Glossary map[string]QualityNamingGlossaryEntry `json:"glossary,omitempty" yaml:"glossary,omitempty"` + AllowedAbbreviations []string `json:"allowed_abbreviations,omitempty" yaml:"allowed_abbreviations,omitempty"` + RoleSuffixWarnThreshold int `json:"role_suffix_warn_threshold,omitempty" yaml:"role_suffix_warn_threshold,omitempty"` +} + +type QualityNamingGlossaryEntry struct { + Avoid []string `json:"avoid,omitempty" yaml:"avoid,omitempty"` } // PerformanceRulesConfig tunes the performance section (checks.performance). diff --git a/internal/codeguard/rules/catalog.go b/internal/codeguard/rules/catalog.go index 0bed1db..c9687e5 100644 --- a/internal/codeguard/rules/catalog.go +++ b/internal/codeguard/rules/catalog.go @@ -4,6 +4,8 @@ import "github.com/devr-tools/codeguard/internal/codeguard/core" var catalog = withSecurityOWASP(mergeRuleCatalogs( qualityCatalog, + qualitySmellCatalog, + qualityErrorDefensiveCatalog, qualityAICatalog, performanceCatalog, performanceRegressionCatalog, diff --git a/internal/codeguard/rules/catalog_fix_templates.go b/internal/codeguard/rules/catalog_fix_templates.go index b67c24d..501658e 100644 --- a/internal/codeguard/rules/catalog_fix_templates.go +++ b/internal/codeguard/rules/catalog_fix_templates.go @@ -17,6 +17,7 @@ const ( var fixTemplates = mergeFixTemplates( qualityFixTemplates, qualityAIFixTemplates, + qualityErrorDefensiveFixTemplates, performanceFixTemplates, performanceRegressionFixTemplates, performanceFrameworkFixTemplates, diff --git a/internal/codeguard/rules/catalog_fix_templates_quality.go b/internal/codeguard/rules/catalog_fix_templates_quality.go index 0299886..88da9e4 100644 --- a/internal/codeguard/rules/catalog_fix_templates_quality.go +++ b/internal/codeguard/rules/catalog_fix_templates_quality.go @@ -40,5 +40,25 @@ var qualityFixTemplates = map[string]core.FixTemplate{ "quality.hidden-side-effect": {Kind: guided, Text: "Rename hidden mutations or split queries/builders from side effects.\n\nBefore:\nfunc buildInvoice() Invoice { repo.Save(...); return invoice }\n\nAfter:\nfunc saveInvoice() (Invoice, error)"}, "quality.mutable-global-state": {Kind: guided, Text: "Move mutable global state behind an instance or synchronized owner.\n\nBefore:\nvar currentUser User\n\nAfter:\ntype SessionStore struct { currentUser User }"}, "quality.redundant-comment": {Kind: deterministic, Text: "Delete comments that restate the next line, or replace them with intent/constraint context.\n\nBefore:\n// validate input\nvalidateInput(input)\n\nAfter:\nvalidateInput(input)"}, + "naming.behavior-mismatch": {Kind: guided, Text: "Rename the function to match the behavior or split the behavior into a query and a command.\n\nBefore:\nfunc getUser(id string) User { audit.Save(id); return repo.Find(id) }\n\nAfter:\nfunc getUser(id string) User { return repo.Find(id) }\nfunc recordUserLookup(id string) { audit.Save(id) }"}, + "naming.boolean-not-predicate": {Kind: deterministic, Text: "Rename boolean values so they read as predicates.\n\nBefore:\nactive := user.Enabled\n\nAfter:\nisActive := user.Enabled"}, + "naming.domain-vocabulary-drift": {Kind: guided, Text: "Use one glossary term for the same domain concept.\n\nBefore:\ntype Restaurant struct{}\ntype VenueDTO struct{}\n\nAfter:\ntype Restaurant struct{}\ntype RestaurantDTO struct{}"}, + "naming.unknown-abbreviation": {Kind: guided, Text: "Spell out unclear abbreviations or add intentional repo vocabulary to quality_rules.naming.allowed_abbreviations.\n\nBefore:\nfunc loadCustAcct()\n\nAfter:\nfunc loadCustomerAccount()"}, + "naming.cardinality-mismatch": {Kind: deterministic, Text: "Align singular/plural wording with the value shape.\n\nBefore:\nuser := []User{}\ncountItems := 1\n\nAfter:\nusers := []User{}\nitemCount := 1"}, + "naming.implementation-leak": {Kind: guided, Text: "Keep domain/API names independent of storage or transport details.\n\nBefore:\ntype SqlOrderResponse struct{}\n\nAfter:\ntype OrderResponse struct{}"}, + "naming.missing-unit": {Kind: deterministic, Text: "Add the unit suffix to numeric duration, size, and money values.\n\nBefore:\ntimeout := 30\n\nAfter:\ntimeoutSeconds := 30"}, + "naming.role-suffix-overuse": {Kind: guided, Text: "Replace vague role suffixes with domain responsibilities.\n\nBefore:\nOrderManager, InvoiceHelper, PaymentProcessor\n\nAfter:\nOrderApprover, InvoiceFormatter, PaymentCapturer"}, + "naming.cross-layer-inconsistency": {Kind: guided, Text: "Use the same concept name across API, domain, and persistence layers.\n\nBefore:\nRestaurantResponse, VenueEntity, MerchantRecord\n\nAfter:\nRestaurantResponse, RestaurantEntity, RestaurantRecord"}, + "function.hidden-mutation": {Kind: guided, Text: "Expose mutation in the function name or isolate it in a command.\n\nBefore:\nfunc enrich(user *User) { user.Name = normalize(user.Name) }\n\nAfter:\nfunc normalizeUserInPlace(user *User) { user.Name = normalize(user.Name) }"}, + "function.inconsistent-return-contract": {Kind: guided, Text: "Return one explicit shape for success, not-found, and error states.\n\nBefore:\nif missing { return nil }\nreturn user\n\nAfter:\nreturn UserResult{User: user, Found: true}"}, + "function.multiple-responsibilities": {Kind: guided, Text: "Extract validation, load, write, send, cache, transform, and observe steps into focused helpers.\n\nBefore:\nfunc handle() { validate(); load(); save(); send(); log() }\n\nAfter:\nfunc handle() { command := validate(); result := service.Apply(command); respond(result) }"}, + "function.orchestration-domain-mix": {Kind: guided, Text: "Keep handlers/jobs focused on boundary orchestration and move domain decisions into a domain helper.\n\nBefore:\nfunc handler(req Request) { if order.Total > 100 { db.Save(order) } }\n\nAfter:\nfunc handler(req Request) { decision := orderPolicy.Decide(order); store.Save(decision) }"}, + "function.partial-result": {Kind: guided, Text: "Make partial results explicit or return no value on failure.\n\nBefore:\nreturn profile, err\n\nAfter:\nif err != nil { return Profile{}, err }\nreturn profile, nil"}, "quality.environment-branching": {Kind: guided, Text: "Move environment-specific behavior out of domain code and into configuration or bootstrap wiring.\n\nBefore:\nif os.Getenv(\"ENV\") == \"production\" {\n\tclient = realGateway\n} else {\n\tclient = fakeGateway\n}\n\nAfter:\n// bootstrap/config selects Gateway once\nservice := NewService(configuredGateway)\n// domain code uses the injected Gateway without checking deployment environment"}, + "smell.god-object": {Kind: guided, Text: "Split a broad type into cohesive collaborators.\n\nBefore:\ntype AccountService struct { repo Repo; cache Cache; mailer Mailer; renderer Renderer }\n// validates, persists, emails, renders, reports, and syncs\n\nAfter:\ntype AccountValidator struct { ... }\ntype AccountNotifier struct { ... }\ntype AccountReporter struct { ... }\n// AccountService coordinates named collaborators."}, + "smell.feature-envy": {Kind: guided, Text: "Move behavior closer to the collaborator whose data is being inspected.\n\nBefore:\nfunc total(order Order) Money { return order.Customer.Account.Region.Currency.Limit }\n\nAfter:\nfunc (order Order) SpendingLimit() Money { return order.Customer.Account.Region.Currency.Limit }"}, + "smell.middle-man": {Kind: guided, Text: "Remove pass-through wrappers or add real policy/translation.\n\nBefore:\nfunc (s Service) Create(x X) error { return s.client.Create(x) }\nfunc (s Service) Update(x X) error { return s.client.Update(x) }\n\nAfter:\n// call client directly, or keep Service only if it validates, authorizes, maps, retries, or records domain policy."}, + "smell.message-chain": {Kind: guided, Text: "Hide deep traversal behind a named operation at the boundary.\n\nBefore:\ncountry := user.Account().Profile().Address().Country().Code()\n\nAfter:\ncountry := user.CountryCode()"}, + "smell.data-clump": {Kind: guided, Text: "Extract repeated primitive parameters into a named value object or options type.\n\nBefore:\nfunc create(customerID string, orderID string, currency string)\nfunc update(customerID string, orderID string, currency string)\n\nAfter:\ntype OrderKey struct { CustomerID, OrderID, Currency string }\nfunc create(key OrderKey)"}, + "smell.switch-on-type": {Kind: guided, Text: "Centralize type/kind dispatch or move behavior into polymorphic implementations.\n\nBefore:\nswitch event.Kind { case Created: handleCreated(event); case Updated: handleUpdated(event) }\n\nAfter:\nhandlers[event.Kind].Handle(event)\n// or event.HandleWith(handler) when the variants own behavior."}, } diff --git a/internal/codeguard/rules/catalog_fix_templates_quality_errors_defensive.go b/internal/codeguard/rules/catalog_fix_templates_quality_errors_defensive.go new file mode 100644 index 0000000..05000ae --- /dev/null +++ b/internal/codeguard/rules/catalog_fix_templates_quality_errors_defensive.go @@ -0,0 +1,29 @@ +package rules + +import "github.com/devr-tools/codeguard/internal/codeguard/core" + +var qualityErrorDefensiveFixTemplates = map[string]core.FixTemplate{ + "error.logged-and-returned": {Kind: guided, Text: "Pick one logging owner for the error path.\n\nBefore:\nlog.Printf(\"save user: %v\", err)\nreturn err\n\nAfter:\nreturn fmt.Errorf(\"save user %s: %w\", userID, err)"}, + "error.generic-message": {Kind: guided, Text: "Replace generic text with safe operation/resource context.\n\nBefore:\nreturn errors.New(\"failed\")\n\nAfter:\nreturn fmt.Errorf(\"parse payment webhook %s: %w\", eventID, err)"}, + "error.wrong-abstraction-level": {Kind: guided, Text: "Translate low-level infrastructure details at the boundary, preserving the cause internally."}, + "error.inconsistent-wrapping": {Kind: guided, Text: "Use one error wrapping style throughout the function so callers can rely on a consistent contract."}, + "error.retryable-not-distinguished": {Kind: guided, Text: "Classify transient/retryable failures separately from permanent failures before retrying."}, + "error.user-message-leaks-internals": {Kind: guided, Text: "Return a safe user message and record DB/transport/stack details only in internal logs or traces."}, + "error.partial-failure-hidden": {Kind: guided, Text: "Return aggregate errors or an explicit partial-result contract for skipped failed items."}, + "error.cleanup-error-ignored": {Kind: guided, Text: "Check close/rollback/delete errors and join them with the primary error when both happen."}, + "error.panic-on-recoverable-path": {Kind: guided, Text: "Return a typed error for recoverable request, validation, parsing, or I/O failures instead of panicking."}, + "error.exception-used-for-control-flow": {Kind: guided, Text: "Use explicit branch results, optionals, or status values for expected outcomes instead of throw/raise."}, + "error.fallback-hides-corruption": {Kind: guided, Text: "Surface decode/validation corruption instead of silently returning default data."}, + "defensive.unvalidated-boundary-input": {Kind: guided, Text: "Validate request, event, payload, or body input before consuming fields or passing it downstream."}, + "defensive.invalid-state-representable": {Kind: guided, Text: "Replace boolean combinations/raw strings with an enum, tagged union, or state machine that encodes valid states."}, + "defensive.null-assumption": {Kind: guided, Text: "Guard nil/null/None/optional values before dereference, or make the boundary type non-nullable."}, + "defensive.integer-overflow": {Kind: guided, Text: "Guard count/size arithmetic before multiplication, addition, shifts, or allocation sizing."}, + "defensive.bounds-assumption": {Kind: guided, Text: "Check length/existence before indexing, or use a safe lookup API."}, + "defensive.unsafe-default": {Kind: guided, Text: "Make security/safety defaults fail closed and require explicit opt-out for unsafe behavior."}, + "defensive.non-exhaustive-branch": {Kind: guided, Text: "Add an explicit default/unreachable branch or exhaustive assertion for enum-like state switches."}, + "defensive.unchecked-external-response": {Kind: guided, Text: "Check transport errors and response status/ok before reading or trusting the body."}, + "defensive.missing-schema-validation": {Kind: guided, Text: "Validate decoded JSON/event payloads with a schema or invariant-checking constructor."}, + "defensive.missing-resource-limit": {Kind: guided, Text: "Apply explicit maximum bytes, item counts, deadlines, or quotas to boundary reads/uploads."}, + "defensive.invalid-state-transition": {Kind: guided, Text: "Route state changes through a transition helper that checks current and next states."}, + "defensive.fail-open-authorization": {Kind: guided, Text: "Fail closed on authorization errors and require an explicit allow decision."}, +} diff --git a/internal/codeguard/rules/catalog_quality.go b/internal/codeguard/rules/catalog_quality.go index d11798f..b63e7c7 100644 --- a/internal/codeguard/rules/catalog_quality.go +++ b/internal/codeguard/rules/catalog_quality.go @@ -360,6 +360,146 @@ var qualityCatalog = map[string]core.RuleMetadata{ Description: "Warns when a comment repeats nearby code instead of explaining intent, constraints, or tradeoffs.", HowToFix: "Delete the restatement or replace it with context that is not visible from the code.", }, + "naming.behavior-mismatch": { + ID: "naming.behavior-mismatch", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage(core.RuleLanguageGo, core.RuleLanguagePython, core.RuleLanguageTypeScript, core.RuleLanguageJavaScript, core.RuleLanguageCPP), + Title: "Name behavior mismatch", + Description: "Warns when query/build/format names perform side effects or command-style names only read.", + HowToFix: "Rename the function to match the dominant behavior or split query and command responsibilities.", + }, + "naming.boolean-not-predicate": { + ID: "naming.boolean-not-predicate", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage(core.RuleLanguageGo, core.RuleLanguagePython, core.RuleLanguageTypeScript, core.RuleLanguageJavaScript, core.RuleLanguageCPP), + Title: "Boolean name is not a predicate", + Description: "Warns when boolean variables, parameters, or boolean-returning functions do not read like predicates.", + HowToFix: "Use names such as isReady, hasAccess, canRetry, shouldSend, or enabled to communicate truth semantics.", + }, + "naming.domain-vocabulary-drift": { + ID: "naming.domain-vocabulary-drift", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage(core.RuleLanguageGo, core.RuleLanguagePython, core.RuleLanguageTypeScript, core.RuleLanguageJavaScript, core.RuleLanguageCPP), + Title: "Domain vocabulary drift", + Description: "Warns when configured glossary concepts appear under multiple terms in the same source.", + HowToFix: "Choose one domain term, configure accepted abbreviations/glossary intentionally, and rename drifted identifiers.", + }, + "naming.unknown-abbreviation": { + ID: "naming.unknown-abbreviation", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage(core.RuleLanguageGo, core.RuleLanguagePython, core.RuleLanguageTypeScript, core.RuleLanguageJavaScript, core.RuleLanguageCPP), + Title: "Unknown abbreviation", + Description: "Warns when identifiers contain abbreviations that are not common or configured for the repository.", + HowToFix: "Spell the term out or add an intentional abbreviation to quality_rules.naming.allowed_abbreviations.", + }, + "naming.cardinality-mismatch": { + ID: "naming.cardinality-mismatch", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage(core.RuleLanguageGo, core.RuleLanguagePython, core.RuleLanguageTypeScript, core.RuleLanguageJavaScript, core.RuleLanguageCPP), + Title: "Cardinality mismatch", + Description: "Warns when plural names are used for scalar values or singular names are used for collection-like values.", + HowToFix: "Align the identifier with the value shape, such as user for one value or users for a collection.", + }, + "naming.implementation-leak": { + ID: "naming.implementation-leak", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage(core.RuleLanguageGo, core.RuleLanguagePython, core.RuleLanguageTypeScript, core.RuleLanguageJavaScript, core.RuleLanguageCPP), + Title: "Implementation vocabulary leak", + Description: "Warns when domain-facing names encode infrastructure details such as SQL, HTTP, Redis, Kafka, or ORM.", + HowToFix: "Keep domain/API names technology-neutral and move infrastructure terms behind adapters or repositories.", + }, + "naming.missing-unit": { + ID: "naming.missing-unit", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage(core.RuleLanguageGo, core.RuleLanguagePython, core.RuleLanguageTypeScript, core.RuleLanguageJavaScript, core.RuleLanguageCPP), + Title: "Missing unit in numeric name", + Description: "Warns when numeric duration, size, or money names omit a unit suffix.", + HowToFix: "Include the unit in the name or use a domain type that carries the unit, such as timeoutMs or priceCents.", + }, + "naming.role-suffix-overuse": { + ID: "naming.role-suffix-overuse", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage(core.RuleLanguageGo, core.RuleLanguagePython, core.RuleLanguageTypeScript, core.RuleLanguageJavaScript, core.RuleLanguageCPP), + Title: "Role suffix overuse", + Description: "Warns when a file repeatedly relies on vague suffixes such as Manager, Helper, Util, Service, or Processor.", + HowToFix: "Rename types/functions after their domain responsibility or split broad role objects into specific collaborators.", + }, + "naming.cross-layer-inconsistency": { + ID: "naming.cross-layer-inconsistency", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage(core.RuleLanguageGo, core.RuleLanguagePython, core.RuleLanguageTypeScript, core.RuleLanguageJavaScript, core.RuleLanguageCPP), + Title: "Cross-layer naming inconsistency", + Description: "Warns when API/domain/persistence layer names use different terms for the same starter domain concept.", + HowToFix: "Standardize the concept name across DTOs, domain objects, persistence records, and adapters.", + }, + "function.hidden-mutation": { + ID: "function.hidden-mutation", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage(core.RuleLanguageGo, core.RuleLanguagePython, core.RuleLanguageTypeScript, core.RuleLanguageJavaScript, core.RuleLanguageCPP), + Title: "Hidden mutation", + Description: "Warns when a function mutates inputs, collaborators, or state without a command-style name.", + HowToFix: "Expose mutation in the name or move mutation into a separate command function.", + }, + "function.inconsistent-return-contract": { + ID: "function.inconsistent-return-contract", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage(core.RuleLanguageGo, core.RuleLanguagePython, core.RuleLanguageTypeScript, core.RuleLanguageJavaScript, core.RuleLanguageCPP), + Title: "Inconsistent return contract", + Description: "Warns when one function mixes empty and value return shapes without a clear contract.", + HowToFix: "Return one explicit result shape, separate not-found/error states, or use a named result type.", + }, + "function.multiple-responsibilities": { + ID: "function.multiple-responsibilities", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage(core.RuleLanguageGo, core.RuleLanguagePython, core.RuleLanguageTypeScript, core.RuleLanguageJavaScript, core.RuleLanguageCPP), + Title: "Multiple responsibilities", + Description: "Warns when one function combines several semantic responsibilities such as validation, loading, writing, sending, caching, transforming, or observing.", + HowToFix: "Extract focused helpers and keep orchestration separate from domain work and infrastructure side effects.", + }, + "function.orchestration-domain-mix": { + ID: "function.orchestration-domain-mix", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage(core.RuleLanguageGo, core.RuleLanguagePython, core.RuleLanguageTypeScript, core.RuleLanguageJavaScript, core.RuleLanguageCPP), + Title: "Orchestration/domain mix", + Description: "Warns when handlers, controllers, jobs, or workers mix request/job orchestration with domain decisions.", + HowToFix: "Move domain decisions into a domain service/helper and keep handlers focused on boundary orchestration.", + }, + "function.partial-result": { + ID: "function.partial-result", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage(core.RuleLanguageGo, core.RuleLanguagePython, core.RuleLanguageTypeScript, core.RuleLanguageJavaScript, core.RuleLanguageCPP), + Title: "Implicit partial result", + Description: "Warns when a function can return a value together with an error without an explicit partial-result contract.", + HowToFix: "Return no value on failure, name the partial contract, or use a result type that distinguishes partial data.", + }, "quality.environment-branching": { ID: "quality.environment-branching", Section: "Code Quality", diff --git a/internal/codeguard/rules/catalog_quality_errors_defensive.go b/internal/codeguard/rules/catalog_quality_errors_defensive.go new file mode 100644 index 0000000..78f1272 --- /dev/null +++ b/internal/codeguard/rules/catalog_quality_errors_defensive.go @@ -0,0 +1,30 @@ +package rules + +import "github.com/devr-tools/codeguard/internal/codeguard/core" + +var qualityErrorDefensiveCatalog = map[string]core.RuleMetadata{ + "error.logged-and-returned": localQualityRule("error.logged-and-returned", "warn", "Logged and returned error", "Warns when the same error is logged and returned, risking duplicate logs at multiple layers.", "Log at one ownership boundary or return the wrapped error for the caller to log."), + "error.generic-message": localQualityRule("error.generic-message", "warn", "Generic error message", "Warns when an error message lacks operation, resource, or decision context.", "Add operation-specific context while preserving the original error where possible."), + "error.wrong-abstraction-level": localQualityRule("error.wrong-abstraction-level", "warn", "Wrong abstraction level in error", "Warns when higher-level error contracts expose lower-level infrastructure details.", "Translate infrastructure failures into domain/API-level errors and retain the cause internally."), + "error.inconsistent-wrapping": localQualityRule("error.inconsistent-wrapping", "warn", "Inconsistent error wrapping", "Warns when a function mixes wrapped errors with bare error returns.", "Use one wrapping convention in the function so callers receive consistent context and causes."), + "error.retryable-not-distinguished": localQualityRule("error.retryable-not-distinguished", "warn", "Retryable failure not distinguished", "Warns when retry paths cannot distinguish transient from permanent failures.", "Return or tag retryable failures explicitly so callers can apply bounded retry policy safely."), + "error.user-message-leaks-internals": localQualityRule("error.user-message-leaks-internals", "warn", "User message leaks internals", "Warns when user-facing errors expose database, transport, stack, or infrastructure internals.", "Show a safe user message and keep technical diagnostics in logs or wrapped internal errors."), + "error.partial-failure-hidden": localQualityRule("error.partial-failure-hidden", "warn", "Partial failure hidden", "Warns when a partial failure path continues or reports success without surfacing failed work.", "Return structured partial-failure information or fail the operation explicitly."), + "error.cleanup-error-ignored": localQualityRule("error.cleanup-error-ignored", "warn", "Cleanup error ignored", "Warns when close, rollback, delete, or cleanup failures are discarded.", "Handle, join, or deliberately document cleanup errors instead of silently discarding them."), + "error.panic-on-recoverable-path": localQualityRule("error.panic-on-recoverable-path", "warn", "Panic on recoverable path", "Warns when recoverable request, validation, or I/O failures are handled with panic/throw.", "Return an explicit error/result for recoverable failures and reserve panic for programmer errors."), + "error.exception-used-for-control-flow": localQualityRule("error.exception-used-for-control-flow", "warn", "Exception used for control flow", "Warns when exception/panic/throw is used for ordinary branch control.", "Use explicit branches or result values for expected control flow."), + "error.fallback-hides-corruption": localQualityRule("error.fallback-hides-corruption", "warn", "Fallback hides corruption", "Warns when fallback success after parse, corruption, or validation failure can hide bad data.", "Fail closed or return a partial/error contract that makes the corrupted input visible."), + + "defensive.unvalidated-boundary-input": localQualityRule("defensive.unvalidated-boundary-input", "warn", "Unvalidated boundary input", "Warns when handler, API, event, or filesystem input is consumed without validation evidence.", "Validate boundary input before using it in domain logic or side effects."), + "defensive.invalid-state-representable": localQualityRule("defensive.invalid-state-representable", "warn", "Invalid state representable", "Warns when booleans or raw status strings can represent impossible state combinations.", "Model state with an enum, sum type, or value object that makes invalid combinations unrepresentable."), + "defensive.null-assumption": localQualityRule("defensive.null-assumption", "warn", "Null assumption", "Warns when nullable boundary values are dereferenced without a nil/null guard.", "Check for nil/null or validate the value before dereferencing."), + "defensive.integer-overflow": localQualityRule("defensive.integer-overflow", "warn", "Integer overflow assumption", "Warns when arithmetic on count, size, or length input lacks an overflow bound check.", "Validate bounds or use a type wide enough for the source range before arithmetic."), + "defensive.bounds-assumption": localQualityRule("defensive.bounds-assumption", "warn", "Bounds assumption", "Warns when indexed access assumes collection bounds without a nearby length check.", "Check length or key existence before indexing."), + "defensive.unsafe-default": localQualityRule("defensive.unsafe-default", "warn", "Unsafe default", "Warns when a config/env fallback can fail open or disable a safety control.", "Choose fail-closed defaults and require explicit opt-out for safety-sensitive settings."), + "defensive.non-exhaustive-branch": localQualityRule("defensive.non-exhaustive-branch", "warn", "Non-exhaustive branch", "Warns when enum-like state/kind/type branching lacks default or exhaustive handling.", "Handle every known case and add a safe default or exhaustive assertion."), + "defensive.unchecked-external-response": localQualityRule("defensive.unchecked-external-response", "warn", "Unchecked external response", "Warns when external responses are consumed without checking status, ok, or transport errors.", "Check status/error/schema before consuming response data."), + "defensive.missing-schema-validation": localQualityRule("defensive.missing-schema-validation", "warn", "Missing schema validation", "Warns when decoded JSON, events, or request payloads are used without schema or invariant validation.", "Validate decoded payloads with schema or domain invariant checks before use."), + "defensive.missing-resource-limit": localQualityRule("defensive.missing-resource-limit", "warn", "Missing resource limit", "Warns when boundary reads or uploads lack explicit size, count, or time limits.", "Apply size/count/time bounds before reading or queueing boundary input."), + "defensive.invalid-state-transition": localQualityRule("defensive.invalid-state-transition", "warn", "Invalid state transition", "Warns when state transitions write terminal states without checking allowed prior state.", "Validate the current state before applying transitions."), + "defensive.fail-open-authorization": localQualityRule("defensive.fail-open-authorization", "warn", "Fail-open authorization", "Warns when authorization failure paths default to allow or success.", "Fail closed on authorization errors and require explicit allow decisions."), +} diff --git a/internal/codeguard/rules/catalog_quality_smells.go b/internal/codeguard/rules/catalog_quality_smells.go new file mode 100644 index 0000000..1d6eee5 --- /dev/null +++ b/internal/codeguard/rules/catalog_quality_smells.go @@ -0,0 +1,102 @@ +package rules + +import "github.com/devr-tools/codeguard/internal/codeguard/core" + +var qualitySmellCatalog = map[string]core.RuleMetadata{ + "smell.god-object": { + ID: "smell.god-object", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage( + core.RuleLanguageCPP, + core.RuleLanguageGo, + core.RuleLanguageJavaScript, + core.RuleLanguagePython, + core.RuleLanguageTypeScript, + ), + Title: "God object", + Description: "Warns when a local type/class accumulates many methods, fields, and responsibility clusters.", + HowToFix: "Split cohesive responsibilities into smaller collaborators or value objects and keep the original type focused on orchestration only when necessary.", + }, + "smell.feature-envy": { + ID: "smell.feature-envy", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage( + core.RuleLanguageCPP, + core.RuleLanguageGo, + core.RuleLanguageJavaScript, + core.RuleLanguagePython, + core.RuleLanguageTypeScript, + ), + Title: "Feature envy", + Description: "Warns when a function or method mostly interrogates one external collaborator instead of its own receiver/context.", + HowToFix: "Move the behavior closer to the data owner, add a named operation on the collaborator, or pass a richer domain object.", + }, + "smell.middle-man": { + ID: "smell.middle-man", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage( + core.RuleLanguageCPP, + core.RuleLanguageGo, + core.RuleLanguageJavaScript, + core.RuleLanguagePython, + core.RuleLanguageTypeScript, + ), + Title: "Middle man", + Description: "Warns when a type/class mostly forwards calls to one collaborator without policy, translation, or ownership.", + HowToFix: "Inline the pass-through layer, or add meaningful policy/translation so the abstraction earns its place.", + }, + "smell.message-chain": { + ID: "smell.message-chain", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage( + core.RuleLanguageCPP, + core.RuleLanguageGo, + core.RuleLanguageJavaScript, + core.RuleLanguagePython, + core.RuleLanguageTypeScript, + ), + Title: "Message chain", + Description: "Warns when code reaches through a long chain of collaborators, increasing coupling to object structure.", + HowToFix: "Introduce a named query/helper at the boundary or move the traversal behind the object that owns the structure.", + }, + "smell.data-clump": { + ID: "smell.data-clump", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage( + core.RuleLanguageCPP, + core.RuleLanguageGo, + core.RuleLanguageJavaScript, + core.RuleLanguagePython, + core.RuleLanguageTypeScript, + ), + Title: "Data clump", + Description: "Warns when the same group of primitive/domain parameters appears repeatedly across functions.", + HowToFix: "Extract the repeated group into a value object, options type, or request DTO that can hold invariants and names.", + }, + "smell.switch-on-type": { + ID: "smell.switch-on-type", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage( + core.RuleLanguageCPP, + core.RuleLanguageGo, + core.RuleLanguageJavaScript, + core.RuleLanguagePython, + core.RuleLanguageTypeScript, + ), + Title: "Switch on type", + Description: "Warns when behavior repeatedly branches on type/kind/discriminator checks that should move behind polymorphism or dispatch.", + HowToFix: "Replace repeated type/kind branches with polymorphic methods, strategy objects, or a centralized dispatch table.", + }, +} diff --git a/pkg/codeguard/sdk_types_config_checks.go b/pkg/codeguard/sdk_types_config_checks.go index e0545bf..216cb2c 100644 --- a/pkg/codeguard/sdk_types_config_checks.go +++ b/pkg/codeguard/sdk_types_config_checks.go @@ -3,6 +3,8 @@ package codeguard import "github.com/devr-tools/codeguard/internal/codeguard/core" type QualityRulesConfig = core.QualityRulesConfig +type QualityNamingConfig = core.QualityNamingConfig +type QualityNamingGlossaryEntry = core.QualityNamingGlossaryEntry type CPPToolingConfig = core.CPPToolingConfig type DesignRulesConfig = core.DesignRulesConfig type PromptRulesConfig = core.PromptRulesConfig diff --git a/tests/checks/design_graph_helpers_test.go b/tests/checks/design_graph_helpers_test.go index d71b6fb..396d9e0 100644 --- a/tests/checks/design_graph_helpers_test.go +++ b/tests/checks/design_graph_helpers_test.go @@ -26,7 +26,7 @@ func assertFindingRuleAbsent(t *testing.T, report codeguard.Report, section stri } for _, finding := range result.Findings { if finding.RuleID == ruleID { - t.Fatalf("section %q unexpectedly contains rule %q: %s", section, ruleID, finding.Message) + t.Fatalf("section %q unexpectedly contains rule %q at %s:%d: %s", section, ruleID, finding.Path, finding.Line, finding.Message) } } return diff --git a/tests/checks/function_precision_test.go b/tests/checks/function_precision_test.go index 67e544d..556c8f0 100644 --- a/tests/checks/function_precision_test.go +++ b/tests/checks/function_precision_test.go @@ -76,3 +76,150 @@ func TestFunctionCommandQueryMixWarnsWhenQueryMutatesState(t *testing.T) { assertFindingRulePresent(t, report, "Code Quality", "function.command-query-mix") assertFindingLevel(t, report, "Code Quality", "function.command-query-mix", "warn") } + +func TestFunctionHiddenMutationWarnsAcrossLanguages(t *testing.T) { + cases := []struct { + name string + language string + file string + source []string + }{ + { + name: "go", + language: "go", + file: "mutation.go", + source: []string{ + "package sample", + "type Audit interface { Save(string) error }", + "func PrepareUser(audit Audit, id string) string {", + "\taudit.Save(id)", + "\treturn id", + "}", + }, + }, + { + name: "python", + language: "python", + file: "mutation.py", + source: []string{ + "def prepare_user(user):", + " user.name = user.name.strip()", + " return user", + }, + }, + { + name: "typescript", + language: "typescript", + file: "mutation.ts", + source: []string{ + "export function prepareUser(user: User): User {", + " user.name = user.name.trim();", + " return user;", + "}", + "interface User { name: string }", + }, + }, + { + name: "javascript", + language: "javascript", + file: "mutation.js", + source: []string{ + "export function prepareUser(user) {", + " user.name = user.name.trim();", + " return user;", + "}", + }, + }, + { + name: "cpp", + language: "cpp", + file: "mutation.cpp", + source: []string{ + "struct User { int score; };", + "User PrepareUser(User& user) {", + " user.score = user.score + 1;", + " return user;", + "}", + }, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, tc.file), strings.Join(tc.source, "\n")) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, tc.language)) + + assertFindingRulePresent(t, report, "Code Quality", "function.hidden-mutation") + }) + } +} + +func TestFunctionResponsibilityAndOrchestrationRules(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "handler.ts"), strings.Join([]string{ + "export async function checkoutHandler(request: Request, response: Response) {", + " validateOrder(request.body);", + " const order = await repository.fetchOrder(request.body.id);", + " if (order.total > 100) {", + " await cache.set(order.id, order);", + " }", + " const payload = transformOrder(order);", + " await repository.saveOrder(payload);", + " await notifier.send(payload);", + " metrics.log(payload);", + " return response.json(payload);", + "}", + "interface Request { body: any }", + "interface Response { json(input: unknown): unknown }", + }, "\n")) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript")) + + assertFindingRulePresent(t, report, "Code Quality", "function.multiple-responsibilities") + assertFindingRulePresent(t, report, "Code Quality", "function.orchestration-domain-mix") +} + +func TestFunctionReturnContractRules(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "returns.go"), strings.Join([]string{ + "package sample", + "type User struct{}", + "func LoadUser(id string, missing bool) (*User, error) {", + "\tuser, err := fetchUser(id)", + "\tif err != nil {", + "\t\treturn user, err", + "\t}", + "\tif missing {", + "\t\treturn nil, nil", + "\t}", + "\treturn user, nil", + "}", + "func fetchUser(id string) (*User, error) { return &User{}, nil }", + }, "\n")) + + report := runQualityPrecisionScan(t, qualityPrecisionConfig(dir)) + + assertFindingRulePresent(t, report, "Code Quality", "function.inconsistent-return-contract") + assertFindingRulePresent(t, report, "Code Quality", "function.partial-result") +} + +func TestFunctionPrecisionSkipsExplicitSingleResponsibility(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "clean.ts"), strings.Join([]string{ + "export function saveUser(user: User): User {", + " repository.save(user);", + " return user;", + "}", + "export function isUserReady(user: User): boolean {", + " return user.enabled === true;", + "}", + "interface User { enabled: boolean }", + }, "\n")) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript")) + + assertFindingRuleAbsent(t, report, "Code Quality", "function.hidden-mutation") + assertFindingRuleAbsent(t, report, "Code Quality", "function.multiple-responsibilities") + assertFindingRuleAbsent(t, report, "Code Quality", "function.inconsistent-return-contract") +} diff --git a/tests/checks/naming_precision_test.go b/tests/checks/naming_precision_test.go index 1867298..933ae68 100644 --- a/tests/checks/naming_precision_test.go +++ b/tests/checks/naming_precision_test.go @@ -10,9 +10,13 @@ import ( ) func qualityPrecisionConfig(dir string) codeguard.Config { + return qualityPrecisionConfigForLanguage(dir, "go") +} + +func qualityPrecisionConfigForLanguage(dir string, language string) codeguard.Config { cfg := codeguard.ExampleConfig() cfg.Name = "quality-precision" - cfg.Targets = []codeguard.TargetConfig{{Name: "repo", Path: dir, Language: "go"}} + cfg.Targets = []codeguard.TargetConfig{{Name: "repo", Path: dir, Language: language}} cfg.Checks.Quality = true cfg.Checks.Design = false cfg.Checks.Security = false @@ -67,3 +71,153 @@ func TestNamingGenericIdentifierSkipsTestFixtures(t *testing.T) { assertFindingRuleAbsent(t, report, "Code Quality", "naming.generic-identifier") } + +func TestNamingBehaviorMismatchWarnsAcrossLanguages(t *testing.T) { + cases := []struct { + name string + language string + file string + source []string + }{ + { + name: "go", + language: "go", + file: "orders.go", + source: []string{ + "package sample", + "type Audit interface { Save(string) error }", + "func GetOrder(audit Audit, id string) (string, error) {", + "\tif err := audit.Save(id); err != nil { return \"\", err }", + "\treturn id, nil", + "}", + }, + }, + { + name: "python", + language: "python", + file: "orders.py", + source: []string{ + "def get_order(audit, order_id):", + " audit.save(order_id)", + " return order_id", + }, + }, + { + name: "typescript", + language: "typescript", + file: "orders.ts", + source: []string{ + "export function getOrder(audit: Audit, orderId: string): string {", + " audit.save(orderId);", + " return orderId;", + "}", + "interface Audit { save(id: string): void }", + }, + }, + { + name: "javascript", + language: "javascript", + file: "orders.js", + source: []string{ + "export function getOrder(audit, orderId) {", + " audit.save(orderId);", + " return orderId;", + "}", + }, + }, + { + name: "cpp", + language: "cpp", + file: "orders.cpp", + source: []string{ + "struct Audit { void Save(const char* id); };", + "const char* GetOrder(Audit& audit, const char* id) {", + " audit.Save(id);", + " return id;", + "}", + }, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, tc.file), strings.Join(tc.source, "\n")) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, tc.language)) + + assertFindingRulePresent(t, report, "Code Quality", "naming.behavior-mismatch") + }) + } +} + +func TestNamingPredicateAndCardinalityPositiveNegative(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "names.ts"), strings.Join([]string{ + "export function evaluate(users: number, user: Array, enabled: boolean): boolean {", + " const active = enabled === true;", + " const isReady = users > 0;", + " return active && isReady;", + "}", + }, "\n")) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript")) + + assertFindingRulePresent(t, report, "Code Quality", "naming.boolean-not-predicate") + assertFindingRulePresent(t, report, "Code Quality", "naming.cardinality-mismatch") +} + +func TestNamingUnitsAbbreviationsAndImplementationLeak(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "names.py"), strings.Join([]string{ + "def build_sql_invoice(cust_id: str, timeout: int, price: int):", + " retry_count = 2", + " timeout_ms = 30", + " return cust_id", + }, "\n")) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "python")) + + assertFindingRulePresent(t, report, "Code Quality", "naming.implementation-leak") + assertFindingRulePresent(t, report, "Code Quality", "naming.missing-unit") + assertFindingRulePresent(t, report, "Code Quality", "naming.unknown-abbreviation") +} + +func TestNamingGlossaryRoleSuffixAndCrossLayerHeuristics(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "layers.go"), strings.Join([]string{ + "package sample", + "type RestaurantResponse struct{}", + "type VenueEntity struct{}", + "type MerchantRecord struct{}", + "type OrderManager struct{}", + "type OrderHelper struct{}", + "type OrderUtil struct{}", + "type OrderProcessor struct{}", + }, "\n")) + cfg := qualityPrecisionConfig(dir) + cfg.Checks.QualityRules.Naming.Glossary = map[string]codeguard.QualityNamingGlossaryEntry{ + "restaurant": {Avoid: []string{"venue", "merchant"}}, + } + + report := runQualityPrecisionScan(t, cfg) + + assertFindingRulePresent(t, report, "Code Quality", "naming.domain-vocabulary-drift") + assertFindingRulePresent(t, report, "Code Quality", "naming.role-suffix-overuse") + assertFindingRulePresent(t, report, "Code Quality", "naming.cross-layer-inconsistency") +} + +func TestNamingPrecisionSkipsClearNames(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "clear.ts"), strings.Join([]string{ + "export function isOrderReady(items: string[], timeoutMs: number): boolean {", + " const hasItems = items.length > 0;", + " return hasItems && timeoutMs > 0;", + "}", + }, "\n")) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript")) + + assertFindingRuleAbsent(t, report, "Code Quality", "naming.boolean-not-predicate") + assertFindingRuleAbsent(t, report, "Code Quality", "naming.cardinality-mismatch") + assertFindingRuleAbsent(t, report, "Code Quality", "naming.missing-unit") +} diff --git a/tests/checks/quality_error_defensive_multilang_test.go b/tests/checks/quality_error_defensive_multilang_test.go new file mode 100644 index 0000000..31ead53 --- /dev/null +++ b/tests/checks/quality_error_defensive_multilang_test.go @@ -0,0 +1,745 @@ +package checks_test + +import ( + "path/filepath" + "strings" + "testing" +) + +var workstreamERules = []string{ + "error.logged-and-returned", + "error.logged-and-ignored", + "error.context-lost", + "error.generic-message", + "error.wrong-abstraction-level", + "error.inconsistent-wrapping", + "error.retryable-not-distinguished", + "error.user-message-leaks-internals", + "error.partial-failure-hidden", + "error.cleanup-error-ignored", + "error.panic-on-recoverable-path", + "error.exception-used-for-control-flow", + "error.fallback-hides-corruption", + "defensive.unvalidated-boundary-input", + "defensive.invalid-state-representable", + "defensive.null-assumption", + "defensive.unchecked-type-assertion", + "defensive.unsafe-numeric-conversion", + "defensive.integer-overflow", + "defensive.bounds-assumption", + "defensive.unsafe-default", + "defensive.non-exhaustive-branch", + "defensive.unchecked-external-response", + "defensive.missing-schema-validation", + "defensive.missing-resource-limit", + "defensive.invalid-state-transition", + "defensive.fail-open-authorization", +} + +// codeguard:ignore quality.max-function-lines until 2026-12-31 -- multi-language fixture matrix kept inline for focused detector coverage. +func TestQualityErrorContractsDetectMultiLanguageSignals(t *testing.T) { + tests := []struct { + name string + language string + file string + source string + rules []string + }{ + { + name: "go", + language: "go", + file: "errors.go", + source: strings.Join([]string{ + "package sample", + "", + "import (", + "\t\"encoding/json\"", + "\t\"errors\"", + "\t\"fmt\"", + "\t\"log\"", + ")", + "", + "type rowsHandle struct{}", + "func (rowsHandle) Close() error { return nil }", + "", + "func SaveProfile(id string) error {", + "\tif err := writeProfile(id); err != nil {", + "\t\tlog.Printf(\"save profile: %v\", err)", + "\t\treturn err", + "\t}", + "\treturn nil", + "}", + "", + "func LoadProfile(id string) error {", + "\tif err := readProfile(id); err != nil {", + "\t\treturn err", + "\t}", + "\tif err := writeProfile(id); err != nil {", + "\t\treturn fmt.Errorf(\"write profile: %w\", err)", + "\t}", + "\treturn errors.New(\"failed\")", + "}", + "", + "func CheckoutHandler() error {", + "\treturn errors.New(\"postgres sql constraint failed\")", + "}", + "", + "func CloseRows(rows rowsHandle) error {", + "\tdefer rows.Close()", + "\treturn nil", + "}", + "", + "func ProcessAll(items []string) error {", + "\tfor _, item := range items {", + "\t\tif err := sendItem(item); err != nil {", + "\t\t\tcontinue", + "\t\t}", + "\t}", + "\treturn nil", + "}", + "", + "func DecodeConfig(raw []byte) map[string]string {", + "\tvar out map[string]string", + "\tif err := json.Unmarshal(raw, &out); err != nil {", + "\t\treturn map[string]string{}", + "\t}", + "\treturn out", + "}", + "", + "func RetryCall() error {", + "\tfor attempt := 0; attempt < 3; attempt++ {", + "\t\tif err := callRemote(); err != nil {", + "\t\t\tretryLater()", + "\t\t}", + "\t}", + "\treturn errors.New(\"failed\")", + "}", + "", + "func HandleRequest(input string) {", + "\tif input == \"\" {", + "\t\tpanic(\"invalid request\")", + "\t}", + "}", + "", + "func writeProfile(string) error { return nil }", + "func readProfile(string) error { return nil }", + "func sendItem(string) error { return nil }", + "func callRemote() error { return nil }", + "func retryLater() {}", + "", + }, "\n"), + rules: []string{ + "error.logged-and-returned", + "error.context-lost", + "error.generic-message", + "error.wrong-abstraction-level", + "error.inconsistent-wrapping", + "error.retryable-not-distinguished", + "error.user-message-leaks-internals", + "error.partial-failure-hidden", + "error.cleanup-error-ignored", + "error.panic-on-recoverable-path", + "error.fallback-hides-corruption", + }, + }, + { + name: "python", + language: "python", + file: "errors.py", + source: strings.Join([]string{ + "import json", + "import logging", + "", + "def load_profile(profile_id):", + " try:", + " return read_profile(profile_id)", + " except Exception as err:", + " logging.error('load failed %s', err)", + " return None", + "", + "def retry_call(client):", + " for attempt in range(3):", + " try:", + " return client.call()", + " except Exception as err:", + " retry_again()", + " raise Exception('failed')", + "", + "def find_user(user_id):", + " if not user_id:", + " raise Exception('not found')", + " return user_id", + "", + "def decode_config(raw):", + " try:", + " return json.loads(raw)", + " except Exception:", + " return {}", + "", + "def process_all(items):", + " for item in items:", + " try:", + " send_item(item)", + " except Exception:", + " continue", + " return []", + "", + }, "\n"), + rules: []string{ + "error.logged-and-ignored", + "error.generic-message", + "error.retryable-not-distinguished", + "error.partial-failure-hidden", + "error.exception-used-for-control-flow", + "error.fallback-hides-corruption", + }, + }, + { + name: "typescript", + language: "typescript", + file: "errors.ts", + source: strings.Join([]string{ + "export function checkoutHandler(): Error {", + " return new Error('sql database failed');", + "}", + "", + "export function parseControl(value: string): string {", + " if (value === 'stop') {", + " throw new Error('stop');", + " }", + " return value;", + "}", + "", + "export function batch(tasks: Promise[]) {", + " const result = Promise.allSettled(tasks);", + " return result;", + "}", + "", + }, "\n"), + rules: []string{ + "error.wrong-abstraction-level", + "error.user-message-leaks-internals", + "error.exception-used-for-control-flow", + "error.partial-failure-hidden", + }, + }, + { + name: "cpp", + language: "cpp", + file: "errors.cpp", + source: strings.Join([]string{ + "#include ", + "#include ", + "", + "std::string HandleApi() {", + " return std::runtime_error(\"redis connection refused\").what();", + "}", + "", + "std::string parseControl(std::string value) {", + " if (value == \"missing\") {", + " throw std::runtime_error(\"missing\");", + " }", + " return value;", + "}", + "", + }, "\n"), + rules: []string{ + "error.wrong-abstraction-level", + "error.user-message-leaks-internals", + "error.exception-used-for-control-flow", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, tt.file), tt.source) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, tt.language)) + + for _, ruleID := range tt.rules { + assertFindingRulePresent(t, report, "Code Quality", ruleID) + } + }) + } +} + +// codeguard:ignore quality.max-function-lines until 2026-12-31 -- multi-language fixture matrix kept inline for focused detector coverage. +func TestQualityDefensiveBoundariesDetectMultiLanguageSignals(t *testing.T) { + tests := []struct { + name string + language string + file string + source string + rules []string + }{ + { + name: "go", + language: "go", + file: "defensive.go", + source: strings.Join([]string{ + "package sample", + "", + "import (", + "\t\"encoding/json\"", + "\t\"io\"", + "\t\"net/http\"", + "\t\"os\"", + ")", + "", + "type DeploymentState struct {", + "\tIsActive bool", + "\tIsDeleted bool", + "}", + "", + "type Request struct { Body Body }", + "type Body struct { ID string }", + "type User struct { Name string }", + "type OrderStatus string", + "type Order struct { Status OrderStatus }", + "", + "func HandleUser(req Request) string {", + "\treturn req.Body.ID", + "}", + "", + "func RenderUser(user *User) string {", + "\treturn user.Name", + "}", + "", + "func First(items []string) string {", + "\treturn items[0]", + "}", + "", + "func Allocate(count int) int {", + "\treturn count * 4096", + "}", + "", + "func AllowByDefault() bool {", + "\treturn os.Getenv(\"AUTHZ_DISABLED\") != \"false\"", + "}", + "", + "func Label(status string) string {", + "\tswitch status {", + "\tcase \"new\":", + "\t\treturn \"new\"", + "\t}", + "\treturn \"\"", + "}", + "", + "func Fetch(url string) string {", + "\tresp, _ := http.Get(url)", + "\tbody, _ := io.ReadAll(resp.Body)", + "\treturn string(body)", + "}", + "", + "func Decode(raw []byte) map[string]string {", + "\tvar payload map[string]string", + "\tjson.Unmarshal(raw, &payload)", + "\treturn payload", + "}", + "", + "func Promote(order *Order) {", + "\torder.Status = \"paid\"", + "}", + "", + "func CanAccess(user string) bool {", + "\tif err := authorize(user); err != nil {", + "\t\treturn true", + "\t}", + "\treturn true", + "}", + "", + "func DecodeValue(value any) string {", + "\treturn value.(string)", + "}", + "", + "func Narrow(count int64) int32 {", + "\treturn int32(count)", + "}", + "", + "func authorize(string) error { return nil }", + "", + }, "\n"), + rules: []string{ + "defensive.unvalidated-boundary-input", + "defensive.invalid-state-representable", + "defensive.null-assumption", + "defensive.unchecked-type-assertion", + "defensive.unsafe-numeric-conversion", + "defensive.integer-overflow", + "defensive.bounds-assumption", + "defensive.unsafe-default", + "defensive.non-exhaustive-branch", + "defensive.unchecked-external-response", + "defensive.missing-schema-validation", + "defensive.missing-resource-limit", + "defensive.invalid-state-transition", + "defensive.fail-open-authorization", + }, + }, + { + name: "python", + language: "python", + file: "defensive.py", + source: strings.Join([]string{ + "import json", + "import os", + "import requests", + "from typing import Optional", + "", + "class SessionState:", + " is_active: bool", + " is_deleted: bool", + "", + "def handle_event(event):", + " return event['user']['id']", + "", + "def render_user(user: Optional[User]):", + " return user.name", + "", + "def first(items):", + " return items[0]", + "", + "def allocate(count):", + " return count * 4096", + "", + "def allow_by_default():", + " return os.environ.get('AUTHZ_DISABLED', 'true')", + "", + "def fetch(url):", + " response = requests.get(url)", + " return response.json()", + "", + "def decode(raw):", + " return json.loads(raw)", + "", + }, "\n"), + rules: []string{ + "defensive.unvalidated-boundary-input", + "defensive.invalid-state-representable", + "defensive.null-assumption", + "defensive.integer-overflow", + "defensive.bounds-assumption", + "defensive.unsafe-default", + "defensive.unchecked-external-response", + "defensive.missing-schema-validation", + }, + }, + { + name: "typescript", + language: "typescript", + file: "defensive.ts", + source: strings.Join([]string{ + "interface SessionState { isActive: boolean; isDeleted: boolean }", + "interface User { name: string }", + "", + "export function handleUser(req: any) {", + " const payload = JSON.parse(req.body);", + " return payload.user.id;", + "}", + "", + "export function renderUser(user: User | null): string {", + " return user.name;", + "}", + "", + "export function first(items: string[]): string {", + " return items[0];", + "}", + "", + "export function allocate(count: number): number {", + " return count * 4096;", + "}", + "", + "export function allowByDefault(): string {", + " return process.env.AUTHZ_DISABLED || 'true';", + "}", + "", + "export function label(status: string): string {", + " switch (status) {", + " case 'new': return 'new';", + " }", + " return '';", + "}", + "", + "export function fetchUser(url: string) {", + " const response = fetch(url);", + " return response.json();", + "}", + "", + "export function canAccess(user: User): boolean {", + " try { return policy(user); } catch (error) { return true; }", + "}", + "", + }, "\n"), + rules: []string{ + "defensive.unvalidated-boundary-input", + "defensive.invalid-state-representable", + "defensive.null-assumption", + "defensive.integer-overflow", + "defensive.bounds-assumption", + "defensive.unsafe-default", + "defensive.non-exhaustive-branch", + "defensive.unchecked-external-response", + "defensive.missing-schema-validation", + "defensive.fail-open-authorization", + }, + }, + { + name: "javascript", + language: "javascript", + file: "defensive.js", + source: strings.Join([]string{ + "export function handleUser(req) {", + " const payload = JSON.parse(req.body);", + " return payload.user.id;", + "}", + "", + "export function first(items) { return items[0]; }", + "export function allocate(count) { return count * 4096; }", + "export function allowByDefault() { return process.env.AUTHZ_DISABLED || 'true'; }", + "", + "export function fetchUser(url) {", + " const response = fetch(url);", + " return response.json();", + "}", + "", + }, "\n"), + rules: []string{ + "defensive.unvalidated-boundary-input", + "defensive.integer-overflow", + "defensive.bounds-assumption", + "defensive.unsafe-default", + "defensive.unchecked-external-response", + "defensive.missing-schema-validation", + }, + }, + { + name: "cpp", + language: "cpp", + file: "defensive.cpp", + source: strings.Join([]string{ + "#include ", + "#include ", + "", + "struct SessionState { bool active; bool deleted; };", + "struct User { std::string name; };", + "struct Order { std::string state; };", + "", + "std::string renderUser(User* user) {", + " return user->name;", + "}", + "", + "std::string first(std::vector items) {", + " return items[0];", + "}", + "", + "int allocate(int count) {", + " return count * 4096;", + "}", + "", + "std::string fetchUser(std::string url) {", + " auto response = http_client.get(url);", + " return response.body;", + "}", + "", + "void promote(Order* order) {", + " order->state = \"paid\";", + "}", + "", + }, "\n"), + rules: []string{ + "defensive.invalid-state-representable", + "defensive.null-assumption", + "defensive.integer-overflow", + "defensive.bounds-assumption", + "defensive.unchecked-external-response", + "defensive.invalid-state-transition", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, tt.file), tt.source) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, tt.language)) + + for _, ruleID := range tt.rules { + assertFindingRulePresent(t, report, "Code Quality", ruleID) + } + }) + } +} + +// codeguard:ignore quality.max-function-lines until 2026-12-31 -- multi-language fixture matrix kept inline for focused detector coverage. +func TestQualityErrorAndDefensiveRulesAllowGuardedPatterns(t *testing.T) { + tests := []struct { + name string + language string + file string + source string + }{ + { + name: "go", + language: "go", + file: "safe.go", + source: strings.Join([]string{ + "package sample", + "", + "import (", + "\t\"encoding/json\"", + "\t\"fmt\"", + "\t\"io\"", + "\t\"net/http\"", + ")", + "", + "type User struct { Name string }", + "type OrderStatus string", + "type Order struct { Status OrderStatus }", + "type Request struct { Body Body }", + "type Body struct { ID string }", + "", + "func HandleUser(req Request) (string, error) {", + "\tif err := validate(req); err != nil { return \"\", fmt.Errorf(\"validate request: %w\", err) }", + "\treturn req.Body.ID, nil", + "}", + "", + "func RenderUser(user *User) (string, error) {", + "\tif user == nil { return \"\", fmt.Errorf(\"user required\") }", + "\treturn user.Name, nil", + "}", + "", + "func First(items []string) (string, error) {", + "\tif len(items) == 0 { return \"\", fmt.Errorf(\"items required\") }", + "\treturn items[0], nil", + "}", + "", + "func Fetch(url string) ([]byte, error) {", + "\tresp, err := http.Get(url)", + "\tif err != nil { return nil, fmt.Errorf(\"fetch profile: %w\", err) }", + "\tif resp.StatusCode >= 400 { return nil, fmt.Errorf(\"fetch profile status %d\", resp.StatusCode) }", + "\treturn io.ReadAll(io.LimitReader(resp.Body, 1024))", + "}", + "", + "func Decode(raw []byte) (map[string]string, error) {", + "\tvar payload map[string]string", + "\tif err := json.Unmarshal(raw, &payload); err != nil { return nil, fmt.Errorf(\"decode payload: %w\", err) }", + "\tif err := validate(payload); err != nil { return nil, err }", + "\treturn payload, nil", + "}", + "", + "func Promote(order *Order) error {", + "\tif order == nil { return fmt.Errorf(\"order required\") }", + "\tif !canTransition(order.Status, \"paid\") { return fmt.Errorf(\"invalid transition from %s\", order.Status) }", + "\torder.Status = \"paid\"", + "\treturn nil", + "}", + "", + "func CanAccess(user string) bool {", + "\tif err := authorize(user); err != nil { return false }", + "\treturn true", + "}", + "", + "func validate(any) error { return nil }", + "func canTransition(OrderStatus, string) bool { return true }", + "func authorize(string) error { return nil }", + "", + }, "\n"), + }, + { + name: "python", + language: "python", + file: "safe.py", + source: strings.Join([]string{ + "import json", + "import requests", + "from typing import Optional", + "", + "class SessionState:", + " status: SessionStatus", + "", + "def handle_event(event):", + " validate(event)", + " return event['user']['id']", + "", + "def render_user(user: Optional[User]):", + " if user is None:", + " raise ValueError('user required')", + " return user.name", + "", + "def first(items):", + " if len(items) == 0:", + " raise ValueError('items required')", + " return items[0]", + "", + "def fetch(url):", + " response = requests.get(url)", + " response.raise_for_status()", + " return response.json()", + "", + "def decode(raw):", + " payload = json.loads(raw)", + " validate_schema(payload)", + " return payload", + "", + }, "\n"), + }, + { + name: "typescript", + language: "typescript", + file: "safe.ts", + source: strings.Join([]string{ + "interface User { name: string }", + "export function handleUser(req: Request) { validate(req); return req; }", + "export function renderUser(user: User | null): string { if (user === null) { throw new Error('user required'); } return user.name; }", + "export function first(items: string[]): string { if (items.length === 0) { throw new Error('items required'); } return items[0]; }", + "export async function fetchUser(url: string) { const response = await fetch(url); if (!response.ok) { throw new Error('fetch user failed'); } return response.json(); }", + "export function decode(raw: string) { const payload = JSON.parse(raw); validateSchema(payload); return payload; }", + "export function canAccess(user: User): boolean { try { return policy(user); } catch (error) { return false; } }", + "", + }, "\n"), + }, + { + name: "javascript", + language: "javascript", + file: "safe.js", + source: strings.Join([]string{ + "export function handleUser(req) { validate(req); return req.body.id; }", + "export function first(items) { if (items.length === 0) { throw new Error('items required'); } return items[0]; }", + "export async function fetchUser(url) { const response = await fetch(url); if (!response.ok) { throw new Error('fetch user failed'); } return response.json(); }", + "export function decode(raw) { const payload = JSON.parse(raw); validateSchema(payload); return payload; }", + "", + }, "\n"), + }, + { + name: "cpp", + language: "cpp", + file: "safe.cpp", + source: strings.Join([]string{ + "#include ", + "#include ", + "#include ", + "struct User { std::string name; };", + "std::string renderUser(User* user) { if (user == nullptr) { throw std::runtime_error(\"user required\"); } return user->name; }", + "std::string first(std::vector items) { if (items.empty()) { throw std::runtime_error(\"items required\"); } return items[0]; }", + "int allocate(int count) { if (count > safeint_limit()) { throw std::runtime_error(\"count too large\"); } return count * 4096; }", + "", + }, "\n"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, tt.file), tt.source) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, tt.language)) + + for _, ruleID := range workstreamERules { + assertFindingRuleAbsent(t, report, "Code Quality", ruleID) + } + }) + } +} diff --git a/tests/checks/quality_smells_test.go b/tests/checks/quality_smells_test.go new file mode 100644 index 0000000..b14fd8b --- /dev/null +++ b/tests/checks/quality_smells_test.go @@ -0,0 +1,380 @@ +package checks_test + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/devr-tools/codeguard/pkg/codeguard" +) + +var structuralSmellRuleIDs = []string{ + "smell.god-object", + "smell.feature-envy", + "smell.middle-man", + "smell.message-chain", + "smell.data-clump", + "smell.switch-on-type", +} + +type structuralSmellCase struct { + name string + language string + path string + source string +} + +func TestQualityStructuralSmellsDetectMultiLanguageSignals(t *testing.T) { + for _, tc := range structuralSmellPositiveCases() { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, tc.path), tc.source) + cfg := qualityPrecisionConfig(dir) + cfg.Name = "quality-structural-smells-" + tc.name + cfg.Targets[0].Language = tc.language + + report := runQualityPrecisionScan(t, cfg) + + for _, ruleID := range structuralSmellRuleIDs { + assertStructuralSmellPresent(t, report, ruleID) + assertFindingLevel(t, report, "Code Quality", ruleID, "warn") + } + }) + } +} + +func assertStructuralSmellPresent(t *testing.T, report codeguard.Report, ruleID string) { + t.Helper() + for _, result := range report.Sections { + if result.Name != "Code Quality" { + continue + } + seen := make([]string, 0, len(result.Findings)) + for _, finding := range result.Findings { + seen = append(seen, finding.RuleID) + if finding.RuleID == ruleID { + return + } + } + t.Fatalf("section %q missing rule %q; saw %s", "Code Quality", ruleID, strings.Join(seen, ", ")) + } + t.Fatalf("section %q not found", "Code Quality") +} + +func TestQualityStructuralSmellsAllowSmallCohesiveCode(t *testing.T) { + for _, tc := range structuralSmellNegativeCases() { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, tc.path), tc.source) + cfg := qualityPrecisionConfig(dir) + cfg.Name = "quality-structural-smells-negative-" + tc.name + cfg.Targets[0].Language = tc.language + + report := runQualityPrecisionScan(t, cfg) + + for _, ruleID := range structuralSmellRuleIDs { + assertFindingRuleAbsent(t, report, "Code Quality", ruleID) + } + }) + } +} + +func structuralSmellPositiveCases() []structuralSmellCase { + return []structuralSmellCase{ + goStructuralSmellPositiveCase(), + pythonStructuralSmellPositiveCase(), + { + name: "typescript", + language: "typescript", + path: "smells.ts", + source: scriptStructuralSmellSource(true), + }, + { + name: "javascript", + language: "javascript", + path: "smells.js", + source: scriptStructuralSmellSource(false), + }, + cppStructuralSmellPositiveCase(), + } +} + +func goStructuralSmellPositiveCase() structuralSmellCase { + return structuralSmellCase{ + name: "go", + language: "go", + path: "smells.go", + source: strings.Join([]string{ + "package sample", + "", + "type AccountCoordinator struct {", + "\trepo string", + "\tcache string", + "\tmailer string", + "\trenderer string", + "\taudit string", + "\tclock string", + "}", + "", + "func (a *AccountCoordinator) ValidateAccount() {}", + "func (a *AccountCoordinator) SaveAccount() {}", + "func (a *AccountCoordinator) SendEmail() {}", + "func (a *AccountCoordinator) RenderReport() {}", + "func (a *AccountCoordinator) CacheAccount() {}", + "func (a *AccountCoordinator) SyncAccount() {}", + "func (a *AccountCoordinator) DeleteAccount() {}", + "func (a *AccountCoordinator) LoadAccount() {}", + "", + "type BillingClient interface { Create(string) error; Update(string) error; Delete(string) error; Find(string) error }", + "type BillingFacade struct { client BillingClient }", + "func (b BillingFacade) Create(value string) error { return b.client.Create(value) }", + "func (b BillingFacade) Update(value string) error { return b.client.Update(value) }", + "func (b BillingFacade) Delete(value string) error { return b.client.Delete(value) }", + "func (b BillingFacade) Find(value string) error { return b.client.Find(value) }", + "", + "func (a *AccountCoordinator) Score(customer Customer) string {", + "\treturn customer.Profile.Name + customer.Profile.Email + customer.Account.Region + customer.Account.Plan + customer.Account.Status", + "}", + "func countryCode(user User) string { return user.Account().Profile().Address().Country().Code() }", + "func createOrder(customerID string, orderID string, currency string) {}", + "func updateOrder(customerID string, orderID string, currency string) {}", + "func cancelOrder(customerID string, orderID string, currency string) {}", + "func handleOne(event Event) { switch event.Kind { case \"created\": case \"updated\": } }", + "func handleTwo(event Event) { switch event.Kind { case \"deleted\": case \"archived\": } }", + "type Customer struct { Profile Profile; Account Account }", + "type Profile struct { Name string; Email string }", + "type Account struct { Region string; Plan string; Status string }", + "type User struct{}", + "func (User) Account() UserAccount { return UserAccount{} }", + "type UserAccount struct{}", + "func (UserAccount) Profile() UserProfile { return UserProfile{} }", + "type UserProfile struct{}", + "func (UserProfile) Address() UserAddress { return UserAddress{} }", + "type UserAddress struct{}", + "func (UserAddress) Country() UserCountry { return UserCountry{} }", + "type UserCountry struct{}", + "func (UserCountry) Code() string { return \"\" }", + "type Event struct { Kind string }", + }, "\n"), + } +} + +func pythonStructuralSmellPositiveCase() structuralSmellCase { + return structuralSmellCase{ + name: "python", + language: "python", + path: "smells.py", + source: strings.Join([]string{ + "class AccountCoordinator:", + " def __init__(self):", + " self.repo = None", + " self.cache = None", + " self.mailer = None", + " self.renderer = None", + " self.audit = None", + " self.clock = None", + " def validate_account(self): pass", + " def save_account(self): pass", + " def send_email(self): pass", + " def render_report(self): pass", + " def cache_account(self): pass", + " def sync_account(self): pass", + " def delete_account(self): pass", + " def load_account(self): pass", + "", + "class BillingFacade:", + " def __init__(self, client):", + " self.client = client", + " def create(self, value):", + " return self.client.create(value)", + " def update(self, value):", + " return self.client.update(value)", + " def delete(self, value):", + " return self.client.delete(value)", + " def find(self, value):", + " return self.client.find(value)", + "class CustomerScorer:", + " def score(self, customer):", + " return customer.profile.name + customer.profile.email + customer.account.region + customer.account.plan + customer.account.status", + "def country_code(user):", + " return user.account().profile().address().country().code()", + "def create_order(customer_id: str, order_id: str, currency: str): pass", + "def update_order(customer_id: str, order_id: str, currency: str): pass", + "def cancel_order(customer_id: str, order_id: str, currency: str): pass", + "def handle_one(event):", + " if event.kind == 'created': pass", + " elif event.kind == 'updated': pass", + "def handle_two(event):", + " if event.kind == 'deleted': pass", + " elif event.kind == 'archived': pass", + }, "\n"), + } +} + +func cppStructuralSmellPositiveCase() structuralSmellCase { + return structuralSmellCase{ + name: "cpp", + language: "c++", + path: "smells.cpp", + source: strings.Join([]string{ + "class AccountCoordinator {", + " Repo repo;", + " Cache cache;", + " Mailer mailer;", + " Renderer renderer;", + " Audit audit;", + " Clock clock;", + " void validateAccount() {}", + " void saveAccount() {}", + " void sendEmail() {}", + " void renderReport() {}", + " void cacheAccount() {}", + " void syncAccount() {}", + " void deleteAccount() {}", + " void loadAccount() {}", + "};", + "class BillingFacade {", + " Client client;", + " Result create(Value value) { return client.create(value); }", + " Result update(Value value) { return client.update(value); }", + " Result remove(Value value) { return client.remove(value); }", + " Result find(Value value) { return client.find(value); }", + "};", + "class CustomerScorer {", + " Text score(Customer customer) {", + " return customer.profile.name + customer.profile.email + customer.account.region + customer.account.plan + customer.account.status;", + " }", + "};", + "Text countryCode(User user) { return user.account().profile().address().country().code(); }", + "void createOrder(String customerId, String orderId, String currency) {}", + "void updateOrder(String customerId, String orderId, String currency) {}", + "void cancelOrder(String customerId, String orderId, String currency) {}", + "void handleOne(Event event) { switch (event.kind) { case Created: break; case Updated: break; } }", + "void handleTwo(Event event) { switch (event.kind) { case Deleted: break; case Archived: break; } }", + }, "\n"), + } +} + +func scriptStructuralSmellSource(typed bool) string { + paramType := "" + fieldType := "" + if typed { + paramType = ": string" + fieldType = ": unknown" + } + return strings.Join([]string{ + "class AccountCoordinator {", + " repo" + fieldType + ";", + " cache" + fieldType + ";", + " mailer" + fieldType + ";", + " renderer" + fieldType + ";", + " audit" + fieldType + ";", + " clock" + fieldType + ";", + " validateAccount() {}", + " saveAccount() {}", + " sendEmail() {}", + " renderReport() {}", + " cacheAccount() {}", + " syncAccount() {}", + " deleteAccount() {}", + " loadAccount() {}", + "}", + "", + "class BillingFacade {", + " client" + fieldType + ";", + " create(value" + paramType + ") { return this.client.create(value) }", + " update(value" + paramType + ") { return this.client.update(value) }", + " delete(value" + paramType + ") { return this.client.delete(value) }", + " find(value" + paramType + ") { return this.client.find(value) }", + "}", + "", + "class CustomerScorer {", + " score(customer" + paramType + ") {", + " return customer.profile.name + customer.profile.email + customer.account.region + customer.account.plan + customer.account.status", + " }", + "}", + "", + "function countryCode(user) { return user.account().profile().address().country().code() }", + "function createOrder(customerId" + paramType + ", orderId" + paramType + ", currency" + paramType + ") {}", + "function updateOrder(customerId" + paramType + ", orderId" + paramType + ", currency" + paramType + ") {}", + "function cancelOrder(customerId" + paramType + ", orderId" + paramType + ", currency" + paramType + ") {}", + "", + "function handleOne(event) { switch (event.kind) { case 'created': break; case 'updated': break } }", + "function handleTwo(event) { switch (event.kind) { case 'deleted': break; case 'archived': break } }", + }, "\n") +} + +func structuralSmellNegativeCases() []structuralSmellCase { + return []structuralSmellCase{ + { + name: "go", + language: "go", + path: "safe.go", + source: strings.Join([]string{ + "package sample", + "", + "type OrderService struct { repo string }", + "func (o OrderService) Save(order Order) error { return nil }", + "func (o OrderService) Find(id string) (Order, error) { return Order{}, nil }", + "func createOrder(key OrderKey) {}", + "func updateOrder(key OrderKey) {}", + "type Order struct{}", + "type OrderKey struct { CustomerID string; OrderID string; Currency string }", + }, "\n"), + }, + { + name: "python", + language: "python", + path: "safe.py", + source: strings.Join([]string{ + "class OrderService:", + " def __init__(self, repo): self.repo = repo", + " def save(self, order): return self.repo.save(order)", + " def find(self, order_id): return self.repo.find(order_id)", + "def create_order(key): return key", + "def update_order(key): return key", + }, "\n"), + }, + { + name: "typescript", + language: "typescript", + path: "safe.ts", + source: strings.Join([]string{ + "class OrderService {", + " repo: Repo", + " save(order: Order) { return this.repo.save(order) }", + " find(orderId: string) { return this.repo.find(orderId) }", + "}", + "function createOrder(key: OrderKey) { return key }", + "function updateOrder(key: OrderKey) { return key }", + }, "\n"), + }, + { + name: "javascript", + language: "javascript", + path: "safe.js", + source: strings.Join([]string{ + "class OrderService {", + " save(order) { return this.repo.save(order) }", + " find(orderId) { return this.repo.find(orderId) }", + "}", + "function createOrder(key) { return key }", + "function updateOrder(key) { return key }", + }, "\n"), + }, + { + name: "cpp", + language: "c++", + path: "safe.cpp", + source: strings.Join([]string{ + "class OrderService {", + " Repo repo;", + " Result save(Order order) { return repo.save(order); }", + " Result find(String orderId) { return repo.find(orderId); }", + "};", + "void createOrder(OrderKey key) {}", + "void updateOrder(OrderKey key) {}", + }, "\n"), + }, + } +} diff --git a/tests/checks/reliability_multilang_test.go b/tests/checks/reliability_multilang_test.go index b875b1c..c9ec053 100644 --- a/tests/checks/reliability_multilang_test.go +++ b/tests/checks/reliability_multilang_test.go @@ -76,6 +76,40 @@ def load_widget(): assertFindingRulePresent(t, report, "Reliability", "reliability.recoverable-panic") } +func TestReliabilityPythonDetectsCancellationShutdownConcurrencyAndLostContext(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "service.py"), ` +import asyncio +import uvicorn + +async def run_many(items): + asyncio.create_task(process(items[0])) + asyncio.create_task(process(items[1])) + asyncio.create_task(process(items[2])) + asyncio.create_task(process(items[3])) + asyncio.create_task(process(items[4])) + +def wrap_error(): + try: + load_customer() + except Exception as err: + raise RuntimeError("customer load failed") + +def main(): + uvicorn.run(app) +`) + + report, err := codeguard.Run(context.Background(), reliabilityLangConfig("reliability-python-parity", dir, "python")) + if err != nil { + t.Fatalf("run: %v", err) + } + + assertFindingRulePresent(t, report, "Reliability", "reliability.missing-cancellation") + assertFindingRulePresent(t, report, "Reliability", "reliability.missing-concurrency-limit") + assertFindingRulePresent(t, report, "Reliability", "reliability.missing-graceful-shutdown") + assertFindingRulePresent(t, report, "Reliability", "reliability.lost-error-context") +} + func TestReliabilityPythonAcceptsBoundedPatterns(t *testing.T) { dir := t.TempDir() writeFile(t, filepath.Join(dir, "safe_worker.py"), ` @@ -166,6 +200,53 @@ function failPayment() { assertFindingRulePresent(t, report, "Reliability", "reliability.recoverable-panic") } +func TestReliabilityTypeScriptDetectsCancellationShutdownResourceConcurrencyAndLostContext(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "service.ts"), ` +import fs from "fs"; + +async function startMany() { + fetchUser("1"); + fetchUser("2"); + fetchUser("3"); + fetchUser("4"); + fetchUser("5"); +} + +function wrapError() { + try { + risky(); + } catch (err) { throw new Error("operation failed"); } +} + +function leakStream() { + const stream = fs.createReadStream("/tmp/input.txt"); + use(stream); + use(stream); + use(stream); + use(stream); + use(stream); + use(stream); + use(stream); + use(stream); + use(stream); +} + +app.listen(3000); +`) + + report, err := codeguard.Run(context.Background(), reliabilityLangConfig("reliability-ts-parity", dir, "typescript")) + if err != nil { + t.Fatalf("run: %v", err) + } + + assertFindingRulePresent(t, report, "Reliability", "reliability.missing-cancellation") + assertFindingRulePresent(t, report, "Reliability", "reliability.missing-concurrency-limit") + assertFindingRulePresent(t, report, "Reliability", "reliability.missing-graceful-shutdown") + assertFindingRulePresent(t, report, "Reliability", "reliability.resource-leak") + assertFindingRulePresent(t, report, "Reliability", "reliability.lost-error-context") +} + func TestReliabilityTypeScriptAcceptsBoundedPatterns(t *testing.T) { dir := t.TempDir() writeFile(t, filepath.Join(dir, "safe_worker.ts"), ` @@ -241,6 +322,53 @@ function failPayment() { assertFindingRulePresent(t, report, "Reliability", "reliability.recoverable-panic") } +func TestReliabilityJavaScriptDetectsCancellationShutdownResourceConcurrencyAndLostContext(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "service.js"), ` +const fs = require("fs"); + +async function startMany() { + fetchUser("1"); + fetchUser("2"); + fetchUser("3"); + fetchUser("4"); + fetchUser("5"); +} + +function wrapError() { + try { + risky(); + } catch (err) { throw new Error("operation failed"); } +} + +function leakStream() { + const stream = fs.createReadStream("/tmp/input.txt"); + use(stream); + use(stream); + use(stream); + use(stream); + use(stream); + use(stream); + use(stream); + use(stream); + use(stream); +} + +app.listen(3000); +`) + + report, err := codeguard.Run(context.Background(), reliabilityLangConfig("reliability-js-parity", dir, "javascript")) + if err != nil { + t.Fatalf("run: %v", err) + } + + assertFindingRulePresent(t, report, "Reliability", "reliability.missing-cancellation") + assertFindingRulePresent(t, report, "Reliability", "reliability.missing-concurrency-limit") + assertFindingRulePresent(t, report, "Reliability", "reliability.missing-graceful-shutdown") + assertFindingRulePresent(t, report, "Reliability", "reliability.resource-leak") + assertFindingRulePresent(t, report, "Reliability", "reliability.lost-error-context") +} + func TestReliabilityDetectsHiddenPartialFailuresAcrossLanguages(t *testing.T) { for _, tc := range hiddenPartialFailureCases() { t.Run(tc.name, func(t *testing.T) { @@ -424,6 +552,65 @@ void FailPayment() { assertFindingRulePresent(t, report, "Reliability", "reliability.recoverable-panic") } +func TestReliabilityCPPDetectsTimeoutCancellationShutdownConcurrencyAndLostContext(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "service.cpp"), ` +#include +#include + +void StartMany(Client& client, Server& server) { + client.Get("/users"); + std::thread([]{}).detach(); + std::thread([]{}).detach(); + std::thread([]{}).detach(); + std::thread([]{}).detach(); + std::thread([]{}).detach(); + server.Run(); +} + +void WrapError() { + try { + Risky(); + } catch (const std::exception& err) { + throw std::runtime_error("operation failed"); + } +} +`) + + report, err := codeguard.Run(context.Background(), reliabilityLangConfig("reliability-cpp-parity", dir, "cpp")) + if err != nil { + t.Fatalf("run: %v", err) + } + + assertFindingRulePresent(t, report, "Reliability", "reliability.missing-timeout") + assertFindingRulePresent(t, report, "Reliability", "reliability.missing-cancellation") + assertFindingRulePresent(t, report, "Reliability", "reliability.missing-concurrency-limit") + assertFindingRulePresent(t, report, "Reliability", "reliability.missing-graceful-shutdown") + assertFindingRulePresent(t, report, "Reliability", "reliability.lost-error-context") +} + +func TestReliabilityCPPDetectsSwallowedCatch(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "consumer.cpp"), ` +#include + +void Consume(Message message) { + try { + Process(message); + } catch (const std::exception& err) { + return; + } +} +`) + + report, err := codeguard.Run(context.Background(), reliabilityLangConfig("reliability-cpp-swallowed", dir, "cpp")) + if err != nil { + t.Fatalf("run: %v", err) + } + + assertFindingRulePresent(t, report, "Reliability", "reliability.swallowed-error") +} + func TestReliabilityCPPAcceptsBoundedPatterns(t *testing.T) { dir := t.TempDir() writeFile(t, filepath.Join(dir, "safe_worker.cpp"), ` diff --git a/tests/cli/features_metadata_test.go b/tests/cli/features_metadata_test.go index 50b95c7..0b8994b 100644 --- a/tests/cli/features_metadata_test.go +++ b/tests/cli/features_metadata_test.go @@ -87,6 +87,142 @@ func TestSDKRuleMetadataForReliabilityRule(t *testing.T) { } } +func TestSDKRuleMetadataForReliabilityParityRules(t *testing.T) { + for _, ruleID := range []string{ + "reliability.missing-cancellation", + "reliability.missing-graceful-shutdown", + "reliability.missing-concurrency-limit", + "reliability.resource-leak", + "reliability.swallowed-error", + "reliability.lost-error-context", + } { + t.Run(ruleID, func(t *testing.T) { + rule := requireRuleMetadata(t, ruleID) + assertExecutionModel(t, rule, codeguard.RuleExecutionModelLanguageAgnostic) + assertLanguageCoverage( + t, + rule, + codeguard.RuleLanguageCoverageFixed, + codeguard.RuleLanguageCPP, + codeguard.RuleLanguageGo, + codeguard.RuleLanguageJavaScript, + codeguard.RuleLanguagePython, + codeguard.RuleLanguageTypeScript, + ) + if rule.FixTemplate.Kind == "" { + t.Fatalf("expected %s to expose a fix template", ruleID) + } + }) + } +} + +func TestSDKRuleMetadataForLocalQualityPrecisionRules(t *testing.T) { + for _, ruleID := range []string{ + "naming.generic-identifier", + "naming.behavior-mismatch", + "naming.boolean-not-predicate", + "naming.domain-vocabulary-drift", + "naming.unknown-abbreviation", + "naming.cardinality-mismatch", + "naming.implementation-leak", + "naming.missing-unit", + "naming.role-suffix-overuse", + "naming.cross-layer-inconsistency", + "function.excessive-parameters", + "function.mixed-abstraction-level", + "function.command-query-mix", + "function.hidden-mutation", + "function.inconsistent-return-contract", + "function.multiple-responsibilities", + "function.orchestration-domain-mix", + "function.partial-result", + "error.logged-and-ignored", + "error.context-lost", + "error.logged-and-returned", + "error.generic-message", + "error.wrong-abstraction-level", + "error.inconsistent-wrapping", + "error.retryable-not-distinguished", + "error.user-message-leaks-internals", + "error.partial-failure-hidden", + "error.cleanup-error-ignored", + "error.panic-on-recoverable-path", + "error.exception-used-for-control-flow", + "error.fallback-hides-corruption", + "defensive.unchecked-type-assertion", + "defensive.unsafe-numeric-conversion", + "defensive.unvalidated-boundary-input", + "defensive.invalid-state-representable", + "defensive.null-assumption", + "defensive.integer-overflow", + "defensive.bounds-assumption", + "defensive.unsafe-default", + "defensive.non-exhaustive-branch", + "defensive.unchecked-external-response", + "defensive.missing-schema-validation", + "defensive.missing-resource-limit", + "defensive.invalid-state-transition", + "defensive.fail-open-authorization", + "maintainability.public-surface-growth", + "maintainability.dependency-growth", + "smell.shotgun-surgery-history", + "smell.divergent-change-history", + } { + t.Run(ruleID, func(t *testing.T) { + rule := requireRuleMetadata(t, ruleID) + assertExecutionModel(t, rule, codeguard.RuleExecutionModelLanguageAgnostic) + assertLanguageCoverage( + t, + rule, + codeguard.RuleLanguageCoverageFixed, + codeguard.RuleLanguageCPP, + codeguard.RuleLanguageGo, + codeguard.RuleLanguageJavaScript, + codeguard.RuleLanguagePython, + codeguard.RuleLanguageTypeScript, + ) + if rule.DefaultLevel != "warn" { + t.Fatalf("%s default level = %q, want warn", ruleID, rule.DefaultLevel) + } + if rule.FixTemplate.Kind == "" { + t.Fatalf("expected local-quality fix template for %s", ruleID) + } + }) + } +} + +func TestSDKRuleMetadataForStructuralSmellRules(t *testing.T) { + for _, ruleID := range []string{ + "smell.god-object", + "smell.feature-envy", + "smell.middle-man", + "smell.message-chain", + "smell.data-clump", + "smell.switch-on-type", + } { + t.Run(ruleID, func(t *testing.T) { + rule := requireRuleMetadata(t, ruleID) + assertExecutionModel(t, rule, codeguard.RuleExecutionModelLanguageAgnostic) + assertLanguageCoverage( + t, + rule, + codeguard.RuleLanguageCoverageFixed, + codeguard.RuleLanguageCPP, + codeguard.RuleLanguageGo, + codeguard.RuleLanguageJavaScript, + codeguard.RuleLanguagePython, + codeguard.RuleLanguageTypeScript, + ) + if rule.DefaultLevel != "warn" { + t.Fatalf("%s default level = %q, want warn", ruleID, rule.DefaultLevel) + } + if rule.FixTemplate.Kind != codeguard.FixTemplateKindGuided { + t.Fatalf("expected guided structural-smell fix template for %s, got %q", ruleID, rule.FixTemplate.Kind) + } + }) + } +} + func TestSDKRuleMetadataForDataRule(t *testing.T) { rule := requireRuleMetadata(t, "data.missing-outbox-strategy") assertExecutionModel(t, rule, codeguard.RuleExecutionModelLanguageAgnostic) diff --git a/tests/codeguard/ai_triage_anthropic_test.go b/tests/codeguard/ai_triage_anthropic_test.go index b2a9283..bad82e2 100644 --- a/tests/codeguard/ai_triage_anthropic_test.go +++ b/tests/codeguard/ai_triage_anthropic_test.go @@ -34,7 +34,11 @@ func triageFixtureConfig(t *testing.T, root string) codeguard.Config { Path: root, Language: "go", }}, - Checks: codeguard.CheckConfig{Quality: true, Context: contextOff()}, + Checks: codeguard.CheckConfig{ + Quality: true, + Context: contextOff(), + QualityRules: codeguard.QualityRulesConfig{LocalPrecision: localPrecisionOff()}, + }, Output: codeguard.OutputConfig{Format: "json"}, Cache: codeguard.CacheConfig{ Enabled: &cacheEnabled, diff --git a/tests/codeguard/ai_triage_test.go b/tests/codeguard/ai_triage_test.go index 9d8d542..a367fde 100644 --- a/tests/codeguard/ai_triage_test.go +++ b/tests/codeguard/ai_triage_test.go @@ -32,8 +32,9 @@ func doThing() error { return nil } Language: "go", }}, Checks: codeguard.CheckConfig{ - Quality: true, - Context: contextOff(), + Quality: true, + Context: contextOff(), + QualityRules: codeguard.QualityRulesConfig{LocalPrecision: localPrecisionOff()}, }, Output: codeguard.OutputConfig{Format: "json"}, Cache: codeguard.CacheConfig{ @@ -79,8 +80,9 @@ func doThing() error { return nil } Language: "go", }}, Checks: codeguard.CheckConfig{ - Quality: true, - Context: contextOff(), + Quality: true, + Context: contextOff(), + QualityRules: codeguard.QualityRulesConfig{LocalPrecision: localPrecisionOff()}, }, Output: codeguard.OutputConfig{Format: "json"}, Cache: codeguard.CacheConfig{ @@ -137,8 +139,9 @@ func doThing() error { return nil } Language: "go", }}, Checks: codeguard.CheckConfig{ - Quality: true, - Context: contextOff(), + Quality: true, + Context: contextOff(), + QualityRules: codeguard.QualityRulesConfig{LocalPrecision: localPrecisionOff()}, }, Output: codeguard.OutputConfig{Format: "json"}, Cache: codeguard.CacheConfig{ diff --git a/tests/codeguard/context_helpers_test.go b/tests/codeguard/context_helpers_test.go index f3bf8ac..e2930ad 100644 --- a/tests/codeguard/context_helpers_test.go +++ b/tests/codeguard/context_helpers_test.go @@ -6,3 +6,10 @@ func contextOff() *bool { off := false return &off } + +// localPrecisionOff keeps older tests focused on the legacy quality finding +// they exercise instead of newer local naming/function/error precision rules. +func localPrecisionOff() *bool { + off := false + return &off +} diff --git a/tests/codeguard/fix_verification_helpers_test.go b/tests/codeguard/fix_verification_helpers_test.go index 7b60d86..14af6fd 100644 --- a/tests/codeguard/fix_verification_helpers_test.go +++ b/tests/codeguard/fix_verification_helpers_test.go @@ -50,5 +50,6 @@ func qualityOnlyConfigForLanguage(dir string, name string, language string) code cfg.Checks.Design = false cfg.Checks.Prompts = false cfg.Checks.CI = false + cfg.Checks.QualityRules.LocalPrecision = localPrecisionOff() return cfg } diff --git a/tests/codeguard/quality_naming_config_test.go b/tests/codeguard/quality_naming_config_test.go new file mode 100644 index 0000000..ed75f97 --- /dev/null +++ b/tests/codeguard/quality_naming_config_test.go @@ -0,0 +1,87 @@ +package codeguard_test + +import ( + "strings" + "testing" + + "github.com/devr-tools/codeguard/pkg/codeguard" +) + +func TestExampleConfigIncludesQualityNamingDefaults(t *testing.T) { + cfg := codeguard.ExampleConfig() + naming := cfg.Checks.QualityRules.Naming + if naming.RoleSuffixWarnThreshold != 4 { + t.Fatalf("role_suffix_warn_threshold = %d, want 4", naming.RoleSuffixWarnThreshold) + } + for _, want := range []string{"api", "id", "json", "sql", "url"} { + if !containsString(naming.AllowedAbbreviations, want) { + t.Fatalf("allowed_abbreviations missing %q: %#v", want, naming.AllowedAbbreviations) + } + } +} + +func TestValidateQualityNamingConfig(t *testing.T) { + tests := []struct { + name string + naming codeguard.QualityNamingConfig + want string + }{ + { + name: "negative role suffix threshold", + naming: codeguard.QualityNamingConfig{RoleSuffixWarnThreshold: -1}, + want: "role_suffix_warn_threshold", + }, + { + name: "blank glossary concept", + naming: codeguard.QualityNamingConfig{Glossary: map[string]codeguard.QualityNamingGlossaryEntry{ + " ": {Avoid: []string{"venue"}}, + }}, + want: "blank concept", + }, + { + name: "blank glossary avoid entry", + naming: codeguard.QualityNamingConfig{Glossary: map[string]codeguard.QualityNamingGlossaryEntry{ + "restaurant": {Avoid: []string{" "}}, + }}, + want: "avoid[0]", + }, + { + name: "blank allowed abbreviation", + naming: codeguard.QualityNamingConfig{AllowedAbbreviations: []string{"id", " "}}, + want: "allowed_abbreviations[1]", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := codeguard.ExampleConfig() + cfg.Checks.QualityRules.Naming = tt.naming + err := codeguard.ValidateConfig(cfg) + if err == nil || !strings.Contains(err.Error(), tt.want) { + t.Fatalf("ValidateConfig error = %v, want containing %q", err, tt.want) + } + }) + } +} + +func TestValidateQualityNamingConfigAcceptsGlossary(t *testing.T) { + cfg := codeguard.ExampleConfig() + cfg.Checks.QualityRules.Naming = codeguard.QualityNamingConfig{ + Glossary: map[string]codeguard.QualityNamingGlossaryEntry{ + "restaurant": {Avoid: []string{"venue", "merchant"}}, + }, + AllowedAbbreviations: []string{"id", "url"}, + RoleSuffixWarnThreshold: 4, + } + if err := codeguard.ValidateConfig(cfg); err != nil { + t.Fatalf("ValidateConfig: %v", err) + } +} + +func containsString(values []string, want string) bool { + for _, value := range values { + if value == want { + return true + } + } + return false +} diff --git a/tests/codeguard/yaml_config_helpers_test.go b/tests/codeguard/yaml_config_helpers_test.go index 21032f6..98f3dd8 100644 --- a/tests/codeguard/yaml_config_helpers_test.go +++ b/tests/codeguard/yaml_config_helpers_test.go @@ -14,6 +14,13 @@ func yamlRoundTripConfig() codeguard.Config { cfg.Checks.QualityRules.LanguageCommands = map[string][]codeguard.CommandCheckConfig{ "typescript": {{Name: "tsc", Command: "npx", Args: []string{"tsc", "--noEmit"}}}, } + cfg.Checks.QualityRules.Naming = codeguard.QualityNamingConfig{ + Glossary: map[string]codeguard.QualityNamingGlossaryEntry{ + "restaurant": {Avoid: []string{"venue", "merchant"}}, + }, + AllowedAbbreviations: []string{"id", "url", "api"}, + RoleSuffixWarnThreshold: 5, + } cfg.Checks.DesignRules.LanguageCommands = map[string][]codeguard.CommandCheckConfig{ "python": {{Name: "import-linter", Command: "lint-imports", Args: []string{"--config", "importlinter.ini"}}}, } @@ -38,7 +45,7 @@ func assertYAMLSchemaMarkers(t *testing.T, path string) { t.Fatalf("read yaml: %v", err) } rendered := string(data) - for _, want := range []string{"supply_chain:", "quality_rules:", "max_file_lines:", "language_commands:", "ci_rules:", "required_workflow_files:", "hybrid_triage:", "candidate_sections:", "function_contract:", "test_commands:", "rule_packs:"} { + for _, want := range []string{"supply_chain:", "quality_rules:", "max_file_lines:", "language_commands:", "naming:", "allowed_abbreviations:", "role_suffix_warn_threshold:", "ci_rules:", "required_workflow_files:", "hybrid_triage:", "candidate_sections:", "function_contract:", "test_commands:", "rule_packs:"} { if !strings.Contains(rendered, want) { t.Fatalf("written yaml missing %q:\n%s", want, rendered) } @@ -51,6 +58,12 @@ func assertYAMLRoundTripConfig(t *testing.T, loaded codeguard.Config, want codeg t.Fatalf("loaded name = %q, want %q", loaded.Name, want.Name) } assertYAMLCommand(t, loaded.Checks.QualityRules.LanguageCommands["typescript"][0].Command, "npx", "loaded command") + if loaded.Checks.QualityRules.Naming.RoleSuffixWarnThreshold != want.Checks.QualityRules.Naming.RoleSuffixWarnThreshold { + t.Fatalf("role_suffix_warn_threshold = %d, want %d", loaded.Checks.QualityRules.Naming.RoleSuffixWarnThreshold, want.Checks.QualityRules.Naming.RoleSuffixWarnThreshold) + } + if got := loaded.Checks.QualityRules.Naming.Glossary["restaurant"].Avoid; len(got) != 2 || got[0] != "venue" || got[1] != "merchant" { + t.Fatalf("naming glossary = %#v, want restaurant avoid [venue merchant]", loaded.Checks.QualityRules.Naming.Glossary) + } assertYAMLCommand(t, loaded.Checks.DesignRules.LanguageCommands["python"][0].Command, "lint-imports", "loaded design command") assertYAMLCommand(t, loaded.Checks.DesignRules.LanguageDiffCommands["go"][0].Name, "api-diff", "loaded diff command") } @@ -77,6 +90,12 @@ checks: supply_chain: true quality_rules: max_file_lines: 123 + naming: + glossary: + restaurant: + avoid: [venue, merchant] + allowed_abbreviations: [id, url] + role_suffix_warn_threshold: 5 coverage_delta: enabled: true min_changed_line_coverage: 77 @@ -146,6 +165,15 @@ func assertSnakeCaseChecks(t *testing.T, loaded codeguard.Config) { if loaded.Checks.QualityRules.CoverageDelta.MinChangedLineCoverage == nil || *loaded.Checks.QualityRules.CoverageDelta.MinChangedLineCoverage != 77 { t.Fatalf("min_changed_line_coverage = %#v, want 77", loaded.Checks.QualityRules.CoverageDelta.MinChangedLineCoverage) } + if got := loaded.Checks.QualityRules.Naming.AllowedAbbreviations; len(got) != 2 || got[0] != "id" || got[1] != "url" { + t.Fatalf("allowed_abbreviations = %#v, want [id url]", got) + } + if loaded.Checks.QualityRules.Naming.RoleSuffixWarnThreshold != 5 { + t.Fatalf("role_suffix_warn_threshold = %d, want 5", loaded.Checks.QualityRules.Naming.RoleSuffixWarnThreshold) + } + if got := loaded.Checks.QualityRules.Naming.Glossary["restaurant"].Avoid; len(got) != 2 || got[0] != "venue" || got[1] != "merchant" { + t.Fatalf("naming glossary = %#v, want restaurant avoid [venue merchant]", loaded.Checks.QualityRules.Naming.Glossary) + } } func assertSnakeCaseAI(t *testing.T, loaded codeguard.Config) {