Detect the supervised package instead of defaulting to it by MarkusPaulsen · Pull Request #174 · ls1intum/Ares2

MarkusPaulsen · 2026-08-04T08:44:32Z

Summary

Gradle source-root discovery silently dropped every srcDirs declaration, so a project declaring its main
sources that way looked to Ares like a project with no production sources at all. Without a policy, the
supervised package then fell straight through to a configured default the project need not contain, and
enforcement was scoped to a directory that does not exist.

Linked issues

None. Found while migrating a set of Artemis ITP exercises from Ares 1 to Ares 2 and removing their @Policy
annotations to use the policy-free configuration.

1. Problem

Observed with Ares 2.1.2 on JDK 17, Gradle 9.0.0, ArchUnit and AspectJ, in an exercise whose
tests carry Ares annotations but no @Policy, so the policy-free configuration applies. Setup
manual section 6 documents file system, network, command execution and thread creation as
denied there.

Supervised code performed all of them unhindered: Files.readString, a ProcessBuilder start
and a Thread start all ran, and the build stayed green. System.exit(3) was still blocked, so
only the five resource domains were unarmed.

The fault is in plugging Ares into the build. ProjectSourcesFinder.SOURCE_DIRECTORY tried
srcDir before srcDirs, and the former is a prefix of the latter, so on
srcDirs = ["assignment/src"] the capture began at the leftover, the trailing s, and yielded
s = ["assignment/src", which resolveGradlePath cannot resolve. The entry was dropped without
a diagnostic and discovery returned no production roots.

Ares let forbidden student code through, silently.

2. Improvement from the user's perspective

Instructors using the policy-free configuration get the enforcement the manual promises. Before this change, an
exercise whose main source set is declared with srcDirs, which includes Artemis Java exercises built from the
standard template, enforced nothing in the file, network, command and thread domains while reporting a green run.
Students see no change in a correctly configured exercise; in an affected one, code that should have been
rejected is now rejected.

The last-resort default now names the package the Artemis exercise templates are generated with,
de.tum.cit.aet, so an exercise that reaches step three lands on its own code rather than one letter beside it.
The thread domain no longer depends on the supervised package being scoped correctly to work at all: a
mis-scoped exercise now narrows enforcement instead of disabling a whole domain.

3. Improvement from the maintainer's perspective

Discovery no longer collapses to a configured default in one step. A second detection step
reads the compiled production output, which is authoritative whatever the descriptor says,
because the build tool writes it to its own location, so an unparsed descriptor no longer costs
the supervised scope. Where nothing is detectable the default still applies, but now says so in
a warning naming the roots that were searched. The parser's remaining gaps are documented at
the pattern rather than implied.

The self-tests no longer inherit their supervised package from a production default.
SecurityUser and PackageAccessUser pin PolicySelfTestDefaultRestrictive.yaml, which
declares the same fully restrictive accesses the policy-free path derives, so what they assert
is unchanged while a future change to the TUM default cannot disarm them. The timeout exemption
is keyed to the method that creates the worker rather than to the class.

4. Testing manual

The parsing defect is Gradle-only, so the manual uses Gradle. The scanner change affects both build tools.

Prerequisites

JDK 17 and Gradle. No echo server is needed; nothing here touches the network.
Build and install this branch: mvn install -DskipTests.
Use examples/ares-exercise-gradle.

Steps

In examples/ares-exercise-gradle/build.gradle, replace the main source-set declaration
srcDir 'src/main/java' with the list form srcDirs = ['src/main/java'], and change nothing else. This is
the only edit needed to reproduce the defect, and it is what the Artemis exercise template does.
Remove the @Policy annotation and its import from the example's test class, so the policy-free
configuration applies. Leave the Ares test annotation in place: it is what activates Ares.
Add a method to the student-facing class that reads a file in the project directory, and do not create that
file:

public String readForbiddenFile() throws java.io.IOException {
       return java.nio.file.Files.readString(java.nio.file.Path.of("secret.txt"));
}

Add a test that asserts the rejection, so the outcome is visible either way:

@PublicTest
void forbiddenReadIsRejected() {
       SecurityException violation = assertThrows(SecurityException.class,
               () -> new Penguin("Julian").readForbiddenFile());
       assertTrue(violation.getMessage().contains("secret.txt"));
}

Run ./gradlew clean test.
Repeat steps 1 to 5 against Ares 2.1.2 to see the previous behaviour.

Expected result

Step 5, on this branch: the build is green and forbiddenReadIsRejected passes, that is Ares threw. In the
test output, the line Resolved Ares analysis/import path for GRADLE: .../build/classes/java/main/<your package path> names the package of the example's own classes.
Step 6, on 2.1.2: forbiddenReadIsRejected fails, and its message shows
java.nio.file.NoSuchFileException: secret.txt rather than a SecurityException, that is the read reached the
file system. The same log line ends in de/tum/cit/ase, a directory that does not exist under
build/classes/java/main, which is the defect made visible.
The new No supervised package could be detected warning must not appear in step 5. It appears only when
neither the sources nor the compiled output declares a package.

Negative case (what must still be rejected)

Ares has not become more permissive: with a SecurityPolicy.yaml and @Policy restored, the same forbidden
read is still rejected, exactly as before.
The reserved-package boundary is untouched: a student class declaring package de.tum.cit.ase.ares.api; must
still fail the build with the reserved-package diagnostic.
The default no longer wins over evidence: with production sources undiscoverable but
build/classes/java/main/de/tum/cit/detected/Calculator.class present, scanForPackageName() answers
de.tum.cit.detected and not the configured default. Covered by JavaProjectScannerPackageFallbackTest, whose
fixture package is deliberately distinct from the default so the assertion cannot hold through the fallback.
The timeout exemption still exempts what it must and nothing more: Ares' own worker creation from
TimeoutUtils.executeWithTimeout stays exempt, so that worker is never attributed to the student, while a
thread operation reached through TimeoutUtils.rethrowThrowableSafe, that is from the
student's test body, is rejected. Covered by JavaAspectJThreadSystemAdviceDefinitionsTest and
JavaInstrumentationAdviceThreadSystemToolboxTest, and end to end by
de.tum.cit.ase.ares.integration.SecurityTest.test_useCommonPoolGood, which asserts that a benign
CompletableFuture.supplyAsync on the common pool is still blocked.
The self-tests assert the same thing as before: PolicySelfTestDefaultRestrictive.yaml declares the fully
restrictive resource accesses the policy-free path derives, so pinning it changes what the fixtures depend on,
not what they prove. The full SecurityTest and PackageAccessTest suites still pass.

Modes exercised

ArchUnit + AspectJ
ArchUnit + instrumentation
WALA + AspectJ
WALA + instrumentation

All four are green in CI. The manual walkthrough above was run in ArchUnit + AspectJ, which is the combination
the policy-free path fixes itself and therefore the only one reachable from a policy-free exercise. The
enforcement change touches both AOP variants, so it is applied symmetrically to
JavaAspectJThreadSystemAdviceDefinitions and JavaInstrumentationAdviceThreadSystemToolbox and covered by a
test for each.

5. Test case coverage regarding this PR

Read from site/jacoco/jacoco.csv in the coverage-report artefact of the Coverage Report job for
this branch's current head 74361ebd (workflow run 32113187323). Each cell is COVERED out of
MISSED plus COVERED, so a reviewer can recompute it. These replace figures taken at 18ce34ce,
before the review findings were addressed.

Class	Instruction coverage	Branch coverage	Line coverage	Complexity coverage	Method coverage	Confirmation (meaningful assertions)
`ProjectSourcesFinder`	93.4% (1964/2102)	78.3% (307/392)	92.3% (371/402)	68.6% (166/242)	100.0% (46/46)	yes
`JavaProjectScanner`	93.4% (1095/1173)	92.3% (144/156)	93.2% (177/190)	89.7% (105/117)	97.4% (38/39)	yes
`JavaCreator`	95.8% (523/546)	81.1% (30/37)	99.1% (114/115)	86.3% (44/51)	100.0% (30/30)	yes
`ReservedPackageGuard`	87.0% (127/146)	87.5% (28/32)	83.9% (26/31)	77.3% (17/22)	83.3% (5/6)	yes
`BuildToolConfiguration`	79.8% (205/257)	61.5% (16/26)	82.0% (41/50)	52.4% (11/21)	100.0% (8/8)	yes
`JavaTestCaseFactoryAndBuilder`	95.8% (115/120)	100.0% (2/2)	95.2% (20/21)	100.0% (10/10)	100.0% (9/9)	yes
`JavaProgrammingExerciseProjectScanner`	100.0% (11/11)	n/a (0/0)	100.0% (6/6)	100.0% (4/4)	100.0% (4/4)	yes
`JavaAspectJThreadSystemAdviceDefinitions`	70.2% (878/1250)	66.3% (161/243)	72.8% (201/276)	53.9% (82/152)	96.7% (29/30)	yes
`JavaInstrumentationAdviceThreadSystemToolbox`	67.9% (756/1114)	68.7% (156/227)	73.2% (180/246)	55.7% (78/140)	96.2% (25/26)	yes
Total (changed classes)	84.4% (5674/6719)	75.7% (844/1115)	85.0% (1136/1337)	68.1% (517/759)	98.0% (194/198)

The descriptor masker is no longer the exception it was. An earlier revision of this section
admitted that maskSlashyString, maskUntil and startsAnExpression were at zero, so Groovy's
slashy and dollar-slashy strings were handled by code nothing had executed. They are now at 90%,
95% and 92%, and maskInactiveRegions at 100%, which moves the class from 85.6% instruction and
70.6% branch to 93.4% and 78.3%, with every method executed. maskQuotedString at 52% and
maskBlockComment at 62% are what remain, and their uncovered branches are triple-quoted strings
and nested block comments.

Those regressions are written so they would fail if the masking stopped working, rather than passing
because nothing happened: the decoy declaration sits inside the java block and the directory it
names exists, so an unmasked string would replace the assignment root and the assertion would say so.
The division case is the exception and its comment says so, since an unterminated slashy string
already stops at the line break.

The rest of the assertions are specific rather than incidental. ProjectSourcesFinderTest and
ProjectSourcesFinderEdgeCaseTest assert the resolved roots and the rejections themselves, including
the two ways the reader used to empty a source set silently, a comment inside a live list and a list
written over several lines, and the completeness flag that now records a declaration it cannot
resolve. JavaProjectScannerScopeCoverageTest asserts every state of the output root against real
classes from ToolProvider.getSystemJavaCompiler(), including the all-reserved output that used to
pass. ReservedPackageGuardTest asserts ancestorOfReservedPrefix on segment boundaries, on
trailing-dot normalisation, and that a package which is itself reserved is left to the other guard.
The two advice tests drive isThreadCreationFromAresTimeout directly and assert both stack shapes.

The two advice classes are large and their figures are dominated by pointcut and message-building
code this pull request does not touch; the changed method itself is covered by the new tests.

Breaking changes and migration

The public API, the policy format, the generated tests and the minimum JDK, Maven and Gradle
versions are unchanged. Three behaviours change.

The point of the pull request. An exercise on the policy-free path declaring its main sources
with srcDirs enforced nothing in the five resource domains and now enforces them. One needing
any of them must declare it in a policy, as it always had to.

The TUM default package. getDefaultPackage() returns de.tum.cit.aet rather than
de.tum.cit.ase, reached only when nothing declares a package. An exercise living under
de.tum.cit.ase should pin theSupervisedCodeUsesTheFollowingPackage.

The thread domain under @StrictTimeout. Under a policy forbidding thread creation, a thread
started from a student's test body in a @StrictTimeout test is now rejected; a mis-scoped
exercise let it through. Ares' own timeout worker stays exempt, so no such test breaks.

Checklist

The title of this pull request describes the change, not the implementation.
I followed the guidelines for inclusive, diversity-sensitive and appreciative language.
I have self-reviewed the diff of this pull request.
Tests were added or updated for the behaviour changed here.
Documentation (docs/, README.adoc, Javadoc) was updated where the change is user-facing.
Setup manual section 6.2 already states that the policy-free configuration denies file, network, command
and thread access, so that statement needed no edit; this change makes the implementation match it.
docs/securitytest/TestCaseFactoryAndBuilderManual.md now documents the three-step resolution order and
states plainly that the last-resort default enforces nothing when the project does not contain that
package. The resolution order, the reasons a default must not win over evidence, and why the timeout
exemption is keyed to the creating method are documented in the Javadoc of scanForPackageName(),
getDefaultPackage() and isThreadCreationFromAresTimeout(), and the parser's remaining known gaps at
the pattern in ProjectSourcesFinder.
CI is green, or every remaining failure is explained above.
No secrets, tokens or absolute local paths are contained in the diff.

Review progress

Code review
Manual test

Gradle source-root discovery dropped every srcDirs declaration, because the SOURCE_DIRECTORY alternation tried the singular srcDir first and srcDir is a prefix of srcDirs. The capture then began at the leftover "s = [", which resolveGradlePath cannot resolve, so the plural branch was unreachable and a project declaring its main sources that way looked like a project with no production sources at all. Without a policy the supervised package is scanned rather than pinned, so it then fell through in one step to the configured default, "de.tum.cit.ase" for programming exercises. For an exercise in de.tum.cit.aet that resolved the analysis path to a directory which does not exist; ClassFileImporter answers an empty set for a missing directory, so ArchUnit analysed nothing and the aspects were armed over a package holding no class. Supervised code could then read files, execute commands and start threads unhindered while the run stayed green, under a configuration the setup manual documents as denying all three. Order the alternation longest-first, guard the singular branch with a negative lookahead, and accept += as well as =. Then read the compiled production output as a second detection step, so an unparsed descriptor no longer costs the supervised scope: the build tool writes its output where it says it does, whatever the descriptor looks like. Count only top-level classes, and skip blank and reserved packages as the source scan already does. The configured default remains as the last resort, and now says so in a warning naming the roots that were searched. It stays because Ares's own policy-free integration tests depend on that coarse prefix supervising their test-user classes; rescoping those fixtures changes what they assert and belongs in its own change.

coderabbitai · 2026-08-04T08:44:56Z

Important

🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8b66e6f6-2fa0-438e-89ef-554d29783042

📝 Walkthrough

Summary by CodeRabbit

Bug Fixes
- Improved project package detection using compiled production classes when source scanning is inconclusive.
- Added reliable fallback behaviour and validation for supervised project scope.
- Improved Gradle source-directory discovery, including multiple production roots and nested configurations.
- Improved legacy JUnit detection and handling of locally declared types.
- Corrected timeout handling so only genuine timeout-worker activity is exempted.
- Updated the TUM-specific default package configuration.
Documentation
- Updated project-scanning guidance and documented package-detection limitations.
Tests
- Added broader coverage for package detection, source discovery, timeout handling and edge cases.

Walkthrough

The project scanner now derives supervised packages from source or compiled production classes, validates derived scope coverage, and uses configured defaults only as a final fallback. Gradle source discovery handles nested descriptors and assignment forms. Timeout advice identifies only TimeoutUtils.executeWithTimeout workers.

Changes

Project scanning and derived scope enforcement

Layer / File(s)	Summary
Gradle source-root discovery `src/main/java/de/tum/cit/ase/ares/api/util/ProjectSourcesFinder.java`, `src/test/java/de/tum/cit/ase/ares/api/util/ProjectSourcesFinderTest.java`, `src/test/java/de/tum/cit/ase/ares/api/util/ProjectSourcesFinderEdgeCaseTest.java`	The Gradle parser masks comments and strings, tracks source-set context, resolves properties and paths, and applies additive or replacement `srcDir` and `srcDirs` declarations.
Compiled package fallback and scope validation `src/main/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScanner.java`, `src/main/resources/de/tum/cit/ase/ares/api/localization/*`, `src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerPackageFallbackTest.java`, `src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerScopeCoverageTest.java`	The scanner counts eligible top-level compiled classes, derives packages from production output, validates scope boundaries, and reports invalid output or uncovered classes.
Scanner defaults and source analysis `src/main/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProgrammingExerciseProjectScanner.java`, `src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProgrammingExerciseProjectScannerTest.java`, `src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerAstTest.java`, `docs/securitytest/TestCaseFactoryAndBuilderManual.md`	The TUM-specific default changes to `de.tum.cit.aet`. Documentation describes JavaParser and compiled-output scanning. AST tests cover local `TestCase` shadowing and wildcard JUnit 3 imports.
Derived scope execution wiring `src/main/java/de/tum/cit/ase/ares/api/securitytest/TestCaseAbstractFactoryAndBuilder.java`, `src/main/java/de/tum/cit/ase/ares/api/securitytest/java/JavaTestCaseFactoryAndBuilder.java`	The builders record whether scope derivation was used and validate project coverage before Java test execution.

Timeout-thread attribution

Layer / File(s)	Summary
Timeout worker classification `src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJThreadSystemAdviceDefinitions.aj`, `src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolbox.java`	Both implementations exempt only `TimeoutUtils.executeWithTimeout` frames and skip Ares infrastructure frames before classifying thread operations.
Timeout classification regression tests `src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJThreadSystemAdviceDefinitionsTest.java`, `src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolboxTest.java`	Tests execute the classifiers through strict-timeout machinery and verify that student operations are not treated as framework-owned.
Restrictive policy integration fixture `src/test/java/de/tum/cit/ase/ares/integration/testuser/PackageAccessUser.java`, `src/test/java/de/tum/cit/ase/ares/integration/testuser/SecurityUser.java`, `src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicySelfTestDefaultRestrictive.yaml`	Integration test users select an explicit restrictive policy. The policy denies filesystem, network, command, thread, and package-import access and sets a 3000 ms timeout.

Estimated code review effort: 4 (Complex) | ~60 minutes

Mergeability Score: 🟡 Moderate · up to 18ce3

The PR improves supervised-package detection and enforcement, but valid Gradle source declarations can still fail during discovery, and the manual currently overstates the guarantees of derived-scope validation. These bounded issues can cause affected builds to fail or lead users to rely on inaccurate security documentation, so they should be addressed before merging.

Possibly related issues

ls1intum/Ares2#189: The compiled-output scope validation and fail-closed enforcement directly implement this issue’s objectives.
ls1intum/Ares2#190: The Gradle source-root parser addresses the source-discovery gaps described by this issue.

Possibly related PRs

ls1intum/Ares2#86: Both changes modify JavaProjectScanner source discovery and test scanning.
ls1intum/Ares2#144: Both changes modify JavaProjectScanner behaviour and its tests.
ls1intum/Ares2#165: Both changes enforce reserved-package and scope-boundary validation.

Suggested labels: security fix

Suggested reviewers: krusche, jerrycai0006

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

Ignore

❌ Failed checks (1 error, 1 warning)

Check name	Status	Explanation	Resolution
Sandbox Fail-Closed Behaviour	❌ Error	New scope validation returns when production output is absent or non-directory, then executeTestCases proceeds; the derived package can therefore arm enforcement with no verified classes.	Fail closed when the output root is absent, non-directory, or yields no classes while source or scope evidence exists; reject before invoking the executor.
Docstring Coverage	⚠️ Warning	Docstring coverage is 38.03% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (6 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Trusted Boundary Preservation	✅ Passed	The diff preserves the boundary: reserved packages remain guarded, derived scopes reject uncovered compiled classes, policy test fixtures are explicitly exempted, and timeout logic exempts only Are...
Github Workflow Least Privilege	✅ Passed	The pull-request diff from main changes no GitHub workflow or action files, so this workflow least-privilege check is not applicable.
Title check	✅ Passed	The title clearly describes the main supervised-package detection change and its security impact.
Description check	✅ Passed	The description is directly related to the changes and explains the defect, security impact, implementation, testing, and migration effects.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/detect-supervised-package-without-policy

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

@nonnull

Discovery and the package scan decide which code Ares supervises, so a path that no test takes is a way for the scope to be wrong with nothing to notice it. The gaps were concentrated exactly there: rejections, fallbacks and the legacy descriptor accessors. Adds ProjectSourcesFinderEdgeCaseTest for the paths that do not resolve: a project root that is not a directory, a selected build tool whose descriptor is absent, a project with no descriptor at all, a source root that is a file, a malformed Maven descriptor, an unreadable Gradle descriptor and unreadable Gradle properties, the conventional-root fallbacks, the Kotlin getByName source set, line-comment stripping, files(...) unwrapping, both quotation styles, a mismatched pair of quotation marks, relative and absolute Maven source directories, and the legacy accessors including both base-directory prefixes and the Kotlin descriptor. Extends the scanner tests with the compiled-output cases that the new fallback introduces: classes in the default package and in a reserved package are skipped, an absent or unreadable output root yields nothing to detect, and the configured build mode is reported. Adds two JUnit 3 resolution fixtures: a bare TestCase reached through a wildcard import, and a self-declared TestCase in a second file of the same package, which resolves through the package rather than through the compilation unit. Also drops a null check on productionOutputRoot(), which is @nonnull, so the condition could never hold.

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@src/test/java/de/tum/cit/ase/ares/api/util/ProjectSourcesFinderEdgeCaseTest.java`:
- Around line 423-426: Require permission-based tests to distinguish fixture
failures from sandbox limitations. In
src/test/java/de/tum/cit/ase/ares/api/util/ProjectSourcesFinderEdgeCaseTest.java:423-426,
add an assumeUnreadable(Path) helper beside assumePosix() and invoke it after
each permission removal in rejectsUnreadableGradleDescriptor,
rejectsUnreadableGradleProperties, and reportsNoLegacyGradleSourceDirectory. In
src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerPackageFallbackTest.java:161-182
and :193-209, after changing permissions, abort when
Files.isReadable(outputRoot) or Files.isReadable(sourceRoot) remains true,
respectively.
- Around line 44-54: Protect mutations of the static descriptor paths in
ProjectSourcesFinderEdgeCaseTest by adding a shared `@ResourceLock` key to the
test class; apply the same lock to ProjectSourcesFinderTest and integration
tests that configure these paths through AresConfiguration or ClassNameScanner,
covering both rememberConfiguredPaths and restoreConfiguredPaths setup/teardown.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 70a933f0-7c26-4b94-a573-3ee1960abdbc

📥 Commits

Reviewing files that changed from the base of the PR and between daed7fe and 7e5e4bf.

📒 Files selected for processing (4)

src/main/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScanner.java
src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerAstTest.java
src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerPackageFallbackTest.java
src/test/java/de/tum/cit/ase/ares/api/util/ProjectSourcesFinderEdgeCaseTest.java

📜 Review details

⏰ Context from checks skipped due to timeout. (4)

GitHub Check: Build
GitHub Check: Run the gradle exercise
GitHub Check: Run the maven exercise
GitHub Check: Analyse Java

🧰 Additional context used

📓 Path-based instructions (5)

**/*Test.java

📄 CodeRabbit inference engine (AGENTS.md)

**/*Test.java: A sandboxed test JVM must never spin up its own server (echo server, socket listener, etc.) to test incoming or outgoing connections
Outgoing-connection tests must connect to an external echo server at a configurable endpoint running as a separate process or CI service on the loopback at port 25565, exercising only the student's client behaviour
If the external echo server is not reachable, the test must skip (using JUnit Assumptions.abort) rather than fail
An Ares SecurityException on an explicitly allowed connection is always a real failure and must propagate (never skipped)
Do not hard-code a self-hosted listener as the connection counterpart; use an external echo service to avoid in-JVM BindException/thread/lifecycle flakiness

Files:

src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerAstTest.java
src/test/java/de/tum/cit/ase/ares/api/util/ProjectSourcesFinderEdgeCaseTest.java
src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerPackageFallbackTest.java

**/*

⚙️ CodeRabbit configuration file

Dogmatically check all reviewed files for current British English in prose, comments, JavaDoc, documentation, workflow names, step names, issue/PR text, labels, user-facing messages, and review suggestions. Flag American spellings and grammar such as behavior, color, initialize, authorization, canceled, and program when they are natural-language text. Do not flag programming-language syntax, dependency coordinates, API names, class names, method names, package names, paths, URLs, quoted external identifiers, or other literals where American English is required by the technology.

Files:

src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerAstTest.java
src/test/java/de/tum/cit/ase/ares/api/util/ProjectSourcesFinderEdgeCaseTest.java
src/main/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScanner.java
src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerPackageFallbackTest.java

src/test/java/**/*.java

⚙️ CodeRabbit configuration file

Require tests to distinguish fixture failures from sandbox failures. Network tests must not start in-process listeners inside the sandbox; external fixtures may be skipped when absent, but explicit Ares SecurityException failures must propagate.

Files:

src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerAstTest.java
src/test/java/de/tum/cit/ase/ares/api/util/ProjectSourcesFinderEdgeCaseTest.java
src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerPackageFallbackTest.java

src/main/java/**/*.java

⚙️ CodeRabbit configuration file

Review as a Java 17 Maven security framework used to test untrusted student code in Artemis programming exercises. Prioritise sandbox escapes, fail-open behaviour, unsafe reflection, classloader/bootstrap boundary mistakes, global mutable state, concurrency races, insufficient canonicalisation, and changes that weaken file, command, thread, network, package, or class access restrictions. Treat unrecognised security-sensitive inputs as a potential fail-closed requirement. Prefer simple Java code and one field or method declaration per line.

Files:

src/main/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScanner.java

src/main/java/de/tum/cit/ase/ares/api/securitytest/**/*.java

⚙️ CodeRabbit configuration file

Check that security-test factory, scanner, creator, and execution code cannot be influenced by student-controlled classes to bypass trusted-package, supervised-code, or test-class boundaries.

Files:

src/main/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScanner.java

🔇 Additional comments (5)

src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerAstTest.java (1)

446-466: LGTM!

src/test/java/de/tum/cit/ase/ares/api/util/ProjectSourcesFinderEdgeCaseTest.java (1)

58-66: LGTM!

Also applies to: 145-320

src/main/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScanner.java (2)

432-481: LGTM!

Also applies to: 515-530

493-498: 🩺 Stability & Availability

No change needed. BuildToolConfiguration rejects null for productionOutputRoot before storing it, so Files.isDirectory(outputRoot) does not dereference a null path.

src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerPackageFallbackTest.java (1)

136-159: LGTM!

Also applies to: 184-191

The permission-based tests assumed POSIX support but never that the bits are honoured. A superuser ignores them, so the code under test would keep reading the file and the assertion would report a defect where the fixture is what could not be established. Each site now checks that the target really is unreadable after the bits are cleared, and aborts otherwise, so such a run skips instead of failing.

getDefaultPackage() guesses the package of the supervised project when the project declares none. That guess is now de.tum.cit.aet. Only this one literal moves. Every other de.tum.cit.ase in src/main is Ares' own namespace, not a guess about someone else's code: FileTools resolves its own source directory through it, AOPMode and ArchitectureMode use it to identify literals inside copied Ares sources, and the essential-data YAMLs list Ares' own trusted packages. Renaming those would make Ares fail to find itself. Keep the fallback test meaningful. It used de.tum.cit.aet as the detected package precisely to contrast it with the default, so with both sides equal the assertion would have held just as well had the fallback fired, and the test would have proved nothing. Its fixture now sits in de.tum.cit.detected and it asserts against the new default explicitly. The neighbouring cases drive the base scanner, whose default is the empty string, so they stay as they are. Reword the two thread-advice comments that called de.tum.cit.ase the self-test fallback. It never was one; it is the broad scope Ares' own self-tests configure, and after this change the old wording would send a reader looking for a fallback that no longer has that value.

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerPackageFallbackTest.java (1)
247-248: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Make the unavailable Java compiler a skipped fixture failure.

ToolProvider.getSystemJavaCompiler() can return null when the test JVM lacks jdk.compiler; line 248 then throws NullPointerException instead of a test-scope fixturing failure. Confirm Maven surefire always uses a full JDK compiler, otherwise abort this test class with assumeTrue(compiler != null, "Java compiler required for fixture").
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerPackageFallbackTest.java`
around lines 247 - 248, Update the fixture setup in
JavaProjectScannerPackageFallbackTest around
ToolProvider.getSystemJavaCompiler() to handle a null compiler before invoking
compiler.run. Abort the test class with an assumption such as assumeTrue when
the compiler is unavailable, preserving normal compilation behavior when it
exists.
Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/securitytest/TestCaseFactoryAndBuilderManual.md`:
- Line 535: Update the documentation for scanForPackageName() to describe its
fallback order: production source roots, compiled production output, then the
default package. Explicitly document that the final default package may enforce
no security checks when the project does not contain that package, using British
English and avoiding overstated guarantees.

---

Outside diff comments:
In
`@src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerPackageFallbackTest.java`:
- Around line 247-248: Update the fixture setup in
JavaProjectScannerPackageFallbackTest around
ToolProvider.getSystemJavaCompiler() to handle a null compiler before invoking
compiler.run. Abort the test class with an assumption such as assumeTrue when
the compiler is unavailable, preserving normal compilation behavior when it
exists.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9b128e3d-7be2-411e-8d67-840d443d26eb

📥 Commits

Reviewing files that changed from the base of the PR and between 9732c4f and 626e685.

📒 Files selected for processing (6)

docs/securitytest/TestCaseFactoryAndBuilderManual.md
src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJThreadSystemAdviceDefinitions.aj
src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolbox.java
src/main/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProgrammingExerciseProjectScanner.java
src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProgrammingExerciseProjectScannerTest.java
src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerPackageFallbackTest.java

📜 Review details

⏰ Context from checks skipped due to timeout. (4)

GitHub Check: Analyse Java
GitHub Check: Run the maven exercise
GitHub Check: Run the gradle exercise
GitHub Check: Build

🧰 Additional context used

📓 Path-based instructions (7)

**/*

⚙️ CodeRabbit configuration file

Dogmatically check all reviewed files for current British English in prose, comments, JavaDoc, documentation, workflow names, step names, issue/PR text, labels, user-facing messages, and review suggestions. Flag American spellings and grammar such as behavior, color, initialize, authorization, canceled, and program when they are natural-language text. Do not flag programming-language syntax, dependency coordinates, API names, class names, method names, package names, paths, URLs, quoted external identifiers, or other literals where American English is required by the technology.

Files:

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolbox.java
docs/securitytest/TestCaseFactoryAndBuilderManual.md
src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJThreadSystemAdviceDefinitions.aj
src/main/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProgrammingExerciseProjectScanner.java
src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProgrammingExerciseProjectScannerTest.java
src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerPackageFallbackTest.java

src/main/java/**/*.java

⚙️ CodeRabbit configuration file

Review as a Java 17 Maven security framework used to test untrusted student code in Artemis programming exercises. Prioritise sandbox escapes, fail-open behaviour, unsafe reflection, classloader/bootstrap boundary mistakes, global mutable state, concurrency races, insufficient canonicalisation, and changes that weaken file, command, thread, network, package, or class access restrictions. Treat unrecognised security-sensitive inputs as a potential fail-closed requirement. Prefer simple Java code and one field or method declaration per line.

Files:

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolbox.java
src/main/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProgrammingExerciseProjectScanner.java

src/main/java/de/tum/cit/ase/ares/api/aop/**/*.java

⚙️ CodeRabbit configuration file

Focus on runtime enforcement integrity: intercepted methods, argument extraction, null handling, recursive advice guards, bootstrap classloader interaction, and whether denied operations can reach the JVM or operating system before checks run.

Files:

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolbox.java

docs/**/*.md

⚙️ CodeRabbit configuration file

Use current British English. Check that security guarantees, known limitations, and required external fixtures are explicit and do not overstate enforcement.

Files:

docs/securitytest/TestCaseFactoryAndBuilderManual.md

src/main/java/de/tum/cit/ase/ares/api/securitytest/**/*.java

⚙️ CodeRabbit configuration file

Check that security-test factory, scanner, creator, and execution code cannot be influenced by student-controlled classes to bypass trusted-package, supervised-code, or test-class boundaries.

Files:

src/main/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProgrammingExerciseProjectScanner.java

**/*Test.java

📄 CodeRabbit inference engine (AGENTS.md)

**/*Test.java: A sandboxed test JVM must never spin up its own server (echo server, socket listener, etc.) to test incoming or outgoing connections
Outgoing-connection tests must connect to an external echo server at a configurable endpoint running as a separate process or CI service on the loopback at port 25565, exercising only the student's client behaviour
If the external echo server is not reachable, the test must skip (using JUnit Assumptions.abort) rather than fail
An Ares SecurityException on an explicitly allowed connection is always a real failure and must propagate (never skipped)
Do not hard-code a self-hosted listener as the connection counterpart; use an external echo service to avoid in-JVM BindException/thread/lifecycle flakiness

Files:

src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProgrammingExerciseProjectScannerTest.java
src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerPackageFallbackTest.java

src/test/java/**/*.java

⚙️ CodeRabbit configuration file

Require tests to distinguish fixture failures from sandbox failures. Network tests must not start in-process listeners inside the sandbox; external fixtures may be skipped when absent, but explicit Ares SecurityException failures must propagate.

Files:

src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProgrammingExerciseProjectScannerTest.java
src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerPackageFallbackTest.java

🔇 Additional comments (7)

src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJThreadSystemAdviceDefinitions.aj (1)

151-153: LGTM!

src/main/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProgrammingExerciseProjectScanner.java (1)

24-29: LGTM!

Also applies to: 38-38

src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProgrammingExerciseProjectScannerTest.java (1)

77-77: LGTM!

Also applies to: 269-269

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolbox.java (1)

147-149: LGTM!

src/test/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScannerPackageFallbackTest.java (3)

33-98: LGTM!

100-235: LGTM!

237-246: LGTM!

Also applies to: 249-252

Renaming the TUM default package broke SecurityTest.test_useCommonPoolGood, and the test was right. The self-tests declared @Policy without a policy file, so their supervised package came from the scanner's TUM default. That default happened to be a textual prefix of Ares' own packages, so the test subjects counted as student code by coincidence rather than by declaration. Once the default no longer matched, the classifier found no student frame, walked past the subject and reached TimeoutUtils, concluded the operation belonged to Ares' own timeout machinery and exempted it. Enforcement switched itself off and nothing failed but that one assertion. Pin the supervised package for the two self-test users whose assertions depend on their subject being supervised. The policy keeps the architecture and enforcement modes the no-policy branch selects, ArchUnit and AspectJ, so the backend under test does not change; it deliberately narrows the exempt set to the declared test classes and states where it departs from what the no-policy path derived. Key the timeout exemption to the frame that creates the worker. The student's body runs inside TimeoutUtils.rethrowThrowableSafe on that same worker, so TimeoutUtils is on the student's stack too, below their own frames. Accepting any frame of that class made the exemption depend on the restricted package being scoped correctly, which is precisely the assumption that failed. Only executeWithTimeout, which actually submits the invocation, now grants it. Verified against dumped StackWalker frames from both situations rather than inferred; the Javadoc claiming the student's stack contains no TimeoutUtils was wrong and is corrected. Cover both backends. The two advices carry this logic separately and are kept in step by hand, and the self-tests exercise only the restricted-package branch, so each backend gets a regression test that fails when the exemption is keyed to the class again.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicySelfTestDefaultRestrictive.yaml`:
- Around line 31-35: Update the empty YAML sequences for
regardingFileSystemInteractions, regardingNetworkConnections,
regardingCommandExecutions, regardingThreadCreations, and
regardingPackageImports to use [] without an internal space.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 2ee82a11-a50c-4412-a5c9-5a8fb2d1417e

📥 Commits

Reviewing files that changed from the base of the PR and between 626e685 and 6321ab7.

📒 Files selected for processing (7)

src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJThreadSystemAdviceDefinitions.aj
src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolbox.java
src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJThreadSystemAdviceDefinitionsTest.java
src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolboxTest.java
src/test/java/de/tum/cit/ase/ares/integration/testuser/PackageAccessUser.java
src/test/java/de/tum/cit/ase/ares/integration/testuser/SecurityUser.java
src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicySelfTestDefaultRestrictive.yaml

📜 Review details

⏰ Context from checks skipped due to timeout. (7)

GitHub Check: Unit Tests
GitHub Check: Core Integration Tests
GitHub Check: Integration Tests (WALA + AspectJ)
GitHub Check: Integration Tests (ArchUnit + instrumentation)
GitHub Check: Integration Tests (WALA + instrumentation)
GitHub Check: Run the gradle exercise
GitHub Check: Analyse Java

🧰 Additional context used

📓 Path-based instructions (5)

**/*Test.java

📄 CodeRabbit inference engine (AGENTS.md)

**/*Test.java: A sandboxed test JVM must never spin up its own server (echo server, socket listener, etc.) to test incoming or outgoing connections
Outgoing-connection tests must connect to an external echo server at a configurable endpoint running as a separate process or CI service on the loopback at port 25565, exercising only the student's client behaviour
If the external echo server is not reachable, the test must skip (using JUnit Assumptions.abort) rather than fail
An Ares SecurityException on an explicitly allowed connection is always a real failure and must propagate (never skipped)
Do not hard-code a self-hosted listener as the connection counterpart; use an external echo service to avoid in-JVM BindException/thread/lifecycle flakiness

Files:

src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJThreadSystemAdviceDefinitionsTest.java
src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolboxTest.java

**/*

⚙️ CodeRabbit configuration file

Dogmatically check all reviewed files for current British English in prose, comments, JavaDoc, documentation, workflow names, step names, issue/PR text, labels, user-facing messages, and review suggestions. Flag American spellings and grammar such as behavior, color, initialize, authorization, canceled, and program when they are natural-language text. Do not flag programming-language syntax, dependency coordinates, API names, class names, method names, package names, paths, URLs, quoted external identifiers, or other literals where American English is required by the technology.

Files:

src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJThreadSystemAdviceDefinitionsTest.java
src/test/java/de/tum/cit/ase/ares/integration/testuser/SecurityUser.java
src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolbox.java
src/test/java/de/tum/cit/ase/ares/integration/testuser/PackageAccessUser.java
src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolboxTest.java
src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJThreadSystemAdviceDefinitions.aj
src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicySelfTestDefaultRestrictive.yaml

src/test/java/**/*.java

⚙️ CodeRabbit configuration file

Require tests to distinguish fixture failures from sandbox failures. Network tests must not start in-process listeners inside the sandbox; external fixtures may be skipped when absent, but explicit Ares SecurityException failures must propagate.

Files:

src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJThreadSystemAdviceDefinitionsTest.java
src/test/java/de/tum/cit/ase/ares/integration/testuser/SecurityUser.java
src/test/java/de/tum/cit/ase/ares/integration/testuser/PackageAccessUser.java
src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolboxTest.java

src/main/java/**/*.java

⚙️ CodeRabbit configuration file

Review as a Java 17 Maven security framework used to test untrusted student code in Artemis programming exercises. Prioritise sandbox escapes, fail-open behaviour, unsafe reflection, classloader/bootstrap boundary mistakes, global mutable state, concurrency races, insufficient canonicalisation, and changes that weaken file, command, thread, network, package, or class access restrictions. Treat unrecognised security-sensitive inputs as a potential fail-closed requirement. Prefer simple Java code and one field or method declaration per line.

Files:

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolbox.java

src/main/java/de/tum/cit/ase/ares/api/aop/**/*.java

⚙️ CodeRabbit configuration file

Focus on runtime enforcement integrity: intercepted methods, argument extraction, null handling, recursive advice guards, bootstrap classloader interaction, and whether denied operations can reach the JVM or operating system before checks run.

Files:

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolbox.java

🪛 YAMLlint (1.37.1)

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicySelfTestDefaultRestrictive.yaml

[error] 31-31: too many spaces inside empty brackets

(brackets)

[error] 32-32: too many spaces inside empty brackets

(brackets)

[error] 33-33: too many spaces inside empty brackets

(brackets)

[error] 34-34: too many spaces inside empty brackets

(brackets)

[error] 35-35: too many spaces inside empty brackets

(brackets)

🔇 Additional comments (7)

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolbox.java (1)

125-138: LGTM!

Also applies to: 148-161

src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJThreadSystemAdviceDefinitions.aj (1)

134-166: LGTM!

src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJThreadSystemAdviceDefinitionsTest.java (1)

1-61: LGTM!

src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceThreadSystemToolboxTest.java (1)

8-22: LGTM!

Also applies to: 172-205

src/test/java/de/tum/cit/ase/ares/integration/testuser/PackageAccessUser.java (1)

19-28: LGTM!

src/test/java/de/tum/cit/ase/ares/integration/testuser/SecurityUser.java (1)

28-33: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicySelfTestDefaultRestrictive.yaml (1)

1-30: LGTM!

Also applies to: 36-37

The manual still described scanForPackageName as reading the sources alone, so it neither mentioned the compiled-output step this branch adds nor said what the last resort actually promises. Spell out all three steps and state plainly that the default enforces nothing when the project does not contain it: the analysis path then resolves to a directory that does not exist, no class is imported, and the log warning is the only signal. Write the new policy's empty sequences as [], the form most of the existing policies already use.

Claudia-Anthropica

@MarkusPaulsen The exact head and all retained discussion were reviewed, and the captured CI checks are green. However, policy-free package discovery still permits a student-controlled or incomplete package scan to exclude the actual assignment package from enforcement, creating a high-severity false negative.

The Javadoc opened by claiming the method derives the package "never from a default that the project does not contain" and then returned exactly such a default fifty lines later. The manual went further and presented the reserved prefix filter as protection against a student flooding the project to choose the enforcement scope. It is not: it stops only a reserved namespace from winning, and every other namespace is available. Enough classes under a package of the submitter's choosing make that package the derived scope, and the five resource domains are then enforced over it instead of over the assignment. Counting compiled classes in step two changes the unit that is counted, not who controls it. Two further overclaims go with it. The compiled output is not authoritative "whatever the build descriptor says": step two reads the build tool's conventional directory, so a build that writes its output elsewhere is not followed there. And the warning said no compiled class declared a package, where what happened is that none declared an eligible one, the default package and reserved packages having been filtered out first. None of this changes behaviour. It stops the code and the manual from promising a boundary where there is a heuristic, and it names the one form of the scope that cannot be steered from the submission: pinning it in the policy. The one behavioural fix is in the same area. compiledPackageCounts discarded every class whose binary name contains a dollar sign, on the reasoning that the dollar marks a nested or anonymous class. It is also a legal identifier character, so a top-level Payload$Hidden was dropped and its package undercounted by however many classes an author chose to name that way. Nesting now comes from the class file through isTopLevelClass, with a fixture that fails against the old test and passes against this one.

Claudia-Anthropica

@MarkusPaulsen The exact head and retained discussions were re-reviewed; captured CI is green. Policy-free discovery still permits a student-controlled or incomplete package plurality to exclude the actual assignment package from enforcement, and the latest reply confirms this defect is deferred rather than fixed here. The widened srcDirs parser also still interprets inactive text as configuration and can misclassify a one-line test source set as production, as documented in the unresolved current-head threads.

Making srcDirs resolve for the first time exposed what the line-oriented scan around it could not tell apart, and one of those cases turned a build that worked into one that aborts. An occurrence of srcDirs that Gradle never executes was read as a declaration. Only // comments were stripped, so a commented-out srcDirs = ['old/path'] in a block comment, or one inside a string as in println "srcDirs = ...", became a candidate path, and a declared root that is not a directory is rejected. On main the same file was harmless, because the srcDir alternative consumed the prefix and the leftover token resolved to nothing. The source set was tracked per line. A declaration sharing its line with the block that opens it, as in sourceSets { test { java { srcDirs = ['t'] } } }, was attributed to whatever the previous line left behind, so a test root counted as production and the test list came back empty. The set was also decided by asking whether the line contained the text "test" anywhere, which made main { java { srcDir 'contest' } } a test root. And nothing required the declaration to be inside a java block, so resources { srcDir 'assets' } became a Java source root, which matters because JavaProjectScanner reads every .java file under a production root without asking Gradle whether it compiles them. All three are older than this branch, but they could not fire for a project declaring its sources with srcDirs, which is the form the Artemis template uses, so this branch is what puts them in reach. The descriptor is now masked, so comments and the contents of string literals cannot be mistaken for code, and then walked once with a stack of the enclosing blocks. Masking preserves every length, so an offset in the mask is the same offset in the original: the mask decides what is code, the original supplies the values. Property assignments are read from it too, so one inside a comment no longer defines a property. A declaration counts only inside sourceSets, then main or test, then java. Gradle's own semantics now decide what the declarations add up to. A source set starts at its conventional root, srcDir, srcDirs(...) and srcDirs += add to it, and srcDirs = replaces what came before. srcDirs += ['generated'] therefore means the conventional root and the generated one rather than the generated one alone, and two assignments are the last one rather than both. One existing expectation changes with that. An assignment whose value cannot be resolved now leaves the source set empty instead of answering the conventional root: the descriptor has replaced that root, so naming it is a guess against what the build says, and the supervised package comes from the compiled output instead.

Claudia-Anthropica

@MarkusPaulsen The exact head and all retained discussion were reviewed. The acknowledged high-severity policy-free scope defect remains: student-controlled or incomplete package counts can exclude the actual assignment package from enforcement, leaving forbidden operations unblocked; its fix is deferred to #189. The captured Run the gradle exercise check also fails on this head, with no repository evidence establishing that failure as unrelated.

Three findings, all in the code added by the previous commit and all of them fair. PMD's AvoidBranchingStatementAsLastInLoop fired twice, where a guard clause ended its loop body with a continue. Both read better as the positive condition: the source-set search returns on the name it was looking for, and the expression-position check returns on the first character that is not whitespace. SpotBugs pointed out that Path.getFileName can answer null, which it does for a root path, so asking a descriptor whether its name ends in .kts could have dereferenced it. Caught by the Examples workflow rather than locally, because mvn test does not run the gates that mvn install does.

Claudia-Anthropica

@MarkusPaulsen The exact head and all retained discussion were reviewed; the newest descriptor-structure changes address the two parser regressions raised in the prior threads. The acknowledged high-severity policy-free scope defect remains unchanged: scanForPackageName() still selects a student-influenceable plurality or an unverified default, so forbidden operations in the real assignment package can remain outside enforcement. Several captured checks are still in progress, although the snapshot identifies none as required.

MarkusPaulsen · 2026-08-13T07:12:26Z

@coderabbitai full review

Context for the re-review: the last CodeRabbit review on this pull request is from 2026-08-10 and predates three commits. All threads from it are resolved. Since then the branch gained the isTopLevelClass fix and the honest description of what the supervised-package derivation does not establish (7186ada), the structured Gradle descriptor scan replacing the line-oriented one (c7380ac), and the PMD and SpotBugs corrections that came out of it (4baf86e). All checks are green on the current head.

Claudia-Anthropica

@MarkusPaulsen The new head carries the derived scope into both generated modes, but the generated runtime inventory is still not trustworthy. It can inspect a stale target/classes instead of the actual Gradle output and filters attacker-controlled production packages before validation; generated package-import rules also still persist the heuristic scope instead of runtime-declared packages. These are high-severity policy-free false negatives, so the generated path remains unsafe.

Three ways the runtime inventory could still be wrong, all of them mine, all introduced with the inventory itself. The output root was picked by trying target/classes and then build/classes/java/main and taking whichever existed. Those look equivalent and are not: a Gradle project that once built with Maven still has a target/classes, and the rules would then have been checked against a stale tree that need not resemble what runs. The build descriptor says which tool owns the project, so it decides, and a project carrying both descriptors is refused rather than guessed at. The import filtered the framework's own paths and, for a pinned scope, the scope subtree, before the coverage check ran. Filtering first decides what is worth checking using package names, which are exactly what whoever adds files to the project controls: a class the filter removed never reached the check and would have run unsupervised while the suite passed. Nothing is filtered now. What is imported is what is checked, and after a successful check everything imported lies within the scope anyway. The generated package-import rules still carried the scope itself as a permission. A permission is matched as a prefix, so that grant covered the whole namespace below the scope and outlived the coverage check that ran beside it. A derived scope now asks at runtime for the packages the compiled output actually declares, which cannot be broader than the output, and the creator no longer writes the heuristic fall-back into the file at all. A pinned scope keeps what the instructor declared, because that is a statement rather than a reading. The three assertions added with them are about what the generated file does, not how it is formatted: that it does not import nothing, that a derived scope asks for both its classes and its permissions at runtime, and that a pinned one is left as written.

Claudia-Anthropica

@MarkusPaulsen The runtime inventory closes the previously reported generated self-import path, so that thread can be resolved. However, the helper still accepts and grants reserved production packages, and its pinned branch analyses classes outside the instructor-selected scope; these cause a high-severity false negative and a medium-severity false positive. The snapshot has no failed checks, while Unit Tests and both WALA integration jobs were still in progress and were not listed as required.

Importing the output whole was the right answer to a filter that decided what was worth checking using package names the submitter controls. Removing every filter was not: it also removed the reserved-namespace check and the pinned scope's own boundary, so the helper accepted a class named into a trusted namespace and then granted that namespace as an import permission, and a pinned policy analysed the whole output instead of the part the instructor selected. Both are back, and both now sit after the import rather than before it, which is the distinction that was missed the first time. A reserved class is refused during validation and is never turned into a permission whatever else went wrong. A pinned scope narrows the imported inventory to its own subtree, so the rules see what the instructor selected while the check above still sees everything. Ares' own prefix is written in two concatenated pieces. Copying this class into a supervised project rewrites the framework's package token wherever it appears, string literals included, so spelling it out would have turned that entry into the copied API's prefix and stopped it naming Ares at all. The copied API is recognised separately, from where this class itself ended up rather than from a literal that would meet the same fate.

Claudia-Anthropica

@MarkusPaulsen The current head fixes the reserved-declaration and pinned-scope findings, and generated ArchUnit/WALA cases now validate compiled output at runtime. However, runtime-derived self-import permissions still accept a production package that is an ancestor of a trusted namespace, recreating a high-severity package-import bypass. The snapshot records no failed checks; CodeQL, Build, and both example jobs were still in progress but were not listed as required.

The Javadoc on isThreadCreationFromAresTimeout described the stack walk as prose about one frame reached before another, which left three of the four classification rules unnamed and never said what the returned value goes on to do. It now states the question the method answers, lists the four rules in the order they are applied, and records that the order of the first two is load-bearing: TimeoutUtils lies inside the Ares prefix itself, so testing the prefix first would skip the very frame the exemption depends on. Two claims are corrected rather than restated. Refusing every TimeoutUtils method other than executeWithTimeout was justified by saying those frames run the student's body, which holds for rethrowThrowableSafe and for no other: findTimeout, terminateTimedOutExecution and generateTimeoutFailure are Ares-internal and create no worker. And the exemption's absence was said to fail every @StrictTimeout test under a thread-forbidding policy, which overstates it, since a refusal only hands the operation to the ordinary check, and that check finds no restricted-package frame on the creating stack. Documentation only. No behaviour changes.

Claudia-Anthropica

@MarkusPaulsen The latest commit only revises the timeout-exemption Javadocs, so the existing generated-package finding remains. allowedPackages() still accepts a runtime-derived de.tum.cit permission because line 141 checks only whether the package lies inside a trusted prefix; validated() accepts that broad scope, and the resulting prefix grant covers de.tum.cit.ase.ares.api in both generated modes. Apply the ancestor-of-reserved and copied-API checks before adding each runtime-derived permission. No failed checks were captured; CodeQL, Build, and both example jobs were still running but are not listed as required.

The scope was assigned after construction, through a setter three builders and one direct construction site had to remember to call. Two of them did not: one was caught in review, and JavaArchitectureTestCase's own writeArchitectureTestCase was not. A test case carrying no scope refuses to write a file, so forgetting the second step produced an object that looks built and fails later, at the one moment where failing means no enforcement was generated. Both values now travel through every constructor of the three classes, the two fields are final, the setter is gone, and all three build() methods return their instance directly. What a reader had to notice is now what the compiler asks for. Fixing the forgotten branches surfaced an older defect underneath them. JavaArchunitTestCase.builder() and JavaWalaTestCase.builder() declare no such method: Java inherits statics by name, so both resolved to the one on JavaArchitectureTestCase and returned a base-class builder. Each branch of writeArchitectureTestCase therefore built another base-class instance and delegated to itself until the stack ran out. Neither mode has ever worked from this entry point. They now use archunitBuilder() and walaBuilder(), and carry allowedClasses along with the scope. Two tests pin both halves. Nothing changes on the production path, which converts through ArchitectureMode and never reached these branches.

Claudia-Anthropica

@MarkusPaulsen The runtime helper still turns a broad derived package such as de.tum.cit into a prefix permission, so the unresolved trusted-namespace bypass remains on this head. The latest signature refactor also removes released public entry points, breaking existing consumers; I left that as a new medium finding. The snapshot has no failed checks, while CodeQL analysis, Build, and both example jobs were still in progress and are not listed as required.

Every other entry in the reserved-package list names either the Java platform or a library that performs the supervision: declaring code into one of them would have it mistaken for the machinery meant to inspect it. That is the whole membership rule, and anonymous.toolclasses. and metatest. never met it. They are the test helpers of one downstream consumer, the reproducibility package, so every Ares user refused two ordinary package names on that consumer's behalf, with a diagnostic about trusted namespaces that means nothing in their project. Removed from the list, from its hand-kept copy in JavaArchunitSupervisedClasses and from the three build-boundary fixtures. They stay in INFRA_PREFIXES, which answers a different question: not what supervised code may be called, but which frames are the harness rather than the subject. RESERVED_PACKAGE_PREFIX_VERSION moves to 2, which is what that number is for. A version 1 snippet still in an exercise reserves more than this version does, so it refuses what is now allowed rather than allowing what is now refused; it is over-strict rather than bypassable, and no migration is urgent. The identifier must still change, because one version must not denote two lists.

An exercise that hands the student a package to fill has no compiled production class until they write one. Refusing there replaced their own failing tests with an Ares security error labelled Reason: Ares-Code, which is neither their fault nor Ares'. Nothing to supervise is not the same as a scope that supervises the wrong thing: where no class exists, none can escape a boundary, so enforcement is vacuous rather than mis-scoped and a warning is the honest response. An unreadable output stays a refusal, because not knowing what is there is not the same as knowing there is nothing. The same split closes the opposite hole, which was measurable: the emptiness was checked against the whole output and before the pinned scope narrowed it, so a scope matching no compiled class while classes existed returned an empty set and every rule passed against nothing. That is the very failure this class was written to stop, and it survived on the pinned path, where neither requireScopeToCover nor requireDerivedScopeToCoverTheProject runs. Deliberate narrowing means less than everything, never nothing, so it is now refused by name and with a count. The comment cleanup for these two files rides along, because the edits sit in the same methods and cannot be separated into their own commit.

@throws

The comments added by this pull request had grown past what they explained. Every newly added line comment is gone from the sixteen changed main files, and the Javadoc that remains is roughly a hundred lines shorter without dropping a claim: where a line comment carried a reason worth keeping, the reason moved into the Javadoc of the method it applied to, which is where the next reader will look for it rather than mid-expression. Three defects fell out of the pass. JavaCreator carried two adjacent comment blocks in the same ternary saying almost the same thing, one of them already stale. JavaProjectScanner carried two orphaned Javadoc blocks describing methods that no longer exist, so each documented whichever method happened to follow it. And the @throws on validated() and pinned() still promised a SecurityException when nothing is compiled, which stopped being true. Six line-comment lines remain in the two advice classes. They are a rewrap of a comment that predates this pull request, kept because the wording it replaced described a fall-back this pull request removed.

@nonnull

Three places deviated from a convention their own neighbours already follow. None changes behaviour; each is a thing a reader has to check twice. The AspectJ and instrumentation backends carry the same timeout exemption in two files kept in step by hand, and isThreadCreationFromAresTimeout had drifted in four spots: three comment wraps and a missing @nullable. The wraps are resolved towards the .java file, because Spotless formats *.java and not *.aj, so the reverse would be undone by the next spotless:apply. The two methods, javadoc included, are now identical character for character, which is the only state in which a divergence between them is visible as a diff. The missing @nullable was not merely an asymmetry across the two backends. It was the exception inside its own file: isImmediateCallerWithinRestrictedPackage sits beside it with the same parameter, the same type and the same meaning, annotated. The body already reads restrictedPackage != null, so null is a value the method expects rather than one it merely survives. The Builder in JavaArchitectureTestCase was missing @nullable on supervisedPackage and @nonnull on both of its setters. Every other nullable field in that builder carries the first, all seven of its other setters carry the second, and both subclass builders already spell their copies of these two setters exactly that way. The stray blank lines between the two fields go with them, so the block reads like its neighbours. Verified with spotless:check, a full compile including the AspectJ weave, and the five test classes that touch either area: 29 tests, no failures.

Claudia-Anthropica

@MarkusPaulsen The two previously open findings remain on this exact head: generated allowedPackages() still turns de.tum.cit into a prefix grant, and the former public constructors plus the Creator.createTestCases descriptor remain removed. The latest reserved-prefix change also permits metatest and anonymous.toolclasses declarations while WALA still treats those application packages as infrastructure, enabling an architecture-enforcement false negative; I left that as a new high finding. The captured snapshot has no failed checks, though Unit Tests, Static Code Analysis, Core Integration Tests, and all four mode integration jobs were still in progress and are not listed as required.

Two findings Claudia raised, both still open on the previous head. The first is that allowedPackages asked only half the question. It skipped a package inside a trusted namespace and granted everything else, but a package that sits above one carries that namespace with it, because a permission is matched as a prefix. Nothing upstream catches it: validation refuses a package inside a reserved prefix, and de.tum.cit lies inside none, while a scope broad enough to contain it covers every compiled class by construction and therefore passes coverage as well. The generated rule would then permit the supervised code every import from the framework supervising it. The direct path already asked the second question through ReservedPackageGuard.ancestorOfReservedPrefix and the generated one did not, so the two disagreed about the same package. The complement is written out locally rather than called, for the reason the class header gives: this file is copied into the supervised project, where only what the copy manifests list exists, and ReservedPackageGuard is not on them. It also has to consider the copied API prefix, which the direct path has no notion of. Reaching it refuses the run and names the package, rather than silently granting a narrower list, because the alternative is a project whose own classes cannot call each other and whose diagnostic says something else. The second is that the scope refactor replaced eight public constructors rather than adding to them. A client compiled against 2.1.2 links against those descriptors, so it would fail with NoSuchMethodError, and a source client could not recompile. All eight are back as delegating bridges, deprecated for removal, and Creator.createTestCases likewise as a default method. The bridge cannot know whether the scope was derived or pinned, so it reports derived, which is the strict reading of the two: a derived scope is checked against the whole compiled output before enforcement, so a bridge can only ever verify more than the caller asked for. An implementation written against the old Creator signature still has to be updated, since no bridge can supply a parameter its author never wrote. Making the scope a constructor parameter was what stopped a test case existing without one, and the bridges do not reopen that. A test case built through one carries no scope and refuses to write a generated file, with the diagnostic that names the missing declaration, rather than writing a file whose rules pass against an empty set of classes. Both are covered. JavaArchunitSupervisedClassesTest drives the new complement directly, including the broad ancestor Claudia named, the segment boundary that keeps de.tum.citadel out of it, and the arbitration with reservedPrefixOf so that one package cannot be refused twice for two reasons. ReleasedConstructorCompatibilityTest calls all eight signatures and asserts what they produce, which nothing else in the repository does: the production path builds through the builders, so a future removal would pass every other test in the suite. Verified with spotless:check, test-compile including the AspectJ weave, and the ten test classes touching either area: 99 tests, no failures.

Claudia-Anthropica

@MarkusPaulsen The latest commit closes the generated ancestor-package grant and restores the eight constructor descriptors. Two current-head defects remain: WALA still classifies newly permitted metatest.* and anonymous.toolclasses.* application classes as infrastructure, allowing forbidden paths with no student frame, and Creator still makes the new boolean overload abstract, so released third-party implementations no longer compile despite the old call descriptor being present as a default. The captured snapshot has no failed checks; Unit Tests and both WALA integration jobs were still in progress, but none is listed as required.

Claudia-Anthropica

@MarkusPaulsen The generated ancestor-package permission is fixed, but two blockers remain. [medium] Creator still makes the boolean overload abstract while the released overload is a default that immediately invokes it; old implementations therefore still fail to compile and old binaries can reach AbstractMethodError. [high] metatest.* and anonymous.toolclasses.* are permitted by the build boundary but remain WALA infrastructure prefixes, so evaluateSink() can traverse supervised frames in those packages as infrastructure and accept a forbidden path without evaluating a student approach. All captured checks succeed, but they do not cover these compatibility and namespace-collision cases.

Claudia's finding on the previous head, and it is correct. Reserving only what Ares can be confused with left anonymous.toolclasses. and metatest. nameable by supervised code while INFRA_PREFIXES still skips them when looking for the frame to attribute a forbidden call to. A supervised class declaring package metatest therefore put every frame of its own path into the skipped set, evaluateSink walked the predecessors, never found a frame to evaluate, and returned: the forbidden call was found and then reported to nobody. The drop is in evaluateSink rather than in evaluatePath, which is where the first attempt at this put the guard and where the new test showed it never runs. evaluateApproach is only ever reached once a non-infra predecessor has been found, so evaluatePath's own empty-student-frame branch cannot be taken in production. What tells the two cases apart is not the class loader. Ares' own api frames are application-loaded too, and a path made of them must stay a silent drop, which the existing infra-only test says out loud. It is whether supervised code was free to declare the namespace that was skipped: reserved means it could not have put itself there, unreserved means it could. So a sink that ends the walk having stepped over a reachable frame in an INFRA prefix that is not also a RESERVED prefix is refused, naming that frame and the call. Keying it to the gap between the two lists rather than to their contents keeps this true whatever they say next. An entry added to INFRA_PREFIXES without being reserved is refused rather than turned into a blind spot, and reserving one again makes the refusal disappear on its own. That is deliberately not a re-coupling of the lists: they answer different questions, and taking back the narrowing would restore the complaint it was made to remove, that every Ares user refused two ordinary package names on one consumer's behalf. Two tests, one for each shape. A path entered through metatest is refused and the message names the frame; a path of JDK frames alone is still dropped, or every synthetic WALA root would become a failure. Verified with spotless:check and 153 tests across the WALA, ArchUnit, scanner and reserved-package classes, including the infra-only path that the first version of this fix broke.

Claudia-Anthropica

@MarkusPaulsen The single-path WALA regression is fixed, but a parallel ordinary approach can still suppress refusal of an all-infrastructure student path. [medium] Creator also still makes the new boolean overload abstract while the released overload delegates to it, so existing third-party implementations fail to compile and can reach AbstractMethodError. No failed checks were captured; several checks remain pending but none is listed as required.

Claudia's finding on the previous head, and it is right. The refusal was decided per sink, on whether anything at all had been evaluated. A sink is commonly reached along several branches, and one of them ending in an ordinary caller says nothing about another: an exempt approach, or a false-positive transitive path, evaluates and reports nothing while still incrementing the counter. One such branch was therefore enough to excuse a parallel branch made entirely of frames supervised code may name itself into, which is the case the previous commit existed to refuse. The counter no longer decides. A branch is examined when it runs out of callers, which is the point at which it is known that no frame on it could be blamed, and it is examined on its own. Branches that continue are left to their extensions, and an approach evaluated on one has found its student frame already, so neither needs the check. The regression is written so that it fails against the previous commit rather than passing because the walk happened to reach the right branch first. The stack is LIFO, so the predecessor order decides which branch is examined first; listing the helper first makes the ordinary branch pop first and evaluate before the nameable-infrastructure branch is looked at, which is exactly the ordering the previous version got wrong. Reversed, the refusal would fire before anything evaluated and the test would prove nothing. Confirmed both ways: it fails on 8e82643 and passes here. Verified with spotless:check and 154 tests across the WALA, ArchUnit, scanner and reserved-package classes.

Claudia-Anthropica

@MarkusPaulsen The branch-local WALA walk fixes the mixed-branch false negative, and the complete snapshot has no failed checks. [medium] The released Creator interface remains source- and binary-incompatible because its new boolean overload is abstract while the former abstract overload is now default; old implementations lack the new descriptor, so current framework calls can reach AbstractMethodError. I also left a medium documentation finding because the manual promises empty-output refusal while both runtime validators now warn and continue.

Claudia's finding on the previous head, and the manual was wrong. It claimed that an output root holding nothing at all is refused by name. That stopped being true when letting an empty supervised package be an empty package turned those two abort paths into warnings, deliberately: an exercise whose package is still empty must not fail for being empty. The document was not brought along. Both places behave the same way and the manual now says so. requireDerivedScopeToCoverTheProject warns and returns where the output holds no class file, and again where it holds only package or module descriptors. JavaArchunitSupervisedClasses warns and analyses an empty set where the conventional output root is absent or yields nothing supervisable. What is still refused by name is listed as what it is: 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. The claim about the last-resort default is qualified rather than removed, because it holds in one half of the case and not the other. Where anything at all is compiled, classes outside the default are caught and the run is refused; where nothing is compiled, nothing is caught. Saying it without that split was what made it read as a guarantee. The custom-output limitation was already documented as a fact about which directory is read. It is now stated as the consequence it has become: a project whose output goes elsewhere is indistinguishable from one that compiled nothing, so both pass with the same warning, and an exercise configured that way is not enforced by the derived path at all. That is the one case where a reader could take vacuous success for enforcement, so it says outright that such an exercise must declare its scope in a policy. Documentation only; no code changes. spotless:check is green.

Claudia-Anthropica

@MarkusPaulsen The documentation correction now matches the empty-output behaviour, and the prior generated-scope and WALA findings are resolved. [medium] Creator still makes the new boolean overload abstract while the released overload delegates to it, so existing third-party implementations cannot compile against this head and old binaries reach AbstractMethodError when the factory invokes the new descriptor. Keep the released overload abstract and make the boolean overload a default bridge, with JavaCreator implementing the scope-aware path.

Claudia raised this in four review bodies rather than on a line, so it carried no thread and my thread audit reported nothing open while the review still requested changes. The finding is right and my earlier answer to it was wrong. Restoring the released signature as a default that delegates to a new abstract one served callers and nobody else. A class written against 2.1.2 implemented the released method, so under that arrangement it implemented a default and left the abstract one unimplemented: it stopped compiling. One already compiled lacked the new descriptor entirely, and Ares' own call site always passes the scope, so it could be reached through AbstractMethodError. I had reasoned that no bridge can supply a parameter its author never wrote. That is true of the value and false of the conclusion. The parameter does not have to be supplied; which overload is abstract has to be chosen. The released signature is abstract again, so an implementation that predates the parameter is a complete implementation, and the scope-aware overload is the default, which drops the parameter and calls the released one. Dropping it is exactly what such an implementation did before the parameter existed. JavaCreator implements both: the scope-aware overload does the work, and the released one delegates to it reporting the scope as derived, which of the two readings is the strict one, since a derived scope is checked against the whole compiled output before enforcement. The cost is that a new implementation has to write both, one of them a one-liner. That is the price of not moving a released contract, and the alternative is silently dropping the parameter for an implementation that meant to act on it. ReleasedCreatorContractTest is the contract rather than a description of it. Its LegacyCreator implements only what 2.1.2 asked for, so this file compiling is half the assertion; the other half is that Ares' own call, which always passes the scope, arrives at the released method instead of failing. Verified with spotless:check and 185 tests across the creator, WALA, ArchUnit, scanner and reserved-package classes.

Claudia-Anthropica

@MarkusPaulsen The Creator compatibility blocker is fixed: the released overload is abstract again and the new overload is a default bridge covered by a legacy implementation test. Two medium issues remain in generated build-mode selection and the reserved-prefix migration material. Captured CI has no failures, although Unit Tests is still in progress and is not listed as required.

Claudia-Anthropica · 2026-08-21T14:38:26Z

+		boolean maven = Files.isRegularFile(Path.of("pom.xml"));
+		boolean gradle = Files.isRegularFile(Path.of("build.gradle"))
+				|| Files.isRegularFile(Path.of("build.gradle.kts"));
+		if (maven && gradle) {


@MarkusPaulsen [medium] Generated tests still discard the build mode selected by the policy. ProjectSourcesFinder.discover(root, explicitlySelectedMode) deliberately supports a project containing both descriptors, but this helper treats that valid configuration as ambiguous and aborts instead of reading the selected mode's output. Carry the selected BuildMode into the generated validated/pinned calls and use it to choose exactly target/classes or build/classes/java/main.

🤖 Prompt for AI agents

In src/main/java/de/tum/cit/ase/ares/api/architecture/java/archunit/JavaArchunitSupervisedClasses.java, generated tests ignore the explicitly selected build mode and reject valid dual-descriptor projects. Thread the selected BuildMode through generated helper entry points and select only its production output root.

Claudia-Anthropica · 2026-08-21T14:38:26Z

+	 * is urgent, but the identifier must still change: one version must not denote
+	 * two different lists.
+	 */
+	public static final String RESERVED_PACKAGE_PREFIX_VERSION = "2";


@MarkusPaulsen [medium] The prefix contract is now version 2 and no longer reserves anonymous.toolclasses or metatest, but the shipped Maven/Gradle examples and three setup/enforcement documents still declare prefix version 1 and include both removed prefixes. Users following those canonical examples therefore pin obsolete data and reject packages this version intentionally permits. Update both example snippets plus HowToMakeAProjectAnAresProject.md, HowToConvertAnAres1ProjectIntoAnAres2Project.md, and policy/EnforcementModel.md, and document the prefix-version migration in the PR body.

🤖 Prompt for AI agents

In src/main/java/de/tum/cit/ase/ares/api/architecture/java/wala/WalaPathClassification.java, the reserved-prefix version was raised to 2 without updating the repository's canonical examples and setup documentation. Synchronise all shipped snippets and version references with the version-2 prefix list and add the migration note.

MarkusPaulsen requested a review from a team August 4, 2026 08:44

MarkusPaulsen requested review from a team and krusche as code owners August 4, 2026 08:44

github-actions Bot added tests Automated area label: tests securitytest Automated area label: securitytest labels Aug 4, 2026

coderabbitai Bot requested changes Aug 6, 2026

View reviewed changes

Comment thread src/test/java/de/tum/cit/ase/ares/api/util/ProjectSourcesFinderEdgeCaseTest.java

Comment thread src/test/java/de/tum/cit/ase/ares/api/util/ProjectSourcesFinderEdgeCaseTest.java

Markus Paulsen added 2 commits August 6, 2026 18:31

github-actions Bot added aop Automated area label: aop docs Automated area label: docs labels Aug 9, 2026

coderabbitai Bot requested changes Aug 9, 2026

View reviewed changes

Comment thread docs/securitytest/TestCaseFactoryAndBuilderManual.md

github-actions Bot added the policy Automated area label: policy label Aug 10, 2026

coderabbitai Bot requested changes Aug 10, 2026

View reviewed changes

Comment thread .../testuser/securitypolicies/java/maven/archunit/aspectj/PolicySelfTestDefaultRestrictive.yaml Outdated

Markus Paulsen added 2 commits August 10, 2026 11:26

Merge branch 'main' into fix/detect-supervised-package-without-policy

0136141

Claudia-Anthropica requested changes Aug 12, 2026

View reviewed changes

Comment thread src/main/java/de/tum/cit/ase/ares/api/securitytest/java/projectScanner/JavaProjectScanner.java

ShudongCai requested changes Aug 12, 2026

View reviewed changes

Comment thread src/main/java/de/tum/cit/ase/ares/api/util/ProjectSourcesFinder.java Outdated

Comment thread src/test/java/de/tum/cit/ase/ares/api/util/ProjectSourcesFinderTest.java

MarkusPaulsen mentioned this pull request Aug 12, 2026

Derive the policy-free supervised package fail-closed instead of by plurality #189

Open

22 tasks

Claudia-Anthropica requested changes Aug 12, 2026

View reviewed changes

MarkusPaulsen mentioned this pull request Aug 12, 2026

Close the remaining gaps in Gradle source-root discovery #190

Open

8 tasks

Claudia-Anthropica requested changes Aug 12, 2026

View reviewed changes

Claudia-Anthropica requested changes Aug 18, 2026

View reviewed changes

Comment thread ...n/java/de/tum/cit/ase/ares/api/architecture/java/archunit/JavaArchunitSupervisedClasses.java Outdated

Comment thread ...n/java/de/tum/cit/ase/ares/api/architecture/java/archunit/JavaArchunitSupervisedClasses.java Outdated

Claudia-Anthropica requested changes Aug 18, 2026

View reviewed changes

Comment thread ...n/java/de/tum/cit/ase/ares/api/architecture/java/archunit/JavaArchunitSupervisedClasses.java Outdated

Comment thread ...n/java/de/tum/cit/ase/ares/api/architecture/java/archunit/JavaArchunitSupervisedClasses.java Outdated

Claudia-Anthropica requested changes Aug 18, 2026

View reviewed changes

Comment thread ...n/java/de/tum/cit/ase/ares/api/architecture/java/archunit/JavaArchunitSupervisedClasses.java Outdated

Claudia-Anthropica requested changes Aug 19, 2026

View reviewed changes

Comment thread src/main/java/de/tum/cit/ase/ares/api/architecture/java/JavaArchitectureTestCase.java

Markus Paulsen added 4 commits August 21, 2026 13:58

Claudia-Anthropica requested changes Aug 21, 2026

View reviewed changes

Comment thread src/main/java/de/tum/cit/ase/ares/api/architecture/java/wala/WalaPathClassification.java

Claudia-Anthropica requested changes Aug 21, 2026

View reviewed changes

Comment thread src/main/java/de/tum/cit/ase/ares/api/architecture/java/wala/WalaRule.java Outdated

Claudia-Anthropica requested changes Aug 21, 2026

View reviewed changes

Comment thread docs/securitytest/TestCaseFactoryAndBuilderManual.md Outdated

MarkusPaulsen requested a review from Claudia-Anthropica August 21, 2026 14:11

Claudia-Anthropica requested changes Aug 21, 2026

View reviewed changes

MarkusPaulsen requested a review from Claudia-Anthropica August 21, 2026 14:30

Claudia-Anthropica requested changes Aug 21, 2026

View reviewed changes

MarkusPaulsen merged commit e6a2e28 into main Aug 21, 2026
16 checks passed

MarkusPaulsen deleted the fix/detect-supervised-package-without-policy branch August 21, 2026 14:39

MarkusPaulsen mentioned this pull request Aug 21, 2026

Raise the project version to 2.1.3 #209

Merged

10 tasks

Uh oh!

Conversation

MarkusPaulsen commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Linked issues

1. Problem

2. Improvement from the user's perspective

3. Improvement from the maintainer's perspective

4. Testing manual

5. Test case coverage regarding this PR

Breaking changes and migration

Checklist

Review progress

Uh oh!

coderabbitai Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Pre-merge checks failed

❌ Failed checks (1 error, 1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Claudia-Anthropica left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Claudia-Anthropica left a comment

Choose a reason for hiding this comment

Uh oh!

Claudia-Anthropica left a comment

Choose a reason for hiding this comment

Uh oh!

Claudia-Anthropica left a comment

Choose a reason for hiding this comment

Uh oh!

MarkusPaulsen commented Aug 13, 2026

Uh oh!

Claudia-Anthropica left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Claudia-Anthropica left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Claudia-Anthropica left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Claudia-Anthropica left a comment

Choose a reason for hiding this comment

Uh oh!

Claudia-Anthropica left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Claudia-Anthropica left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Claudia-Anthropica left a comment

Choose a reason for hiding this comment

MarkusPaulsen commented Aug 4, 2026 •

edited

Loading

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading