Stop executing fork-controlled Maven in the privileged fork Sonar workflow (#232)

.github/workflows/dependency-review.yml

@@ -68,3 +68,11 @@ jobs:
           # Fail closed with licenses derived from the project's full dependency tree,
           # replacing the deprecated deny-licenses option.
           allow-licenses: MIT, Apache-2.0, BSD-2-Clause, BSD-3-Clause, EPL-1.0, EPL-2.0, CDDL-1.0, CDDL-1.1, ISC, Unlicense, CC0-1.0
+          # Per-dependency exception to the allowlist above: sonarqube-scan-action
+          # (used in fork-sonar.yml) is LGPL-3.0. The action only executes the
+          # scanner on the runner — use of an unmodified tool, not distribution
+          # or a derivative work — so LGPL obligations do not apply. The purl is
+          # deliberately version-less: dependency-review-action's matcher ignores
+          # the version anyway, and it is the usage as a CI action that makes the
+          # license unproblematic, independent of the pinned revision.
+          allow-dependencies-licenses: 'pkg:githubactions/SonarSource/sonarqube-scan-action'

.github/workflows/fork-sonar.yml

@@ -8,6 +8,15 @@
 # It runs on `workflow_run` in the base-repo context (with secrets), and is
 # protected by the `fork-ci` GitHub Environment with Required reviewers.
 # Do NOT remove that environment gate.
+#
+# Security model (see docs/CONTRIBUTING.md, "CI for pull requests from forks"):
+# nothing fork-controlled is ever *executed* here. Maven only runs against the
+# trusted base-branch checkout (to resolve the dependency classpath); the fork
+# PR head is then fetched as plain git objects and analysed by the standalone
+# Sonar scanner, whose configuration is written by this workflow and passed via
+# `project.settings`, so a `sonar-project.properties`, `pom.xml`, `.mvn/`, or
+# wrapper script in the fork tree is inert data. Do NOT reintroduce a Maven
+# invocation (or any build tool) on the fork tree in this workflow.
 name: Fork SonarCloud (manual approval)
 
 on:
@@ -36,8 +45,8 @@ jobs:
       SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
     steps:
       - name: Download successful fork PR analysis inputs
-        # Store outside the workspace so the later source checkout (which cleans
-        # the workspace) does not remove these files.
+        # Store outside the workspace so the later checkout (which cleans the
+        # workspace) does not remove these files.
         uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1
         with:
           name: sonar-analysis-inputs
@@ -53,23 +62,25 @@ jobs:
         run: |
           env_file="$RUNNER_TEMP/sonar-analysis-data/pr-event.env"
           pr_number=$(grep -m1 '^pr_number=' "$env_file" | cut -d= -f2-)
+          pr_head_sha=$(grep -m1 '^pr_head_sha=' "$env_file" | cut -d= -f2-)
           pr_head_ref=$(grep -m1 '^pr_head_ref=' "$env_file" | cut -d= -f2-)
           pr_base_ref=$(grep -m1 '^pr_base_ref=' "$env_file" | cut -d= -f2-)
           [[ "$pr_number" =~ ^[0-9]+$ ]] || { echo "invalid pr_number"; exit 1; }
+          [[ "$pr_head_sha" =~ ^[0-9a-f]{40}$ ]] || { echo "invalid pr_head_sha"; exit 1; }
           [[ "$pr_head_ref" =~ ^[A-Za-z0-9._/-]+$ ]] || { echo "invalid pr_head_ref"; exit 1; }
           [[ "$pr_base_ref" =~ ^[A-Za-z0-9._/-]+$ ]] || { echo "invalid pr_base_ref"; exit 1; }
           {
             echo "pr_number=$pr_number"
+            echo "pr_head_sha=$pr_head_sha"
             echo "pr_head_ref=$pr_head_ref"
             echo "pr_base_ref=$pr_base_ref"
           } >> "$GITHUB_OUTPUT"
 
-      - name: Check out fork PR source (from the base repo's PR ref)
+      - name: Check out trusted base branch
+        # No `ref`: this is the base repository's default branch, i.e. trusted
+        # code. It is the only tree Maven ever runs against in this workflow.
         uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
         with:
-          # The fork head SHA is not present in the base repo, but the PR head
-          # ref is. Check that out so Sonar can read the analysed source.
-          ref: refs/pull/${{ steps.meta.outputs.pr_number }}/head
           fetch-depth: 0
 
       - name: Set up JDK 17
@@ -86,26 +97,79 @@ jobs:
           key: ${{ runner.os }}-sonar-${{ hashFiles('**/pom.xml') }}
           restore-keys: ${{ runner.os }}-sonar
 
+      - name: Resolve dependency classpath from the trusted base pom
+        # The Java analyzer needs the dependency jars for full type resolution.
+        # Resolve them from the base branch's pom (not the fork's) so that the
+        # only pom Maven parses is trusted. A dependency added by the fork PR
+        # is simply unresolved in the analysis; it is never downloaded here.
+        run: |
+          mvn -B -q -DskipTests -Dfmt.skip -pl core install
+          mvn -B -q -pl processor dependency:build-classpath \
+            -Dmdep.includeScope=test \
+            -Dmdep.pathSeparator=, \
+            -DexcludeGroupIds=io.github.java-helpers \
+            -Dmdep.outputFile="$RUNNER_TEMP/sonar-libraries.txt"
+
+      - name: Fetch fork PR source as git data (not executed)
+        # The PR head exists in the base repo as refs/pull/N/head. Fetching and
+        # checking it out only writes files and gives Sonar the SCM history it
+        # needs to compute changed lines; nothing in the fork tree is run. The
+        # checked-out commit must be the one the analysis inputs were built from.
+        env:
+          PR_NUMBER: ${{ steps.meta.outputs.pr_number }}
+          PR_HEAD_SHA: ${{ steps.meta.outputs.pr_head_sha }}
+        run: |
+          git fetch --no-tags origin "refs/pull/$PR_NUMBER/head"
+          [ "$(git rev-parse FETCH_HEAD)" = "$PR_HEAD_SHA" ] || {
+            echo "::error::PR head $(git rev-parse FETCH_HEAD) does not match analysed commit $PR_HEAD_SHA"
+            exit 1
+          }
+          git -c advice.detachedHead=false checkout --detach FETCH_HEAD
+
       - name: Restore compiled classes and coverage reports
         run: |
           src="$RUNNER_TEMP/sonar-analysis-data"
           mkdir -p core/target/classes processor/target/classes
+          mkdir -p core/target/test-classes processor/target/test-classes
           cp -a "$src/core-classes/." core/target/classes/ 2>/dev/null || true
           cp -a "$src/processor-classes/." processor/target/classes/ 2>/dev/null || true
+          cp -a "$src/core-test-classes/." core/target/test-classes/ 2>/dev/null || true
+          cp -a "$src/processor-test-classes/." processor/target/test-classes/ 2>/dev/null || true
           mkdir -p processor/target/site/jacoco processor/target/site/jacoco-aggregate
           cp -a "$src/processor-jacoco.xml" processor/target/site/jacoco/jacoco.xml 2>/dev/null || true
           cp -a "$src/jacoco-aggregate.xml" processor/target/site/jacoco-aggregate/jacoco.xml 2>/dev/null || true
 
+      - name: Write scanner configuration
+        # Written outside the workspace and selected via `project.settings`, so
+        # a sonar-project.properties in the fork tree is never read.
+        # Keep the sonar.* values in sync with the <properties> in pom.xml.
+        run: |
+          libraries="$(cat "$RUNNER_TEMP/sonar-libraries.txt")"
+          cat > "$RUNNER_TEMP/sonar-project.properties" <<EOF
+          sonar.projectKey=java-helpers_simple-builders
+          sonar.organization=java-helpers
+          sonar.projectName=Simple Builders
+          sonar.sources=core/src/main/java,processor/src/main/java
+          sonar.tests=core/src/test/java,processor/src/test/java
+          sonar.java.source=17
+          sonar.java.binaries=core/target/classes,processor/target/classes
+          sonar.java.test.binaries=core/target/test-classes,processor/target/test-classes
+          sonar.java.libraries=$libraries
+          sonar.java.test.libraries=$libraries
+          sonar.coverage.jacoco.xmlReportPaths=processor/target/site/jacoco-aggregate/jacoco.xml,processor/target/site/jacoco/jacoco.xml
+          sonar.exclusions=**/example/**
+          sonar.coverage.exclusions=**/processor/model/**,**/exceptions/**,**/generated/**
+          sonar.scm.provider=git
+          EOF
+
       - name: Publish fork PR analysis to SonarCloud
         if: env.SONAR_TOKEN != ''
+        uses: SonarSource/sonarqube-scan-action@22918119ff8e1ca75a623e15c8296b6ea4fbe28f # v8.2.1
         env:
-          PR_KEY: ${{ steps.meta.outputs.pr_number }}
-          PR_BRANCH: ${{ steps.meta.outputs.pr_head_ref }}
-          PR_BASE: ${{ steps.meta.outputs.pr_base_ref }}
-        run: |
-          mvn -B org.sonarsource.scanner.maven:sonar-maven-plugin:sonar \
-            --file pom.xml \
-            -DskipTests \
-            -Dsonar.pullrequest.key="$PR_KEY" \
-            -Dsonar.pullrequest.branch="$PR_BRANCH" \
-            -Dsonar.pullrequest.base="$PR_BASE"
+          SONAR_HOST_URL: https://sonarcloud.io
+        with:
+          args: >
+            -Dproject.settings=${{ runner.temp }}/sonar-project.properties
+            -Dsonar.pullrequest.key=${{ steps.meta.outputs.pr_number }}
+            -Dsonar.pullrequest.branch=${{ steps.meta.outputs.pr_head_ref }}
+            -Dsonar.pullrequest.base=${{ steps.meta.outputs.pr_base_ref }}

.github/workflows/maven.yml

@@ -106,6 +106,8 @@ jobs:
         mkdir -p sonar-analysis-data
         cp -a core/target/classes sonar-analysis-data/core-classes 2>/dev/null || true
         cp -a processor/target/classes sonar-analysis-data/processor-classes 2>/dev/null || true
+        cp -a core/target/test-classes sonar-analysis-data/core-test-classes 2>/dev/null || true
+        cp -a processor/target/test-classes sonar-analysis-data/processor-test-classes 2>/dev/null || true
         cp -a processor/target/site/jacoco-aggregate/jacoco.xml sonar-analysis-data/jacoco-aggregate.xml 2>/dev/null || true
         cp -a processor/target/site/jacoco/jacoco.xml sonar-analysis-data/processor-jacoco.xml 2>/dev/null || true
         {

README.md

@@ -51,6 +51,8 @@ Simple Builders generates fluent, type-safe builders for your **existing** class
 
 Use Simple Builders when you want fluent, type-safe builders for the classes and records you already have, generated as plain readable source, with no bytecode manipulation and no IDE plugin — and no lock-in: because the builders are ordinary generated Java, you can drop the dependency at any time by copying the generated builder classes into your own sources, and they keep working.
 
+For performance benchmark results comparing Simple Builders, Simple Minimal Builder, RecordBuilder, and Lombok, see the [Performance Analysis Guide](performance-test/docs/PERFORMANCE_ANALYSIS.md#benchmark-results).
+
 ### Doing what other builders advertise — the Simple Builders way
 
 - **Required fields:** Primitive fields and fields annotated with an annotation named `NotNull` or `NonNull` are non-nullable; constructor parameters are builder inputs. `build()` enforces the required/non-null contract with `IllegalStateException` ([configuration details](#required-fields-and-null-safety)).

core/pom.xml

@@ -75,7 +75,7 @@
     <plugin.central.publishing.version>0.11.0</plugin.central.publishing.version>
     <plugin.cyclonedx.version>2.9.3</plugin.cyclonedx.version>
     <plugin.code-format.version>2.29</plugin.code-format.version>
-    <plugin.maven.compiler.version>3.15.0</plugin.maven.compiler.version>
+    <plugin.maven.compiler.version>3.16.0</plugin.maven.compiler.version>
     <plugin.maven.source.version>3.4.0</plugin.maven.source.version>
     <plugin.maven.javadoc.version>3.12.0</plugin.maven.javadoc.version>
     <plugin.maven.gpg.version>3.2.8</plugin.maven.gpg.version>

docs/CONTRIBUTING.md

@@ -375,6 +375,27 @@ requests from forks, so the secret-backed quality checks are handled specially:
   run** (in the Actions/Environments prompt) before it executes. It does not
   rebuild or retest fork code with the token.
 
+  The workflow is designed so that **no fork-controlled content is executed**
+  while the token is present, independent of the approval gate:
+  - Maven runs only against the trusted base-branch checkout, to resolve the
+    dependency classpath. The fork's `pom.xml`, `.mvn/` directory and wrapper
+    scripts are never parsed or run (they can execute arbitrary code, which is
+    what SonarCloud rule S7631 flags for privileged `workflow_run` jobs).
+  - The fork PR head is then fetched as plain git data from the base repo's
+    `refs/pull/N/head`, verified to be the commit the CI artifact was built
+    from, and checked out so the scanner can read the sources and compute
+    changed lines from SCM history.
+  - Analysis runs with the standalone Sonar scanner (`sonarqube-scan-action`),
+    not the Maven plugin. Its configuration is written by the workflow outside
+    the workspace and selected via `project.settings`, so a
+    `sonar-project.properties` in the fork tree is ignored (that file could
+    otherwise point the scanner at an attacker-supplied Java executable).
+
+  Residual exposure is limited to the scanner *reading* untrusted sources and
+  bytecode, which is the same exposure as analysing any PR. The `sonar.*`
+  properties in the workflow mirror those in the root `pom.xml`; keep them in
+  sync when changing analysis settings.
+
 ## Questions?
 
 If you have questions or need help:

example-custom-generator/pom.xml

@@ -17,7 +17,7 @@
     <code-format.version>2.25</code-format.version>
     <commons-lang.version>3.20.0</commons-lang.version>
 
-    <plugin.maven.compiler.version>3.15.0</plugin.maven.compiler.version>
+    <plugin.maven.compiler.version>3.16.0</plugin.maven.compiler.version>
     <maven.compiler.source>${java.version}</maven.compiler.source>
     <maven.compiler.target>${java.version}</maven.compiler.target>
     <maven.compiler.release>${java.version}</maven.compiler.release>

example/pom.xml

@@ -19,7 +19,7 @@
     <junit-jupiter.version>6.1.3</junit-jupiter.version>
     <jackson-databind.version>2.22.2</jackson-databind.version>
 
-    <plugin.maven.compiler.version>3.15.0</plugin.maven.compiler.version>
+    <plugin.maven.compiler.version>3.16.0</plugin.maven.compiler.version>
     <plugin.maven.deploy.version>3.1.4</plugin.maven.deploy.version>
 
     <maven.compiler.source>${java.version}</maven.compiler.source>

performance-test/docs/PERFORMANCE_ANALYSIS.md

@@ -15,6 +15,7 @@ processor to measure processing time. Three scripts work together:
 | `run_performance_measurement.py` | Run N compilations and aggregate timing results |
 | `compare_performance.py` | Compare results from multiple measurement runs side-by-side |
 | `run_full_comparison.py` | Run all frameworks end-to-end and compare (convenience) |
+| `run_full_analysis.sh` | Full analysis: cross-framework comparison plus formatting-mode analysis (recommended entry point) |
 
 ## Supported Builder Types
 
@@ -30,7 +31,19 @@ metrics. Builder types without JSON reports only measure overall wall time.
 
 ## Quick Start
 
-Run all four frameworks with N runs each, then compare:
+For the complete analysis (cross-framework comparison plus formatting-mode
+breakdown), run the top-level script:
+
+```bash
+./scripts/run_full_analysis.sh
+```
+
+It runs all four frameworks with wall-time only, then runs simple-builder with
+each formatting mode (`jdt`, `lightweight`, `none`) and JSON tracking enabled,
+and finally prints comparisons. `RUNS=5 ./scripts/run_full_analysis.sh`
+overrides the default of 10 runs per measurement.
+
+To run only the cross-framework comparison:
 
 ```bash
 python3 scripts/run_full_comparison.py --runs 10
@@ -56,6 +69,10 @@ python3 scripts/generate_classes.py --builder-type simple-builder --force
 python3 scripts/run_performance_measurement.py --runs 30 --label sb-30runs --builder-type simple-builder
 ```
 
+`run_performance_measurement.py` also accepts `--formatting-mode <jdt|lightweight|none>`
+to control source formatting for simple-builders types, and `--no-tracking` to
+skip JSON processor metrics (wall-time only).
+
 Results are written to `performance-test/performance-reports/<label>/`. Compare paths
 are relative to that directory (or use absolute paths). The parent directory
 name is used as the column label.
@@ -127,3 +144,85 @@ performance-test/
   "wallTimeOnly": true
 }
 ```
+
+## Benchmark Results
+
+The following results are from a 10-iteration run of
+[`run_full_analysis.sh`](../scripts/run_full_analysis.sh)
+(wall-time-only cross-framework part, equivalent to
+`run_full_comparison.py --runs 10 --no-tracking`).
+All frameworks were measured with wall-time only (no JSON tracking overhead) to
+ensure a fair comparison. Note that the builder counts differ: Simple Builders
+and Lombok generate one builder per class, while RecordBuilder generates fewer
+(295) because the test dataset contains fewer records than plain classes.
+
+### Wall-Time Comparison (10 runs, wall-time only)
+
+| Framework | Builders | Wall Avg (s) | Wall Min (s) | Wall Max (s) | Per Builder (ms) |
+|-----------|----------|-------------|-------------|-------------|-----------------|
+| Simple Builder (`@SimpleBuilder`) | 1079 | 56.2 | 54.1 | 61.6 | 50.2 |
+| Simple Minimal Builder (`@SimpleMinimalBuilder`) | 1079 | 23.2 | 22.8 | 23.9 | 20.3 |
+| RecordBuilder (`@RecordBuilder`) | 295 | 7.0 | 6.9 | 7.1 | 19.6 |
+| Lombok (`@Builder`) | 1077 | 7.2 | 7.0 | 7.5 | 5.6 |
+
+Key observations:
+
+- **Lombok** is fastest per builder but instruments bytecode at compile time
+  rather than generating separate source files, so the comparison is not
+  apples-to-apples.
+- **RecordBuilder** generates far fewer builders (295 vs 1079) because the test
+  dataset contains fewer records than plain classes. However, its per-builder
+  cost (~19.6 ms) is nearly identical to Simple Minimal Builder (~20.3 ms) — the
+  wall-time difference is almost entirely due to the lower builder count, not
+  per-builder efficiency.
+- **Simple Minimal Builder** is ~2.4x faster than Simple Builder. The speedup
+  comes from two factors: fewer generated methods (no collection helpers,
+  conditional logic, supplier/consumer setters, Javadoc, etc.) and
+  correspondingly less source formatting work.
+- **Simple Builder** is the slowest due to its full feature set. The per-builder
+  cost (~50 ms) is dominated by code generation and formatting.
+
+### Processor-Internal Breakdown (with JSON tracking)
+
+For deeper insight into where time is spent inside the Simple Builders
+processor, enable the processor's internal performance tracker with
+`-Asimplebuilder.performanceTracking=true`. This adds minor overhead but
+provides phase-level breakdowns in the JSON report.
+
+A 10-run measurement of Simple Builder (full features, JDT formatting, with
+tracking enabled) shows the following phase distribution:
+
+| Phase | Avg (s) | Share |
+|-------|---------|-------|
+| Config Resolution | 0.08 | <1% |
+| Builder Def Extraction | 0.76 | ~2% |
+| DTO Mapping | 0.04 | <1% |
+| Code Generation | 42.9 | ~93% |
+| **Processor Total** | **46.1** | |
+| **Wall Total** | **55.5** | |
+
+Code generation dominates at ~93% of processor time. This includes Roaster
+source construction, string serialization, and source formatting. The same run
+set compared across the three formatting modes (Simple Builder, 1079 builders)
+shows the following:
+
+| Formatting Mode | Wall Avg (s) | Processor Avg (s) | Formatting Phase (s) | vs NONE |
+|-----------------|-------------|-------------------|----------------------|---------|
+| NONE (raw Roaster) | 41.8 | 30.7 | 0.03 | — |
+| LIGHTWEIGHT | 42.7 | 31.3 | 0.14 | +2% |
+| JDT (default) | 55.5 | 46.1 | 17.6 | +33% |
+
+The Eclipse JDT formatter is expensive: ~17.5s across 1079 builders (~16 ms per
+builder), roughly a third of total wall time. The lightweight formatter, by
+contrast, is nearly free — its regex/string post-processing adds only ~0.1s.
+`none` and `lightweight` are effectively equivalent in speed, so the choice
+between them is about output readability, not performance. For
+performance-sensitive builds (e.g. large generated codebases), `lightweight` or
+`none` is recommended over `jdt`.
+
+### Running the Benchmarks
+
+To reproduce the wall-time comparison, see [Quick Start](#quick-start) above.
+For processor-internal breakdowns, run
+[`run_performance_measurement.py`](../scripts/run_performance_measurement.py)
+without `--no-tracking` to enable JSON reporting.