Skip to content

ci(automation): enhance repository automation and code quality workflows (#320) - #360

Open
KHARSHAVARDHAN-eng wants to merge 2 commits into
apache:masterfrom
KHARSHAVARDHAN-eng:feature/automation-analysis-320
Open

ci(automation): enhance repository automation and code quality workflows (#320)#360
KHARSHAVARDHAN-eng wants to merge 2 commits into
apache:masterfrom
KHARSHAVARDHAN-eng:feature/automation-analysis-320

Conversation

@KHARSHAVARDHAN-eng

Copy link
Copy Markdown

Description

This PR addresses the recommendations from the Automation Analysis report (#320) by introducing automatic code formatting, test coverage enforcement, PR commit validation, release notes generation, and developer automation documentation.

Key Changes

  1. Automatic Code Formatting (computer/pom.xml):
    • Added spotless-maven-plugin (v2.30.0) for automated Java code formatting (mvn spotless:apply and mvn spotless:check).
  2. Test Coverage Tracking (computer/pom.xml):
    • Added jacoco-maven-plugin (v0.8.8) with prepare-agent and report goals to automatically generate test coverage reports during mvn test.
  3. Commit & PR Title Validation (.github/workflows/commit-check.yml):
    • Added a GitHub Action workflow to validate PR titles against the Conventional Commits specification.
  4. Automated Release Notes (.github/workflows/release-notes.yml):
    • Added a GitHub Action workflow to generate release drafts and changelogs automatically when tags (v*) are pushed.
  5. Documentation (docs/automation-guide.md):
    • Created comprehensive documentation detailing formatting commands, test coverage generation, RAT license checks, and PR contribution guidelines.

Reference

Fixes #320

…pache#355)

- Create computer-rust crate with high-performance CSR graph representation, PageRank, SSSP, and atomic aggregator kernels
- Implement C-ABI export layer (computer_rust_c_api.h) for FFI interoperability
- Add dataset fixtures (Karate Club, synthetic power-law) and differential tolerance check suite
- Add Java RustKernelBridge in computer-core with graceful fallback logic and unit tests
- Add Go RustKernelBridge in vermeer with fallback execution and unit tests
- Create .github/workflows/rust-ci.yml for Rust linting, testing, and formatting
- Add docs/rust-modernization-roadmap.md detailing architecture, guardrails, baselines, and newcomer-friendly child tasks
…ows (apache#320)

- Add spotless-maven-plugin to computer/pom.xml for automated Java code formatting (mvn spotless:apply / spotless:check)
- Add jacoco-maven-plugin to computer/pom.xml for automated test coverage report generation
- Create .github/workflows/commit-check.yml to validate PR titles against Conventional Commits formatting rules
- Create .github/workflows/release-notes.yml for automated GitHub release draft generation
- Create docs/automation-guide.md documenting repository code quality tools and PR guidelines
@dosubot dosubot Bot added size:XXL This PR changes 1000+ lines, ignoring generated files. documentation Improvements or additions to documentation labels Aug 10, 2026

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: The exact head introduces correctness and native-integration blockers in the new Rust kernels, Java/Go bridges, and CI workflows. Evidence: static review of the exact base/head diff, plus exact-head workflow runs showing Rust CI startup_failure and other required checks in action_required.

pub fn from_edges(num_vertices: u32, edges: &[(u32, u32, f64)]) -> Self {
let mut degree = vec![0; num_vertices as usize];
for &(src, _dst, _weight) in edges {
if src < num_vertices {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ important — The degree pass counts every edge with a valid source, while the fill pass skips destinations outside num_vertices. For an edge such as (1, 99, 1.0), row_offsets reserves a slot that remains the default (target=0, weight=0), so PageRank and SSSP process a fabricated edge. Count only edges with both endpoints valid, or reject invalid endpoints at the API boundary.

return -1;
}
let builder = unsafe { &mut *handle };
builder.edges.push((src, dst, weight));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ important — computer_graph_add_edge accepts arbitrary f64 weights, but SsspKernel uses Dijkstra. Negative weights can return incorrect shortest paths and a reachable negative cycle can keep lowering distances and growing the heap; non-finite weights are also unbounded input. Reject non-finite and negative weights here, or change the algorithm and document the supported weight domain.

use crate::RUST_KERNEL_VERSION;
use std::ffi::CString;
use std::os::raw::c_char;
use std::ptr;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ important — std::ptr is unused. The added Rust workflow runs cargo clippy --all-targets -- -D warnings, so this import is promoted to an error and prevents the Rust CI job from reaching its tests. Remove the import or use it deliberately.

return ranks;
}

private static native String nativeGetVersion();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ important — This Java native declaration does not match the library's exported ABI: Rust exports the C function computer_kernel_version, but no JNI symbol for nativeGetVersion, and computePageRank never calls a native function. If the library loads, isAvailable() becomes true while version lookup falls back after UnsatisfiedLinkError and computation still runs in Java. Add JNI/C-ABI bindings for the required calls and make availability reflect callable symbols.


func NewRustKernelBridge() *RustKernelBridge {
return &RustKernelBridge{
available: false,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ important — available is hard-coded to false, and this package contains no cgo declaration, library loading, or Rust C-ABI call. Vermeer therefore always executes the Go fallback even when the Rust library is deployed, making the new native bridge unreachable. Implement initialization and native calls, or remove the native-bridge claim and keep this as an explicitly fallback-only implementation.

push:
branches:
- master
- /^release-.*$/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ important — GitHub Actions branch filters use glob patterns, not regular expressions. ^/release-.*$/ therefore does not match normal release-* branches, so Rust CI is skipped for release-branch pushes. Replace it with a supported glob such as release-* and verify the trigger.

on:
push:
tags:
- 'v*'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ important — This workflow triggers only for v* tags, while the repository's existing release tags use the 1.0.0/1.5.0/1.7.0 form. A normal version tag will not run this workflow and will not produce draft release notes. Align the trigger with the repository's release convention or update the release process consistently.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: no. Summary: The automation change is sound in intent, but the new parent-level JaCoCo block duplicates the JaCoCo already configured in computer/computer-test/pom.xml, and the child's explicit 0.8.4 overrides the declared 0.8.8 in exactly the module that feeds the Codecov upload — leaving a 0.8.8 agent paired with a 0.8.4 aggregator; Spotless duplicates a Checkstyle rule that is already enforced and is not wired into any build; and both new workflows omit a permissions: block, which the release job actually needs (contents: write). Evidence: mvn -o help:effective-pom at head e533ae8 shows jacoco 0.8.4 with four merged executions for computer-test versus 0.8.8 for computer-core; checkstyle.xml:47,49 already declares RedundantImport/UnusedImports with maven-checkstyle-plugin bound to validate; mvn -B spotless:check passes across all 10 modules today; stale.yml, codeql-analysis.yml and rerun-ci.yml all declare explicit least-privilege permissions:. Note: the release-notes.yml v* tag mismatch was independently reproduced (all 8 repo tags are unprefixed, e.g. 1.7.0) but is omitted here as already reported on this head. This review covers only the 4 files in commit e533ae8; the +1508 lines of Rust/JNI/Go come from the stacked, unmerged #359. gh pr checks 360 reports no checks on this branch, so the added workflows are unexercised.

Comment thread computer/pom.xml
</java>
</configuration>
</plugin>
<plugin>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ important — This duplicates JaCoCo, which is already configured in computer/computer-test/pom.xml:176-198 (v0.8.4, pre-test/prepare-agent + post-test/report-aggregate writing to ${basedir}/../target/site/jacoco, the exact path computer-ci.yml:110-115 uploads to Codecov). A parent-level <build><plugins> entry is inherited by every module and merges with the child declaration instead of replacing it.

mvn -o help:effective-pom at this head:

  • -pl computer-test → version resolves to 0.8.4 (the child's explicit version wins, so the declared 0.8.8 never takes effect here) with four executions: the new prepare-agent and report plus the existing pre-test and post-test.
  • -pl computer-core → version 0.8.8.

So sibling modules instrument with the 0.8.8 agent while computer-test's 0.8.4 report-aggregate analyzes those jacoco.exec files, and computer-test runs a redundant report alongside report-aggregate in the same test phase. (The duplicated prepare-agent itself is harmless — the second execution overwrites argLine with an identical value rather than adding a second -javaagent.)

Please drop this block, or consolidate to a single declaration: remove the computer-test one, settle on one version, and keep the report-aggregate output path that Codecov consumes.

jobs:
generate-release-notes:
name: Generate Release Draft & Notes
runs-on: ubuntu-latest

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ important — This job declares no permissions: block, but softprops/action-gh-release documents permissions: contents: write as required to create a release. With no block the token falls back to the repository/org default; where that default is read-only the step fails with HTTP 403 at tag time, i.e. exactly when a release is being cut. This is separate from the tag-pattern issue already raised below: correcting the trigger alone still leaves the job unable to create the draft.

It also diverges from this repo's convention of explicit least privilege — stale.yml:11-13 (issues: write, pull-requests: write), codeql-analysis.yml:23-26 (actions: read, contents: read, security-events: write), rerun-ci.yml:10 (permissions: {}).

Please add to this job:

    permissions:
      contents: write

(I could not read the repository's effective default workflow permission — the API returns 403 for my token — but declaring it explicitly is correct either way.)

Comment thread computer/pom.xml
</execution>
</executions>
</plugin>
<plugin>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 minor — The only step configured here is <removeUnusedImports/>, which duplicates a rule the build already enforces: checkstyle.xml:47,49 declares RedundantImport and UnusedImports, and maven-checkstyle-plugin binds check to the validate phase with failsOnError=true (computer/pom.xml:373-394).

There is also no <executions> binding and computer-ci.yml was not updated to call spotless:check, so nothing runs automatically despite the PR describing "Automatic Code Formatting" — only a manual mvn spotless:apply. For what it's worth the plugin does work and the tree is already clean: mvn -B spotless:check passes across all 10 modules at this head.

Either configure steps Checkstyle does not already cover (import order, license header, a formatter) and wire spotless:check into a phase and CI, or drop the plugin along with its docs/automation-guide.md section — as configured it adds a build dependency for no net capability.

jobs:
validate-pr-title:
name: Validate PR Title & Format
runs-on: ubuntu-latest

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 minor — This job also declares no permissions: block, so it inherits the default token scope. It only needs to read the PR title, and the rest of this repo pins least privilege explicitly (stale.yml:11-13, codeql-analysis.yml:23-26, rerun-ci.yml:10).

Please add:

    permissions:
      pull-requests: read

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: no. Summary: the single biggest simplification is to rebase onto master and drop commit ada86aa. This is two PRs in one branch: the 4-file / +206-line automation change belongs to issue #320, and the 21-file / +1508-line Rust modernization work belongs to #355 and #359. Evidence: gh api repos/apache/hugegraph-computer/commits/ada86aa (21 files, +1508) against .../commits/e533ae8 (4 files, +206); issue #320 is an automation-maturity audit that never mentions Rust; gh -R apache/hugegraph-computer pr view 359 --json files shows six of those files are absent from #359's head c1fc10a too; git grep over the head tree shows RustKernelBridge and NewRustKernelBridge have no caller outside their own tests, while computer-algorithm/.../pagerank/PageRank.java and vermeer/algorithms/pagerank.go already implement the algorithm.

Comment thread computer-rust/Cargo.toml
# limitations under the License.

[package]
name = "hugegraph-computer-rust"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ This is two PRs wearing one coat.

The branch carries two commits against master:

  • ada86aa, 21 files, +1508: this crate, rust-ci.yml, RustKernelBridge.java, rust_bridge.go, the roadmap doc.
  • e533ae8, 4 files, +206: computer/pom.xml (Spotless and JaCoCo), commit-check.yml, release-notes.yml, docs/automation-guide.md.

Issue #320, which this PR says it fixes, is an automation-maturity audit: it recommends code formatting and test coverage as next steps and flags commit validation and release notes among the gaps. It does not mention Rust anywhere. The Rust work has its own issue (#355) and its own open PR (#359).

It is not even a clean duplicate of #359. Six files here are absent from #359's head c1fc10a (15 files, +1124):

computer-rust/src/kernel/aggregator.rs
computer-rust/src/kernel/sssp.rs
computer/computer-core/.../rust/RustKernelBridge.java
computer/computer-test/.../rust/RustKernelBridgeTest.java
vermeer/apps/compute/rust_bridge.go
vermeer/apps/compute/rust_bridge_test.go

That is +529 lines of Rust kernel and native-bridge code with no issue and no PR of its own, arriving on an automation PR. (Correcting myself: my earlier review said the Rust came from the stacked, unmerged #359. Six of these files are not in #359 either.)

Please rebase onto master and drop ada86aa. What is left maps one-to-one onto #320 and can be reviewed in one sitting, and the Rust review stays on #359 where it already has a thread.

return "1.5.0-java-fallback";
}

public static double[] computePageRank(double[][] adjMatrix, double dampingFactor,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ Nothing calls this, and both algorithms already ship here.

@imbajin has already noted at line 119 that computePageRank never reaches native code. The part worth acting on is what sits underneath: this is a dense adjacency-matrix PageRank in pure Java that rebuilds every vertex's out-degree by scanning all n cells per row on every iteration, and vermeer/apps/compute/rust_bridge.go writes it again with available hard-coded to false. The repository already has:

  • computer/computer-algorithm/src/main/java/org/apache/hugegraph/computer/algorithm/centrality/pagerank/PageRank.java
  • vermeer/algorithms/pagerank.go and vermeer/algorithms/sssp.go

Those two handle dangling mass and converge on an L1 norm; these fallbacks converge on maxDiff, which is L-infinity. So the new copies are not a drop-in for the old ones, they are a fourth and fifth implementation with different numerics. And nothing calls them: RustKernelBridge and NewRustKernelBridge appear nowhere outside RustKernelBridgeTest.java and rust_bridge_test.go. Nothing builds libhugegraph_computer_rust into any artifact either, so System.loadLibrary(LIB_NAME) cannot succeed on anything this repo currently ships.

Please delete both bridge files and their tests. When the shared library actually exists and is packaged, the bridge is System.loadLibrary plus

private static native double[] nativePageRank(double[][] adj, double damping, int maxIter, double tol);

with no hand-rolled fallback at all. (@imbajin already offered removal as one of two options on the Go side; this is the evidence for taking it on both.)

Comment thread computer-rust/src/lib.rs
*/

pub mod ffi;
pub mod fixtures;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 177 lines of this crate are reachable only from their own #[cfg(test)] blocks, and this line puts the test helpers in the library's public API.

  • kernel/aggregator.rs (100 lines): a CAS-loop lock-free f64 accumulator. AtomicAggregator has no caller. The only references outside the pub use below are in its own test module, and no kernel in this crate is parallel: the single thread::spawn in the whole diff is in that test. A lock-free accumulator for concurrency that does not exist yet is scaffolding.
  • fixtures/tolerance.rs (77 lines): DifferentialTolerance is only ever compared against hand-written literals in its own unit test.

Please delete aggregator.rs until a parallel kernel needs it, and tolerance.rs until something actually runs a differential. That is roughly 180 lines with no behaviour change. fixtures/dataset.rs has to stay public as it is, since benches/kernel_bench.rs:19 imports GraphFixture from the library, and cargo clippy --all-targets in rust-ci.yml builds that bench.

One related note: docs/rust-modernization-roadmap.md states the parity guardrail as differential correctness testing enforcing an L1 distance of at most 1e-6 against ground-truth outputs. Nothing compares these kernels to the Java or Go baselines, so the helper that guardrail is named after has no differential to run.

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

Labels

documentation Improvements or additions to documentation size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Automation analysis

3 participants