Skip to content

Commit 40b5bd8

Browse files
author
Markus Paulsen
committed
Merge origin/main into the documentation branch
The pull request had gone conflicting against main, and a conflicting pull request has no merge ref, so GitHub could not schedule any `pull_request` workflow. Only the labeler ran, on `pull_request_target`. The last two pushes therefore had no continuous integration at all, which is why this merge is here rather than later. One conflict, in the test-case factory manual. Main rewrote the scanning section when the scanner stopped matching regular expressions and started reading the syntax tree with the imports resolved. This branch still carried the regex description, `CLASS_PATTERN` and the rest. Main's text describes what the code does, so it wins; git had already followed the rename from `docs/securitytest/TestCaseFactoryAndBuilderManual.md`. The version bump survives the merge: the pom reads 2.1.3 and nothing below it refers to Ares. The two remaining mentions of 2.1.2 are in `ReleasedCreatorContractTest` and `ReleasedConstructorCompatibilityTest`, which say what the 2.1.2 release published and are kept callable. Those are statements about a release that happened, not coordinates, and 2.1.3 has published nothing. The prose gate found one enforced violation in the text main brought over, an "actually" the sentence does not need, now gone. Three advisory counts rose with that text: active-voice from 760 to 772, long-sentence from 111 to 115, and prefer-must from 150 to 151. Those numbers are recorded rather than fixed. A merge that carries in a page written before these rules existed is the one case where a ceiling rises honestly, and both `cli.mjs` and the writing-rules page now say so, along with the expectation that the next pass over that page lowers them. Verified on the merge: 77 scanner fixtures, lint:prose at 0 enforced, eslint, tsc, a strict build, 15 browser tests, and 644 documentation structure tests compiled against main's production code.
2 parents 69bc705 + e6a2e28 commit 40b5bd8

82 files changed

Lines changed: 5282 additions & 322 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.github/PULL_REQUEST_TEMPLATE.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,19 @@
174174
should read. If a claim cannot be observed anywhere, either add the diagnostic that
175175
makes it observable, or do not ask for it.
176176
177+
Have the reviewer look at the result, not at an exit code. A command that exits zero
178+
says the command ran. It does not say that what it produced is right, and a manual made
179+
of green commands asks a reviewer to review your exit codes rather than your change.
180+
Every claim in section 2 needs a step where the thing itself is in front of them: the
181+
page as a reader gets it, the message a student is shown, the generated file, the row in
182+
the report. Name what they must see there, and name what would be wrong.
183+
184+
This matters most where a build cannot see the defect. A site builds cleanly while a box
185+
renders as plain text, a policy is read without complaint while the rule it was meant to
186+
express is not enforced, and a test passes while asserting nothing. Where a suite in this
187+
repository already looks at such a result for you, run it as a step and say what it
188+
covers, rather than leaving a reviewer to assume the build covered it.
189+
177190
Negative case: equally important for a security tool. State what must still be
178191
rejected, and how a reviewer confirms that Ares has not become more permissive.
179192

.github/scripts/CheckPullRequestTemplate.java

Lines changed: 248 additions & 41 deletions
Large diffs are not rendered by default.

.github/workflows/codeql.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ jobs:
3939
cache: "maven"
4040

4141
- name: Initialise CodeQL
42-
uses: github/codeql-action/init@5595ccaf912efad79be6eef63a5619ff05969be3 # v4.37.6
42+
uses: github/codeql-action/init@ff2f1c621b7f889edc0d3c761ac2e6a3f8cdb0dd # v4.37.7
4343
with:
4444
languages: java
4545
build-mode: manual
@@ -48,4 +48,4 @@ jobs:
4848
run: mvn -B -DskipTests package
4949

5050
- name: Perform CodeQL analysis
51-
uses: github/codeql-action/analyze@5595ccaf912efad79be6eef63a5619ff05969be3 # v4.37.6
51+
uses: github/codeql-action/analyze@ff2f1c621b7f889edc0d3c761ac2e6a3f8cdb0dd # v4.37.7

AGENTS.md

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -105,13 +105,16 @@ rule is an approximation in the other direction too, since four columns under a
105105
continue that paragraph in Markdown; the Javadoc of the checker says why the line is drawn
106106
where it is.
107107

108-
Two shapes pay for that four-column rule, and both were accepted knowingly. An indented
109-
example showing the template's own skeleton, a bare `1.` inside a comment, is read as a
110-
blank somebody forgot. And a literal comment marker inside code, above a genuine comment
111-
further down, can leave the genuine one unhidden, so a heading inside it is counted and the
112-
author is told they wrote a section twice. Both are wrong about a body written in good
113-
faith, and both are the accepted cost of the rule that closes a hole where text hid from
114-
the length count. It deliberately does not require checklist boxes to be ticked. It
108+
One shape pays for that four-column rule, knowingly: an indented example showing the
109+
template's own skeleton, a bare `1.` inside a comment, is read as a blank somebody forgot.
110+
That is wrong about a body written in good faith, and is the accepted cost of closing a
111+
hole where text hid from the length count.
112+
113+
What is not prose is found in one left-to-right walk rather than by searching for comments,
114+
fenced blocks and code spans separately. That walk is what settles the overlaps between the
115+
three: a fence opening a line beats a code span left open above it because Markdown decides
116+
blocks first, and a span covers what it holds, so a comment marker inside one is text rather
117+
than the start of a comment. It deliberately does not require checklist boxes to be ticked. It
115118
re-runs when the description is edited, so a failure is fixed by editing the body rather
116119
than by pushing a commit.
117120

@@ -128,6 +131,21 @@ PR_BODY="$(cat body.md)" java .github/scripts/CheckPullRequestTemplate.java
128131
The checker is a single-file Java program, run through the source-code launcher of JDK 11 or
129132
newer, so it needs no build step and adds no language to the repository. CI runs it on 21.
130133

134+
**Only the template's own headings.** The check reports every line it reads as a heading that
135+
the template does not define, sub-headings included. A section of your own is never checked: the
136+
checker cuts out the required headings only, so whatever sits under an invented one is measured
137+
as part of the section above it, and what its title promises is never looked for. Put the text
138+
in the section it belongs to, or in a comment on the pull request, which is where anything
139+
outside the template's shape goes.
140+
141+
What it reads as a heading is a line of up to three spaces, then one to six hashes, then a
142+
space, a tab or the end of the line, outside comments, fenced blocks and code between backticks.
143+
`#hashtag` and seven hashes are therefore not headings to it. It does not find a heading
144+
underlined with equals signs, nor one indented into a quotation or a list; those pass. It does
145+
not recognise raw HTML blocks either, so such a line inside `<pre>` or `<details>` is reported.
146+
Put a line you do not mean as a heading in a fenced code block, where the checker does not treat
147+
it as one.
148+
131149
**Changing the template is two edits, not one.** The required headings, the character
132150
limits and the phrases that answer a section live in one ordered map, `SECTIONS`, at the
133151
top of `.github/scripts/CheckPullRequestTemplate.java`, in the order the template puts them

documentation/docs/contributor/subsystems/securitytest/test-case-factory-and-builder.md

Lines changed: 47 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -459,7 +459,7 @@ Defines five scanning methods that auto-detect project metadata:
459459
|---|---|---|
460460
| `scanForBuildMode()` | `BuildMode` | Whether the project uses Maven (`pom.xml`) or Gradle (`build.gradle`) |
461461
| `scanForTestClasses()` | `String[]` | Fully qualified names of all classes in the **test source directory** containing `@Test` or `@Property` annotations, or extending JUnit 3's `TestCase` |
462-
| `scanForPackageName()` | `String` | The most frequently used non-reserved package declaration across all `.java` files |
462+
| `scanForPackageName()` | `String` | The most frequently used non-reserved package: taken from the production sources, otherwise from the compiled production output, otherwise the configured default |
463463
| `scanForMainClassInPackage()` | `String` | The class containing `public static void main(String[])` |
464464
| `scanForTestPath()` | `Path` | The file system path to the test source directory |
465465

@@ -468,33 +468,65 @@ Defines five scanning methods that auto-detect project metadata:
468468
| Aspect | Detail |
469469
|---|---|
470470
| **Implements** | `ProjectScanner` |
471-
| **Technique** | Regex-based source code analysis. Walks all `.java` files under the project root and applies four compiled regex patterns. |
471+
| **Technique** | JavaParser-backed source analysis. Walks the `.java` files under the discovered source roots and reads the parsed syntax tree; the compiled output is read with ArchUnit's `ClassFileImporter` where the sources yield nothing. |
472472

473-
**Regex patterns:**
473+
**What is read from the syntax tree:**
474474

475-
| Pattern | Matches | Used by |
475+
| Fact | Read from | Used by |
476476
|---|---|---|
477-
| `CLASS_PATTERN` | `public [final\|abstract\|strictfp] class ClassName` | `extractClassName()` |
478-
| `PACKAGE_PATTERN` | `package com.example.foo;` | `extractPackageName()` |
479-
| `MAIN_METHOD_PATTERN` | `public static void main(String[] args)` (including varargs) | `extractMainClass()` |
480-
| `TEST_ANNOTATION_PATTERN` | `@Test` or `@Property` | `extractTestClass()` (which treats classes containing `extends TestCase` as test classes) |
477+
| Package declaration | the compilation unit's `PackageDeclaration` | `scanForPackageName()` |
478+
| Type declarations | the top-level `TypeDeclaration`s, nested types included | `scanForMainClassInPackage()`, `scanForTestClasses()` |
479+
| `main` method | a `public static void main(String[])` declaration, varargs included | `scanForMainClassInPackage()` |
480+
| Test classes | a `@Test` or `@Property` annotation, or a JUnit 3 `TestCase` supertype resolved through the imports of the file | `scanForTestClasses()` |
481+
482+
Resolving the supertype through the imports is why this is not a regex: `extends TestCase` names a type, and which type it names depends on what the file imported.
481483

482484
**Scanning pipeline:**
483485

484486
```
485-
ProjectSourcesFinder.findProjectSourcesPath()
486-
→ Files.find(sourcePath, MAX_VALUE, isJavaFile)
487-
→ Files.readString(file)
488-
→ extractor.apply(content)
487+
ProjectSourcesFinder.discover(projectRoot, mode) → BuildToolConfiguration
488+
→ configuration.productionSourceRoots() / testSourceRoots()
489+
→ Files.walk(root), filtered to *.java and sorted
490+
→ JavaParser.parse(file) → CompilationUnit
491+
and, where the sources answer nothing:
492+
→ ClassFileImporter().importPath(productionOutputRoot)
489493
```
490494

491-
**`scanForPackageName()` algorithm:** First filters out reserved infrastructure prefixes (via `ReservedPackageGuard.reservedPrefixOf(...)`) so a student cannot flood the project with files in a trusted namespace to make it the derived enforcement scope → counts the frequency of every remaining `package` declaration across all files → returns the most common one. This heuristic works because in a typical student project, the main source package appears in the majority of files.
495+
The sort is not cosmetic: it is what makes two runs over one project agree.
496+
The legacy `findProjectSourcesPath()` route still exists for callers that
497+
predate `BuildToolConfiguration`, and differs in kind: it returns the
498+
descriptor's own string, relative and unvalidated, where `discover(...)`
499+
canonicalises every root and refuses one that escapes the project.
500+
501+
**`scanForPackageName()` algorithm:** Resolution runs in three steps, each reached only when the previous one finds nothing at all.
502+
503+
1. **Production sources.** Reserved infrastructure prefixes are filtered out first (via `ReservedPackageGuard.reservedPrefixOf(...)`), so a package inside a trusted namespace cannot become the derived enforcement scope. The frequency of every remaining `package` declaration is counted and the most common one wins. This heuristic works because in a typical student project the main source package appears in the majority of files.
504+
2. **Compiled production output.** Only top-level classes are counted, so a package is not weighted by how many nested or anonymous classes it happens to contain; nesting is read from the class file rather than from the `$` in the binary name, which is a legal identifier character. This step covers a project whose build descriptor the source-root discovery cannot parse, because the build tool writes its output to the conventional directory the scanner reads.
505+
3. **The configured default** (see [Section 10.3](#103-javaprogrammingexerciseprojectscanner)), with a warning naming the roots that were searched.
506+
507+
Step 1 is skipped entirely when the discovered source roots are not known to be the whole of the main source set. A Gradle descriptor can declare a root this reader cannot resolve, such as a computed list, and `BuildToolConfiguration.productionRootsComplete()` reports that. Counting declarations across part of a project produces an answer indistinguishable from one taken across all of it, so a partial set is not counted at all and the compiled output is read instead.
508+
509+
> **The derived package is a heuristic. What turns it into a boundary is the check that follows it.**
510+
>
511+
> Before enforcement is armed, `requireDerivedScopeToCoverTheProject()` reads the compiled production output and refuses the run unless **every** executable top-level class declares a non-blank, non-reserved package that is the derived scope or lies below it, compared on segment boundaries so that `de.tum.cit.aet` does not swallow `de.tum.cit.aetevil`. A class the scope leaves out, a class in the default package, a class in a reserved package, and an output root that exists but cannot be read are each refused by name. This runs on the policy-free path only: a pinned policy may deliberately supervise part of the output, and narrowing it is then the instructor's decision.
512+
>
513+
> **An output root holding nothing passes, with a warning, and so does one holding only `package-info` or `module-info`.** There is then no supervisable class, so enforcement is vacuous rather than mis-scoped, and an exercise whose supervised package is still empty must not fail for being empty. The generated test does the same: `JavaArchunitSupervisedClasses` warns and analyses an empty set rather than refusing. In both places that log line is the only signal, and a suite that analyses nothing reports success.
514+
>
515+
> That closes the case where a decoy package is voted the scope while the assignment runs beside it. Three things it still does not establish.
516+
>
517+
> The **vote is influenceable by whoever can add files to the project**, and in an Artemis exercise that includes the student. The check above refuses a scope that leaves compiled classes out, but not one drawn *around* them: a scope that covers everything passes by construction. The package-import allow-list no longer follows the scope for that reason, and names the packages the validated output declares instead.
518+
>
519+
> The **output directory is assumed, not read**. Step 2 and the check both look in `target/classes` or `build/classes/java/main`, so a build that writes its output elsewhere is not followed there. Together with the vacuous pass above, that is the sharp edge of this section: a project whose output goes somewhere else looks exactly like a project that compiled nothing, and both pass with the same warning. An exercise configured with a custom output destination is therefore not enforced by the derived path at all, and nothing fails to say so. Such an exercise must declare its scope in a policy.
520+
>
521+
> **The last-resort default guarantees nothing by itself.** If the project does not contain it, the analysis path resolves to a directory that does not exist. Where anything at all is compiled, the check above catches it: those classes lie outside the default, so the run is refused by name. Where nothing is compiled it does not, and the warning is again all a reader gets. During generation, before anything is compiled, the same is true.
522+
>
523+
> An exercise that needs a scope it can rely on declares its package in the security policy. The scanner is then not consulted at all, which is the only version of this that cannot be steered from the submission.
492524
493525
**`scanForTestClasses()` algorithm:** Scans only the **test source directory** (see `scanForTestPath()`) and returns every class whose file contains a `@Test` / `@Property` annotation or `extends TestCase`.
494526

495527
**`scanForMainClassInPackage()` algorithm:** Collects all classes with a `main` method → prefers a class named `Main` or `Application` → otherwise returns the first match → defaults to `"Main"`.
496528

497-
**`scanForTestPath()` algorithm:** Checks for Gradle's custom `srcDir 'test'`falls back to `src/test/java`.
529+
**`scanForTestPath()` algorithm:** Answers the first discovered test source root; without a build configuration it accepts the conventional `src/test/java`, or a bare `test/` directory for the Artemis Gradle layout, and otherwise falls back to the literal `src/test/java` **whether or not it exists**. That fall-back is a placeholder forced by the non-null return type rather than a claim, and no production code currently consults this method.
498530

499531
### 10.3 `JavaProgrammingExerciseProjectScanner`
500532

@@ -505,7 +537,7 @@ ProjectSourcesFinder.findProjectSourcesPath()
505537

506538
| Override | Default in `JavaProjectScanner` | Override in `JavaProgrammingExerciseProjectScanner` |
507539
|---|---|---|
508-
| Default package | `""` (empty string) | `"de.tum.cit.ase"` |
540+
| Default package | `""` (empty string) | `"de.tum.cit.aet"` |
509541
| Default main class | `"Main"` | `"Main"` (unchanged) |
510542

511543
When the base scanner finds no package or main class, these TUM-specific defaults ensure reasonable behaviour for Artemis-hosted exercises.

documentation/docs/contributor/writing-rules.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,11 @@ it cannot rise without somebody writing the higher number down in a commit. A ru
112112
entry has a ceiling of zero, so adding a rule and leaving it unrecorded fails rather than
113113
passing unnoticed.
114114

115+
One case raises a number honestly: a merge that brings in a page written before these rules
116+
existed. The text arrives already over the line, and holding the merge hostage to an editorial
117+
pass on somebody else's page helps nobody. Record the higher number in the merge commit, say
118+
which merge raised it, and let the next pass over that page bring it down again.
119+
115120
That is what stops 1,192 advisory findings quietly becoming 1,500. The counts are not a target to
116121
drive to zero. `active-voice` in particular never reaches it, because a security reference
117122
describes what happens to code and some of that is genuinely agentless. They are a direction.
Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
{
2-
"why": "The advisory findings each rule is allowed. These numbers may only fall; see scripts/prose/cli.mjs. Lower one by improving the prose, never by editing it here.",
2+
"why": "The advisory findings each rule is allowed. Lower one by improving the prose, never by editing it here. A number rises only where a merge brings in text written before these rules existed, and only in the commit that brings it; see scripts/prose/cli.mjs.",
33
"ceiling": {
4-
"active-voice": 760,
4+
"active-voice": 772,
55
"address-the-reader": 45,
66
"context-filler": 19,
7-
"long-sentence": 111,
7+
"long-sentence": 115,
88
"no-intensifiers": 24,
9-
"prefer-must": 150,
9+
"prefer-must": 151,
1010
"present-tense": 83
1111
}
1212
}

documentation/scripts/prose/cli.mjs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -249,8 +249,9 @@ async function accept() {
249249

250250
const counts = [...advisoryCounts(findings)].sort((left, right) => left[0].localeCompare(right[0]));
251251
await writeFile(CEILING, `${JSON.stringify({
252-
why: 'The advisory findings each rule is allowed. These numbers may only fall; see '
253-
+ 'scripts/prose/cli.mjs. Lower one by improving the prose, never by editing it here.',
252+
why: 'The advisory findings each rule is allowed. Lower one by improving the prose, never '
253+
+ 'by editing it here. A number rises only where a merge brings in text written before '
254+
+ 'these rules existed, and only in the commit that brings it; see scripts/prose/cli.mjs.',
254255
ceiling: Object.fromEntries(counts),
255256
}, null, 2)}\n`, 'utf8');
256257

pom.xml

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@
4545
${user.home}${file.separator}.m2${file.separator}repository${file.separator}org${file.separator}aspectj${file.separator}aspectjrt${file.separator}${aspectj-version}${file.separator}aspectjrt-${aspectj-version}.jar
4646
</aspectj-path>
4747
<!-- 3. AOP: 3.2 Byte Buddy -->
48-
<byte-buddy-version>1.18.11</byte-buddy-version>
48+
<byte-buddy-version>1.18.12</byte-buddy-version>
4949
<byte-buddy-path>
5050
${project.build.directory}${file.separator}${project.artifactId}-${project.version}-agent.jar
5151
</byte-buddy-path>
@@ -62,18 +62,18 @@
6262
<!-- 4. AST: 4.1 JavaParser -->
6363
<java-parser-version>3.28.2</java-parser-version>
6464
<!-- 5. Formats: YAML and XML (JSON handled via Jackson) -->
65-
<yaml-version>2.22.1</yaml-version>
65+
<yaml-version>2.22.2</yaml-version>
6666
<!-- 6. Large Tooling Libraries -->
6767
<commons-io-version>2.22.0</commons-io-version>
6868
<jaxb-api-version>2.3.1</jaxb-api-version>
6969
<!-- 7. Small Tooling Libraries -->
7070
<jgrapht-core-version>1.5.3</jgrapht-core-version>
7171
<!-- 8. Logging -->
72-
<logback-version>1.6.1</logback-version>
72+
<logback-version>1.6.3</logback-version>
7373
<!-- 8. API -->
7474
<apiguardian-api-version>1.1.2</apiguardian-api-version>
7575
<!-- Others -->
76-
<spotless.version>3.9.0</spotless.version>
76+
<spotless.version>3.10.0</spotless.version>
7777
<!-- if JaCoCo is not executed -->
7878
<argLine></argLine>
7979
<!-- Isolate test classes so WALA state and test-created threads cannot accumulate
@@ -874,7 +874,7 @@
874874
<plugin>
875875
<groupId>com.github.spotbugs</groupId>
876876
<artifactId>spotbugs-maven-plugin</artifactId>
877-
<version>4.10.3.0</version>
877+
<version>4.10.4.0</version>
878878
<configuration>
879879
<excludeFilterFile>${basedir}${file.separator}.settings${file.separator}spotbugs-exclude.xml</excludeFilterFile>
880880
<failOnError>true</failOnError>

0 commit comments

Comments
 (0)