Skip to content

feat(rules): add Scala-specific review rules - #1095

Open
Qiyuanqiii wants to merge 1 commit into
alibaba:mainfrom
Qiyuanqiii:codex/scala-review-rules
Open

feat(rules): add Scala-specific review rules#1095
Qiyuanqiii wants to merge 1 commit into
alibaba:mainfrom
Qiyuanqiii:codex/scala-review-rules

Conversation

@Qiyuanqiii

@Qiyuanqiii Qiyuanqiii commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Description

Summary

  • Add dedicated Scala review guidance focused on actionable correctness, concurrency, resource-safety, compatibility, and security defects.
  • Route existing .scala review inputs to scala.md instead of the generic default rules.
  • Add resolver regression coverage proving that a nested Scala source file loads the Scala rule.
  • Synchronize the rule mapping table across the English, Chinese, Japanese, Korean, and Russian documentation.
  • Keep the change within the existing embedded-rule mechanism: no production Go path, file eligibility, resolver precedence, or public interface changes.

Existing Gap and Fix

.scala is already present in internal/config/allowlist/supported_file_types.json, so Scala files enter review. However, system_rules.json had no Scala mapping and the embedded rule documents had no scala.md. A Scala source file without a higher-priority user rule therefore fell back to default.md.

This change adds:

**/*.scala -> scala.md

The resolver regression case uses:

src/main/scala/com/example/Service.scala

and verifies that the resolved rule contains Scala Review Principles, a stable heading absent from default.md. Removing the mapping or rule document, or falling back to the generic rule, makes the test fail.

Manual verification now reports:

File: src/main/scala/com/example/Service.scala
Source: System built-in
Pattern: **/*.scala
Rule:
#### Scala Review Principles

Rule Coverage

Area Representative defect patterns
Null and absence Java nullability boundaries, Some(null), unsafe Option.get, .nn, and unsafeNulls
Collections and laziness Partial operations, single-use iterators, lazy views, mapValues, infinite LazyList / Stream, and proven quadratic work
Futures and concurrency Callback ordering, bounded execution contexts, blocking, nested or discarded futures, eager Future.apply, and racing promises
Pattern matching and erasure Reachable uncovered cases, refutable bindings, lowercase variable patterns, erased generic tests, and warning suppression
Initialization Overridden values during superclass construction, declaration order, escaping this, lazy cycles, and retried lazy side effects
Equality and numerics equals / hashCode laws, mutable keys, array content equality, Ordering, narrowing, overflow, NaN, and range endpoints
Exceptions and resources NonFatal, Try extraction, owned-resource closure, lazy values escaping resource scopes, and cleanup semantics
Interoperability Java collection adapters and nullability, Scala.js dynamic/facade boundaries, Scala Native/JNI ownership and layout
Compatibility and security Published-library contracts, case-class product shape, dynamic execution, path confinement, secrets, and secure randomness
Tests Unobserved asynchronous assertions, timing dependence, array assertion semantics, and parallel shared state

Precision and False-Positive Controls

The rule document deliberately avoids becoming a Scala style guide.

  • Inspect scalaVersion, crossScalaVersions, compiler options, and the target platform before making version- or runtime-dependent claims.
  • Do not assume that Scala 3 enables explicit nulls, strict equality, or safe initialization.
  • Distinguish standard-library collections and Future semantics from Cats Effect, ZIO, Akka, Pekko, Spark, Play, or framework-owned abstractions.
  • Verify callers, input ranges, collection scale, execution contexts, ownership, and lifecycle before reporting non-local behavior.
  • Do not restate compiler, linter, or analyzer diagnostics unless warnings are suppressed, non-fatal in the project, or have a demonstrated consequence.
  • Treat style, formatting, and idiom preferences as out of scope.
  • Apply JVM, Scala.js, or Scala Native findings only after the build establishes the relevant target.

The final wording was checked against primary Scala documentation. That review tightened several cross-version and cross-platform boundaries, including maps with explicit defaults, Future.apply versus arbitrary futures, Scala 2 refutable-pattern failures, runtimeChecked, Using suppression semantics, Java collection adapters, and correctly declared Scala.js facades.

Compatibility and Non-Goals

  • File selection is unchanged because .scala was already allowlisted.
  • Custom, project, and global rule precedence is unchanged.
  • Existing default exclusions are unchanged.
  • This does not add .sbt or .sc support.
  • This does not invoke a Scala parser, compiler, linter, or build tool.
  • This does not prescribe an effect system, actor framework, collection library, or target platform.
  • No CLI, public API, or configuration schema changes are introduced.
  • No production Go code changes are introduced.
  • As with other built-in rule changes, the canonical rule identity changes; the existing resume guard may reject sessions created with the previous rule configuration rather than resuming them under different guidance.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

Validation Environment

  • Host: Windows, amd64
  • Go: go1.26.5 windows/amd64
  • Base: eda25482c74dff2f03bed2648497f23c512effe9 (origin/main)
  • Commit: f44e27b

GNU Make was unavailable on the validation host, so each constituent Makefile command was executed directly.

Go and Repository Validation

  • Full make test equivalent passed for every package returned by go list ./..., excluding /extensions/: go test -v -race -count=1 ...
  • Project coverage passed at 90.9%; internal/config/rules coverage was 91.7%
  • License verification passed
  • English-only verification passed across 452 scanned source files
  • go mod tidy, gofmt -s -w ., and go vet across the Makefile package set completed successfully
  • scripts/add-license.sh reported zero headers to add because the new file is Markdown
  • git diff --check, the staged whitespace check, and a separate no-index check for the new file passed
  • Every changed file uses LF; the new rule document is UTF-8 without a BOM
  • The translation-sync unit test passed
  • ocr rules check src/main/scala/com/example/Service.scala reported the system pattern **/*.scala and the Scala rule

JavaScript and Pages Validation

The complete root JavaScript GitHub Actions test sequence did not pass on this Windows host because the action-contract harness received status=null from its spawned shell. Its translation-sync test passed independently.

Installation under pages/ was attempted, but npm install made no progress for an extended period and was terminated. Consequently, the Pages lint, test, typecheck, and build scripts were not run locally. The pull-request Pages workflow subsequently completed successfully, covering lint, tests, type checking, and the production build.

Current-Head GitHub Actions

All checks on commit f44e27b passed:

  • Full CI test suite
  • Windows test suite
  • Linux arm64 cross-compilation
  • Darwin amd64 and arm64 cross-compilation
  • Windows amd64 and arm64 cross-compilation
  • CodeQL for Go, JavaScript/TypeScript, and Actions
  • Pages lint, test, typecheck, build, and size checks
  • Translation synchronization
  • OpenCodeReview PR review with 0 findings
  • CLA

OCR Self-Review

The repository-required OCR review completed successfully:

Session: bb3e7c37-0f15-4f62-97f8-b42e50d7c75d
Selected: 1
Completed: 1
Findings: 0

OCR selected only system_rules.json. The six Markdown files were excluded as unsupported_ext, and system_rules_test.go was excluded by the default test-path filter. This result is therefore not presented as full eight-file OCR coverage; the Scala document received a separate line-by-line factual review against primary Scala sources.

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a line-by-line self-review
  • I have added a deterministic resolver test that proves the feature works
  • The complete Go unit and race-test suite passes locally
  • Every repository test suite passes locally; see the Windows action-contract limitation and incomplete Pages dependency installation above
  • I have updated all maintained documentation locales
  • All required current-head GitHub checks pass
  • I have signed the CLA

Known Limitations

  • OCR reviewed only 1 of the 8 changed files; its clean result does not cover the new Scala document, localized tables, or resolver test.
  • Pages lint, test, typecheck, and build were not run locally because dependency installation did not complete; the hosted Pages CI covered and passed those commands.
  • The root action-contract JavaScript harness did not complete successfully on the Windows validation host.
  • PR fix(rules): route Kotlin script files to Kotlin review rules #1091 changes the adjacent Kotlin mapping, resolver test, and the same five locale tables. If it merges first, this branch may require a small rebase that preserves both **/*.{kt,kts} and **/*.scala plus both regression cases.

Related Issues

Closes #1094.

@github-actions

Copy link
Copy Markdown
Contributor

OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scala files fall back to generic review rules despite being supported

1 participant